feat(ci): keep Discord forum tags live (re-apply on state/label change via bot) - #1722
feat(ci): keep Discord forum tags live (re-apply on state/label change via bot)#1722mithileshgau wants to merge 5 commits into
Conversation
Tags were only set at post creation (webhook), so a PR stayed tagged "open"
forever and later label changes never showed. Webhooks can't edit applied_tags
after creation, so re-apply them with the bot token (now has Manage Posts) on
every event:
- New helper discord_set_tags <thread_id> <applied_tags_json> (mirrors
discord_archive_thread): PATCH /channels/{thread} with applied_tags. No-op
unless DISCORD_GITHUB_BOT_TOKEN is set, a thread id is known, and the list is
non-empty (empty = unconfigured channel, never clobber to untagged).
- Called in all three notifiers just before the archive step, with the
already-computed APPLIED_TAGS and FINAL_THREAD_ID (set while thread is active).
State tags now move open→closed→merged/draft/answered and label tags stay in
sync. Reuses the existing bot token; webhook still does all posting.
Depends on #1530 (marker-lookup fix) to reach the update path.
Closes #1721
🤖 Internal: Discord sync markerAuto-managed by the Discord notification workflow. Stores the linked Discord message ID and forum thread ID. Do not edit or delete. |
📝 WalkthroughWalkthroughDiscord issue, pull request, and discussion workflows now use ChangesDiscord forum thread synchronization
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant GitHubNotifier
participant discord_sync_thread
participant DiscordAPI
GitHubNotifier->>discord_sync_thread: FINAL_THREAD_ID, archive state, APPLIED_TAGS
discord_sync_thread->>DiscordAPI: PATCH archived and applied_tags
DiscordAPI-->>discord_sync_thread: HTTP status
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In @.github/workflows/scripts/discord-helper.sh:
- Around line 184-188: Update the PATCH request in the tag-update flow to
surface HTTP 4xx/5xx failures by using the shared retry/status mechanism or
capturing and logging the response status, while preserving best-effort
execution so the job does not fail. Replace the silent response discard and
unconditional success behavior around the curl invocation, keeping the existing
Discord endpoint and payload unchanged.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 2fcc4f26-02a6-41d6-a76f-9cff12e6873f
📒 Files selected for processing (4)
.github/workflows/discord-discussions.yml.github/workflows/discord-issues.yml.github/workflows/discord-pr.yml.github/workflows/scripts/discord-helper.sh
Deep review caught a real bug: discord_set_tags ran before (un)archiving, but Discord rejects an applied_tags edit on an already-archived thread (400, verified). So re-tagging silently failed on reopen and on threads auto-archived by inactivity — a reopened PR kept its stale "closed/merged" tag. Fold archive + tag sync into a single helper discord_sync_thread <thread_id> <archived> <tags> that sends archived and applied_tags in one PATCH, which works from any state (verified open→close→reopen). Replaces discord_archive_thread + discord_set_tags and their call sites in all three notifiers; also removes the redundant re-tag on the create path. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
The archive/tag PATCH discarded its response with `-o /dev/null … || true`, so 4xx/5xx (e.g. missing Manage Threads → 403) looked successful. Capture the HTTP status and emit a ::warning:: on non-2xx, while staying best-effort (always return 0, never fail the job). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In @.github/workflows/scripts/discord-helper.sh:
- Around line 172-176: Distinguish unavailable tag-sync configuration from
configured sync resolving to an empty tag list. Update discord-helper.sh around
discord_sync_thread() and its body construction to accept an explicit
sync-configured flag and omit applied_tags only when configuration is
unavailable; when configured, always include applied_tags, including []. Pass
this flag through the caller sites in .github/workflows/discord-discussions.yml
(lines 344-350), .github/workflows/discord-issues.yml (lines 238-245), and
.github/workflows/discord-pr.yml (lines 340-346), preserving their existing sync
behavior.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 9ef24414-aae8-475d-9d13-5f40fecd21e1
📒 Files selected for processing (4)
.github/workflows/discord-discussions.yml.github/workflows/discord-issues.yml.github/workflows/discord-pr.yml.github/workflows/scripts/discord-helper.sh
discord_applied_tags returns [] both when a channel has no tag config and when a configured section resolves to no matching IDs (e.g. a state/label name missing from the ids map). discord_sync_thread already omits applied_tags (never sends []) in that case, which is deliberate — clobbering to empty on a config typo is worse than leaving existing tags stale. Add a ::warning:: so a real misconfiguration is diagnosable instead of silent. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
asclearuc
left a comment
asclearuc
left a comment
There was a problem hiding this comment.
Thanks @mithileshgau — nicely scoped change, and the single-PATCH (archived + applied_tags) reasoning plus the safe empty-tags handling are well justified in the comments.
Approving: no blockers and CI is green. There is one non-blocking should fix inline on discord-helper.sh — route the sync PATCH through the existing discord_curl so it inherits the 429 retry the create path already has. For the record, CodeRabbit's Major on this thread (#1722 (comment)) was withdrawn by the bot (#1722 (comment)) after your clobber-risk rebuttal, so it does not block.
| body="{\"archived\": ${archived}, \"applied_tags\": ${tags}}" | ||
| fi | ||
| local status | ||
| status=$(curl -sS -o /dev/null -w '%{http_code}' --connect-timeout 10 --max-time 30 -X PATCH \ |
There was a problem hiding this comment.
should fix — reuse discord_curl so the sync PATCH inherits the 429 retry (DRY with the create path).
discord_sync_thread calls curl directly here and hand-captures the status with -w '%{http_code}'. This file already has discord_curl (lines 36–78), which the create-POST path uses: it writes the status to DISCORD_STATUS_FILE and retries on 429 (honoring Retry-After) and on 5xx. This PATCH does not, so a single Discord 429 during a busy relabel drops the tag/archive update (only a ::warning:: is logged) until the next event fires — which works against this PR's goal of keeping tags live. The PATCH sets a fixed target (archived + applied_tags), so it is idempotent and safe to retry. Route it through discord_curl and read the status back from DISCORD_STATUS_FILE, like the create path:
discord_curl -X PATCH \
-H "Authorization: Bot ${DISCORD_GITHUB_BOT_TOKEN}" \
-H "Content-Type: application/json" \
-d "$body" \
"https://discord.com/api/v10/channels/${thread_id}" >/dev/null 2>&1 || true
status=$(cat "$DISCORD_STATUS_FILE" 2>/dev/null || echo "")The existing case "$status" warning and return 0 stay as-is. (CodeRabbit's r3672402796 raised the status-logging angle; this is the follow-on retry gap, not a re-open of that thread.)
There was a problem hiding this comment.
Good catch — fixed. discord_sync_thread now routes through discord_curl (without --no-retry-5xx, since this PATCH is idempotent) and reads the status back from DISCORD_STATUS_FILE, exactly as suggested. Verified end-to-end under bash: 429/5xx now get the same retry as the create path; the existing warning + return 0 (best-effort) stay unchanged.
…etry Reviewer (asclearuc) caught: discord_sync_thread hand-rolled its own curl call instead of reusing discord_curl (already used by the create-POST path), so it had no retry on 429/5xx. The PATCH sets an absolute target state (archived + full applied_tags replace), so it's idempotent and safe to retry — unlike create-POST, this must NOT pass --no-retry-5xx. A single rate limit during a busy relabel was silently dropping the tag/archive update until the next event, working against the goal of keeping tags live. Verified end-to-end under bash (the real execution shell in CI): status read back via DISCORD_STATUS_FILE, tag applied, thread archived. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In @.github/workflows/scripts/discord-helper.sh:
- Around line 189-193: Update the PATCH request using discord_curl so it
suppresses only the response body while preserving stderr output, including
retry diagnostics for 429/5xx responses and transport failures; keep the
existing || true behavior unchanged.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: df7b705b-8712-4ad4-91ba-41c5a666e7a4
📒 Files selected for processing (1)
.github/workflows/scripts/discord-helper.sh
| discord_curl -X PATCH \ | ||
| -H "Authorization: Bot ${DISCORD_GITHUB_BOT_TOKEN}" \ | ||
| -H "Content-Type: application/json" \ | ||
| -d "{\"archived\": ${archived}}" \ | ||
| "https://discord.com/api/v10/channels/${thread_id}" || true | ||
| -d "$body" \ | ||
| "https://discord.com/api/v10/channels/${thread_id}" >/dev/null 2>&1 || true |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Preserve discord_curl retry diagnostics.
Line [193] redirects both stdout and stderr to /dev/null, discarding the shared helper’s 429/5xx and transport-failure warnings. Discard only the response body while keeping stderr visible:
Proposed fix
- "https://discord.com/api/v10/channels/${thread_id}" >/dev/null 2>&1 || true
+ "https://discord.com/api/v10/channels/${thread_id}" >/dev/null || true📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| discord_curl -X PATCH \ | |
| -H "Authorization: Bot ${DISCORD_GITHUB_BOT_TOKEN}" \ | |
| -H "Content-Type: application/json" \ | |
| -d "{\"archived\": ${archived}}" \ | |
| "https://discord.com/api/v10/channels/${thread_id}" || true | |
| -d "$body" \ | |
| "https://discord.com/api/v10/channels/${thread_id}" >/dev/null 2>&1 || true | |
| discord_curl -X PATCH \ | |
| -H "Authorization: Bot ${DISCORD_GITHUB_BOT_TOKEN}" \ | |
| -H "Content-Type: application/json" \ | |
| -d "$body" \ | |
| "https://discord.com/api/v10/channels/${thread_id}" >/dev/null || true |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In @.github/workflows/scripts/discord-helper.sh around lines 189 - 193, Update
the PATCH request using discord_curl so it suppresses only the response body
while preserving stderr output, including retry diagnostics for 429/5xx
responses and transport failures; keep the existing || true behavior unchanged.
kwit75
left a comment
kwit75
left a comment
There was a problem hiding this comment.
Approving. The single-PATCH decision is the load-bearing one and the reasoning behind it is right — Discord rejects an applied_tags edit on an already-archived thread with a 400, so tags-then-archive as two calls would fail on exactly the cases you want it to work: reopen, and threads Discord auto-archived by inactivity. Setting both in one PATCH is what makes it work from any starting state, and it is worth that it is spelled out in the comment rather than left for someone to rediscover.
I checked the thing most likely to make this silently do nothing. discord_sync_thread is called with "$APPLIED_TAGS", and if that variable were only assigned inside the create branch it would be empty on the update path — the guard would fire, tags would never be applied, and the feature would look installed while doing nothing. It is assigned unconditionally at the top level of the run: block in all three workflows (discord-issues.yml:133, discord-pr.yml:235, discord-discussions.yml:191), well before the sync call. So it is genuinely in scope. Also confirmed discord_curl writes $DISCORD_STATUS_FILE, so the status check reads a real value rather than an empty string.
Three things I think you got right that are easy to get wrong:
- Omitting the key rather than sending
applied_tags: []. An empty resolve is ambiguous — no tag config, or config present and nothing matched — and clobbering to untagged on a typo indiscord-forum-tags.jsonwould be a silent data loss that nobody notices for weeks. Leaving them untouched and emitting a warning is the right pair: safe default, visible signal. - Routing through
discord_curlfor the retry, and saying why it is safe. The PATCH sets an absolute target state, so it is idempotent and retry-safe, unlike the create POST. That distinction is exactly the kind of thing a later "let's make this consistent" commit would flatten, so having the reason inline protects it. - Surfacing non-2xx as a
::warning::while still returning 0. A 403 from a missing Manage Threads permission would otherwise be invisible — the job stays green and the tags quietly stop tracking. This is the difference between a green run that worked and a green run that did nothing, which is a distinction worth engineering for.
One thought, not a blocker. The no-tags warning fires on every run for any channel without tag config. If that is a legitimate state for one of the three channels rather than always a misconfiguration, it will become background noise and stop being read. Worth a look after a few days of real runs — if it is firing routinely somewhere, either that channel wants a config entry or the warning wants to be conditional on the config file existing.
CI green, and you say bot permissions are verified on all three channels. Good to merge.
3 participants
Summary
openforever, and later label changes never appeared (webhooks can't editapplied_tagsafter creation).discord_set_tags, called in all three notifiers before the archive step with theAPPLIED_TAGSthe workflow already computes.Type
feature (CI)
Testing
Tests added or updated
Tested locally
./builder testpassesYAML parses (3 workflows);
shellcheckclean on the helper.Live-tested against the real PR forum channel: created a post tagged
open, randiscord_set_tags→ tag becamemerged(200); empty-list guard verified (does not clear tags on an unconfigured channel); cleaned up.Reuses
DISCORD_GITHUB_BOT_TOKEN; no new secret. Webhook still does all posting/patching.Checklist
Dependency
Depends on #1530 (marker-lookup fix) — without it the notifier never reaches the update path, so the live re-tag can't fire. #1530 should merge first. Also requires the bot to have Manage Posts + View Channel on the forum channels (already granted).
Linked Issue
Closes #1721
Summary by CodeRabbit