Skip to content

review: loop-escalation-wire's ALLOWED_DISCORD_HOSTS missing canary/ptb hosts its two siblings accept #9288

Description

@JSONbored

⚠️ Definition of Done: this issue must be completed in full, in a single PR. Do not split this
work across multiple PRs, and do not defer any Deliverable below to a follow-up issue. A PR that
satisfies only some of the Deliverables, stubs a required test, or leaves a checkbox
partially-done does NOT resolve this issue and will be closed.

Context

src/review/loop-escalation-wire.ts:31 defines:

const ALLOWED_DISCORD_HOSTS = new Set(["discord.com", "discordapp.com"]);

Two other Discord-webhook validators in this same codebase accept a wider, correct set:

  • src/review/alerts.ts:101new Set(["discord.com", "discordapp.com", "canary.discord.com", "ptb.discord.com"]), explicitly commented as "byte-faithful from reviewbot src/core/notify.ts".
  • src/services/notify-discord.ts:11 — the identical four-host set.

loop-escalation-wire.ts's narrower two-host set means its own isValidDiscordWebhook (used at
line 91) silently rejects a valid canary.discord.com or ptb.discord.com webhook URL that the
other two notification paths in this repo would accept — the notification is dropped as
invalid_global_webhook with no indication of why to the operator who configured
DISCORD_WEBHOOK_URL. There is no comment in loop-escalation-wire.ts explaining an
intentionally narrower scope, and test/unit/loop-escalation-wire.test.ts has zero test coverage
of the host allowlist, so this asymmetry is untested and unintentional.

Requirements

  • ALLOWED_DISCORD_HOSTS in src/review/loop-escalation-wire.ts must match the four-host set
    already used by src/review/alerts.ts and src/services/notify-discord.ts:
    discord.com, discordapp.com, canary.discord.com, ptb.discord.com.
  • Do not change isValidDiscordWebhook's other validation logic (path/protocol checks) — only the
    host allowlist constant.
  • Do not touch src/review/alerts.ts or src/services/notify-discord.ts — they are already
    correct; this issue only fixes the third, out-of-sync copy.

Deliverables

  • ALLOWED_DISCORD_HOSTS in src/review/loop-escalation-wire.ts updated to the four-host set
    matching the other two files exactly.
  • test/unit/loop-escalation-wire.test.ts gains test cases asserting a canary.discord.com
    and a ptb.discord.com webhook URL are both accepted by isValidDiscordWebhook (or the
    code path that calls it), and that a non-Discord host is still rejected (regression guard
    for the existing behavior).

Both deliverables are required in this single PR.

Test Coverage Requirements

This repo enforces 99%+ Codecov patch coverage, branch-counted, on every changed line/branch in
src/**. The new/extended test cases in test/unit/loop-escalation-wire.test.ts must exercise
both the newly-accepted hosts and a still-rejected non-Discord host as a regression check.

Expected Outcome

A canary.discord.com or ptb.discord.com webhook configured for loop-escalation notifications
is accepted the same way it already is by this repo's other two Discord notification paths,
instead of being silently dropped.

Links & Resources

  • src/review/loop-escalation-wire.ts:31,91 (the file to fix)
  • src/review/alerts.ts:101 (precedent — correct four-host set)
  • src/services/notify-discord.ts:11 (precedent — correct four-host set)
  • test/unit/loop-escalation-wire.test.ts (test file to extend)

Metadata

Metadata

Assignees

No one assigned

    Labels

    gittensor:bugGittensor-scored bug fix — scores a 0.05x multiplier.help wantedExtra attention is needed

    Projects

    No projects

      Relationships

      None yet

      Development

      No branches or pull requests

      Issue actions