fix(pr-review): code review fixes — null-line guard, max iterationId, missing tests, MARKER_FOUND guard

- match-finding.mjs: skip threads where start/end is null instead of
  defaulting to line 0, which caused false positive matches near top of file
- detect-prior-review.mjs: use max() over all parsed iteration IDs instead of
  last-seen; thread insertion order no longer affects priorIterationId
- review-pr.md (Step 3.5): add exit-code guard and value validation for
  MARKER_FOUND to prevent silent wrong-direction fallback on node failure
- tests: add 6 new cases (31 total) covering null-end classifyThread,
  deleted-file [0,0] hunks, numeric ADO status 2/5, null-start/end matchFinding,
  and priorIterationId max-ordering in detectPriorReview

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>

Author: orioltf

apps/claude-code/pr-review/commands/review-pr.md

@@ -193,7 +193,12 @@ const prefix = '✅ Review complete — Iteration ' + process.env.PID
 const found = threads.some(t => t.threadId === sid && (t.comments ?? []).some(c => (c.content ?? '').startsWith(prefix)))
 console.log(found ? 'true' : 'false')
 EOJS
-  )
+  ) || { echo "ERROR: partial-run check script failed — falling back to first-review mode for safety."; MARKER_FOUND="false"; }
+
+  if [ "$MARKER_FOUND" != "true" ] && [ "$MARKER_FOUND" != "false" ]; then
+    echo "ERROR: unexpected MARKER_FOUND value '${MARKER_FOUND}' — falling back to first-review mode for safety."
+    MARKER_FOUND="false"
+  fi
 
   if [ "$MARKER_FOUND" = "false" ]; then
     echo "No completion marker for Iteration $PRIOR_ITERATION_ID — partial prior run. Falling back to first-review mode."

apps/claude-code/pr-review/scripts/re-review/detect-prior-review.mjs

@@ -41,12 +41,15 @@ export function detectPriorReview({ threads, signaturePrefix }) {
 		isSummaryThread: isSummaryCandidate && t.threadId === maxSummaryId,
 	}))
 
-	// Last "Iteration N" seen across all bot comments in thread order
+	// Highest "Iteration N" seen across all bot comments — use max, not last-seen,
+	// so thread insertion order does not affect the result.
 	let priorIterationId = null
 	for (const t of priorThreads) {
 		for (const c of t.comments) {
 			const parsed = parseSignature(c.content ?? '')
-			if (parsed) priorIterationId = parsed.iterationId
+			if (parsed && (priorIterationId === null || parsed.iterationId > priorIterationId)) {
+				priorIterationId = parsed.iterationId
+			}
 		}
 	}
 

apps/claude-code/pr-review/scripts/re-review/match-finding.mjs

