Compare commits

...

20 Commits

Author SHA1 Message Date
Alex Verkhovsky b73670700b docs(code-review): expand story file validation to include empty and malformed files 2026-01-05 07:39:56 -08:00
Alex Verkhovsky 5a16c3a102 fix(code-review): halt on git command failure instead of silently treating as NO_GIT 2026-01-05 07:27:12 -08:00
Alex Verkhovsky 58e0b6a634 docs(code-review): add generic error handling for git commands 2026-01-05 07:26:50 -08:00
Alex Verkhovsky 2785d382d5 docs(code-review): use {sprint_status} variable instead of expanded path 2026-01-05 07:24:11 -08:00
Alex Verkhovsky 551a2ccb53 docs(code-review): use variable reference for sprint-status path 2026-01-05 07:21:44 -08:00
Alex Verkhovsky 3fc411d9c9 docs(code-review): clarify sprint_status file definition and location 2026-01-05 07:19:31 -08:00
Alex Verkhovsky ec30b580e7 refactor(code-review): use Skip to for flow control directive in substep 4
Skip to substep 5 correctly communicates jumping past the rest of the git
discovery logic in substep 4 when git repo is not found. Proceed would
suggest normal sequential flow, but we are skipping the conditional branch.

🤖 Generated with [Claude Code](https://claude.com/claude-code)

Co-Authored-By: Claude Haiku 4.5 <noreply@anthropic.com>
2026-01-05 07:13:41 -08:00
Alex Verkhovsky 9e6e991b53 fix(code-review): correct flow control directive in substep 4
Changed "Skip to substep 6" (which does not exist) to "Proceed to substep 5".
Step only has 5 substeps. After setting NO_GIT flag, workflow continues to
substep 5 (Cross-Reference Story vs Git), not to a non-existent substep 6.

Fixes h2 finding from adversarial review.

🤖 Generated with [Claude Code](https://claude.com/claude-code)

Co-Authored-By: Claude Haiku 4.5 <noreply@anthropic.com>
2026-01-05 07:09:56 -08:00
Alex Verkhovsky dbdaae1be7 refactor(code-review): remove NEXT directive from completion checklist
The checklist validates work done DURING step execution.
The NEXT directive is OUTPUT of completion, not a validation criterion.
It happens AFTER the checklist passes, so it does not belong there.

🤖 Generated with [Claude Code](https://claude.com/claude-code)

Co-Authored-By: Claude Haiku 4.5 <noreply@anthropic.com>
2026-01-05 06:34:33 -08:00
Alex Verkhovsky 1636bd5a55 refactor(code-review): remove redundant 'immediately' from halt instruction
'Immediately' is implied by HALT. No timing choice exists.

🤖 Generated with [Claude Code](https://claude.com/claude-code)

Co-Authored-By: Claude Haiku 4.5 <noreply@anthropic.com>
2026-01-05 06:33:05 -08:00
Alex Verkhovsky 53045d35b1 refactor(code-review): move NEXT STEP DIRECTIVE after COMPLETION CHECKLIST
Logical flow: verify checklist → then declare next step
Not: declare next step → then verify checklist

🤖 Generated with [Claude Code](https://claude.com/claude-code)

Co-Authored-By: Claude Haiku 4.5 <noreply@anthropic.com>
2026-01-05 06:31:19 -08:00
Alex Verkhovsky b3643af6dc refactor(code-review): remove redundancy and clarify halt instruction
- Remove redundant "Do NOT proceed to the next step" (halt already means this)
- Change "item" to "criterion" (more precise terminology)

🤖 Generated with [Claude Code](https://claude.com/claude-code)

Co-Authored-By: Claude Haiku 4.5 <noreply@anthropic.com>
2026-01-05 06:30:31 -08:00
Alex Verkhovsky 4ba6e19303 refactor(code-review): rename SUCCESS METRICS to COMPLETION CHECKLIST
Correct terminology:
- "Metrics" implies quantitative measurement
- These are actually pass/fail criteria for step completion
- Section is self-validation checklist, not measurement data

Reframe as checkpoint before proceeding to next step:
- Add "Before proceeding to the next step, verify ALL of the following:"
- Change "If any metric" to "If any item"
- Explicit instruction: "Do NOT proceed to the next step" if checklist fails

🤖 Generated with [Claude Code](https://claude.com/claude-code)

Co-Authored-By: Claude Haiku 4.5 <noreply@anthropic.com>
2026-01-05 06:29:52 -08:00
Alex Verkhovsky 38ab12da85 refactor(code-review): remove cargo cult failure modes repetition from step-01
FAILURE MODES section was just inverted SUCCESS METRICS. Not valuable.
Replaced with single catch-all statement: failure to meet any success metric = failure.

Let actual failure modes emerge from usage patterns, not speculation.

🤖 Generated with [Claude Code](https://claude.com/claude-code)

Co-Authored-By: Claude Haiku 4.5 <noreply@anthropic.com>
2026-01-05 06:17:37 -08:00
Alex Verkhovsky 0ae6799cb6 refactor(code-review): remove project context loading from step-01
Step-01 focus is: load story + discover git changes. Nothing else.

Project context loading belongs in step-04 (Context-Aware Review) where it
provides audit rules, principles, and requirements for validating AC
implementation against project standards.

(See implementation-notes.md for detail)

🤖 Generated with [Claude Code](https://claude.com/claude-code)

Co-Authored-By: Claude Haiku 4.5 <noreply@anthropic.com>
2026-01-05 06:04:34 -08:00
Alex Verkhovsky e479b4164c refactor(code-review): add checkpoint for empty git changes and exclude ignored files
Step-01 substeps 5:
- If no git changes detected: halt and ask user "Continue anyway?"
  Allows AC audit on story File List even if no code changes in git
- Exclude git-ignored files from discrepancy comparison
  Prevents false positives if story modified only ignored files

🤖 Generated with [Claude Code](https://claude.com/claude-code)

Co-Authored-By: Claude Haiku 4.5 <noreply@anthropic.com>
2026-01-05 06:02:17 -08:00
Alex Verkhovsky 71a1c325f7 refactor(code-review): add rename detection to git change discovery
Step-01 substep-4:
- Use git diff -M to detect renamed/moved files
- Include deleted, renamed files in git_changed_files
- Adversarial reviewer needs to see deletions (e.g., critical code removed)
- Downstream steps will handle these appropriately (documented in implementation-notes)

🤖 Generated with [Claude Code](https://claude.com/claude-code)

Co-Authored-By: Claude Haiku 4.5 <noreply@anthropic.com>
2026-01-05 05:54:32 -08:00
Alex Verkhovsky 59c58b2e2c refactor(code-review): clean up step-01 substep 3 and add error handling
Substep 3 (Extract File List):
- Removed repetitive wording
- Reference {story_content} variable instead of generic "story file"
- Add error handling: if Dev Agent Record/File List not found, set story_file_list = NO_FILE_LIST
- Consistent with NO_GIT pattern used elsewhere

🤖 Generated with [Claude Code](https://claude.com/claude-code)

Co-Authored-By: Claude Haiku 4.5 <noreply@anthropic.com>
2026-01-05 05:47:46 -08:00
Alex Verkhovsky 18ac3c931a refactor(code-review): audit step-01 substeps and success/failure criteria
Step 01 audit findings:
- Substep 3 was extracting items not needed by step-01 (ACs, tasks, changelog)
  Trimmed to only extract story_file_list (needed for git comparison)
- Success/failure criteria now explicitly guard story_content completeness
  since downstream steps depend on the full file content
- Removed "downstream" jargon in favor of "later steps"

🤖 Generated with [Claude Code](https://claude.com/claude-code)

Co-Authored-By: Claude Haiku 4.5 <noreply@anthropic.com>
2026-01-05 05:42:04 -08:00
Alex Verkhovsky 8fc7db7b97 refactor(code-review): remove implementation notes from step-01
Implementation notes for the workflow should be collected in a dedicated
implementation-notes.md file, not embedded in step files. This keeps each
step focused and defers editorial comments to a separate tracking document.

🤖 Generated with [Claude Code](https://claude.com/claude-code)

Co-Authored-By: Claude Haiku 4.5 <noreply@anthropic.com>
2026-01-05 05:33:27 -08:00
1 changed files with 41 additions and 42 deletions

View File

@ -13,6 +13,7 @@ These variables MUST be set in this step and available to all subsequent steps:
- `story_path` - Path to the story file being reviewed
- `story_key` - Story identifier (e.g., "1-2-user-authentication")
- `story_content` - Complete, unmodified file content from story_path (loaded in substep 2)
- `story_file_list` - Files claimed in story's Dev Agent Record → File List
- `git_changed_files` - Files actually changed according to git
- `git_discrepancies` - Mismatches between `story_file_list` and `git_changed_files`
@ -27,13 +28,13 @@ Ask user: "Which story would you like to review?"
**Try input as direct file path first:**
If input resolves to an existing file:
- Verify it's in `sprint_status` with status `review` or `done`
- Verify it's in {sprint_status} with status `review` or `done`
- If verified → set `story_path` to that file path
- If NOT verified → Warn user the file is not in sprint_status (or wrong status). Ask: "Continue anyway?"
- If NOT verified → Warn user the file is not in {sprint_status} (or wrong status). Ask: "Continue anyway?"
- If yes → set `story_path`
- If no → return to user prompt (ask "Which story would you like to review?" again)
**Search sprint_status** (if input is not a direct file):
**Search {sprint_status}** (if input is not a direct file):
Search for stories with status `review` or `done`. Match by priority:
1. Story number resembles input closely enough (e.g., "1-2" matches "1 2", "1.2", "one dash two", "one two"; "1-32" matches "one thirty two"). Do NOT match if numbers differ (e.g., "1-33" does not match "1-32")
2. Exact story name/key (e.g., "1-2-user-auth-api")
@ -47,50 +48,69 @@ Search for stories with status `review` or `done`. Match by priority:
### 2. Load Story File
- Read COMPLETE story file from {story_path}
- Extract `story_key` from filename (e.g., "1-2-user-authentication.md" → "1-2-user-authentication") or story metadata
**Load file content:**
Read the complete contents of {story_path} and assign to `story_content` WITHOUT filtering, truncating or summarizing. If {story_path} cannot be read, is empty, or obviously doesn't have the story: report the error to the user and HALT the workflow.
### 3. Parse Story Sections
**Extract story identifier:**
Verify the filename ends with `.md` extension. Remove `.md` to get `story_key` (e.g., "1-2-user-authentication.md" → "1-2-user-authentication"). If filename doesn't end with `.md` or the result is empty: report the error to the user and HALT the workflow.
Extract and store:
### 3. Extract File List from Story
- **Story**: Title, description, status
- **Acceptance Criteria**: All ACs with their requirements
- **Tasks/Subtasks**: All tasks with completion status ([x] vs [ ])
- **Dev Agent Record → File List**: Claimed file changes
- **Change Log**: History of modifications
Extract `story_file_list` from the Dev Agent Record → File List section of {story_content}.
Set `story_file_list` = list of files from Dev Agent Record → File List
**If Dev Agent Record or File List section not found:** Report to user and set `story_file_list` = NO_FILE_LIST.
### 4. Discover Git Changes
Check if git repository exists.
**If NOT a git repo:** Set `git_changed_files` = NO_GIT, `git_discrepancies` = NO_GIT. Skip to substep 6.
**If NOT a git repo:** Set `git_changed_files` = NO_GIT, `git_discrepancies` = NO_GIT. Skip to substep 5.
**If git repo detected:**
```bash
git status --porcelain
git diff --name-only
git diff --cached --name-only
git diff -M --name-only
git diff -M --cached --name-only
```
Compile `git_changed_files` = union of modified, staged, and new files.
If any git command fails: Report the error to the user and HALT the workflow.
Compile `git_changed_files` = union of modified, staged, new, deleted, and renamed files.
### 5. Cross-Reference Story vs Git
Compare {story_file_list} with {git_changed_files}:
**If {git_changed_files} is empty:**
Ask user: "No git changes detected. Continue anyway?"
- If **no**: HALT the workflow
- If **yes**: Continue to comparison
**Compare {story_file_list} with {git_changed_files}:**
Exclude git-ignored files from the comparison (run `git check-ignore` if needed).
Set `git_discrepancies` with categories:
- **files_in_git_not_story**: Files changed in git but not in story File List
- **files_in_story_not_git**: Files in story File List but no git changes
- **files_in_story_not_git**: Files in story File List but no git changes (excluding git-ignored)
- **uncommitted_undocumented**: Uncommitted changes not tracked in story
### 6. Load Project Context
---
- Load {project_context} if exists (**/project-context.md) for coding standards
## COMPLETION CHECKLIST
Before proceeding to the next step, verify ALL of the following:
- `story_path` identified and loaded
- `story_key` extracted
- `story_content` captured completely and unmodified
- `story_file_list` compiled from Dev Agent Record (or NO_FILE_LIST if not found)
- `git_changed_files` discovered via git commands (or NO_GIT if not a git repo)
- `git_discrepancies` calculated
**If any criterion is not met:** Report to the user and HALT the workflow.
---
@ -100,24 +120,3 @@ Set `git_discrepancies` with categories:
"**NEXT:** Loading `step-02-build-attack-plan.md`"
---
## SUCCESS METRICS
- `story_path` identified and loaded
- `story_key` extracted
- All story sections parsed
- `story_file_list` compiled from Dev Agent Record
- `git_changed_files` discovered via git commands
- `git_discrepancies` calculated
- `project_context` loaded if exists
- Explicit NEXT directive provided
## FAILURE MODES
- Proceeding without story file loaded
- Missing `story_key` extraction
- Not parsing all story sections
- Skipping git change discovery
- Not calculating discrepancies
- No explicit NEXT directive at step completion