Skip to content

ci(slack): escape mrkdwn + fetch timeout (sync notifier with OGAD) #509

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
alichherawalla merged 1 commit into from
Jul 9, 2026
Merged

ci(slack): escape mrkdwn + fetch timeout (sync notifier with OGAD) #509

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
11 changes: 9 additions & 2 deletions scripts/notify-slack-release.mjs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -35,15 +35,20 @@ const server = process.env.GITHUB_SERVER_URL || 'https://github.com';
const repo = process.env.GITHUB_REPOSITORY || '';
const releaseUrl = process.env.RELEASE_URL || (repo && version ? `${server}/${repo}/releases/tag/v${version}` : '');

// Slack mrkdwn reserves & < > — a raw commit subject containing them (release notes are raw
// commit subjects) would misrender or be read as a <link|mention>. Escape the note body only;
// NOT the intentional <url|label> link line below.
const esc = (s) => s.replace(/&/g, '&amp;').replace(/</g, '&lt;').replace(/>/g, '&gt;');

let notes = '';
try { notes = readFileSync(notesFile, 'utf8').trim(); } catch { /* notes optional */ }
// Slack section text caps at 3000 chars; keep well under and never dump a wall.
if (notes.length > 2600) { notes = `${notes.slice(0, 2600)}\n…`; }

const tag = label ? ` \`${label}\`` : '';
const header = `:package: *${product}* \`${version || 'release'}\`${tag}`;
const header = `:package: *${esc(product)}* \`${version || 'release'}\`${tag}`;
Comment on lines +38 to +49

@qodo-code-review qodo-code-review Bot Jul 9, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Informational

3. Escape comment mismatch 🐞 Bug ⚙ Maintainability

The new comment says only the note body is escaped, but esc() is also applied to the header product
string. This mismatch is misleading for future maintainers reading the comment to understand what is
(and isn’t) escaped.
Agent Prompt 
### Issue description
A comment claims the script escapes "the note body only", but the code also escapes `product` in the header. This is a documentation/maintainability issue that can confuse future edits.

### Issue Context
Escaping is correctly *not* applied to the explicit Slack link markup line (`<url|label>`). The comment should reflect that the non-link text portions (header/body) may be escaped.

### Fix Focus Areas
- scripts/notify-slack-release.mjs[38-49]

### Recommended fix
Update the comment to reflect the actual intent, e.g.:
- “Escape Slack-reserved `& < >` in user-controlled text (header + notes). Do not escape the explicit `<url|label>` link markup line below.”

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

const linkLine = releaseUrl ? `<${releaseUrl}|Download / release page>` : '';
const body = notes || '_No release notes generated for this build._';
const body = notes ? esc(notes) : '_No release notes generated for this build._';
Comment on lines +41 to +51

@qodo-code-review qodo-code-review Bot Jul 9, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Remediation recommended

2. Escape breaks length cap 🐞 Bug ≡ Correctness

In notify-slack-release.mjs, release notes are truncated to 2600 characters before being escaped,
but esc() can expand the text (e.g., '&' → '&amp;') and push the final mrkdwn section over the
3000-character Slack limit. This can cause Slack to reject the block/payload and drop the release
announcement despite the pre-escape truncation.
Agent Prompt 
### Issue description
The script truncates `notes` to 2600 chars before escaping `&`, `<`, `>` via `esc()`. Because escaping expands characters into multi-byte entities, the final `body` text can exceed Slack’s 3000-char limit for section text, causing the announcement request to fail.

### Issue Context
This script is explicitly trying to stay under Slack’s 3000-character cap, but the cap is currently enforced on the *unescaped* text rather than the final rendered/posted mrkdwn string.

### Fix Focus Areas
- scripts/notify-slack-release.mjs[41-51]

### Recommended fix
Implement an `escapeAndTruncateMrkdwn(text, maxLen)` helper that:
1) Escapes `&`, `<`, `>` while building the output.
2) Stops when the *escaped output* reaches `maxLen` (e.g. 2600).
3) Appends `\n…` when truncation occurs.

This avoids post-escape length expansion and avoids slicing in the middle of an escape sequence (e.g., cutting `&amp;` into `&am`).

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools


const blocks = [
{ type: 'section', text: { type: 'mrkdwn', text: header } },
Comment on lines +38 to 54

@qodo-code-review qodo-code-review Bot Jul 9, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Remediation recommended

1. Slack notifier missing regression test 📘 Rule violation ▣ Testability

The PR changes Slack message escaping and adds fetch timeouts, which alters release-notification
behavior but does not include an automated regression test to pin the new behavior. This risks
silent regressions in the release workflow and violates the requirement to add/update tests for
behavior changes.
Agent Prompt 
## Issue description
`scripts/notify-slack-release.mjs` behavior changed (mrkdwn escaping + request timeout), but there is no regression test added/updated in this PR to ensure the new behavior stays correct.

## Issue Context
This script runs in release workflows and is part of the release job’s observable behavior (Slack message formatting and network-call timeout). A regression test should verify escaping of `&`, `<`, `>` in the note/body/header and ensure fetch calls include a timeout signal.

## Fix Focus Areas
- scripts/notify-slack-release.mjs[38-78]
- __tests__/unit/scripts/notify-slack-release.test.ts[1-200]

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

Expand All @@ -59,6 +64,7 @@ try {
method: 'POST',
headers: { 'Content-Type': 'application/json; charset=utf-8' },
body: JSON.stringify({ text: fallbackText, blocks, unfurl_links: false }),
signal: AbortSignal.timeout(10000),
});
const t = await res.text().catch(() => '');
if (!res.ok) { warn(`webhook not ok: HTTP ${res.status} ${t}`); process.exit(0); }
Expand All @@ -68,6 +74,7 @@ try {
method: 'POST',
headers: { 'Content-Type': 'application/json; charset=utf-8', Authorization: `Bearer ${token}` },
body: JSON.stringify({ channel, text: fallbackText, blocks, unfurl_links: false }),
signal: AbortSignal.timeout(10000),
});
const j = await res.json().catch(() => ({}));
if (!res.ok || !j.ok) { warn(`chat.postMessage not ok: HTTP ${res.status} ${j.error || ''}`); process.exit(0); }
Expand Down