Skip to content

Commit bf8019f

Browse files
authored
🤖 fix: accept Resolved mark on Codex summary findings (#220)
## Summary `scripts/check_codex_comments.sh` now accepts the ` · **Resolved**` suffix that Codex appends to finding lines in its review-summary card. A finding marked Resolved, or whose review thread is resolved, no longer fails the `Codex Comments` check. Unresolved findings still fail. ## Background Codex now writes resolved findings in the summary card as, for example: ``` - 🟡 [Title](#212 (comment)) · **Medium** · **Resolved** ``` The finding regex required the line to end right after the severity (`· \*\*(Critical|High|Medium|Low)\*\*$`). A marked line did not match, so the whole card counted as an unresolved comment. This turned the non-required `Codex Comments` check red on #212 (run 37137600873), although all 11 Codex threads on that PR were resolved. ## Implementation - `finding_regex` takes an optional ` · \*\*Resolved\*\*` group after the severity, anchored at end of line. - `is_resolved_finding` clears a finding that links to this PR when the exact Resolved mark is present, or (as before) when its linked thread is a resolved thread the bot started. - The Resolved mark alone clears a finding, even when its thread is missing from the thread list. This trusts the bot-authored card, as requested. An open bot thread still blocks through the separate thread count. - Unchanged: the card shape checks, the same-repo/same-PR link check, the finding count check, and the separate count of unresolved bot threads. A finding marked Resolved whose thread is still open keeps the check red through that thread count. ## Validation - Tests first: the new cases failed on `origin/main` (3 of 68 failed), then passed with the fix (68 of 68). - New frozen fixture `summary-security-advisory-findings-resolved-pr212.txt`: the #212 summary card (comment 5970636463) with only the two finding titles replaced. It has one marked and one unmarked finding, as in the failing run. - New cases: marked + thread resolved (pass), marked + thread missing (pass), real #212 card with both threads resolved (pass); unmarked #212 finding with an unresolved or missing thread (fail), marked finding with an unresolved thread (fail), marked finding linking another PR (fail), and lookalike marks `**Unresolved**`, `**resolved**`, unbolded, repeated, trailing text (all fail). - `make test-scripts`, and `shellcheck` 0.11.0 on both scripts. No workflow changed, so actionlint was not needed. ## Risks Low. Only the non-required `Codex Comments` check uses this parser. The change widens one exact, end-anchored suffix, and only on bot-authored cards that already pass the structural checks. --- _Generated with `mux` • Model: `anthropic:claude-opus-5-5` • Thinking: `high`_ <!-- mux-attribution: model=anthropic:claude-opus-5-5 thinking=high -->
1 parent 8afd850 commit bf8019f

3 files changed

Lines changed: 108 additions & 6 deletions

File tree

‎scripts/check_codex_comments.sh‎

Lines changed: 9 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -173,8 +173,11 @@ done
173173
# more Running/Completed rows, fixed "About Codex" footer). The card may also
174174
# carry one "### Security findings" section with one "#### Advisory findings
175175
# (N)" list of exactly N links to review threads on this PR; it only counts
176-
# as a non-finding when every linked thread is a resolved thread that the bot
177-
# started. An unresolved or missing thread keeps the card blocking.
176+
# as a non-finding when every finding is cleared. Codex appends
177+
# " · **Resolved**" to a finding it considers resolved, which clears it.
178+
# An unmarked finding is cleared only when its linked thread is a resolved
179+
# thread that the bot started; an unresolved or missing thread keeps the
180+
# card blocking. Unresolved bot threads also block on their own below.
178181
# - the explicit clean security verdict ("No security issues were found")
179182
# Recognition is structural and line-anchored: the summary and security shapes
180183
# 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 \\((?<n>[1-9][0-9]{0,2})\\)
226229
# The link label may contain brackets (e.g. `args[0]` or escaped `\[`); only "](" ends it.
227230
def finding_regex:
228231
"^- [^ ]{1,4} \\[(?:[^\\]]|\\](?!\\())+\\]\\(https://github\\.com/(?<owner>[^/()]+)/(?<repo>[^/()]+)/pull/(?<pr>[0-9]+)"
229-
+ "#discussion_r(?<id>[0-9]+)\\) · \\*\\*(Critical|High|Medium|Low)\\*\\*$";
232+
+ "#discussion_r(?<id>[0-9]+)\\) · \\*\\*(Critical|High|Medium|Low)\\*\\*(?<mark> · \\*\\*Resolved\\*\\*)?$";
230233
231-
# True when the finding line links to a resolved, bot-started thread on this PR.
234+
# True when the finding line links to this PR and either Codex marked it
235+
# " · **Resolved**" or it links to a resolved, bot-started thread.
232236
def is_resolved_finding:
233237
(capture(finding_regex) // null) as $m
234238
| $m != null
235239
and $m.owner == $owner and $m.repo == $repo and $m.pr == $pr
236-
and ($resolved | any(. == $m.id));
240+
and ($m.mark == " · **Resolved**" or ($resolved | any(. == $m.id)));
237241
238242
# Input: the card lines between the status table's blank lines and the About
239243
# footer. Valid when empty, or when it is exactly one security findings section

‎scripts/check_codex_comments_test.sh‎

Lines changed: 70 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -7,7 +7,11 @@
77
# (human requester logins in the full-page fixture are replaced by "alice"),
88
# except summary-security-advisory-finding-pr89.txt: the PR #89 summary card
99
# (comment 5812405711) with its finding title replaced by "Example advisory
10-
# finding". Its link, severity and layout are unchanged.
10+
# finding". Its link, severity and layout are unchanged. And
11+
# summary-security-advisory-findings-resolved-pr212.txt: the PR #212 summary
12+
# card (comment 5970636463) with its two finding titles replaced by "Example
13+
# advisory finding" and "Second example advisory finding". Codex marked the
14+
# first finding " · **Resolved**"; links, severities and layout are unchanged.
1115
#
1216
# Usage: ./scripts/check_codex_comments_test.sh
1317
set -euo pipefail
@@ -22,6 +26,12 @@ BOT="chatgpt-codex-connector"
2226
PRIMARY_SUMMARY_SHA256="cb7624eb0869f631aca6e3efde64656369924c9e5741020e78fa11287a36af2e"
2327
# sha256 of the PR #89 summary card with one advisory finding (see above).
2428
FINDING_SUMMARY_SHA256="d46fcf1cc41042be58dd18b514e14e8e2fd6c6a57957d5b20ce2e90aac1733ee"
29+
# sha256 of the PR #212 summary card with one finding marked Resolved (see above).
30+
RESOLVED_SUMMARY_SHA256="c5a0153b7193277d220854109050d6d38100a1421a088a1447eefafe4f1f9363"
31+
# Review comment IDs the PR #212 card's findings link to: the first is marked
32+
# Resolved in the card, the second is not.
33+
PR212_MARKED_ID=4173840086
34+
PR212_UNMARKED_ID=4173883868
2535
# Review comment ID the PR #89 card's finding links to (discussion_r<ID>).
2636
FINDING_ID=4092909628
2737
# 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
170180
echo "❌ Assertion failed: summary-security-advisory-finding-pr89.txt sha256 ${actual_sha} != ${FINDING_SUMMARY_SHA256}"
171181
exit 1
172182
fi
183+
actual_sha=$(sha256sum "${BODIES}/summary-security-advisory-findings-resolved-pr212.txt" | cut -d' ' -f1)
184+
if [ "$actual_sha" != "$RESOLVED_SUMMARY_SHA256" ]; then
185+
echo "❌ Assertion failed: summary-security-advisory-findings-resolved-pr212.txt sha256 ${actual_sha} != ${RESOLVED_SUMMARY_SHA256}"
186+
exit 1
187+
fi
173188

174189
# finding_card [sed-script] -> comments page holding the PR #89 card, optionally edited
175190
finding_card() {
176191
page comments "$(comment_node "$BOT" "$(body summary-security-advisory-finding-pr89 | sed -e "${1:-}")")"
177192
}
178193

194+
# resolved_card [sed-script] -> comments page holding the PR #212 card, optionally edited
195+
resolved_card() {
196+
page comments "$(comment_node "$BOT" "$(body summary-security-advisory-findings-resolved-pr212 | sed -e "${1:-}")")"
197+
}
198+
199+
# Appends a suffix to the PR #89 card's finding line (after its severity).
200+
mark_finding_sed() {
201+
printf '/discussion_r%s/s/$/%s/' "$FINDING_ID" "$1"
202+
}
203+
RESOLVED_SUFFIX=' · **Resolved**'
204+
179205
# Second finding line and count, for the two-finding cases.
180206
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))/}"
181207

