Skip to content

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

Closed

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.

Activity

  1. added a commit that references this issue on Aug 10, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

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