feat(skills): add edge case hunter as parallel review layer in PR review#1791
Open
alexeyv wants to merge 3 commits intobmad-code-org:mainfrom
Open
feat(skills): add edge case hunter as parallel review layer in PR review#1791alexeyv wants to merge 3 commits intobmad-code-org:mainfrom
alexeyv wants to merge 3 commits intobmad-code-org:mainfrom
Conversation
🤖 Augment PR SummarySummary: Extends Raven’s Verdict PR review skill to run an adversarial review and an edge-case-hunter review in parallel, then merge results. 🤖 Was this summary useful? React with 👍 or 👎 |
![augmentcode[bot]](https://avatars.githubusercontent.com/in/1027498?s=60&v=4){kind=small}
📝 WalkthroughWalkthroughUpdates to the edge-case-hunter task definition modify output formatting to return an array structure, refine content validation to halt on empty/unreadable content, implement re-analysis logic when zero findings occur, and expand LLM instructions with exhaustive path enumeration guidance. Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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: 6
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/core/tasks/review-edge-case-hunter.xml (1)
27-31:⚠️ Potential issue | 🟠 MajorFlow-order mandate conflicts with re-analysis behavior.
You require exact step order, but the second-pass analysis is only described in halt-conditions (outside flow). This is contradictory execution guidance.
Proposed fix
<step n="3" title="Present Findings"> <action>Output findings as a JSON array following the output-format specification exactly</action> </step> + <step n="4" title="Zero-Findings Recheck"> + <action>If zero findings, rerun Step 2 once using the stricter checklist in halt-conditions</action> + </step>Also applies to: 60-60
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/core/tasks/review-edge-case-hunter.xml` around lines 27 - 31, The flow mandates exact ordered execution but the re-analysis behavior is only defined under halt-conditions, creating a contradiction between the <flow> -> <step> -> <action> sequence and the separate <halt-conditions> description; update the XML so the "second-pass analysis" behavior is either (A) moved into the ordered flow as an explicit <step> (e.g., add a <step id="second-pass-analysis"> with required <action> children) or (B) document a clear, single exception in the <flow> header that allows conditional re-analysis triggered by the same halt conditions, and remove the conflicting instruction that "DO NOT skip steps or change the sequence" or adjust it to allow this defined exception; ensure references to the existing <step>, <action>, and <halt-conditions> tags are updated accordingly so the behavior is not ambiguous.
🧹 Nitpick comments (3)
src/core/tasks/review-edge-case-hunter.xml (3)
27-27: Reintroduces non-standardized action verb in instruction text.Using “Execute” in an action instruction regresses the verb standardization pattern used elsewhere in BMAD task directives.
Based on learnings: when standardizing verbs, action instructions should use “load and follow” or “read and follow,” not “execute/invoke/run.”
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/core/tasks/review-edge-case-hunter.xml` at line 27, The instruction text "Execute steps in the flow section IN EXACT ORDER unless a halt-condition triggers" reintroduces the non-standard verb "Execute"; update this directive to use the standardized verb pattern (e.g., replace "Execute" with "Load and follow" or "Read and follow") so it aligns with other BMAD task directives—ensure the rest of the wording and emphasis (all caps "IN EXACT ORDER" and the halt-condition clause) remain unchanged and verify the updated line reads like "Load and follow the steps in the flow section IN EXACT ORDER unless a halt-condition triggers" (or "Read and follow..." if preferred).
24-24: Deduplicate within this layer before returning findings.The task outputs raw findings but does not prevent duplicate entries for the same unhandled path. Upstream merge should not be forced to clean intra-layer duplicates.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/core/tasks/review-edge-case-hunter.xml` at line 24, The task currently returns raw findings and may include duplicate entries for the same unhandled path; before returning from the task (the output array commonly named "findings" or the function that builds it, e.g., the task's main handler/run/execute method in review-edge-case-hunter), deduplicate entries within this layer by normalizing each finding to a canonical key (path + any distinguishing attributes) and filtering duplicates so the returned array contains unique findings (preserve order if needed); ensure an empty array [] is still returned when no unhandled paths are found.
24-24: Require deterministic output ordering.No ordering rule means equivalent runs can emit findings in different order, which destabilizes dedup and tests.
Proposed fix
-No extra text, no explanations, no markdown wrapping. An empty array [] is valid when no unhandled paths are found. +No extra text, no explanations, no markdown wrapping. +Sort findings by location, then trigger_condition. +An empty array [] is valid when no unhandled paths are found.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/core/tasks/review-edge-case-hunter.xml` at line 24, The task output must be deterministic: before serializing/emitting the findings from the review-edge-case-hunter task, sort the findings array by a stable key (for example: primary sort by file/path, then by line/column, then by rule id/message) and then serialize; also ensure that when there are no unhandled paths you still emit the empty array [] as-is. Locate the code that collects/emits the "findings" array in the review-edge-case-hunter task and add a stable sort step (e.g., sort(findings, (a,b) => compare(a.path,a.line,a.ruleId))) right before the output/serialization call so subsequent runs produce a deterministic order.
🤖 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/core/tasks/review-edge-case-hunter.xml`:
- Line 21: The "guard_snippet" value can contain quotes or newlines that break
JSON output; modify the code that emits/parses guard_snippet (referencing the
"guard_snippet" field/key) to enforce JSON-safe content by requiring a
single-line, escaped string (e.g., replace newlines with \n and escape quotes)
or validate/reject multi-line/raw quote content before emitting; ensure the
producer escapes quotes and control characters or runs the snippet through a
JSON-encoding step so the output remains valid JSON.
- Line 42: The "unreadable" term in the action "If content to review is empty or
unreadable, HALT per halt-conditions" is undefined; update
review-edge-case-hunter.xml to replace the ambiguous condition with explicit,
deterministic acceptance criteria (e.g., treat "unreadable" as any of:
base64/charset decode failure, empty diff, malformed hunk syntax, missing
required metadata) and implement precise checks that evaluate those cases
(decode validation, diff non-empty, hunk parser success, required fields
present). Also make the halt decision reference the concrete checks (not a
free-form "unreadable") and ensure the code path that enforces "halt-conditions"
records the exact failure reason (decode error, empty diff, malformed hunk,
etc.) so runs halt consistently and provide reproducible logs.
- Around line 17-24: The output-format element in review-edge-case-hunter.xml
mandates "Return ONLY a valid JSON array..." but the spec also tells the process
to immediately HALT, causing ambiguity for callers; update the spec text in the
<output-format> element to define a machine-readable halt behavior by specifying
that the tool must return a JSON array and, if a halt is required, include a
standardized halt object (e.g., an object with a "halt": true field or a
dedicated "trigger_condition" value) so callers always receive a valid JSON
array; edit the <output-format> content to replace the conflicting HALT
instruction with this explicit JSON halt representation and apply the same
change to the other occurrences referenced (lines 42 and 60-62) so all outputs
remain parseable.
- Line 60: The <condition> rule currently says "re-analyze once with increased
scrutiny" but is too vague; implement a concrete second-pass checklist: when the
initial analysis returns zero findings, invoke a second analysis call with
explicit parameters (e.g., set scrutinyLevel="high" or analysisPass="second"),
supply a defined boundaryMatrix and include a branchInventory parameter to
enumerate branches to inspect, and if the second pass still yields zero findings
return an empty array []; update the <condition> logic and any associated
analysis invocation (the re-analysis code path) to pass these named parameters
so results are reproducible.
- Around line 41-42: The "Load the content to review" action currently allows
ambiguous "provided input or context"; change it to strictly load only the
explicit provided input (e.g., diff or full-file content) and remove any
allowance for external "context" to enforce scope isolation—update the action
text in review-edge-case-hunter.xml to something like "Load the content to
review from the provided input (diff or full-file content only)" and ensure the
subsequent halt condition (the existing "If content to review is empty or
unreadable, HALT per halt-conditions") triggers if any external context is
detected or if only contextual sources would be used.
- Line 19: The "location" field in review-edge-case-hunter.xml is too narrow
(currently "file:line") and must be extended to represent ranges and hunks;
update the producer and consumer logic that reads/writes the "location" key to
emit and accept a richer format such as "file:start-end" for multi-line ranges
and a hunk form (e.g., include a "hunk" token or "file@hunk:start-end") so you
can represent gaps and hunk-only locations; change the code that
constructs/parses "location" (the serializer/deserializer handling the
"location" key in review-edge-case-hunter.xml) to support both legacy
"file:line" and the new range/hunk formats and add unit tests covering
single-line, multi-line range, and hunk-only cases.
---
Outside diff comments:
In `@src/core/tasks/review-edge-case-hunter.xml`:
- Around line 27-31: The flow mandates exact ordered execution but the
re-analysis behavior is only defined under halt-conditions, creating a
contradiction between the <flow> -> <step> -> <action> sequence and the separate
<halt-conditions> description; update the XML so the "second-pass analysis"
behavior is either (A) moved into the ordered flow as an explicit <step> (e.g.,
add a <step id="second-pass-analysis"> with required <action> children) or (B)
document a clear, single exception in the <flow> header that allows conditional
re-analysis triggered by the same halt conditions, and remove the conflicting
instruction that "DO NOT skip steps or change the sequence" or adjust it to
allow this defined exception; ensure references to the existing <step>,
<action>, and <halt-conditions> tags are updated accordingly so the behavior is
not ambiguous.
---
Nitpick comments:
In `@src/core/tasks/review-edge-case-hunter.xml`:
- Line 27: The instruction text "Execute steps in the flow section IN EXACT
ORDER unless a halt-condition triggers" reintroduces the non-standard verb
"Execute"; update this directive to use the standardized verb pattern (e.g.,
replace "Execute" with "Load and follow" or "Read and follow") so it aligns with
other BMAD task directives—ensure the rest of the wording and emphasis (all caps
"IN EXACT ORDER" and the halt-condition clause) remain unchanged and verify the
updated line reads like "Load and follow the steps in the flow section IN EXACT
ORDER unless a halt-condition triggers" (or "Read and follow..." if preferred).
- Line 24: The task currently returns raw findings and may include duplicate
entries for the same unhandled path; before returning from the task (the output
array commonly named "findings" or the function that builds it, e.g., the task's
main handler/run/execute method in review-edge-case-hunter), deduplicate entries
within this layer by normalizing each finding to a canonical key (path + any
distinguishing attributes) and filtering duplicates so the returned array
contains unique findings (preserve order if needed); ensure an empty array [] is
still returned when no unhandled paths are found.
- Line 24: The task output must be deterministic: before serializing/emitting
the findings from the review-edge-case-hunter task, sort the findings array by a
stable key (for example: primary sort by file/path, then by line/column, then by
rule id/message) and then serialize; also ensure that when there are no
unhandled paths you still emit the empty array [] as-is. Locate the code that
collects/emits the "findings" array in the review-edge-case-hunter task and add
a stable sort step (e.g., sort(findings, (a,b) =>
compare(a.path,a.line,a.ruleId))) right before the output/serialization call so
subsequent runs produce a deterministic order.
ℹ️ Review info
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (2)
.claude/skills/bmad-os-review-pr/SKILL.mdis excluded by!.claude/**.claude/skills/bmad-os-review-pr/prompts/instructions.mdis excluded by!.claude/**
📒 Files selected for processing (1)
src/core/tasks/review-edge-case-hunter.xml
bmadcode
requested changes
Mar 1, 2026
alexeyv
added a commit
to alexeyv/BMAD-METHOD
that referenced
this pull request
Mar 1, 2026
- Remove "Task tool" reference per maintainer; use generic "subagents" - Fix nested triple-backtick fencing with four-tick outer fence - Widen location format to support multi-line ranges and hunk refs - Add JSON-safety constraint to guard_snippet field - Tighten input loading to "strictly from provided input" - Replace vague "unreadable" with "cannot be decoded as text" - Replace vague "increased scrutiny" with concrete re-analysis checklist - Resolve HALT-immediately vs re-analysis conflict in LLM instructions Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Wire review-edge-case-hunter.xml into bmad-os-review-pr as a second review layer running in parallel with the adversarial review. Both subagents receive the same PR diff concurrently. Findings are merged, deduplicated, and tagged by source before tone transformation.
- Show array wrapper [{}] in output-format example to match JSON array
contract, and document empty array [] as valid output
- Consolidate empty-content handling: step 1 now defers to halt-conditions
instead of defining separate "ask and abort" behavior
- Zero-findings halt no longer contradicts JSON contract: re-analyze once,
then return [] instead of ambiguous "HALT or re-analyze"
- Soften "Execute ALL steps" to acknowledge halt-conditions can interrupt
- Remove "Task tool" reference per maintainer; use generic "subagents" - Fix nested triple-backtick fencing with four-tick outer fence - Widen location format to support multi-line ranges and hunk refs - Add JSON-safety constraint to guard_snippet field - Tighten input loading to "strictly from provided input" - Replace vague "unreadable" with "cannot be decoded as text" - Replace vague "increased scrutiny" with concrete re-analysis checklist - Resolve HALT-immediately vs re-analysis conflict in LLM instructions Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
alexeyv
force-pushed
the
feat/pr-review-edge-case-layer
branch
from
fd44200 to
9351582
Compare
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
2 participants
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
review-edge-case-hunter.xml) as a second review layer inbmad-os-review-pr, running in parallel with the adversarial review as concurrent subagents[Adversarial],[Edge Case],[Both])Design
The two layers are intentionally orthogonal:
Running them as parallel subagents keeps latency ~same as before (single-layer review).
Test plan
npm test)