diff --git a/skills/chatgpt-review/scripts/lib/prompt.mjs b/skills/chatgpt-review/scripts/lib/prompt.mjs index 33d97e60..bf584d78 100644 --- a/skills/chatgpt-review/scripts/lib/prompt.mjs +++ b/skills/chatgpt-review/scripts/lib/prompt.mjs @@ -54,15 +54,44 @@ export function buildPrompt({ mode, target, context = '', publish = false, pass } export function extractReportedMetadata(text) { - const shaPatterns = [ - /\bpass[-\s]?\d+\s+reviewed\s+(?:head(?:\s+sha)?|sha)\s*:\s*`?([0-9a-f]{40})\b/i, - /\b(?:current|new|updated|latest)\s+(?:reviewed\s+)?(?:head(?:\s+sha)?|sha)\s*:\s*`?([0-9a-f]{40})\b/i, - /^(?!\s*(?:previous(?:ly)?|prior|old|earlier)\b).*?\breviewed\s+(?:head(?:\s+sha)?|sha)\s*:\s*`?([0-9a-f]{40})\b/im, - ]; - const labelledSha = shaPatterns - .map((pattern) => text.match(pattern)?.[1]) - .find(Boolean); - const sha = (labelledSha ?? text.match(/\b[0-9a-f]{40}\b/i)?.[0])?.toLowerCase() ?? null; + // Issue #669 (plus a related word-order variant found live while fixing it) — + // ChatGPT's own phrasing for "which SHA did I just review" varies more than a + // small, fixed set of regexes can keep up with word-for-word. Real observed + // variants from actual /ship runs: "Pass-2 reviewed head: X" (pass-N before + // "reviewed"), "Reviewed pass-3 head: X" (pass-N between "reviewed" and + // "head"), either sometimes wrapped in Markdown emphasis ("**`X`**") between + // the label's colon and the backtick. Chasing each newly observed phrasing + // with one more regex is the same trap this project has already named + // elsewhere (build/lib/check-legacy-owners.mjs's #642 review-pass-1 comment, + // about hand-chasing trivia code points instead of matching what the grammar + // actually allows): every fix closes the one case it was written for and + // leaves the next phrasing variant open. Instead, find EVERY "[prefix] reviewed + // [pass-N] head/sha:" occurrence with one flexible pattern (word order and + // Markdown emphasis both tolerated), reject any whose OWN LINE also mentions a + // previous/prior/anchor/old/earlier qualifier (checked across the whole line, + // not only if that word happens to be first), and take the LAST surviving + // occurrence — every real multi-pass review observed states the earlier/anchor + // SHA before the current one, never after. Requiring the "reviewed ... head/ + // sha:" label (rather than just scanning for the last bare 40-hex string in + // the whole text) still avoids being fooled by an unrelated commit SHA + // mentioned later in the same message for some other reason. + const OLD_QUALIFIER = /\b(?:previous(?:ly)?|prior|anchor|old|earlier)\b/i; + const labelledShaPattern = /\b(?:(?:current|new|updated|latest|pass[-\s]?\d+)\s+)?reviewed\s+(?:pass[-\s]?\d+\s+)?(?:head(?:\s+sha)?|sha)\s*:[\s*_]*`?([0-9a-f]{40})\b/gi; + const candidates = [...text.matchAll(labelledShaPattern)].filter((match) => { + // Only the text BEFORE the match, back to the start of its own line — never + // after. A qualifier describing THIS SHA always precedes it ("Previously + // reviewed SHA:", "Reviewed anchor SHA:"); a paragraph frequently keeps + // multiple sentences on one physical line with no newline between them, and + // a later, unrelated sentence on that same line can independently mention + // one of these words (observed live: "...it is one commit ahead of the + // requested anchor." following a valid, unqualified match earlier in the + // same line) — including text after the match would reject a good match for + // a reason that has nothing to do with it. + const lineStart = text.lastIndexOf('\n', match.index) + 1; + const precedingText = text.slice(lineStart, match.index); + return !OLD_QUALIFIER.test(precedingText); + }); + const sha = (candidates.at(-1)?.[1] ?? text.match(/\b[0-9a-f]{40}\b/i)?.[0])?.toLowerCase() ?? null; const commentUrl = text.match(/https:\/\/github\.com\/[\w.-]+\/[\w.-]+\/(?:issues|pull)\/\d+#(?:issuecomment|pullrequestreview)-\d+/i)?.[0] ?? null; return { reportedReviewedSha: sha, reportedGithubCommentUrl: commentUrl }; } diff --git a/skills/chatgpt-review/tests/core.test.mjs b/skills/chatgpt-review/tests/core.test.mjs index 2bce999b..a516a324 100644 --- a/skills/chatgpt-review/tests/core.test.mjs +++ b/skills/chatgpt-review/tests/core.test.mjs @@ -161,6 +161,32 @@ test('plain reviewed-head labels exclude previous-head lines', () => { assert.equal(extractReportedMetadata(`Previously reviewed head: ${previous}\nReviewed head: ${current}`).reportedReviewedSha, current); }); +// Issue #669 — confirmed live on a real /ship code-review loop (issue #642, PR #668): +// ChatGPT wrapped its reported SHA in Markdown bold ("**``**") between the label's +// colon and the backtick, which defeated every labelled pattern and silently fell back to +// the wrong (earlier-mentioned) SHA. +test('a Markdown-bolded reviewed-head SHA is still extracted over an earlier bolded SHA', () => { + const previous = 'a'.repeat(40); + const current = 'b'.repeat(40); + const text = `Previously reviewed SHA: **\`${previous}\`**\nPass-2 reviewed head: **\`${current}\`**. The PR remained on that head.`; + assert.equal(extractReportedMetadata(text).reportedReviewedSha, current); +}); + +// Issue #669 follow-up — found live in the SAME pass-3 review this fix was verified +// against: a different word order ("Reviewed pass-3 head:" — "reviewed" before the pass +// number, rather than "Pass-2 reviewed head:" — pass number before "reviewed") that the +// original fix for the Markdown-bold case alone did not cover. Also exercises the +// qualifier-word-after-the-match trap: the real text's second sentence, on the SAME +// physical line as the valid match with no newline between them, happens to end with +// "...ahead of the requested anchor." — an unrelated later use of a qualifier word that +// must NOT retroactively invalidate the match that precedes it. +test('a different reviewed-head word order is extracted, and a later unrelated qualifier word on the same line does not invalidate it', () => { + const previous = 'c'.repeat(40); + const current = 'd'.repeat(40); + const text = `Reviewed anchor SHA: **\`${previous}\`**\nReviewed pass-3 head: **\`${current}\`**. The PR head has not moved since pass 2; it is one commit ahead of the requested anchor.`; + assert.equal(extractReportedMetadata(text).reportedReviewedSha, current); +}); + test('sensitive paths and binary patches are rejected or stripped', () => { assert.equal(isSensitivePath('.env.local'), true); assert.equal(isSensitivePath('keys/id_ed25519'), true);