Compare commits
9 Commits
beeb2e1778
...
04d54d1734
| Author | SHA1 | Date |
|---|---|---|
|
|
04d54d1734 | |
|
|
c45cc35c60 | |
|
|
0d505257d3 | |
|
|
26e415f2fd | |
|
|
f927bb3e92 | |
|
|
fd0d64d59c | |
|
|
bd450d0c71 | |
|
|
4424ffff3e | |
|
|
1f2962a78b |
|
|
@ -13,14 +13,16 @@ You are assisting the user in reviewing a change.
|
||||||
|
|
||||||
- **Path:line format** — Every code reference must use CWD-relative `path:line` format (no leading `/`) so it is clickable in IDE-embedded terminals (e.g., `src/auth/middleware.ts:42`).
|
- **Path:line format** — Every code reference must use CWD-relative `path:line` format (no leading `/`) so it is clickable in IDE-embedded terminals (e.g., `src/auth/middleware.ts:42`).
|
||||||
- **Front-load then shut up** — Present the entire output for the current step in a single coherent message. Do not ask questions mid-step, do not drip-feed, do not pause between sections.
|
- **Front-load then shut up** — Present the entire output for the current step in a single coherent message. Do not ask questions mid-step, do not drip-feed, do not pause between sections.
|
||||||
- **Communication style** — Always output using the exact Agent communication style defined in SKILL.md and the loaded config.
|
- **Language** — Speak in `{communication_language}`. Write any file output in `{document_output_language}`.
|
||||||
|
|
||||||
## INITIALIZATION
|
## INITIALIZATION
|
||||||
|
|
||||||
Load and read full config from `{project-root}/_bmad/bmm/config.yaml` and resolve:
|
Load and read full config from `{project-root}/_bmad/bmm/config.yaml` and resolve:
|
||||||
|
|
||||||
- `implementation_artifacts`
|
- `implementation_artifacts`
|
||||||
|
- `planning_artifacts`
|
||||||
- `communication_language`
|
- `communication_language`
|
||||||
|
- `document_output_language`
|
||||||
|
|
||||||
## FIRST STEP
|
## FIRST STEP
|
||||||
|
|
||||||
|
|
|
||||||
|
|
@ -0,0 +1,38 @@
|
||||||
|
# Generate Review Trail
|
||||||
|
|
||||||
|
Generate a review trail from the diff and codebase context. A generated trail is lower quality than an author-produced one, but far better than none.
|
||||||
|
|
||||||
|
## Follow Global Step Rules in SKILL.md
|
||||||
|
|
||||||
|
## INSTRUCTIONS
|
||||||
|
|
||||||
|
1. Get the full diff against the appropriate baseline (same rules as Surface Area Stats in step-01).
|
||||||
|
2. Read changed files in full — not just diff hunks. Surrounding code reveals intent that hunks alone miss. If total file content exceeds ~50k tokens, read only the files with the largest diff hunks in full and use hunks for the rest.
|
||||||
|
3. If a spec exists, use its Intent section to anchor concern identification.
|
||||||
|
4. Identify 2–5 concerns: cohesive design intents that each explain *why* behind a cluster of changes. Prefer functional groupings and architectural boundaries over file-level splits. A single-concern change is fine — don't invent groupings.
|
||||||
|
5. For each concern, select 1–4 `path:line` stops — locations where the concern is most visible. Prefer entry points, decision points, and boundary crossings over mechanical changes.
|
||||||
|
6. Lead with the entry point — the highest-leverage stop a reviewer should see first. Inside each concern, order stops so each builds on the previous. End with peripherals (tests, config, types).
|
||||||
|
7. Format each stop using `path:line` per the global step rules:
|
||||||
|
|
||||||
|
```
|
||||||
|
**{Concern name}**
|
||||||
|
|
||||||
|
- {one-line framing, ≤15 words}
|
||||||
|
`src/path/to/file.ts:42`
|
||||||
|
```
|
||||||
|
|
||||||
|
When there is only one concern, omit the bold label — just list the stops directly.
|
||||||
|
|
||||||
|
## PRESENT
|
||||||
|
|
||||||
|
Output after the orientation:
|
||||||
|
|
||||||
|
```
|
||||||
|
I built a review trail for this {change_type} (no author-produced trail was found):
|
||||||
|
|
||||||
|
{generated trail}
|
||||||
|
```
|
||||||
|
|
||||||
|
The generated trail serves as the Suggested Review Order for subsequent steps. Set `review_mode` to `full-trail` — a trail now exists, so all downstream steps should treat it as one.
|
||||||
|
|
||||||
|
If git is unavailable or the diff cannot be retrieved, return to step-01 with: "Could not generate trail — git unavailable."
|
||||||
|
|
@ -4,56 +4,73 @@ Display: `[Orientation] → Walkthrough → Detail Pass → Testing`
|
||||||
|
|
||||||
## Follow Global Step Rules in SKILL.md
|
## Follow Global Step Rules in SKILL.md
|
||||||
|
|
||||||
- The conversation context before this skill was triggered IS your starting point — not a blank slate.
|
|
||||||
- The user may have already mentioned a spec file, a commit, a branch, a PR, or described the change. Use what they gave you.
|
|
||||||
- For PR references, resolve to a commit or branch using whatever tools are available (e.g., `gh pr view` for GitHub). If resolution fails, ask for a SHA or branch.
|
|
||||||
|
|
||||||
## FIND THE CHANGE
|
## FIND THE CHANGE
|
||||||
|
|
||||||
1. **What did the user already tell you?** Look at the conversation for a spec path, commit ref, branch, PR, or description of the change.
|
The conversation context before this skill was triggered IS your starting point — not a blank slate. Check in this order — stop as soon as the change is identified:
|
||||||
2. **Fill in the other half.** If they gave a spec, look for `baseline_commit` in its frontmatter. If they gave a commit, check `{implementation_artifacts}` for a spec whose `baseline_commit` matches. If they gave both, use both.
|
|
||||||
3. **If they gave nothing**, check current branch and HEAD. Confirm: "I see HEAD is `<short-sha>` on `<branch>` — is this the change you want to review?"
|
1. **Explicit argument**
|
||||||
4. **If you still can't identify a change**, ask:
|
Did the user pass a PR, commit SHA, branch, or spec file this message?
|
||||||
|
- PR reference → resolve to branch/commit via `gh pr view`. If resolution fails, ask for a SHA or branch.
|
||||||
|
- Spec file, commit, or branch → use directly.
|
||||||
|
|
||||||
|
2. **Recent conversation**
|
||||||
|
Do the last few messages reveal what change the user wants reviewed? Look for spec paths, commit refs, branches, PRs, or descriptions of a change. Use the same routing as above.
|
||||||
|
|
||||||
|
3. **Sprint tracking**
|
||||||
|
Check for a sprint status file (`*sprint-status*`) in `{implementation_artifacts}` or `{planning_artifacts}`. If found, scan for stories with status `review`:
|
||||||
|
- Exactly one → suggest it and confirm with the user.
|
||||||
|
- Multiple → present as numbered options.
|
||||||
|
- None → fall through.
|
||||||
|
|
||||||
|
4. **Current git state**
|
||||||
|
Check current branch and HEAD. Confirm: "I see HEAD is `<short-sha>` on `<branch>` — is this the change you want to review?"
|
||||||
|
|
||||||
|
5. **Ask**
|
||||||
|
If none of the above identified a change, ask:
|
||||||
- What changed and why?
|
- What changed and why?
|
||||||
- Which commit, branch, or PR should I look at?
|
- Which commit, branch, or PR should I look at?
|
||||||
- Do you have a spec, bug report, or anything else that explains what this change is supposed to do?
|
- Do you have a spec, bug report, or anything else that explains what this change is supposed to do?
|
||||||
|
|
||||||
If after 3 exchanges you still can't identify a change, HALT.
|
If after 3 exchanges you still can't identify a change, HALT.
|
||||||
|
|
||||||
|
Never ask extra questions beyond what the cascade prescribes. If a step above already identified the change, skip the remaining steps.
|
||||||
|
|
||||||
|
## ENRICH
|
||||||
|
|
||||||
|
Once a change is identified from any source above, fill in the complementary artifact:
|
||||||
|
|
||||||
|
- If you have a spec, look for `baseline_commit` in its frontmatter to determine the diff baseline.
|
||||||
|
- If you have a commit or branch, check `{implementation_artifacts}` for a spec whose `baseline_commit` is an ancestor of that commit/branch (i.e., the spec describes work done on top of that baseline).
|
||||||
|
- If you found both a spec and a commit/branch, use both.
|
||||||
|
|
||||||
## DETERMINE WHAT YOU HAVE
|
## DETERMINE WHAT YOU HAVE
|
||||||
|
|
||||||
Set `change_type` based on how the user referred to the change:
|
Set `change_type` to match how the user referred to the change — `PR`, `commit`, `branch`, or their own words (e.g. `auth refactor`). Default to `change` if ambiguous.
|
||||||
|
|
||||||
| User referenced | `change_type` |
|
Set `review_mode` — pick the first match:
|
||||||
|---|---|
|
|
||||||
| A pull request | `PR` |
|
|
||||||
| A commit | `commit` |
|
|
||||||
| A branch | `branch` |
|
|
||||||
| A description (e.g. "the auth refactor") | use their words (e.g. `auth refactor`) |
|
|
||||||
| Nothing specific / ambiguous | `change` |
|
|
||||||
|
|
||||||
| What you have | Review Mode | Intent Source |
|
1. **`full-trail`** — ENRICH found a spec with a `## Suggested Review Order` section. Intent source: spec's Intent section.
|
||||||
|---|---|---|
|
2. **`spec-only`** — ENRICH found a spec but it has no Suggested Review Order. Intent source: spec's Intent section.
|
||||||
| Spec with `## Suggested Review Order` | `full-trail` | Spec's Intent section |
|
3. **`bare-commit`** — no spec found. Intent source: commit message. If the commit message is terse (under 10 words), scan the diff for the primary change pattern and draft a one-sentence intent. Flag it as `[inferred]` in the output so the user can correct it.
|
||||||
| Spec without trail | `spec-only` | Spec's Intent section |
|
|
||||||
| Commit ref, no spec | `bare-commit` | Commit message |
|
|
||||||
|
|
||||||
**Terse commit messages** (under 10 words): Scan the diff for the primary change pattern and draft a one-sentence intent. Confirm with the user before proceeding.
|
|
||||||
|
|
||||||
## PRODUCE ORIENTATION
|
## PRODUCE ORIENTATION
|
||||||
|
|
||||||
### Intent Summary
|
### Intent Summary
|
||||||
|
|
||||||
- If intent comes from a spec's Intent section, display it verbatim regardless of length — it's already written to be concise.
|
- If intent comes from a spec's Intent section, display it verbatim regardless of length — it's already written to be concise.
|
||||||
- For other sources (commit messages, bug reports, user description): if ≤200 tokens, display verbatim. If longer, distill to ≤200 tokens and link to the full source.
|
- For other sources (commit messages, bug reports, user description): if ≤200 tokens, display verbatim. If longer, distill to ≤200 tokens. Link to the full source when one exists (e.g. a file path or URL).
|
||||||
- Format: `> **Intent:** {summary}`
|
- Format: `> **Intent:** {summary}`
|
||||||
|
|
||||||
### Surface Area Stats
|
### Surface Area Stats
|
||||||
|
|
||||||
Compute from `git diff --stat` against the appropriate baseline:
|
Best-effort stats derived from the diff. Try these baselines in order:
|
||||||
|
|
||||||
- **With spec**: Use `baseline_commit` from frontmatter. If missing, diff `HEAD~1..HEAD` and tell the user stats reflect only the latest commit.
|
1. `baseline_commit` from the spec's frontmatter.
|
||||||
- **Bare commit**: Diff against parent (`commit~1..commit`). For merge commits, use `--first-parent`.
|
2. Branch merge-base against `main` (or the default branch).
|
||||||
|
3. `HEAD~1..HEAD` (latest commit only — tell the user).
|
||||||
|
4. If git is unavailable or all of the above fail, skip stats and note: "Could not compute stats."
|
||||||
|
|
||||||
|
Use `git diff --stat` and `git diff --numstat` for file-level counts, and scan the full diff content for the richer metrics.
|
||||||
|
|
||||||
Display as:
|
Display as:
|
||||||
|
|
||||||
|
|
@ -61,13 +78,13 @@ Display as:
|
||||||
N files changed · M modules touched · ~L lines of logic · B boundary crossings · P new public interfaces
|
N files changed · M modules touched · ~L lines of logic · B boundary crossings · P new public interfaces
|
||||||
```
|
```
|
||||||
|
|
||||||
- **Files changed**: from `git diff --stat`
|
- **Files changed**: count from `git diff --stat`.
|
||||||
- **Modules touched**: distinct top-level directories with changes
|
- **Modules touched**: distinct top-level directories with changes (from `--stat` file paths).
|
||||||
- **Lines of logic**: added/modified lines excluding blanks, imports, formatting. `~` because approximate.
|
- **Lines of logic**: added/modified lines excluding blanks, imports, formatting. Scan diff content; `~` because approximate.
|
||||||
- **Boundary crossings**: changes spanning more than one top-level module. `0` if single module.
|
- **Boundary crossings**: changes spanning more than one top-level module. `0` if single module.
|
||||||
- **New public interfaces**: new exports, endpoints, public methods. `0` if none.
|
- **New public interfaces**: new exports, endpoints, public methods found in the diff. `0` if none.
|
||||||
|
|
||||||
If git is unavailable or a command fails, show what you can and note what's missing.
|
Omit any metric you cannot compute rather than guessing.
|
||||||
|
|
||||||
### Present
|
### Present
|
||||||
|
|
||||||
|
|
@ -81,38 +98,7 @@ If git is unavailable or a command fails, show what you can and note what's miss
|
||||||
|
|
||||||
## FALLBACK TRAIL GENERATION
|
## FALLBACK TRAIL GENERATION
|
||||||
|
|
||||||
**Skip this section if review mode is `full-trail`.**
|
If review mode is not `full-trail`, read fully and follow `./generate-trail.md` to build one from the diff. Then return here and continue to NEXT. If trail generation fails (e.g., git unavailable), the original review mode is preserved — step-02 handles this with its non-trail path.
|
||||||
|
|
||||||
When no Suggested Review Order exists, generate one from the diff and codebase context. A generated trail is lower quality than an author-produced one, but far better than none.
|
|
||||||
|
|
||||||
1. Get the full diff against the appropriate baseline (same rules as Surface Area Stats).
|
|
||||||
2. Read changed files in full — not just diff hunks. Surrounding code reveals intent that hunks alone miss. If total file content exceeds ~50k tokens, read only the files with the largest diff hunks in full and use hunks for the rest.
|
|
||||||
3. If a spec exists, use its Intent section to anchor concern identification.
|
|
||||||
4. Identify 2–5 concerns: cohesive design intents that each explain *why* behind a cluster of changes. Prefer functional groupings and architectural boundaries over file-level splits. A single-concern change is fine — don't invent groupings.
|
|
||||||
5. For each concern, select 1–4 `path:line` stops — locations where the concern is most visible. Prefer entry points, decision points, and boundary crossings over mechanical changes.
|
|
||||||
6. Lead with the entry point — the highest-leverage stop a reviewer should see first. Inside each concern, order stops so each builds on the previous. End with peripherals (tests, config, types).
|
|
||||||
7. Format each stop using `path:line` per the global step rules:
|
|
||||||
|
|
||||||
```
|
|
||||||
**{Concern name}**
|
|
||||||
|
|
||||||
- {one-line framing, ≤15 words}
|
|
||||||
`src/path/to/file.ts:42`
|
|
||||||
```
|
|
||||||
|
|
||||||
When there is only one concern, omit the bold label — just list the stops directly.
|
|
||||||
|
|
||||||
Present after the orientation output:
|
|
||||||
|
|
||||||
```
|
|
||||||
I built a review trail for this {change_type} (no author-produced trail was found):
|
|
||||||
|
|
||||||
{generated trail}
|
|
||||||
```
|
|
||||||
|
|
||||||
Set review mode to `full-trail`. The generated trail is the Suggested Review Order for subsequent steps.
|
|
||||||
|
|
||||||
If git is unavailable or the diff cannot be retrieved, skip this section and proceed with the original review mode. Note: "Could not generate trail — git unavailable."
|
|
||||||
|
|
||||||
## NEXT
|
## NEXT
|
||||||
|
|
||||||
|
|
|
||||||
|
|
@ -11,14 +11,14 @@ Display: `Orientation → [Walkthrough] → Detail Pass → Testing`
|
||||||
|
|
||||||
### Identify Concerns
|
### Identify Concerns
|
||||||
|
|
||||||
**With Suggested Review Order** (`full-trail` mode):
|
**With Suggested Review Order** (`full-trail` mode — the normal path, including when step-01 generated a trail):
|
||||||
|
|
||||||
1. Read the Suggested Review Order stops from the spec (or from conversation context if generated by step-01 fallback).
|
1. Read the Suggested Review Order stops from the spec (or from conversation context if generated by step-01 fallback).
|
||||||
2. Resolve each stop to a file in the current repo. Output in `path:line` format per the standing rule.
|
2. Resolve each stop to a file in the current repo. Output in `path:line` format per the standing rule.
|
||||||
3. Read the diff to understand what each stop actually does.
|
3. Read the diff to understand what each stop actually does.
|
||||||
4. Group stops by concern. Stops that share a design intent belong together even if they're in different files. A stop may appear under multiple concerns if it serves multiple purposes.
|
4. Group stops by concern. Stops that share a design intent belong together even if they're in different files. A stop may appear under multiple concerns if it serves multiple purposes.
|
||||||
|
|
||||||
**Without Suggested Review Order** (`spec-only` or `bare-commit` mode):
|
**Without Suggested Review Order** (fallback when trail generation failed, e.g., git unavailable):
|
||||||
|
|
||||||
1. Get the diff against the appropriate baseline (same rules as step 1).
|
1. Get the diff against the appropriate baseline (same rules as step 1).
|
||||||
2. Identify concerns by reading the diff for cohesive design intents:
|
2. Identify concerns by reading the diff for cohesive design intents:
|
||||||
|
|
@ -73,12 +73,17 @@ Take your time — click through the stops, read the diff, trace the logic. Whil
|
||||||
- "party mode on whether this schema migration is safe"
|
- "party mode on whether this schema migration is safe"
|
||||||
- or just ask anything
|
- or just ask anything
|
||||||
|
|
||||||
When you are done with the walkthrough:
|
When you're ready, say **next** and I'll surface the highest-risk spots.
|
||||||
- **"detail pass"** — I'll surface the highest-risk spots and run code review
|
|
||||||
- **"testing"** — I'll suggest how to observe the behavior
|
|
||||||
- **"I've seen enough"** — tell me what to do about this {change_type} and we wrap up
|
|
||||||
```
|
```
|
||||||
|
|
||||||
|
## EARLY EXIT
|
||||||
|
|
||||||
|
If at any point the human signals they want to make a decision about this {change_type} (e.g., "let's ship it", "this needs a rethink", "I'm done reviewing", or anything suggesting they're ready to decide), confirm their intent:
|
||||||
|
|
||||||
|
- If they want to **approve and ship** → read fully and follow `./step-05-wrapup.md`
|
||||||
|
- If they want to **reject and rework** → read fully and follow `./step-05-wrapup.md`
|
||||||
|
- If you misread them → acknowledge and continue the current step.
|
||||||
|
|
||||||
## NEXT
|
## NEXT
|
||||||
|
|
||||||
When the human signals readiness for the next phase, read fully and follow `./step-03-detail-pass.md`
|
Default: read fully and follow `./step-03-detail-pass.md`
|
||||||
|
|
|
||||||
|
|
@ -76,10 +76,17 @@ End the message with:
|
||||||
|
|
||||||
You've seen the design and the risk landscape. From here:
|
You've seen the design and the risk landscape. From here:
|
||||||
- **"dig into [area]"** — I'll deep-dive that specific area with correctness focus
|
- **"dig into [area]"** — I'll deep-dive that specific area with correctness focus
|
||||||
- **"testing"** — I'll suggest how to observe the behavior
|
- **"next"** — I'll suggest how to observe the behavior
|
||||||
- **"I've seen enough"** — tell me what to do about this {change_type} and we wrap up
|
|
||||||
```
|
```
|
||||||
|
|
||||||
|
## EARLY EXIT
|
||||||
|
|
||||||
|
If at any point the human signals they want to make a decision about this {change_type} (e.g., "let's ship it", "this needs a rethink", "I'm done reviewing", or anything suggesting they're ready to decide), confirm their intent:
|
||||||
|
|
||||||
|
- If they want to **approve and ship** → read fully and follow `./step-05-wrapup.md`
|
||||||
|
- If they want to **reject and rework** → read fully and follow `./step-05-wrapup.md`
|
||||||
|
- If you misread them → acknowledge and continue the current step.
|
||||||
|
|
||||||
## TARGETED RE-REVIEW
|
## TARGETED RE-REVIEW
|
||||||
|
|
||||||
When the human says "dig into [area]" (e.g., "dig into the auth changes", "dig into the schema migration"):
|
When the human says "dig into [area]" (e.g., "dig into the auth changes", "dig into the schema migration"):
|
||||||
|
|
@ -96,4 +103,4 @@ The human can trigger multiple targeted re-reviews. Each time, present new findi
|
||||||
|
|
||||||
## NEXT
|
## NEXT
|
||||||
|
|
||||||
When the human signals readiness for testing, read fully and follow `./step-04-testing.md`
|
Read fully and follow `./step-04-testing.md`
|
||||||
|
|
|
||||||
|
|
@ -66,26 +66,9 @@ End the message with:
|
||||||
```
|
```
|
||||||
---
|
---
|
||||||
|
|
||||||
You've seen the change and how to verify it. From here:
|
You've seen the change and how to verify it. When you're ready to make a call, just say so.
|
||||||
- **"I've seen enough"** — we wrap up this {change_type}
|
|
||||||
- or just ask anything
|
|
||||||
```
|
```
|
||||||
|
|
||||||
## WRAP-UP
|
## NEXT
|
||||||
|
|
||||||
When the human says "I've seen enough" or signals they're done:
|
When the human signals they're ready to make a decision about this {change_type}, read fully and follow `./step-05-wrapup.md`
|
||||||
|
|
||||||
```
|
|
||||||
---
|
|
||||||
|
|
||||||
Review complete. What's the call on this {change_type}?
|
|
||||||
- **Approve** — ship it
|
|
||||||
- **Request changes** — I'll help draft feedback
|
|
||||||
- **Discuss** — something's still on your mind
|
|
||||||
```
|
|
||||||
|
|
||||||
Act on their decision:
|
|
||||||
|
|
||||||
- **Approve**: Acknowledge briefly. If reviewing a PR, offer to approve via `gh pr review --approve` — but confirm with the human before executing, since this is a visible action on a shared resource.
|
|
||||||
- **Request changes**: Ask what needs changing. Help draft specific, actionable feedback tied to `path:line` locations.
|
|
||||||
- **Discuss**: Open conversation — answer questions, explore concerns, dig into any aspect.
|
|
||||||
|
|
|
||||||
|
|
@ -0,0 +1,22 @@
|
||||||
|
# Step 5: Wrap-Up
|
||||||
|
|
||||||
|
Display: `Orientation → Walkthrough → Detail Pass → Testing → [Wrap-Up]`
|
||||||
|
|
||||||
|
## Follow Global Step Rules in SKILL.md
|
||||||
|
|
||||||
|
## PROMPT FOR DECISION
|
||||||
|
|
||||||
|
```
|
||||||
|
---
|
||||||
|
|
||||||
|
Review complete. What's the call on this {change_type}?
|
||||||
|
- **Approve** — ship it (I can help with interactive patching first if needed)
|
||||||
|
- **Rework** — back to the drawing board (revert, revise the spec, try a different approach)
|
||||||
|
- **Discuss** — something's still on your mind
|
||||||
|
```
|
||||||
|
|
||||||
|
## ACT ON DECISION
|
||||||
|
|
||||||
|
- **Approve**: Acknowledge briefly. If the human wants to patch something before shipping, help apply the fix interactively. If reviewing a PR, offer to approve via `gh pr review --approve` — but confirm with the human before executing, since this is a visible action on a shared resource.
|
||||||
|
- **Rework**: Ask what went wrong — was it the approach, the spec, or the implementation? Help the human decide on next steps (revert commit, open an issue, revise the spec, etc.). Help draft specific, actionable feedback tied to `path:line` locations if the change is a PR from someone else.
|
||||||
|
- **Discuss**: Open conversation — answer questions, explore concerns, dig into any aspect. After discussion, return to the decision prompt above.
|
||||||
Loading…
Reference in New Issue