@@ -242,6 +268,20 @@ CASE_PR=89 run_case summary_finding_full_id_string_above_2p53_resolved 0 "$CLEAN
242268
"$(finding_card "s/discussion_r${FINDING_ID}/discussion_r${BIG_ID}/")" \
243269
"$(page reviewThreads "$(thread_node "$BOT" true "$BIG_ID")")"
244270

271+
# Codex appends " · **Resolved**" to a finding once its thread is resolved
272+
# (PR #212). Every linked thread is resolved here.
273+
CASE_PR=212 run_case summary_findings_marked_resolved_pr212 0 "$CLEAN_MSG" \
274+
"$(resolved_card)" \
275+
"$(page reviewThreads "$(thread_node "$BOT" true "$PR212_MARKED_ID")" "$(thread_node "$BOT" true "$PR212_UNMARKED_ID")")"
276+
277+
CASE_PR=89 run_case summary_finding_marked_resolved_thread_resolved 0 "$CLEAN_MSG" \
278+
"$(finding_card "$(mark_finding_sed "$RESOLVED_SUFFIX")")" \
279+
"$(page reviewThreads "$(thread_node "$BOT" true "$FINDING_ID")")"
280+
281+
# The Resolved mark alone clears a finding, even when its thread is not listed.
282+
CASE_PR=89 run_case summary_finding_marked_resolved_thread_missing 0 "$CLEAN_MSG" \
283+
"$(finding_card "$(mark_finding_sed "$RESOLVED_SUFFIX")")" "$NO_THREADS"
284+
245285
# --- Findings and lookalikes (must stay blocking) --------------------------
246286

