fix: remove hardcoded channel IDs and add permission-failure visibility - #619
Merged
Conversation
Redirects the still-in-testing tropical auto-poster to the dev channel via the warning_channel:tropical override instead of the public production channel it had been silently failing against (403 Missing Permissions on every attempt). Removes the hardcoded fallback channel IDs in config.py, cogs/tropical.py, and cogs/fronts.py in favor of the same env-backed fallback-chain convention used everywhere else. Adds utils/discord_send.py (safe_send/safe_create_thread) so a missing- permissions failure in any background auto-poster is logged clearly instead of blending into generic exception noise, applied across tropical, mesoscale/MD, outlooks, watches, warnings, scp, ncar, reports, and recorder. Also fixes tropical's dedup-before-send, which permanently skipped a product on any failed post instead of retrying once the underlying issue was fixed.
📝 WalkthroughWalkthroughChangesThe PR centralizes Discord send and thread error handling, adds environment-backed channel routing, gates posting state on successful delivery, and updates tropical and SPC outlook products to use discussion threads with compact summaries and chunked raw content. Discord Posting and Routing
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant TropicalCog
participant DiscordHelpers
participant Discord
participant StateStore
TropicalCog->>DiscordHelpers: safe_send product summary
DiscordHelpers->>Discord: send message
Discord-->>DiscordHelpers: message or failure
DiscordHelpers-->>TropicalCog: result
TropicalCog->>DiscordHelpers: safe_create_thread(message)
DiscordHelpers->>Discord: create discussion thread
Discord-->>DiscordHelpers: thread or failure
TropicalCog->>DiscordHelpers: safe_send summary and raw product text
DiscordHelpers->>Discord: send thread content
TropicalCog->>StateStore: record deduplication key after success
Estimated code review effort: 4 (Complex) | ~45 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
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
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
cogs/warnings.py (1)
979-983: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winSafe-send migration is incomplete for the most safety-critical posting paths. Both files migrated a sibling send in the same function/file to
safe_send, but left another send path on rawchannel.send()with only generic exception logging (no channel name/ID) — missing exactly the permission-failure visibility this PR is meant to add, for the highest-stakes cases (new severe-warning issuance/updates, watch cancellations).
cogs/warnings.py#L979-L983,1053-L1058,1150-L1156: route_post_warning'schannel.send(embed=embed)(L1155) throughsafe_send, and update the_discover_and_post_warning/CON-update callers to check a truthy return instead of catchingdiscord.HTTPException(needed to preserve the claim-rollback-on-failure semantics).cogs/watches.py#L480-L491: replace the cancellationchannel.send(embed=embed)+try/except discord.HTTPExceptionwithsafe_send(...), gating the existingactive_watches[watch_num] = inforollback on a falsy return, mirroring the new-watch loop directly below it.🤖 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 `@cogs/warnings.py` around lines 979 - 983, Complete the safe-send migration for the specified warning and watch posting paths: in cogs/warnings.py at lines 979-983, 1053-1058, and 1150-1156, route _post_warning’s channel.send(embed=embed) through safe_send and update the _discover_and_post_warning and CON-update callers to use its truthy result while preserving claim rollback on failure; in cogs/watches.py at lines 480-491, replace the cancellation send and HTTPException handler with safe_send and restore active_watches[watch_num] only when the send returns falsy, matching the new-watch loop behavior.
🧹 Nitpick comments (1)
utils/discord_send.py (1)
47-49: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winInconsistent failure-logging level vs.
safe_send.
safe_create_thread's generic-exception branch useslogger.warningwithout a traceback, whilesafe_send's equivalent branch useslogger.exception. This module's stated purpose is to avoid failures being "buried in a traceback" — but this branch does the opposite for non-Forbidden thread-creation errors, making them harder to diagnose than send failures.🛠️ Proposed fix
except Exception as e: - logger.warning(f"Failed to create thread for {context}: {e}") + logger.exception(f"Failed to create thread for {context}: {e}") return None🤖 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 `@utils/discord_send.py` around lines 47 - 49, Update the generic exception handler in safe_create_thread to use logger.exception instead of logger.warning, preserving the existing failure message and return None behavior so thread-creation errors include their traceback consistently with safe_send.
🤖 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 `@cogs/outlooks.py`:
- Around line 169-174: Update both full-text thread-posting sites in
cogs/outlooks.py (lines 169-186 and 316-333) to send through safe_send instead
of calling thread.send directly, preserving the existing post content and state
flow while using the helper’s permission-aware logging.
---
Outside diff comments:
In `@cogs/warnings.py`:
- Around line 979-983: Complete the safe-send migration for the specified
warning and watch posting paths: in cogs/warnings.py at lines 979-983,
1053-1058, and 1150-1156, route _post_warning’s channel.send(embed=embed)
through safe_send and update the _discover_and_post_warning and CON-update
callers to use its truthy result while preserving claim rollback on failure; in
cogs/watches.py at lines 480-491, replace the cancellation send and
HTTPException handler with safe_send and restore active_watches[watch_num] only
when the send returns falsy, matching the new-watch loop behavior.
---
Nitpick comments:
In `@utils/discord_send.py`:
- Around line 47-49: Update the generic exception handler in safe_create_thread
to use logger.exception instead of logger.warning, preserving the existing
failure message and return None behavior so thread-creation errors include their
traceback consistently with safe_send.
🪄 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: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 0a9c23f8-773c-45c7-94be-1633efb86d6d
📒 Files selected for processing (15)
.env.exampleCHANGELOG.mdcogs/fronts.pycogs/mesoscale.pycogs/ncar.pycogs/outlooks.pycogs/recorder.pycogs/reports.pycogs/scp.pycogs/tropical.pycogs/warnings.pycogs/watches.pyconfig.pytests/test_warnings.pyutils/discord_send.py
Comment on lines
+169
to
+174
| thread = await safe_create_thread( | ||
| msg, | ||
| context=f"SPC Day {day} outlook", | ||
| name=f"SPC Day {day} Outlook", | ||
| auto_archive_duration=1440, | ||
| ) |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
python3 - <<'PY'
from pathlib import Path
path = Path("cogs/outlooks.py")
text = path.read_text()
for needle in [
"safe_create_thread(",
"thread.send(",
"safe_send(",
]:
print(f"\n== {needle} ==")
for i, line in enumerate(text.splitlines(), 1):
if needle in line:
print(f"{i}: {line}")
PYRepository: full-bars/spc-bot
Length of output: 562
🏁 Script executed:
python3 - <<'PY'
from pathlib import Path
path = Path("cogs/outlooks.py")
lines = path.read_text().splitlines()
for start, end in [(140, 200), (285, 345)]:
print(f"\n--- lines {start}-{end} ---")
for i in range(start, end + 1):
if i <= len(lines):
print(f"{i}: {lines[i-1]}")
PYRepository: full-bars/spc-bot
Length of output: 6378
🏁 Script executed:
python3 - <<'PY'
from pathlib import Path
# Find safe_send / safe_create_thread definitions and usages.
for path in Path(".").rglob("*.py"):
text = path.read_text(errors="ignore")
if "def safe_send" in text or "async def safe_send" in text or "safe_send(" in text:
if path.as_posix() != "cogs/outlooks.py":
print(f"\n== {path} ==")
for i, line in enumerate(text.splitlines(), 1):
if "def safe_send" in line or "async def safe_send" in line or "safe_send(" in line:
print(f"{i}: {line}")
PYRepository: full-bars/spc-bot
Length of output: 1233
🏁 Script executed:
python3 - <<'PY'
from pathlib import Path
# Show the helper and any thread-related wrappers/usages.
for file in ["utils/discord_send.py", "utils/discord_thread.py", "utils/discord.py"]:
p = Path(file)
if p.exists():
print(f"\n=== {file} ===")
lines = p.read_text().splitlines()
for i, line in enumerate(lines, 1):
if "safe_send" in line or "safe_create_thread" in line or "thread" in line.lower():
print(f"{i}: {line}")
print("\n=== thread.send / safe_send on thread-like objects ===")
for path in Path(".").rglob("*.py"):
text = path.read_text(errors="ignore")
if "thread.send(" in text or "safe_send(" in text and "thread" in text:
# print only files that look relevant
if path.as_posix() in {
"cogs/outlooks.py",
"cogs/tropical.py",
"cogs/mesoscale.py",
"cogs/warnings.py",
"cogs/scp.py",
"cogs/reports.py",
"cogs/watches.py",
"cogs/ncar.py",
"cogs/recorder.py",
}:
print(f"\n== {path} ==")
for i, line in enumerate(text.splitlines(), 1):
if "thread.send(" in line or "safe_send(" in line:
print(f"{i}: {line}")
PYRepository: full-bars/spc-bot
Length of output: 1492
🏁 Script executed:
python3 - <<'PY'
from pathlib import Path
p = Path("utils/discord_send.py")
lines = p.read_text().splitlines()
for i, line in enumerate(lines, 1):
print(f"{i}: {line}")
PYRepository: full-bars/spc-bot
Length of output: 2182
🏁 Script executed:
python3 - <<'PY'
from pathlib import Path
# Inspect the analogous thread-target send in cogs/tropical.py and any surrounding flow.
p = Path("cogs/tropical.py")
lines = p.read_text().splitlines()
for start, end in [(240, 290)]:
print(f"\n--- lines {start}-{end} ---")
for i in range(start, end + 1):
if i <= len(lines):
print(f"{i}: {lines[i-1]}")
PYRepository: full-bars/spc-bot
Length of output: 2228
🏁 Script executed:
python3 - <<'PY'
from pathlib import Path
p = Path("cogs/outlooks.py")
lines = p.read_text().splitlines()
for start, end in [(150, 240), (300, 380)]:
print(f"\n--- lines {start}-{end} ---")
for i in range(start, end + 1):
if i <= len(lines):
print(f"{i}: {lines[i-1]}")
PYRepository: full-bars/spc-bot
Length of output: 8113
Route the thread full-text post through safe_send. The direct thread.send(...) calls bypass the helper’s permission-aware logging, so failures here are only surfaced by the outer catch after the post state has already been recorded.
cogs/outlooks.py#L169-L186cogs/outlooks.py#L316-L333
📍 Affects 1 file
cogs/outlooks.py#L169-L174(this comment)cogs/outlooks.py#L316-L321
🤖 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 `@cogs/outlooks.py` around lines 169 - 174, Update both full-text
thread-posting sites in cogs/outlooks.py (lines 169-186 and 316-333) to send
through safe_send instead of calling thread.send directly, preserving the
existing post content and state flow while using the helper’s permission-aware
logging.
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
cogs/reports.py (1)
282-285: 🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy liftDo not mark a multi-report LSR product as posted after the first successful send.
This handler splits one product into multiple reports, but
product_idis persisted immediately after any single successful message. If a later report fails, the next invocation skips the entire product and that report can never retry. Track posted state per report, or defer the product-level marker until every report succeeds while preventing duplicate sends.🤖 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 `@cogs/reports.py` around lines 282 - 285, Update the handler around safe_send and add_posted_report so a multi-report LSR product is not marked posted after only the first successful report. Track completion per report, or defer the product-level marker until all reports send successfully, while preserving duplicate-send prevention and allowing failed reports to retry on later invocations.
🤖 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.
Outside diff comments:
In `@cogs/reports.py`:
- Around line 282-285: Update the handler around safe_send and add_posted_report
so a multi-report LSR product is not marked posted after only the first
successful report. Track completion per report, or defer the product-level
marker until all reports send successfully, while preserving duplicate-send
prevention and allowing failed reports to retry on later invocations.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 275e771c-4b4c-4b27-ad0d-840aa5951a8b
📒 Files selected for processing (2)
CHANGELOG.mdcogs/reports.py
🚧 Files skipped from review as they are similar to previous changes (1)
- CHANGELOG.md
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 `@cogs/tropical.py`:
- Around line 390-409: Update the tropical product posting flow around
summary_embed, raw_text_embed, and safe_send so text exceeding Discord’s
4096-character embed description limit is split into ordered continuation embeds
rather than truncated. Preserve all parsed summary and raw_text content,
including the existing titles and colors where appropriate, and ensure each
resulting embed remains within the limit.
🪄 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: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 183114b8-d65a-4b2c-af2b-29a63d63f60f
📒 Files selected for processing (2)
cogs/tropical.pytests/test_tropical.py
- _post_warning: uses safe_send instead of raw channel.send, returns (None, area) on failure so callers can abort cleanly - _discover_and_post_warning: guard against None msg, remove obsolete HTTPException catch - CON update loop: remove redundant try/except (safe_send handles it) - Watch cancellation: uses safe_send with watch rollback on failure - LSR reports: defer add_posted_report until all reports succeed
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
cogs/warnings.py (1)
771-776: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winPropagate failed-send results before advancing warning state.
safe_sendnow returnsNoneon failure, but these paths still suppress retries:
cogs/warnings.py#L771-L776: make_handle_cancellationreturn success and only let its callers removeactive_warnings/ mark the product handled after a cancellation was posted. Otherwise a failed cancellation is never retried.cogs/warnings.py#L978-L980: capture_post_warning’s message and skip the lateradd_posted_warning(... area=curr_area ...)update when it isNone; otherwise the changed area is persisted despite no update post and future polls will not retry it.🤖 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 `@cogs/warnings.py` around lines 771 - 776, Update cogs/warnings.py at lines 771-776 and 978-980: make _handle_cancellation return whether safe_send posted successfully, and have its callers remove active_warnings or mark the product handled only on success so failed cancellations retry. In the warning flow, capture _post_warning’s returned message and call add_posted_warning(... area=curr_area ...) only when that result is not None.
🤖 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.
Outside diff comments:
In `@cogs/warnings.py`:
- Around line 771-776: Update cogs/warnings.py at lines 771-776 and 978-980:
make _handle_cancellation return whether safe_send posted successfully, and have
its callers remove active_warnings or mark the product handled only on success
so failed cancellations retry. In the warning flow, capture _post_warning’s
returned message and call add_posted_warning(... area=curr_area ...) only when
that result is not None.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 5b60db00-8400-40eb-ac45-9666c0e3e38d
📒 Files selected for processing (5)
cogs/reports.pycogs/tropical.pycogs/warnings.pycogs/watches.pyutils/discord_send.py
🚧 Files skipped from review as they are similar to previous changes (4)
- utils/discord_send.py
- cogs/reports.py
- cogs/watches.py
- cogs/tropical.py
1 task
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
warning_channel:tropicaloverride) instead of the public production channel it was defaulting to — that channel was in fact silently failing every post withdiscord.errors.Forbidden: 403 Missing Permissions.config.py'stropical_channel_id(now uses the same_optional_intfallback chain as every other channel), a dead/unusedDEV_CHANNEL_IDconstant incogs/tropical.py, and a hardcodedWEATHER_CHAT_CHANNEL_IDincogs/fronts.py(now a proper.env-backed config value).utils/discord_send.py(safe_send/safe_create_thread) so a missing-permissions failure in any background auto-poster logs a clear ERROR with the channel name/ID, instead of blending into generic exception noise or (in a couple of spots) being swallowed entirely. Applied to tropical, mesoscale/MD, outlooks, watches, warnings, scp, ncar, reports, and recorder — scoped to background task-loop posts only; interactive slash-command replies already surface errors to the invoking user and were left untouched.Test plan
pytest tests/— 569 passedruff check/ruff formatcleanconfig.TROPICAL_CHANNEL_IDcorrectly falls through toDEV_CHANNEL_IDwhenTROPICAL_CHANNEL_IDis unsetwarning_channel:tropical→ dev channel) ahead of this mergeSummary by CodeRabbit
.env.examplenotes for tropical/weather channel routing.