@@ -22,8 +22,9 @@ export function matchFinding({ finding, priorThreads, driftLines = 3 }) {
 	for (const t of priorThreads) {
 		if (t.isSummaryThread) continue
 		if (t.filePath !== filePath) continue
-		const ts = (t.start?.line ?? 0) - driftLines
-		const te = (t.end?.line ?? 0) + driftLines
+		if (t.start == null || t.end == null) continue
+		const ts = t.start.line - driftLines
+		const te = t.end.line + driftLines
 		if (Math.max(fs, ts) <= Math.min(fe, te)) return t
 	}
 	return null

apps/claude-code/pr-review/tests/classify-thread.test.mjs

@@ -110,4 +110,60 @@ describe('classifyThread', () => {
 		const result = classifyThread({ thread, diffHunks: [], signaturePrefix: SIGNATURE_PREFIX })
 		assert.equal(result, 'pending')
 	})
+
+	it('thread with null end, file changed in diff → pending (intersection requires both start and end)', () => {
+		/** @type {import('../scripts/re-review/classify-thread.mjs').PriorThread} */
+		const thread = {
+			threadId: 102,
+			filePath: '/src/feature.ts',
+			start: { line: 42 },
+			end: null,
+			comments: [{ content: `Finding.\n---\n${SIGNATURE_PREFIX} — Iteration 1` }],
+			status: 'active',
+		}
+		const result = classifyThread({ thread, diffHunks: withChangesDiff, signaturePrefix: SIGNATURE_PREFIX })
+		assert.equal(result, 'pending')
+	})
+
+	it('file present in diff with all-zero hunks (deleted) → obsolete', () => {
+		/** @type {import('../scripts/re-review/classify-thread.mjs').PriorThread} */
+		const thread = {
+			threadId: 103,
+			filePath: '/src/deleted.ts',
+			start: { line: 5 },
+			end: { line: 5 },
+			comments: [{ content: `Finding.\n---\n${SIGNATURE_PREFIX} — Iteration 1` }],
+			status: 'active',
+		}
+		/** @type {import('../scripts/re-review/classify-thread.mjs').DiffHunk[]} */
+		const hunks = [{ filePath: '/src/deleted.ts', startLine: 0, endLine: 0 }]
+		const result = classifyThread({ thread, diffHunks: hunks, signaturePrefix: SIGNATURE_PREFIX })
+		assert.equal(result, 'obsolete')
+	})
+
+	it('numeric status 2 (wontFix) → addressed', () => {
+		/** @type {import('../scripts/re-review/classify-thread.mjs').PriorThread} */
+		const thread = {
+			threadId: 104,
+			filePath: '/src/api.ts',
+			start: { line: 42 },
+			end: { line: 42 },
+			comments: [{ content: `Finding.\n---\n${SIGNATURE_PREFIX} — Iteration 1` }],
+			status: 2,
+		}
+		assert.equal(classifyThread({ thread, diffHunks: noChangeDiff, signaturePrefix: SIGNATURE_PREFIX }), 'addressed')
+	})
+
+	it('numeric status 5 (byDesign) → addressed', () => {
+		/** @type {import('../scripts/re-review/classify-thread.mjs').PriorThread} */
+		const thread = {
+			threadId: 105,
+			filePath: '/src/api.ts',
+			start: { line: 42 },
+			end: { line: 42 },
+			comments: [{ content: `Finding.\n---\n${SIGNATURE_PREFIX} — Iteration 1` }],
+			status: 5,
+		}
+		assert.equal(classifyThread({ thread, diffHunks: noChangeDiff, signaturePrefix: SIGNATURE_PREFIX }), 'addressed')
+	})
 })

apps/claude-code/pr-review/tests/detect-prior-review.test.mjs

@@ -49,6 +49,28 @@ describe('detectPriorReview', () => {
 		assert.notEqual(result.summaryThread, null)
 	})
 
+	it('priorIterationId is highest iteration seen, regardless of thread order', () => {
+		/** @type {import('../scripts/re-review/detect-prior-review.mjs').RawADOThread[]} */
+		const threads = [
+			{
+				id: 1,
+				threadContext: null,
+				comments: [{ content: `## PR Review Summary\n\nSummary.\n---\n${SIGNATURE_PREFIX} — Iteration 2` }],
+				status: 'active',
+			},
+			{
+				id: 2,
+				threadContext: { filePath: '/src/api.ts', rightFileStart: { line: 5 }, rightFileEnd: { line: 5 } },
+				comments: [{ content: `Finding.\n---\n${SIGNATURE_PREFIX} — Iteration 1` }],
+				status: 'active',
+			},
+		]
+		const result = detectPriorReview({ threads, signaturePrefix: SIGNATURE_PREFIX })
+		// Summary thread (Iteration 2) appears before inline thread (Iteration 1)
+		// priorIterationId must be 2 (max), not 1 (last-seen in array order)
+		assert.equal(result.priorIterationId, 2)
+	})
+
 	it('highest threadId wins when multiple summary candidates present', () => {
 		/** @type {import('../scripts/re-review/detect-prior-review.mjs').RawADOThread[]} */
 		const threads = [

apps/claude-code/pr-review/tests/match-finding.test.mjs

@@ -67,4 +67,11 @@ describe('matchFinding', () => {
 		const result = matchFinding({ finding, priorThreads, driftLines: 0 })
 		assert.equal(result, null)
 	})
+
+	it('thread with null start/end does not match any line-based finding', () => {
+		const finding = { filePath: '/src/api.ts', startLine: 1, endLine: 3 }
+		const priorThreads = [makeThread(9, '/src/api.ts', null, null)]
+		const result = matchFinding({ finding, priorThreads })
+		assert.equal(result, null)
+	})
 })