Problem
extractReportedMetadata() in skills/chatgpt-review/scripts/lib/prompt.mjs uses three
labelled-SHA regexes to find "the reviewed head SHA" ChatGPT reports in its own response
text, falling back to text.match(/\b[0-9a-f]{40}\b/i)?.[0] (the first bare 40-hex-char
string anywhere in the text) when none of the three match.
None of the three labelled patterns tolerate Markdown emphasis (**...**) between the
label/colon and the backtick-wrapped SHA. When ChatGPT writes, e.g.:
Previously reviewed SHA: **`650378dac12120d1618644f65659bee37b377e6e`**
Pass-2 reviewed head: **`a2318aa2612ab65f1f059649ad51506744b667d4`**.
— a completely reasonable, naturally-occurring formatting choice for a multi-pass PR
review response — all three patterns fail to match (their \s*:\s* cannot cross the **
characters), so extraction falls back to "first 40-hex string in the text," which is the
previous/older SHA mentioned earlier in the message, not the actual current head
ChatGPT just reviewed.
Why it matters
reportedReviewedSha feeds directly into the /ship skill's merge gate
(skills/ship/SKILL.md step 2.7): "certified head... reviewed SHA equals this unit's
current PR head" is one of four required conditions before an automatic merge. A wrong
SHA here — silently reporting an older head as "reviewed" — undermines exactly the check
meant to catch a stale-head certification (ChatGPT approving a PR without having actually
seen its latest commit).
Confirmed live
Found and reproduced while shipping issue #642 (PR #668): code-review-pass.workflow.mjs
passes 2 and 3 both returned reviewedSha: "650378d..." (the pre-pass-1-fix head) even
though the actual PR head was a2318aa... (post-fix) and ChatGPT's real, published GitHub
review comments both explicitly and correctly said Reviewed head: a2318aa...``.
Verified by reading the raw response_text in the real CLI output files
(`chatgpt-review-pr-668-pass{2,3}.json`) and testing each of the three regexes against it
directly — all three return `null`; only the bare-40-hex fallback fires, and it picks the
wrong (first-occurring, older) SHA.
This did not affect #642's own merge correctness — the coordinator manually verified the
actual GitHub PR review comment text before merging — but relying on manual verification
defeats the purpose of having this field automated at all, and a future /ship run might
not catch it.
Intended fix
Make the three labelled-SHA regexes in extractReportedMetadata() tolerate optional
Markdown emphasis (**/__, possibly */_) between the label/colon and the
backtick-wrapped SHA — e.g. allow [\s*_]* instead of bare \s* in that position, or
strip Markdown emphasis characters from the text before applying the existing patterns.
Add a regression test reproducing this exact case (a response containing both a
"previously reviewed" and a "reviewed head" SHA, both wrapped in **...**) asserting the
correct (second, current) SHA is extracted.
Acceptance criteria
Problem
extractReportedMetadata()inskills/chatgpt-review/scripts/lib/prompt.mjsuses threelabelled-SHA regexes to find "the reviewed head SHA" ChatGPT reports in its own response
text, falling back to
text.match(/\b[0-9a-f]{40}\b/i)?.[0](the first bare 40-hex-charstring anywhere in the text) when none of the three match.
None of the three labelled patterns tolerate Markdown emphasis (
**...**) between thelabel/colon and the backtick-wrapped SHA. When ChatGPT writes, e.g.:
— a completely reasonable, naturally-occurring formatting choice for a multi-pass PR
review response — all three patterns fail to match (their
\s*:\s*cannot cross the**characters), so extraction falls back to "first 40-hex string in the text," which is the
previous/older SHA mentioned earlier in the message, not the actual current head
ChatGPT just reviewed.
Why it matters
reportedReviewedShafeeds directly into the/shipskill's merge gate(
skills/ship/SKILL.mdstep 2.7): "certified head... reviewed SHA equals this unit'scurrent PR head" is one of four required conditions before an automatic merge. A wrong
SHA here — silently reporting an older head as "reviewed" — undermines exactly the check
meant to catch a stale-head certification (ChatGPT approving a PR without having actually
seen its latest commit).
Confirmed live
Found and reproduced while shipping issue #642 (PR #668):
code-review-pass.workflow.mjspasses 2 and 3 both returned
reviewedSha: "650378d..."(the pre-pass-1-fix head) eventhough the actual PR head was
a2318aa...(post-fix) and ChatGPT's real, published GitHubreview comments both explicitly and correctly said
Reviewed head:a2318aa...``.Verified by reading the raw
response_textin the real CLI output files(`chatgpt-review-pr-668-pass{2,3}.json`) and testing each of the three regexes against it
directly — all three return `null`; only the bare-40-hex fallback fires, and it picks the
wrong (first-occurring, older) SHA.
This did not affect #642's own merge correctness — the coordinator manually verified the
actual GitHub PR review comment text before merging — but relying on manual verification
defeats the purpose of having this field automated at all, and a future
/shiprun mightnot catch it.
Intended fix
Make the three labelled-SHA regexes in
extractReportedMetadata()tolerate optionalMarkdown emphasis (
**/__, possibly*/_) between the label/colon and thebacktick-wrapped SHA — e.g. allow
[\s*_]*instead of bare\s*in that position, orstrip Markdown emphasis characters from the text before applying the existing patterns.
Add a regression test reproducing this exact case (a response containing both a
"previously reviewed" and a "reviewed head" SHA, both wrapped in
**...**) asserting thecorrect (second, current) SHA is extracted.
Acceptance criteria
extractReportedMetadatacorrectly extracts the current reviewed SHA when it iswrapped in Markdown bold (
**\sha`**`) and an earlier "previously reviewed" SHA(also bold) appears earlier in the same text.