Compare commits

..

3 Commits

Author SHA1 Message Date
don-petry 05d0b4d8b4
Merge 33bd6b5d86 into 1050415351 2026-04-05 03:23:53 +00:00
DJ 33bd6b5d86 fix: improve test convention discovery and require skip reason documentation
Address PR #2212 review feedback:
- Expand "Discover conventions" to check configured test tooling (test
  scripts, dependency manifests, config files) before falling back to
  language-idiomatic defaults, so repos with test infrastructure but no
  test files yet pick the correct framework.
- Require explicit skip reason in final summary output when skipping
  tests due to missing infrastructure, improving traceability.
- Apply consistently to both step-03-implement.md and step-oneshot.md.

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
2026-04-04 20:23:37 -07:00
Alex Verkhovsky 1050415351
refactor(code-review): harmonize step-01 intent cascade (#2206)
* refactor(code-review): harmonize step-01 intent cascade with quick-dev and checkpoint-preview

Replace keyword-matching entry point with 5-tier priority cascade:
explicit argument → recent conversation → sprint tracking → git state → ask.
Diff-mode keyword detection preserved as sub-check within tier 1.

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>

* fix(code-review): address review findings in step-01 intent cascade

- Set {spec_file} immediately in Tier 1 when spec provided
- Add staged/uncommitted handlers to instruction 3 dispatch table
- Replace undefined {branch}/{sha} placeholders with angle brackets
- Fix {story_key} vs {{story-id}} placeholder mismatch
- Correct "wants reviewed" grammar to "wants to be reviewed"

---------

Co-authored-by: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
2026-04-04 20:07:15 -07:00
3 changed files with 50 additions and 21 deletions

View File

@ -15,18 +15,37 @@ story_key: '' # set at runtime when discovered from sprint status
## INSTRUCTIONS ## INSTRUCTIONS
1. **Detect review intent from invocation text.** Check the triggering prompt for phrases that map to a review mode: 1. **Find the review target.** 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 review target is identified:
**Tier 1 — Explicit argument.**
Did the user pass a PR, commit SHA, branch, spec file, or diff source this message?
- PR reference → resolve to branch/commit via `gh pr view`. If resolution fails, ask for a SHA or branch.
- Commit or branch → use directly.
- Spec file → set `{spec_file}` to the provided path. Check its frontmatter for `baseline_commit`. If found, use as diff baseline. If not found, continue the cascade (a spec alone does not identify a diff source).
- Also scan the argument for diff-mode keywords that narrow the scope:
- "staged" / "staged changes" → Staged changes only - "staged" / "staged changes" → Staged changes only
- "uncommitted" / "working tree" / "all changes" → Uncommitted changes (staged + unstaged) - "uncommitted" / "working tree" / "all changes" → Uncommitted changes (staged + unstaged)
- "branch diff" / "vs main" / "against main" / "compared to {branch}" → Branch diff (extract base branch if mentioned) - "branch diff" / "vs main" / "against main" / "compared to <branch>" → Branch diff (extract base branch if mentioned)
- "commit range" / "last N commits" / "{sha}..{sha}" → Specific commit range - "commit range" / "last N commits" / "<from-sha>..<to-sha>" → Specific commit range
- "this diff" / "provided diff" / "paste" → User-provided diff (do not match bare "diff" — it appears in other modes) - "this diff" / "provided diff" / "paste" → User-provided diff (do not match bare "diff" — it appears in other modes)
- When multiple phrases match, prefer the most specific match (e.g., "branch diff" over bare "diff"). - When multiple keywords match, prefer the most specific (e.g., "branch diff" over bare "diff").
- **If a clear match is found:** Announce the detected mode (e.g., "Detected intent: review staged changes only") and proceed directly to constructing `{diff_output}` using the corresponding sub-case from instruction 3. Skip to instruction 4 (spec question).
- **If no match from invocation text, check sprint tracking.** Look for a sprint status file (`*sprint-status*`) in `{implementation_artifacts}` or `{planning_artifacts}`. If found, scan for any story with status `review`. Handle as follows: **Tier 2 — Recent conversation.**
- **Exactly one `review` story:** Set `{story_key}` to the story's key (e.g., `1-2-user-auth`). Suggest it: "I found story {{story-id}} in `review` status. Would you like to review its changes? [Y] Yes / [N] No, let me choose". If confirmed, use the story context to determine the diff source (branch name derived from story slug, or uncommitted changes). If declined, clear `{story_key}` and fall through to instruction 2. Do the last few messages reveal what the user wants to be reviewed? Look for spec paths, commit refs, branches, PRs, or descriptions of a change. Apply the same diff-mode keyword scan and routing as Tier 1.
- **Multiple `review` stories:** Present them as numbered options alongside a manual choice option. Wait for user selection. If the user selects a story, set `{story_key}` to the selected story's key and use the selected story's context to determine the diff source as in the single-story case above, and proceed to instruction 3. If the user selects the manual choice, clear `{story_key}` and fall through to instruction 2.
- **If no match and no sprint tracking:** Fall through to instruction 2. **Tier 3 — Sprint tracking.**
Look for a sprint status file (`*sprint-status*`) in `{implementation_artifacts}` or `{planning_artifacts}`. If found, scan for stories with status `review`:
- **Exactly one `review` story:** Set `{story_key}` to the story's key (e.g., `1-2-user-auth`). Suggest it: "I found story <story-id> in `review` status. Would you like to review its changes? [Y] Yes / [N] No, let me choose". If confirmed, use the story context to determine the diff source (branch name derived from story slug, or uncommitted changes). If declined, clear `{story_key}` and fall through.
- **Multiple `review` stories:** Present them as numbered options alongside a manual choice option. Wait for user selection. If a story is selected, set `{story_key}` and use its context to determine the diff source. If manual choice is selected, clear `{story_key}` and fall through.
- **None:** Fall through.
**Tier 4 — Current git state.**
If version control is unavailable, skip to Tier 5. Otherwise, check the current branch and HEAD. If the branch is not `main` (or the default branch), confirm: "I see HEAD is `<short-sha>` on `<branch>` — do you want to review this branch's changes?" If confirmed, treat as a branch diff against `main`. If declined, fall through.
**Tier 5 — Ask.**
Fall through to instruction 2.
Never ask extra questions beyond what the cascade prescribes. If a tier above already identified the target, skip the remaining tiers and proceed to instruction 3 (construct diff).
2. HALT. Ask the user: **What do you want to review?** Present these options: 2. HALT. Ask the user: **What do you want to review?** Present these options:
- **Uncommitted changes** (staged + unstaged) - **Uncommitted changes** (staged + unstaged)
@ -36,13 +55,17 @@ story_key: '' # set at runtime when discovered from sprint status
- **Provided diff or file list** (user pastes or provides a path) - **Provided diff or file list** (user pastes or provides a path)
3. Construct `{diff_output}` from the chosen source. 3. Construct `{diff_output}` from the chosen source.
- For **staged changes only**: run `git diff --cached`.
- For **uncommitted changes** (staged + unstaged): run `git diff HEAD`.
- For **branch diff**: verify the base branch exists before running `git diff`. If it does not exist, HALT and ask the user for a valid branch. - For **branch diff**: verify the base branch exists before running `git diff`. If it does not exist, HALT and ask the user for a valid branch.
- For **commit range**: verify the range resolves. If it does not, HALT and ask the user for a valid range. - For **commit range**: verify the range resolves. If it does not, HALT and ask the user for a valid range.
- For **provided diff**: validate the content is non-empty and parseable as a unified diff. If it is not parseable, HALT and ask the user to provide a valid diff. - For **provided diff**: validate the content is non-empty and parseable as a unified diff. If it is not parseable, HALT and ask the user to provide a valid diff.
- For **file list**: validate each path exists in the working tree. Construct `{diff_output}` by running `git diff HEAD -- <path1> <path2> ...`. If any paths are untracked (new files not yet staged), use `git diff --no-index /dev/null <path>` to include them. If the diff is empty (files have no uncommitted changes and are not untracked), ask the user whether to review the full file contents or to specify a different baseline. - For **file list**: validate each path exists in the working tree. Construct `{diff_output}` by running `git diff HEAD -- <path1> <path2> ...`. If any paths are untracked (new files not yet staged), use `git diff --no-index /dev/null <path>` to include them. If the diff is empty (files have no uncommitted changes and are not untracked), ask the user whether to review the full file contents or to specify a different baseline.
- After constructing `{diff_output}`, verify it is non-empty regardless of source type. If empty, HALT and tell the user there is nothing to review. - After constructing `{diff_output}`, verify it is non-empty regardless of source type. If empty, HALT and tell the user there is nothing to review.
4. Ask the user: **Is there a spec or story file that provides context for these changes?** 4. **Set the spec context.**
- If `{spec_file}` is already set (from Tier 1 or Tier 2): verify the file exists and is readable, then set `{review_mode}` = `"full"`.
- Otherwise, ask the user: **Is there a spec or story file that provides context for these changes?**
- If yes: set `{spec_file}` to the path provided, verify the file exists and is readable, then set `{review_mode}` = `"full"`. - If yes: set `{spec_file}` to the path provided, verify the file exists and is readable, then set `{review_mode}` = `"full"`.
- If no: set `{review_mode}` = `"no-spec"`. - If no: set `{review_mode}` = `"no-spec"`.

View File

@ -32,16 +32,19 @@ Hand `{spec_file}` to a sub-agent/task and let it implement. If no sub-agents ar
### Tests ### Tests
**This is mandatory, not optional.** After implementation, write tests for every new or modified behavior. Follow the project's testing conventions discovered from `{project_context}`, CLAUDE.md, or the existing test suite. **This is mandatory, not optional.** After implementation, write tests for every new or modified behavior. Follow the project's testing conventions discovered from `{project_context}`, CLAUDE.md, the existing test suite, and any configured test tooling.
1. **Discover conventions.** Identify the project's test framework, file-naming patterns, and test directory structure from existing tests. If no existing tests exist, use the project's language-idiomatic defaults. 1. **Discover conventions.** Identify the project's test framework, file-naming patterns, and test directory structure in this order:
- First, inspect existing tests.
- If existing tests are missing or insufficient, inspect the repo's configured test tooling: test scripts, dependencies/devDependencies, and test config files (for example `package.json`, `pytest.ini`, `pyproject.toml`, `jest.config.*`, `vitest.config.*`, `mocha` config, `rspec` config, `go test` conventions, etc.).
- Only if neither existing tests nor configured tooling establish conventions should you fall back to the project's language-idiomatic defaults.
2. **Write tests.** Cover: 2. **Write tests.** Cover:
- Happy-path behavior for each new or changed feature. - Happy-path behavior for each new or changed feature.
- Edge cases and error scenarios from the I/O & Edge-Case Matrix (if present in the spec). - Edge cases and error scenarios from the I/O & Edge-Case Matrix (if present in the spec).
- Regressions — any behavior that could break due to the change. - Regressions — any behavior that could break due to the change.
3. **Run tests.** Execute the test suite. All new and existing tests must pass. If any test fails, fix the implementation or the test before proceeding. 3. **Run tests.** Execute the test suite. All new and existing tests must pass. If any test fails, fix the implementation or the test before proceeding.
If the project has no test infrastructure at all (no test framework, no test directory, no test scripts), note this in the spec under `## Verification` and skip — but this is the only acceptable reason to skip tests. If the project has no test infrastructure at all (no existing tests, no test framework, no relevant test config, no test directory, and no test scripts), note this in the spec under `## Verification` and skip — but this is the only acceptable reason to skip tests. Record this skip reason explicitly in the final summary output.
### Self-Check ### Self-Check

View File

@ -18,13 +18,16 @@ Implement the clarified intent directly.
### Tests ### Tests
**This is mandatory, not optional.** After implementation, write tests for every new or modified behavior. Follow the project's testing conventions discovered from `{project_context}`, CLAUDE.md, or the existing test suite. **This is mandatory, not optional.** After implementation, write tests for every new or modified behavior. Follow the project's testing conventions discovered from `{project_context}`, CLAUDE.md, the existing test suite, and any configured test tooling.
1. **Discover conventions.** Identify the project's test framework, file-naming patterns, and test directory structure from existing tests. If no existing tests exist, use the project's language-idiomatic defaults. 1. **Discover conventions.** Identify the project's test framework, file-naming patterns, and test directory structure in this order:
- First, inspect existing tests.
- If existing tests are missing or insufficient, inspect the repo's configured test tooling: test scripts, dependencies/devDependencies, and test config files (for example `package.json`, `pytest.ini`, `pyproject.toml`, `jest.config.*`, `vitest.config.*`, `mocha` config, `rspec` config, `go test` conventions, etc.).
- Only if neither existing tests nor configured tooling establish conventions should you fall back to the project's language-idiomatic defaults.
2. **Write tests.** Cover happy-path behavior, edge cases, and regressions. 2. **Write tests.** Cover happy-path behavior, edge cases, and regressions.
3. **Run tests.** All new and existing tests must pass. Fix failures before proceeding. 3. **Run tests.** All new and existing tests must pass. Fix failures before proceeding.
If the project has no test infrastructure at all (no test framework, no test directory, no test scripts), skip — but this is the only acceptable reason to skip tests. If the project has no test infrastructure at all (no existing tests, no test framework, no relevant test config, no test directory, and no test scripts), skip — but this is the only acceptable reason to skip tests. Record this skip reason explicitly in the final summary output.
### Review ### Review