247287
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
363403
"$(finding_card "/discussion_r${FINDING_ID}/a **P1** Missing bounds check.")" \
364404
"$(page reviewThreads "$(thread_node "$BOT" true "$FINDING_ID")")"
365405

406+
# The unmarked PR #212 finding still needs its own resolved thread.
407+
CASE_PR=212 run_case summary_pr212_unmarked_finding_thread_unresolved 1 "Found 1 unminimized regular comment(s) from bot" \
408+
"$(resolved_card)" \
409+
"$(page reviewThreads "$(thread_node "$BOT" true "$PR212_MARKED_ID")" "$(thread_node "$BOT" false "$PR212_UNMARKED_ID")")"
410+
411+
CASE_PR=212 run_case summary_pr212_unmarked_finding_thread_missing 1 "Found 1 unminimized regular comment(s) from bot" \
412+
"$(resolved_card)" "$(page reviewThreads "$(thread_node "$BOT" true "$PR212_MARKED_ID")")"
413+
414+
# A Resolved mark clears the card, but an unresolved thread still blocks on its own.
415+
CASE_PR=89 run_case summary_finding_marked_resolved_thread_unresolved 1 "Found 1 unresolved review thread(s) from bot" \
416+
"$(finding_card "$(mark_finding_sed "$RESOLVED_SUFFIX")")" \
417+
"$(page reviewThreads "$(thread_node "$BOT" false "$FINDING_ID")")"
418+
419+
# The mark only counts on a finding that links to this PR.
420+
run_case summary_finding_marked_resolved_links_other_pr 1 "Found 1 unminimized regular comment(s) from bot" \
421+
"$(finding_card "$(mark_finding_sed "$RESOLVED_SUFFIX")")" "$NO_THREADS"
422+
423+
# Only the exact " · **Resolved**" suffix is a mark.
424+
for case_suffix in \
425+
'unresolved| · **Unresolved**' \
426+
'lowercase| · **resolved**' \
427+
'unbolded| · Resolved' \
428+
'repeated| · **Resolved** · **Resolved**' \
429+
'trailing_text| · **Resolved** **P1** Missing bounds check.'; do
430+
CASE_PR=89 run_case "summary_finding_lookalike_mark_${case_suffix%%|*}" 1 \
431+
"Found 1 unminimized regular comment(s) from bot" \
432+
"$(finding_card "$(mark_finding_sed "${case_suffix#*|}")")" "$NO_THREADS"
433+
done
434+
366435
run_case summary_plus_finding_comment 1 "Found 1 unminimized regular comment(s) from bot" \
367436
"$(page comments "$(comment_node "$BOT" "$(body summary-completed-both)")" "$(comment_node "$BOT" "**P2** Unused parameter.")")" "$NO_THREADS"
368437

Lines changed: 29 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,29 @@
1+
<!-- codex-pull-request-review-summary -->
2+
<!-- codex-security-review:v1 {"blockingSeverityThreshold":"P0","headSha":"a4bf9f1180438d6ef9bcb91aa5449ca81b2829c8","mergeGateEnabled":false,"pullRequestNumber":212,"repository":"coder/coder-k8s","status":"completed"} -->
3+
## Codex Review Summary
4+
5+
This comment shows the latest Codex review activity on this pull request.
6+
7+
| Review | Status | Commit | Review trigger |
8+
| --- | --- | --- | --- |
9+
| 📝 **Code Review** | ✅ **Completed** <relative-time datetime="2026-10-03T16:42:03.042488Z">2026-10-03T16:42:03.042488Z</relative-time> | `a4bf9f1` | Manual request |
10+
| 🔒 **Security Review** | ✅ **Completed** <relative-time datetime="2026-10-03T16:44:14.113174Z">2026-10-03T16:44:14.113174Z</relative-time> | `a4bf9f1` | Manual request |
11+
12+
### Security findings
13+
14+
#### Advisory findings (2)
15+
16+
- 🟡 [Example advisory finding](https://github.com/coder/coder-k8s/pull/212#discussion_r4173840086) · **Medium** · **Resolved**
17+
- 🟡 [Second example advisory finding](https://github.com/coder/coder-k8s/pull/212#discussion_r4173883868) · **Medium**
18+
19+
<details> <summary>ℹ️ About Codex in GitHub</summary>
20+
<br/>
21+
22+
[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
23+
- Open a pull request for review
24+
- Mark a draft as ready
25+
- Comment "@codex review" or "@codex security review".
26+
27+
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings.
28+
29+
</details>

0 commit comments

Comments
 (0)