Skip to content

chatgpt-review: extractReportedMetadata misextracts reviewed SHA when Markdown-bolded #669

Description

@BorisTyshkevich

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

  • extractReportedMetadata correctly extracts the current reviewed SHA when it is
    wrapped in Markdown bold (**\sha`**`) and an earlier "previously reviewed" SHA
    (also bold) appears earlier in the same text.
  • Existing extraction behavior for plain (non-bold) SHA mentions is unchanged.
  • Regression test added covering the exact real-world shape reproduced above.

Metadata

Metadata

Assignees

No one assigned

    Labels

    inboxFiled mid-task; not yet triaged into the roadmap

    Type

    No type

    Projects

    No projects

    Milestone

    No milestone

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions