From 93a7b7d3d5f2199f5f97a109669e9694c9e7ee98 Mon Sep 17 00:00:00 2001 From: Thomas Kosiewski Date: Sat, 3 Oct 2026 17:21:38 +0000 Subject: [PATCH] =?UTF-8?q?=F0=9F=A4=96=20fix:=20accept=20Resolved=20mark?= =?UTF-8?q?=20on=20Codex=20summary=20findings?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Codex now appends " · **Resolved**" to finding lines in its review summary card. The finding regex required the line to end right after the severity, so a marked finding counted as unresolved and turned the Codex Comments check red on #212. Accept the exact optional suffix. A finding marked Resolved, or whose bot-started thread is resolved, is cleared. Unresolved findings and unresolved bot threads still block. --- _Generated with `mux` • Model: `anthropic:claude-opus-5-5` • Thinking: `high`_ --- scripts/check_codex_comments.sh | 14 ++-- scripts/check_codex_comments_test.sh | 71 ++++++++++++++++++- ...urity-advisory-findings-resolved-pr212.txt | 29 ++++++++ 3 files changed, 108 insertions(+), 6 deletions(-) create mode 100644 scripts/testdata/check_codex_comments/bodies/summary-security-advisory-findings-resolved-pr212.txt diff --git a/scripts/check_codex_comments.sh b/scripts/check_codex_comments.sh index c630e991..45742bf5 100755 --- a/scripts/check_codex_comments.sh +++ b/scripts/check_codex_comments.sh @@ -173,8 +173,11 @@ done # more Running/Completed rows, fixed "About Codex" footer). The card may also # carry one "### Security findings" section with one "#### Advisory findings # (N)" list of exactly N links to review threads on this PR; it only counts -# as a non-finding when every linked thread is a resolved thread that the bot -# started. An unresolved or missing thread keeps the card blocking. +# as a non-finding when every finding is cleared. Codex appends +# " · **Resolved**" to a finding it considers resolved, which clears it. +# An unmarked finding is cleared only when its linked thread is a resolved +# thread that the bot started; an unresolved or missing thread keeps the +# card blocking. Unresolved bot threads also block on their own below. # - the explicit clean security verdict ("No security issues were found") # Recognition is structural and line-anchored: the summary and security shapes # must match line for line, so a marker alone, a quoted marker, or a summary @@ -226,14 +229,15 @@ def advisory_heading_regex: "^#### Advisory findings \\((?[1-9][0-9]{0,2})\\) # The link label may contain brackets (e.g. `args[0]` or escaped `\[`); only "](" ends it. def finding_regex: "^- [^ ]{1,4} \\[(?:[^\\]]|\\](?!\\())+\\]\\(https://github\\.com/(?[^/()]+)/(?[^/()]+)/pull/(?[0-9]+)" - + "#discussion_r(?[0-9]+)\\) · \\*\\*(Critical|High|Medium|Low)\\*\\*$"; + + "#discussion_r(?[0-9]+)\\) · \\*\\*(Critical|High|Medium|Low)\\*\\*(? · \\*\\*Resolved\\*\\*)?$"; -# True when the finding line links to a resolved, bot-started thread on this PR. +# True when the finding line links to this PR and either Codex marked it +# " · **Resolved**" or it links to a resolved, bot-started thread. def is_resolved_finding: (capture(finding_regex) // null) as $m | $m != null and $m.owner == $owner and $m.repo == $repo and $m.pr == $pr - and ($resolved | any(. == $m.id)); + and ($m.mark == " · **Resolved**" or ($resolved | any(. == $m.id))); # Input: the card lines between the status table's blank lines and the About # footer. Valid when empty, or when it is exactly one security findings section diff --git a/scripts/check_codex_comments_test.sh b/scripts/check_codex_comments_test.sh index 24c00ce7..9d00f288 100755 --- a/scripts/check_codex_comments_test.sh +++ b/scripts/check_codex_comments_test.sh @@ -7,7 +7,11 @@ # (human requester logins in the full-page fixture are replaced by "alice"), # except summary-security-advisory-finding-pr89.txt: the PR #89 summary card # (comment 5812405711) with its finding title replaced by "Example advisory -# finding". Its link, severity and layout are unchanged. +# finding". Its link, severity and layout are unchanged. And +# summary-security-advisory-findings-resolved-pr212.txt: the PR #212 summary +# card (comment 5970636463) with its two finding titles replaced by "Example +# advisory finding" and "Second example advisory finding". Codex marked the +# first finding " · **Resolved**"; links, severities and layout are unchanged. # # Usage: ./scripts/check_codex_comments_test.sh set -euo pipefail @@ -22,6 +26,12 @@ BOT="chatgpt-codex-connector" PRIMARY_SUMMARY_SHA256="cb7624eb0869f631aca6e3efde64656369924c9e5741020e78fa11287a36af2e" # sha256 of the PR #89 summary card with one advisory finding (see above). FINDING_SUMMARY_SHA256="d46fcf1cc41042be58dd18b514e14e8e2fd6c6a57957d5b20ce2e90aac1733ee" +# sha256 of the PR #212 summary card with one finding marked Resolved (see above). +RESOLVED_SUMMARY_SHA256="c5a0153b7193277d220854109050d6d38100a1421a088a1447eefafe4f1f9363" +# Review comment IDs the PR #212 card's findings link to: the first is marked +# Resolved in the card, the second is not. +PR212_MARKED_ID=4173840086 +PR212_UNMARKED_ID=4173883868 # Review comment ID the PR #89 card's finding links to (discussion_r). FINDING_ID=4092909628 # 2^53 + 1: the smallest integer a JSON number cannot carry exactly through jq. @@ -170,12 +180,28 @@ if [ "$actual_sha" != "$FINDING_SUMMARY_SHA256" ]; then echo "❌ Assertion failed: summary-security-advisory-finding-pr89.txt sha256 ${actual_sha} != ${FINDING_SUMMARY_SHA256}" exit 1 fi +actual_sha=$(sha256sum "${BODIES}/summary-security-advisory-findings-resolved-pr212.txt" | cut -d' ' -f1) +if [ "$actual_sha" != "$RESOLVED_SUMMARY_SHA256" ]; then + echo "❌ Assertion failed: summary-security-advisory-findings-resolved-pr212.txt sha256 ${actual_sha} != ${RESOLVED_SUMMARY_SHA256}" + exit 1 +fi # finding_card [sed-script] -> comments page holding the PR #89 card, optionally edited finding_card() { page comments "$(comment_node "$BOT" "$(body summary-security-advisory-finding-pr89 | sed -e "${1:-}")")" } +# resolved_card [sed-script] -> comments page holding the PR #212 card, optionally edited +resolved_card() { + page comments "$(comment_node "$BOT" "$(body summary-security-advisory-findings-resolved-pr212 | sed -e "${1:-}")")" +} + +# Appends a suffix to the PR #89 card's finding line (after its severity). +mark_finding_sed() { + printf '/discussion_r%s/s/$/%s/' "$FINDING_ID" "$1" +} +RESOLVED_SUFFIX=' · **Resolved**' + # Second finding line and count, for the two-finding cases. TWO_FINDINGS_SED="s/^#### Advisory findings (1)\$/#### Advisory findings (2)/;/discussion_r${FINDING_ID}/{p;s/discussion_r${FINDING_ID}/discussion_r$((FINDING_ID + 1))/}" @@ -242,6 +268,20 @@ CASE_PR=89 run_case summary_finding_full_id_string_above_2p53_resolved 0 "$CLEAN "$(finding_card "s/discussion_r${FINDING_ID}/discussion_r${BIG_ID}/")" \ "$(page reviewThreads "$(thread_node "$BOT" true "$BIG_ID")")" +# Codex appends " · **Resolved**" to a finding once its thread is resolved +# (PR #212). Every linked thread is resolved here. +CASE_PR=212 run_case summary_findings_marked_resolved_pr212 0 "$CLEAN_MSG" \ + "$(resolved_card)" \ + "$(page reviewThreads "$(thread_node "$BOT" true "$PR212_MARKED_ID")" "$(thread_node "$BOT" true "$PR212_UNMARKED_ID")")" + +CASE_PR=89 run_case summary_finding_marked_resolved_thread_resolved 0 "$CLEAN_MSG" \ + "$(finding_card "$(mark_finding_sed "$RESOLVED_SUFFIX")")" \ + "$(page reviewThreads "$(thread_node "$BOT" true "$FINDING_ID")")" + +# The Resolved mark alone clears a finding, even when its thread is not listed. +CASE_PR=89 run_case summary_finding_marked_resolved_thread_missing 0 "$CLEAN_MSG" \ + "$(finding_card "$(mark_finding_sed "$RESOLVED_SUFFIX")")" "$NO_THREADS" + # --- Findings and lookalikes (must stay blocking) -------------------------- run_case real_finding_comment 1 "$BLOCK_MSG" \ @@ -363,6 +403,35 @@ CASE_PR=89 run_case summary_finding_with_extra_text 1 "Found 1 unminimized regul "$(finding_card "/discussion_r${FINDING_ID}/a **P1** Missing bounds check.")" \ "$(page reviewThreads "$(thread_node "$BOT" true "$FINDING_ID")")" +# The unmarked PR #212 finding still needs its own resolved thread. +CASE_PR=212 run_case summary_pr212_unmarked_finding_thread_unresolved 1 "Found 1 unminimized regular comment(s) from bot" \ + "$(resolved_card)" \ + "$(page reviewThreads "$(thread_node "$BOT" true "$PR212_MARKED_ID")" "$(thread_node "$BOT" false "$PR212_UNMARKED_ID")")" + +CASE_PR=212 run_case summary_pr212_unmarked_finding_thread_missing 1 "Found 1 unminimized regular comment(s) from bot" \ + "$(resolved_card)" "$(page reviewThreads "$(thread_node "$BOT" true "$PR212_MARKED_ID")")" + +# A Resolved mark clears the card, but an unresolved thread still blocks on its own. +CASE_PR=89 run_case summary_finding_marked_resolved_thread_unresolved 1 "Found 1 unresolved review thread(s) from bot" \ + "$(finding_card "$(mark_finding_sed "$RESOLVED_SUFFIX")")" \ + "$(page reviewThreads "$(thread_node "$BOT" false "$FINDING_ID")")" + +# The mark only counts on a finding that links to this PR. +run_case summary_finding_marked_resolved_links_other_pr 1 "Found 1 unminimized regular comment(s) from bot" \ + "$(finding_card "$(mark_finding_sed "$RESOLVED_SUFFIX")")" "$NO_THREADS" + +# Only the exact " · **Resolved**" suffix is a mark. +for case_suffix in \ + 'unresolved| · **Unresolved**' \ + 'lowercase| · **resolved**' \ + 'unbolded| · Resolved' \ + 'repeated| · **Resolved** · **Resolved**' \ + 'trailing_text| · **Resolved** **P1** Missing bounds check.'; do + CASE_PR=89 run_case "summary_finding_lookalike_mark_${case_suffix%%|*}" 1 \ + "Found 1 unminimized regular comment(s) from bot" \ + "$(finding_card "$(mark_finding_sed "${case_suffix#*|}")")" "$NO_THREADS" +done + run_case summary_plus_finding_comment 1 "Found 1 unminimized regular comment(s) from bot" \ "$(page comments "$(comment_node "$BOT" "$(body summary-completed-both)")" "$(comment_node "$BOT" "**P2** Unused parameter.")")" "$NO_THREADS" diff --git a/scripts/testdata/check_codex_comments/bodies/summary-security-advisory-findings-resolved-pr212.txt b/scripts/testdata/check_codex_comments/bodies/summary-security-advisory-findings-resolved-pr212.txt new file mode 100644 index 00000000..571d778c --- /dev/null +++ b/scripts/testdata/check_codex_comments/bodies/summary-security-advisory-findings-resolved-pr212.txt @@ -0,0 +1,29 @@ + + +## Codex Review Summary + +This comment shows the latest Codex review activity on this pull request. + +| Review | Status | Commit | Review trigger | +| --- | --- | --- | --- | +| 📝 **Code Review** | ✅ **Completed** 2026-10-03T16:42:03.042488Z | `a4bf9f1` | Manual request | +| 🔒 **Security Review** | ✅ **Completed** 2026-10-03T16:44:14.113174Z | `a4bf9f1` | Manual request | + +### Security findings + +#### Advisory findings (2) + +- 🟡 [Example advisory finding](https://github.com/coder/coder-k8s/pull/212#discussion_r4173840086) · **Medium** · **Resolved** +- 🟡 [Second example advisory finding](https://github.com/coder/coder-k8s/pull/212#discussion_r4173883868) · **Medium** + +
ℹ️ About Codex in GitHub +
+ +[Your team has set up Codex to review pull requests in this repo](https://chatgpt.com/codex/cloud/settings/general). Reviews are triggered when you +- Open a pull request for review +- Mark a draft as ready +- Comment "@codex review" or "@codex security review". + +Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. + +
\ No newline at end of file