|
| 1 | +--- |
| 2 | +name: fix-findings |
| 3 | +description: Fix findings from the active review session — reads reviewer findings files, applies fixes by priority, and updates the resolution log. Use after pasting reviewer output into findings files. |
| 4 | +owner: chalk |
| 5 | +version: "1.0.0" |
| 6 | +metadata-version: "1" |
| 7 | +allowed-tools: Bash, Read, Edit, Grep, Glob, Write |
| 8 | +argument-hint: "[reviewer-name|all] e.g. codex, gemini, gpt4, or omit for all" |
| 9 | +--- |
| 10 | + |
| 11 | +# Fix Review Session Findings |
| 12 | + |
| 13 | +Read findings from the active review session and apply fixes to the codebase, then update the resolution log. |
| 14 | + |
| 15 | +## Step 1: Resolve the active session |
| 16 | + |
| 17 | +Look for the current session: |
| 18 | + |
| 19 | +1. Check `.chalk/reviews/.current-session` |
| 20 | +2. If not found, check for the most recent session directory under `.chalk/reviews/sessions/` |
| 21 | +3. If nothing found, stop and tell the user to run `/create-review` first to start a session |
| 22 | + |
| 23 | +Store the session ID as `{session}`. |
| 24 | + |
| 25 | +## Step 2: Determine which findings to load |
| 26 | + |
| 27 | +Based on `$ARGUMENTS`: |
| 28 | + |
| 29 | +- If a reviewer name is provided (e.g. `codex`, `gemini`, `gpt4`, `claude`), sanitize it to kebab-case and load only `.chalk/reviews/sessions/{session}/{reviewer}.findings.md` |
| 30 | +- If `all` or no argument → load all `*.findings.md` files in the session directory |
| 31 | + |
| 32 | +Discover available findings dynamically using the Glob tool: |
| 33 | + |
| 34 | +``` |
| 35 | +.chalk/reviews/sessions/{session}/*.findings.md |
| 36 | +``` |
| 37 | + |
| 38 | +Do NOT use shell `ls` for discovery — use the Glob tool to avoid command injection and handle special characters safely. |
| 39 | + |
| 40 | +If a findings file doesn't exist or contains only template placeholder text, skip it and note that no findings are available from that reviewer. |
| 41 | + |
| 42 | +If no findings are available from any reviewer, stop and tell the user to paste reviewer output into the findings files first. Show the expected path: `.chalk/reviews/sessions/{session}/{reviewer-name}.findings.md` |
| 43 | + |
| 44 | +## Step 3: Parse findings |
| 45 | + |
| 46 | +From each findings file, extract the findings table. Each row typically has: |
| 47 | + |
| 48 | +- **ID** — finding identifier (e.g. `F-001`, `G-001`, or any prefix) |
| 49 | +- **Severity** — `P0`, `P1`, `P2`, `P3` |
| 50 | +- **File:Line** — source location (e.g. `src/main/auth.ts:172`) |
| 51 | +- **Issue** — description of the problem |
| 52 | +- **Failure mode** or **Impact** — what goes wrong |
| 53 | +- **Recommended fix** or **Suggested fix** — how to fix it |
| 54 | +- **Confidence** — reviewer confidence score (if present) |
| 55 | + |
| 56 | +Sort all findings across all reviewers by severity: P0 > P1 > P2 > P3. |
| 57 | + |
| 58 | +Deduplicate findings that target the same file and line (or overlapping line range) across reviewers. Two findings are duplicates if they reference the same file path AND the same line number (or lines within 5 of each other) AND describe a similar category of issue. When deduplicating, keep all IDs, use the more detailed description, and note both reviewers as sources. |
| 59 | + |
| 60 | +## Step 4: Present findings and get confirmation |
| 61 | + |
| 62 | +Before applying any fixes, present the user with a prioritized summary: |
| 63 | + |
| 64 | +| Priority | ID(s) | Source | File:Line | Issue (1-line) | Action | |
| 65 | +|----------|--------|--------|-----------|----------------|--------| |
| 66 | +| P0 | G-001 | gemini | src/main/auth.ts:31 | Plaintext token storage | Fix | |
| 67 | +| P1 | F-001 | codex | src/main/index.ts:182 | Stale auth target | Fix | |
| 68 | +| ... | ... | ... | ... | ... | ... | |
| 69 | + |
| 70 | +Ask the user which findings to fix. Options: |
| 71 | +- **All** — fix everything P0-P2 (skip P3 unless trivial) |
| 72 | +- **Blocking only** — fix P0 and P1 only |
| 73 | +- **Let me choose** — user picks specific finding IDs |
| 74 | + |
| 75 | +## Step 5: Apply fixes |
| 76 | + |
| 77 | +For each finding to fix (in priority order): |
| 78 | + |
| 79 | +1. **Validate the file path** — confirm the path is relative and within the repository root. Reject any absolute paths or paths containing `..` that escape the repo |
| 80 | +2. **Read the file** at the specified path and line — read at least 30 lines of surrounding context (15 above, 15 below) to understand the code structure |
| 81 | +3. **Design the fix** using the reviewer's suggested fix as guidance, but verify it makes sense in context |
| 82 | +4. **Show the proposed fix to the user and ask for explicit confirmation before applying it** |
| 83 | +5. **Apply the fix** using Edit tool |
| 84 | +6. **Verify** the fix doesn't break surrounding code or introduce new issues |
| 85 | + |
| 86 | +Rules: |
| 87 | +- Do NOT blindly copy suggested fixes — they're guidance, not exact patches |
| 88 | +- If a fix requires changes across multiple files, make all related changes together |
| 89 | +- If a fix is unclear or would require significant refactoring beyond the finding's scope, skip it and mark as `deferred` in the resolution log |
| 90 | +- For P3 findings: only fix if the change is a single-line or obvious one-liner (e.g. rename, add a null check, fix a typo). Otherwise skip |
| 91 | +- After fixing deduplicated findings, note all IDs as resolved |
| 92 | + |
| 93 | +## Step 6: Update the resolution log |
| 94 | + |
| 95 | +Write or update `.chalk/reviews/sessions/{session}/resolution.md`. |
| 96 | + |
| 97 | +If the file already exists, preserve any metadata comments at the top. Fill in: |
| 98 | + |
| 99 | +```markdown |
| 100 | +# Finding Resolution Log |
| 101 | + |
| 102 | +## Summary |
| 103 | +- Session: {session} |
| 104 | +- Item: {inferred from session name} |
| 105 | +- Reviewers: {list of reviewer sources found — e.g. codex, gemini} |
| 106 | +- Decision owner: |
| 107 | + |
| 108 | +## Findings |
| 109 | + |
| 110 | +| ID | Severity | Source | File:Line | Decision | Notes | |
| 111 | +|---|---|---|---|---|---| |
| 112 | +| G-001 | P0 | gemini | src/main/auth.ts:31 | fixed | Wrapped token persistence with safeStorage | |
| 113 | +| F-001 | P1 | codex | src/main/index.ts:182 | fixed | Rebind auth target in createWindow | |
| 114 | + |
| 115 | +Decision values: |
| 116 | +- fixed |
| 117 | +- accepted-risk |
| 118 | +- deferred |
| 119 | +- not-repro |
| 120 | + |
| 121 | +## Follow-up Tasks |
| 122 | +- {deferred work, test gaps, items needing manual verification} |
| 123 | + |
| 124 | +## Final Gate |
| 125 | +- Build: |
| 126 | +- Tests: |
| 127 | +- Ready to merge: yes/no |
| 128 | +``` |
| 129 | + |
| 130 | +## Step 7: Summary |
| 131 | + |
| 132 | +After all fixes are applied, show: |
| 133 | + |
| 134 | +1. A results table: |
| 135 | + |
| 136 | +| ID(s) | Severity | Source | Status | What was done | |
| 137 | +|--------|----------|--------|--------|---------------| |
| 138 | +| G-001 | P0 | gemini | Fixed | Wrapped token persistence with safeStorage | |
| 139 | +| F-001 | P1 | codex | Fixed | Rebind auth target in createWindow | |
| 140 | +| G-004 | P3 | gemini | Skipped | Trivial but not blocking | |
| 141 | + |
| 142 | +2. The resolution log path |
| 143 | +3. Suggest next steps: |
| 144 | + - Run build/tests to verify nothing broke |
| 145 | + - Run `/commit` to commit the fixes |
| 146 | + - If all blocking findings are resolved, create or update the PR |
| 147 | + |
| 148 | +## Rules |
| 149 | + |
| 150 | +- ALWAYS validate that file paths from findings are relative and within the repository root — reject absolute paths or paths with `..` escaping the repo |
| 151 | +- ALWAYS read the file with surrounding context before applying any fix |
| 152 | +- Do NOT modify files that aren't referenced in findings |
| 153 | +- Do NOT fix things that aren't in the findings — stay scoped |
| 154 | +- If multiple reviewers suggest conflicting fixes for the same issue, present both suggestions to the user and let them choose which approach to take |
| 155 | +- Keep fixes minimal — address the finding, don't refactor surrounding code |
| 156 | +- If build or typecheck breaks after a fix, attempt to resolve but note it in the resolution log |
0 commit comments