Skip to content

[groom] slack-notify.sh: webhook mode and two of its five positional parameters are unreachable - #441

Open
cloud-code-bot[bot] wants to merge 1 commit into
mainfrom
groom/github-workflows-whole-repo-github-cursor-review-39932667195-0
Open

cloud-code-bot[bot] wants to merge 1 commit into
mainfrom
groom/github-workflows-whole-repo-github-cursor-review-39932667195-0

Conversation

@cloud-code-bot

Copy link
Copy Markdown
Contributor

🤖 Auto-built by the groom sweep — this is a machine-proposed refactor. It runs full CI + cursor-review and requires human review; it is NOT auto-merged. Ranked from a CONFIRMED groom finding. · run

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):

  • Deleted the whole webhook branch — the SLACK_WEBHOOK_URL read, the MENTION_USER_ID mention text, the jq payload builder, the jq empty validator, the curl POST and the HTTP-code parse.
  • Deleted the two positional parameters only that branch could reach: the 4th (details_file) and the 5th (channel, which defaulted to a hardcoded #prod-alerts), along with the ADDITIONAL_DETAILS file-reading plumbing and the DM-side DM_TEXT wrapper that appended it (the DM payload now interpolates $MESSAGE directly — it was always identical, since ADDITIONAL_DETAILS could only ever be empty).
  • Rewrote the fall-through at the end of the file: it now reports that no DM target is configured (SLACK_BOT_TOKEN and/or DM_GITHUB_USER unset) and exits 0 instead of 1.
  • Retitled the header comment and usage line for the single surviving mode.

The two call sites in .github/workflows/cursor-review.yml (notify-start and notify-complete) are unchanged: both already pass exactly three positional arguments and set only SLACK_BOT_TOKEN, DM_GITHUB_USER and DM_EMAIL_MAP.

Why

The deleted code was unreachable: a repo-wide grep finds SLACK_WEBHOOK_URL and MENTION_USER_ID only 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 are continue-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 by cursor-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.sh is 221 lines and documents two delivery modes in its header (:6-11). Only DM mode is reachable.

Verified:

  • SLACK_WEBHOOK_URL and MENTION_USER_ID are set by NOTHING in the tree - a repo-wide grep matches them only inside slack-notify.sh itself (:20, :131, :137-140, :205).
  • The only two call sites are cursor-review.yml:7500 (job notify-start) and :7681 (job notify-complete). Both pass THREE positional arguments and set only SLACK_BOT_TOKEN, DM_GITHUB_USER and DM_EMAIL_MAP (see cursor-review.yml:7486-7494).
  • So the fourth positional (DETAILS_FILE, :18) and the fifth (CHANNEL, defaulting to a hardcoded #prod-alerts, :19) are never supplied, ADDITIONAL_DETAILS (:32-35) is always empty, and lines 136-221 - the mention text, the jq payload builder, the jq empty validator, 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, and bump-cursor-review-callers.yml:284 watches it on exactly that path. There is no test suite - test-cursor-review-scripts.yml:98 runs shellcheck -x on it and nothing else - so re-run shellcheck after the edit and eyeball the two notify steps' output once.

…arameters are unreachable

Auto-built groom refactor (github-workflows:whole-repo:github-cursor-review-slack-notify-sh).
Review required — do not auto-merge.
@cloud-code-bot cloud-code-bot Bot added cursor-review Multi-model cursor review groom Refactor / tech-debt cleanup candidate filed by the groom sweep (finds-only) groom-pr Auto-built groom refactor PR — human-review only, NEVER auto-merged labels Oct 23, 2026
@coderabbitai

coderabbitai Bot commented Oct 23, 2026

Copy link
Copy Markdown
Contributor

Important

Review skipped

Bot user detected.

To trigger a single review, invoke the @coderabbitai review command.

⚙️ Run configuration
  • Source mode: Auto
  • Repository: Unavailable
  • Branch: Unavailable
  • File: No configuration file from this repository applied
  • Commit: Unavailable
  • Layers: Organization UI
  • Review profile: ASSERTIVE
  • Plan: Advanced
  • Run ID: f685d64d-4889-4051-bb73-334fa4d03d9a

You can disable this status message by setting the reviews.review_status to false in the CodeRabbit configuration file.

Use the checkbox below for a quick retry:

  • 🔍 Trigger review
  • Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

Comment @coderabbitai help to get the list of available commands.

@github-actions github-actions Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔍 Cursor Review — Consolidated panel

Triggered by @cloud-code-bot[bot].

⚠️ Panel did not produce any findings.

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)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

cursor-review Multi-model cursor review groom Refactor / tech-debt cleanup candidate filed by the groom sweep (finds-only) groom-pr Auto-built groom refactor PR — human-review only, NEVER auto-merged

Projects

None yet

Development

Successfully merging this pull request may close these issues.

0 participants