fix: remove hardcoded channel IDs and add permission-failure visibility#619
Open
full-bars wants to merge 3 commits into
Open
fix: remove hardcoded channel IDs and add permission-failure visibility#619full-bars wants to merge 3 commits into
full-bars wants to merge 3 commits into
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 adds failure-aware Discord send and thread helpers, moves channel IDs to environment-backed configuration, migrates multiple cogs to success-gated posting, and updates tropical and SPC outlook posts to use discussion threads. 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 full product text
DiscordHelpers->>Discord: send thread content
TropicalCog->>StateStore: record deduplication key after success
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
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