Skip to content

Commit f3518d1

Browse files
igerberclaude
andcommitted
Address CI review findings: secret scan, block parser, tiered matching, pricing
P0: Expand secret scan (Step 3c) to cover full source files, import-expanded files, and --include-files — not just diff hunks P1: Rewrite parse_review_findings() as block-based parser supporting multi-line Severity/Impact/Concrete-fix format. Add fail-safe: when parsing yields zero findings but severity markers exist, preserve prior findings instead of marking all as addressed. Return type changed to tuple[list[dict], bool]. P1: Implement tiered finding matching — primary key uses (severity, file_basename, summary[:50]) when file path available; fallback uses (severity, summary[:50]) only for findings without file paths and only with unique candidates. Prevents cross-file misattribution while still handling missing locations. P1: Fix estimate_cost() to match longest model prefix first, so gpt-4.1-mini and o3-mini snapshots get correct mini-tier pricing instead of parent. P1: Add branch/base validation to review-state.json reuse in skill Step 4. Discard stale state when stored branch or base_ref doesn't match current. P2: In delta mode, derive source/import context from delta changed-files list instead of full branch changed-files. P2: Add 7 regression tests: multi-line parser, parse uncertainty flag, same-summary-different-files, mini pricing, o3-mini pricing, delta context derivation, branch/base validation support. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
1 parent 68eb149 commit f3518d1

3 files changed

Lines changed: 418 additions & 164 deletions

File tree

.claude/commands/ai-review-local.md

Lines changed: 58 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -156,15 +156,71 @@ Options:
156156

157157
If user selects abort, clean up temp files and stop. If continue, proceed.
158158

159+
### Step 3c: Full-File Secret Scan (for standard/deep context)
160+
161+
When `--context` is not `minimal`, full source files will be uploaded to OpenAI. The diff-based scan in Step 3b only covers changed lines, so scan the full content of every file that will be transmitted:
162+
163+
```bash
164+
# Category 1: Changed diff_diff/ source files (standard/deep)
165+
upload_scan_files=""
166+
if [ "$context_level" != "minimal" ]; then
167+
upload_scan_files=$(git diff --name-only "${comparison_ref}...HEAD" | grep "^diff_diff/.*\.py$" || true)
168+
fi
169+
170+
# Category 2: All diff_diff/*.py for deep mode (conservative superset of import expansion)
171+
if [ "$context_level" = "deep" ]; then
172+
upload_scan_files=$(find diff_diff -name "*.py" -not -path "*/__pycache__/*" 2>/dev/null | sort -u)
173+
fi
174+
175+
# Category 3: --include-files (resolve paths same as script does)
176+
if [ -n "$include_files" ]; then
177+
for f in $(echo "$include_files" | tr ',' ' '); do
178+
if [ -f "diff_diff/$f" ]; then upload_scan_files="$upload_scan_files diff_diff/$f"
179+
elif [ -f "$f" ]; then upload_scan_files="$upload_scan_files $f"
180+
fi
181+
done
182+
fi
183+
184+
# Scan using same canonical content patterns from Step 3b (never echo secret values)
185+
if [ -n "$upload_scan_files" ]; then
186+
secret_hits=$(grep -rlE "(AKIA[A-Z0-9]{16}|ghp_[a-zA-Z0-9]{36}|sk-[a-zA-Z0-9]{48}|gho_[a-zA-Z0-9]{36}|[Aa][Pp][Ii][_-]?[Kk][Ee][Yy][[:space:]]*[=:]|[Ss][Ee][Cc][Rr][Ee][Tt][_-]?[Kk][Ee][Yy][[:space:]]*[=:]|[Pp][Aa][Ss][Ss][Ww][Oo][Rr][Dd][[:space:]]*[=:]|[Pp][Rr][Ii][Vv][Aa][Tt][Ee][_-]?[Kk][Ee][Yy]|[Bb][Ee][Aa][Rr][Ee][Rr][[:space:]]+[a-zA-Z0-9_-]+|[Tt][Oo][Kk][Ee][Nn][[:space:]]*[=:])" $upload_scan_files 2>/dev/null || true)
187+
sensitive_hits=$(echo "$upload_scan_files" | tr ' ' '\n' | grep -iE "(\.env|credentials|secret|\.pem|\.key|\.p12|\.pfx|id_rsa|id_ed25519)$" || true)
188+
fi
189+
```
190+
191+
If either `secret_hits` or `sensitive_hits` is non-empty, use AskUserQuestion:
192+
```
193+
Warning: Potential secrets detected in source files that would be uploaded to OpenAI
194+
(full-file scan, not just diff hunks):
195+
- <list filenames>
196+
197+
Options:
198+
1. Abort — review and remove secrets before retrying
199+
2. Continue — I confirm these are not real secrets
200+
```
201+
202+
If user selects abort, clean up temp files and stop. If continue, proceed.
203+
159204
### Step 4: Handle Re-Review State
160205

161-
Check for existing review state and generate delta diff if applicable:
206+
Check for existing review state and generate delta diff if applicable. **Validate that the
207+
stored state matches the current branch and base** before reusing — stale state from a
208+
different branch can contaminate re-review context.
162209

163210
```bash
164211
# Check for review-state.json
165212
if [ -f .claude/reviews/review-state.json ]; then
166-
# Read last_reviewed_commit from the JSON file
213+
# Read state fields
167214
last_reviewed_commit=$(python3 -c "import json; print(json.load(open('.claude/reviews/review-state.json')).get('last_reviewed_commit', ''))")
215+
stored_branch=$(python3 -c "import json; print(json.load(open('.claude/reviews/review-state.json')).get('branch', ''))")
216+
stored_base=$(python3 -c "import json; print(json.load(open('.claude/reviews/review-state.json')).get('base_ref', ''))")
217+
218+
# Validate branch/base match
219+
if [ "$stored_branch" != "$branch_name" ] || [ "$stored_base" != "$comparison_ref" ]; then
220+
echo "Warning: review-state.json is from branch '$stored_branch' (base: '$stored_base'), but current is '$branch_name' (base: '$comparison_ref'). Discarding stale state."
221+
rm -f .claude/reviews/review-state.json
222+
last_reviewed_commit=""
223+
fi
168224

169225
if [ -n "$last_reviewed_commit" ] && git cat-file -t "$last_reviewed_commit" >/dev/null 2>&1; then
170226
# SHA is reachable

0 commit comments

Comments
 (0)