Repository navigation
[groom] slack-notify.sh: webhook mode and two of its five positional parameters are unreachable - #441
Conversation
…arameters are unreachable Auto-built groom refactor (github-workflows:whole-repo:github-cursor-review-slack-notify-sh). Review required — do not auto-merge.
|
Important Review skippedBot user detected. To trigger a single review, invoke the ⚙️ Run configuration
You can disable this status message by setting the Use the checkbox below for a quick retry:
Comment |
There was a problem hiding this comment.
🔍 Cursor Review — Consolidated panel
Triggered by @cloud-code-bot[bot].
Every reviewer in the matrix failed to contribute — see the panel summary for which cells errored, and the run logs for the underlying cause.
Panel: 0/6 reviewers contributed findings.
Reviewers that did not contribute: claude-opus-5-5-xhigh:adversarial (error), gpt-5.6-sol-xhigh:adversarial (error), kimi-k3-max:adversarial (error), claude-opus-5-5-xhigh:edge-case (error), gpt-5.6-sol-xhigh:edge-case (error), kimi-k3-max:edge-case (error)
ELI-5
When a Cursor review starts and finishes, a small shell script sends the person who triggered it a Slack direct message. That script also carried a second, entirely separate way of posting to Slack — an "incoming webhook" path — that nothing in this repo has ever switched on: the environment variables and the extra command-line arguments it needs are set by no caller anywhere in the tree. This PR deletes that unused half (about 95 lines) and keeps the direct-message path exactly as it was, so the script now does one thing and the code you read is the code that runs. It also fixes the last line of the script: when there is no Slack token or no recipient to DM, the script used to announce "no delivery method configured" and exit with an error; it now says plainly that the DM is not configured and exits cleanly, which is what this directory's README already documents.
What changed
One file,
.github/cursor-review/slack-notify.sh(221 → 119 lines):SLACK_WEBHOOK_URLread, theMENTION_USER_IDmention text, the jq payload builder, thejq emptyvalidator, thecurlPOST and the HTTP-code parse.details_file) and the 5th (channel, which defaulted to a hardcoded#prod-alerts), along with theADDITIONAL_DETAILSfile-reading plumbing and the DM-sideDM_TEXTwrapper that appended it (the DM payload now interpolates$MESSAGEdirectly — it was always identical, sinceADDITIONAL_DETAILScould only ever be empty).SLACK_BOT_TOKENand/orDM_GITHUB_USERunset) and exits 0 instead of 1.The two call sites in
.github/workflows/cursor-review.yml(notify-startandnotify-complete) are unchanged: both already pass exactly three positional arguments and set onlySLACK_BOT_TOKEN,DM_GITHUB_USERandDM_EMAIL_MAP.Why
The deleted code was unreachable: a repo-wide grep finds
SLACK_WEBHOOK_URLandMENTION_USER_IDonly inside this script, and the only two invocations pass three arguments, so the 4th and 5th parameters were never supplied and the webhook branch could never execute. Dead code in a notification path is a maintenance tax and a false signal — it reads as a supported second mode that no caller can actually use.Behavior of the live DM path is preserved exactly: the lookup, the payload fields and every exit code in that branch are untouched, and the one expression that changed (
$DM_TEXT→$MESSAGE) was provably equal because its only other input could never be non-empty.The one deliberate behavior change is the final exit code. That branch is reached whenever the token or the DM recipient is missing — including the designed-for case of a bot-authored PR whose triggerer resolves to empty — and it would exit 1 there, contradicting
.github/cursor-review/README.md, which describes the script as no-ops-without-a-token. Both call steps arecontinue-on-error: true, so this could only ever have surfaced as a misleading red annotation on an otherwise fine run, never as a failed job; exiting 0 aligns the code with the documented contract and removes that noise. The script is loaded only from this repo's pinned checkout bycursor-review.yml, so no external caller can depend on the removed parameters or on the old exit code.Verifier rationale
Problem
.github/cursor-review/slack-notify.shis 221 lines and documents two delivery modes in its header (:6-11). Only DM mode is reachable.Verified:
slack-notify.shitself (:20, :131, :137-140, :205).cursor-review.yml:7500(jobnotify-start) and:7681(jobnotify-complete). Both pass THREE positional arguments and set only SLACK_BOT_TOKEN, DM_GITHUB_USER and DM_EMAIL_MAP (seecursor-review.yml:7486-7494).#prod-alerts, :19) are never supplied, ADDITIONAL_DETAILS (:32-35) is always empty, and lines 136-221 - the mention text, the jq payload builder, thejq emptyvalidator, the curl and its HTTP-code parse - cannot execute.Correction to the finder
The finder said lines 131-221 are unreachable. Lines 131-134 ARE reachable: that is the branch taken when SLACK_BOT_TOKEN is unset, and it prints "No delivery method configured..." and exits 1. That exit is tolerated today only because both call steps are
continue-on-error: true, and it contradicts.github/cursor-review/README.md:264, which describes the script as no-ops-without-a-token. The genuinely dead region is 136-221 plus the DETAILS_FILE and CHANNEL parameters and the ADDITIONAL_DETAILS plumbing - roughly 95 lines.Fix
Delete webhook mode, MENTION_USER_ID, the DETAILS_FILE and CHANNEL parameters and the ADDITIONAL_DETAILS plumbing (:18-19, :31-35, :85-88 and :136-221). Rewrite the fall-through branch to say what it actually means - no SLACK_BOT_TOKEN / DM_GITHUB_USER configured - and make its exit code a deliberate, documented decision: exit 0 to match the README, or keep exit 1 and fix the README. Retitle the header for the one mode that survives. The file drops to roughly 125 lines.
Risk
Low, and grep-provable. The script cannot be invoked from outside this repo: it is loaded only from this repo's pinned checkout by
cursor-review.yml, andbump-cursor-review-callers.yml:284watches it on exactly that path. There is no test suite -test-cursor-review-scripts.yml:98runsshellcheck -xon it and nothing else - so re-run shellcheck after the edit and eyeball the two notify steps' output once.