diff --git a/README.md b/README.md index b9a3f1e..13675a4 100644 --- a/README.md +++ b/README.md @@ -283,8 +283,9 @@ off, so the page exists on one host only. and search terms are passed to yt-dlp after `--`, so they are never read as options. - **Rate limiting:** `!play`, which starts a yt-dlp process each time, takes - at most 3 calls per 5 seconds and one at a time per user - (`tests/test_rate_limits.py` triggers both limits). Effect changes are + at most 3 calls per 5 seconds and one at a time per user, and `!lyrics`, + which calls LRCLIB and Genius, at most 3 per 10 seconds per user + (`tests/test_rate_limits.py` triggers each limit). Effect changes are throttled to one every three seconds per server, and `/` commands are only registered through the owner-only `!sync`, because Discord rate-limits syncing. diff --git a/cogs/effects.py b/cogs/effects.py index 702cf61..3473db9 100644 --- a/cogs/effects.py +++ b/cogs/effects.py @@ -58,6 +58,8 @@ class Effect: # `ctx.invoked_with` says which name the user actually typed. _EFFECT_NAMES = list(EFFECTS) +_NO_EFFECT = Effect("", "Audio effects removed ✅") + class Effects(commands.Cog, name="🎛️ Audio Effects"): def __init__(self, bot: commands.Bot) -> None: @@ -71,8 +73,7 @@ def _throttled(self, ctx: GuildContext) -> bool: self._last_change[ctx.guild.id] = now return False - async def _switch_to(self, ctx: GuildContext, name: str | None, filter_str: str, label: str, - rate: float = 1.0) -> None: + async def _switch_to(self, ctx: GuildContext, name: Optional[str], effect: Effect) -> None: """Throttle, apply, and report — the whole path every effect command takes.""" if self._throttled(ctx): await ctx.send(embed=error_embed( @@ -80,8 +81,8 @@ async def _switch_to(self, ctx: GuildContext, name: str | None, filter_str: str, ), ephemeral=True) return player = players.get(ctx.guild.id) - if player and player.apply_effect(name, filter_str, rate): - await ctx.send(embed=success_embed(label)) + if player and player.apply_effect(name, effect.filter, effect.rate): + await ctx.send(embed=success_embed(effect.label)) else: await ctx.send(embed=error_embed("Nothing is playing."), ephemeral=True) @@ -92,14 +93,13 @@ async def _switch_to(self, ctx: GuildContext, name: str | None, filter_str: str, async def apply_effect(self, ctx: GuildContext) -> None: # Always set here: this callback only runs when invoked by one of its names. name = (ctx.invoked_with or _EFFECT_NAMES[0]).lower() - effect = EFFECTS[name] - await self._switch_to(ctx, name, effect.filter, effect.label, effect.rate) + await self._switch_to(ctx, name, EFFECTS[name]) @hybrid_command(name="reset", aliases=["fxreset", "noeffect"]) @same_voice_channel() async def reset_effect(self, ctx: GuildContext) -> None: """Remove all audio effects.""" - await self._switch_to(ctx, None, "", "Audio effects removed ✅") + await self._switch_to(ctx, None, _NO_EFFECT) @hybrid_command(name="effect") @app_commands.describe(name="The effect to apply; leave it out to see the active one") @@ -118,7 +118,7 @@ async def effect(self, ctx: GuildContext, name: Optional[str] = None) -> None: f"Unknown effect `{name}`. See `{ctx.clean_prefix}effects`."), ephemeral=True) return if await in_same_voice_channel(ctx): - await self._switch_to(ctx, name, chosen.filter, chosen.label, chosen.rate) + await self._switch_to(ctx, name, chosen) @hybrid_command(name="effects") async def list_effects(self, ctx: GuildContext) -> None: diff --git a/cogs/lyrics.py b/cogs/lyrics.py index 8c0784e..af0cedf 100644 --- a/cogs/lyrics.py +++ b/cogs/lyrics.py @@ -310,6 +310,9 @@ async def show_current(self, dest: discord.abc.Messageable, # -- The command --------------------------------------------------- @hybrid_command(aliases=["ly"]) + # One lookup is up to two LRCLIB and two Genius requests; spamming it would + # get the bot's address rate-limited by both. + @commands.cooldown(rate=3, per=10.0, type=commands.BucketType.user) async def lyrics(self, ctx: GuildContext, *, query: Optional[str] = None) -> None: """Follow the lyrics of the current song, or look a song up.""" diff --git a/cogs/music.py b/cogs/music.py index a6b755c..5b0b290 100644 --- a/cogs/music.py +++ b/cogs/music.py @@ -5,6 +5,7 @@ import aiohttp import discord from discord import app_commands +from discord.channel import VocalGuildChannel from discord.ext import commands from services import media, suggestions @@ -30,14 +31,18 @@ def _is_playlist_url(query: str) -> bool: """Detect playlist/set/album URLs across supported sites.""" - q = query.lower() - if not q.startswith("http"): + if not media.is_url(query): return False + q = query.lower() if "list=" in q and "watch?v=" not in q: return True # YouTube playlist return "/sets/" in q or "/album/" in q # SoundCloud set / Bandcamp album +def _has_listeners(channel: VocalGuildChannel) -> bool: + return any(not member.bot for member in channel.members) + + async def _reply(ctx: GuildContext, outcome: controls.Outcome) -> None: """Report an action's outcome; under `/`, a failure only to whoever asked.""" embed = success_embed(outcome.message) if outcome.ok else error_embed(outcome.message) @@ -110,7 +115,7 @@ async def play(self, ctx: GuildContext, *, query: str) -> None: if len(query) > MAX_QUERY_LEN: await ctx.send(embed=error_embed("That query is too long.")) return - if query.startswith("http") and not await media.is_public_url(query): + if media.is_url(query) and not await media.is_public_url(query): await ctx.send(embed=error_embed( "I can only play links to public websites.")) return @@ -120,13 +125,13 @@ async def play(self, ctx: GuildContext, *, query: str) -> None: async with ctx.typing(): if _is_playlist_url(query): - tracks = await media.get_playlist(query, loop=self.bot.loop) + tracks = await media.get_playlist(query) if not tracks: await ctx.send(embed=error_embed("Couldn't load that playlist.")) return return await self._enqueue(ctx, player, tracks, "playlist") - track = await media.search(query, loop=self.bot.loop) + track = await media.search(query) if not track: await ctx.send(embed=error_embed(f"No results found for `{query}`.")) return @@ -224,7 +229,9 @@ async def nowplaying(self, ctx: GuildContext) -> None: if not player or not player.current: await ctx.send(embed=error_embed("Nothing is playing right now.")) return - await ctx.send(embed=now_playing_embed(player.current, ctx.author, loop_mode=player.loop_mode)) + requester = player.current.get("requester") or ctx.me # autoplay picks have none + await ctx.send(embed=now_playing_embed(player.current, requester, + loop_mode=player.loop_mode)) @hybrid_command() @same_voice_channel() @@ -320,9 +327,9 @@ async def on_voice_state_update(self, member: discord.Member, return if before.channel != vc.channel: return - if len([m for m in vc.channel.members if not m.bot]) == 0: + if not _has_listeners(vc.channel): await asyncio.sleep(60) - if vc.is_connected() and len([m for m in vc.channel.members if not m.bot]) == 0: + if vc.is_connected() and not _has_listeners(vc.channel): player = players.get(member.guild.id) if player: player.destroy() diff --git a/services/media.py b/services/media.py index 70ecb1f..cd4e356 100644 --- a/services/media.py +++ b/services/media.py @@ -118,18 +118,28 @@ def _build_track(info: dict[str, Any], *, query: str = "") -> Track: } +def is_url(query: str) -> bool: + """ + Whether ``query`` is a link rather than search terms. + + The one definition every caller shares: it decides whether the private + network check runs, so any second, stricter copy of it is a way around that + check. Case-insensitive, because yt-dlp accepts ``HTTP://`` as readily. + """ + return query.lower().startswith(("http://", "https://")) + + def _first_thumb(info: dict[str, Any]) -> Optional[str]: thumbs = info.get("thumbnails") or [] return thumbs[-1]["url"] if thumbs else None -def _run(opts: dict[str, Any], target: str, *, - loop: asyncio.AbstractEventLoop) -> asyncio.Future[Any]: +def _run(opts: dict[str, Any], target: str) -> asyncio.Future[Any]: """Run yt-dlp's blocking extract_info in a thread executor.""" def _extract() -> Any: with yt_dlp.YoutubeDL(opts) as ydl: return ydl.extract_info(target, download=False) - return loop.run_in_executor(None, _extract) + return asyncio.get_running_loop().run_in_executor(None, _extract) def _search_target(query: str) -> tuple[str, bool]: @@ -143,21 +153,19 @@ def _search_target(query: str) -> tuple[str, bool]: for prefix, engine in _SEARCH_PREFIXES.items(): if lowered.startswith(prefix): return engine + query[len(prefix):].strip(), True - if query.startswith("http"): - return query, False # a URL — extract it directly + if is_url(query): + return query, False # extract it directly return f"ytsearch1:{query}", True # ── Public API ──────────────────────────────────────────────────────── -async def search(query: str, *, - loop: Optional[asyncio.AbstractEventLoop] = None) -> Optional[Track]: +async def search(query: str) -> Optional[Track]: """Resolve a single track from a search term or any supported URL.""" - loop = loop or asyncio.get_event_loop() target, flat_ok = _search_target(query) opts = {**YTDL_OPTIONS, "extract_flat": flat_ok} try: - info = await _run(opts, target, loop=loop) + info = await _run(opts, target) info = _first_entry(info) return _build_track(info, query=query) if info else None except Exception as e: @@ -165,13 +173,11 @@ async def search(query: str, *, return None -async def search_many(query: str, limit: int = 5, *, - loop: Optional[asyncio.AbstractEventLoop] = None) -> list[Track]: +async def search_many(query: str, limit: int = 5) -> list[Track]: """Return up to ``limit`` YouTube search results (metadata only).""" - loop = loop or asyncio.get_event_loop() opts = {**YTDL_OPTIONS, "extract_flat": True} try: - info = await _run(opts, f"ytsearch{limit}:{query}", loop=loop) + info = await _run(opts, f"ytsearch{limit}:{query}") entries = (info or {}).get("entries", []) return [_build_track(e, query=query) for e in entries if e] except Exception as e: @@ -179,13 +185,11 @@ async def search_many(query: str, limit: int = 5, *, return [] -async def get_playlist(url: str, *, - loop: Optional[asyncio.AbstractEventLoop] = None) -> list[Track]: +async def get_playlist(url: str) -> list[Track]: """Extract every track from a playlist/set/album URL (metadata only).""" - loop = loop or asyncio.get_event_loop() opts = {**YTDL_OPTIONS, "noplaylist": False, "extract_flat": True} try: - info = await _run(opts, url, loop=loop) + info = await _run(opts, url) entries = (info or {}).get("entries", []) return [_build_track(e) for e in entries if e] except Exception as e: @@ -193,13 +197,12 @@ async def get_playlist(url: str, *, return [] -async def related(track: Track, *, - loop: Optional[asyncio.AbstractEventLoop] = None) -> Optional[Track]: +async def related(track: Track) -> Optional[Track]: """Approximate a 'related' track for autoplay via a themed search.""" seed = track.get("uploader") or track.get("title") or "" if not seed: return None - for cand in await search_many(f"{seed} mix", limit=5, loop=loop): + for cand in await search_many(f"{seed} mix", limit=5): if cand.get("url") and cand["url"] != track.get("url"): return cand return None diff --git a/services/suggestions.py b/services/suggestions.py index fde0e0e..64f6a8e 100644 --- a/services/suggestions.py +++ b/services/suggestions.py @@ -12,6 +12,8 @@ import aiohttp +from services.media import is_url + log = logging.getLogger("loopify.suggestions") SUGGEST_URL = "https://suggestqueries.google.com/complete/search" @@ -23,7 +25,7 @@ async def fetch(session: aiohttp.ClientSession, query: str) -> list[str]: """Up to MAX_SUGGESTIONS search terms completing ``query``.""" query = query.strip() - if not query or query.startswith("http"): + if not query or is_url(query): return [] try: async with session.get( diff --git a/tests/test_commands.py b/tests/test_commands.py index 34b851d..510c4f1 100644 --- a/tests/test_commands.py +++ b/tests/test_commands.py @@ -155,13 +155,28 @@ async def test_play_rejects_an_over_length_query(music_cog, ctx): @pytest.mark.parametrize("url", [ "http://192.168.1.1/admin", "http://127.0.0.1:8080/", + # yt-dlp lowercases the scheme itself, so case must not skip the check — + # least of all on the playlist path, whose own detection ignores case. + "HTTP://192.168.1.1/admin", + "Http://10.0.0.1/playlist?list=PL123", + "HTTPS://10.0.0.1/sets/x", ]) async def test_play_refuses_links_into_private_networks(music_cog, ctx, url): ctx.author.voice.channel = MagicMock() - with patch("cogs.music.media.search", new=AsyncMock()) as search: + with patch("cogs.music.media.search", new=AsyncMock()) as search, \ + patch("cogs.music.media.get_playlist", new=AsyncMock()) as playlist: await Music.play.callback(music_cog, ctx, query=url) assert "public" in sent_text(ctx) search.assert_not_awaited() + playlist.assert_not_awaited() + + +async def test_play_does_not_mistake_a_search_for_a_link(music_cog, ctx): + music_cog._ensure_voice = AsyncMock(return_value=False) + with patch("cogs.music.media.is_public_url", new=AsyncMock()) as check: + await Music.play.callback(music_cog, ctx, query="httpster live") + check.assert_not_awaited() + music_cog._ensure_voice.assert_awaited_once() async def test_shuffle_on_an_empty_queue(music_cog, ctx): @@ -366,9 +381,8 @@ async def test_effect_by_name_applies_it(effects_cog, ctx): ctx.author.voice.channel = ctx.voice_client with patch.object(effects_cog, "_switch_to", new=AsyncMock()) as switch: await Effects.effect.callback(effects_cog, ctx, "Nightcore") - name, flt, _label, rate = switch.await_args.args[1:] - assert name == "nightcore" and flt == EFFECTS["nightcore"].filter - assert rate == EFFECTS["nightcore"].rate + name, effect = switch.await_args.args[1:] + assert name == "nightcore" and effect is EFFECTS["nightcore"] async def test_an_unknown_effect_name_is_refused(effects_cog, ctx): @@ -390,3 +404,31 @@ async def test_effects_list_under_slash_points_at_effect(effects_cog, ctx): await Effects.list_effects.callback(effects_cog, ctx) text = sent_text(ctx) assert "/effect" in text and "`/bass`" not in text + + +# -- what the read-only commands show ----------------------------------- + +async def test_nowplaying_credits_whoever_queued_the_track(music_cog, ctx, fake_bot, + fake_guild, track_factory): + queued_by = MagicMock(mention="<@queued-by>") + player = players.get_or_create(fake_bot, ctx.guild, ctx.channel) + try: + player.current = track_factory("Song", requester=queued_by) + ctx.author.mention = "<@asked-for-np>" + await Music.nowplaying.callback(music_cog, ctx) + finally: + players.discard(fake_guild.id) + embed = ctx.send.await_args.kwargs["embed"] + requested = next(f.value for f in embed.fields if "Requested" in f.name) + assert requested == "<@queued-by>" + + +@pytest.mark.parametrize("page,shown", [(0, 1), (-1, 1), (99, 3)]) +def test_queue_pages_out_of_range_show_the_nearest_real_one(track_factory, page, shown): + from utils.embeds import queue_embed + + queue = [track_factory(f"T{i}") for i in range(1, 26)] # three pages + embed = queue_embed(queue, None, page=page) + assert embed.footer.text.startswith(f"Page {shown}/3") + first = (shown - 1) * 10 + 1 + assert embed.fields[0].value.startswith(f"`{first}.` **T{first}**") diff --git a/tests/test_errors.py b/tests/test_errors.py index 36e2dc6..44ec31f 100644 --- a/tests/test_errors.py +++ b/tests/test_errors.py @@ -149,6 +149,8 @@ async def test_an_unexpected_error_is_acknowledged_and_logged(ctx, caplog): assert "went wrong" in sent_text(ctx).lower() assert "something broke internally" in caplog.text, "the operator still needs it" + # A bug, not a user mistake: it must stand out from the warnings around it. + assert any(r.levelname == "ERROR" and r.exc_info for r in caplog.records) async def test_the_acknowledgement_leaks_no_internals(ctx): diff --git a/tests/test_media_helpers.py b/tests/test_media_helpers.py index 7967b62..1cb8e7a 100644 --- a/tests/test_media_helpers.py +++ b/tests/test_media_helpers.py @@ -45,6 +45,16 @@ def test_urls_are_passed_through_untouched(url): assert flat_ok is False, "a URL must be extracted directly, not flat-listed" +def test_url_detection_ignores_the_case_of_the_scheme(): + target, flat_ok = media._search_target("HTTPS://www.youtube.com/watch?v=abc") + assert target == "HTTPS://www.youtube.com/watch?v=abc" + assert flat_ok is False + + +def test_a_word_starting_with_http_is_a_search_not_a_link(): + assert media._search_target("httpster live") == ("ytsearch1:httpster live", True) + + def test_a_prefix_wins_over_url_detection(): """`sc: https://...` should search SoundCloud, not extract the URL.""" target, _ = media._search_target("sc: https://example.invalid/x") diff --git a/tests/test_rate_limits.py b/tests/test_rate_limits.py index 4f7911b..48cff58 100644 --- a/tests/test_rate_limits.py +++ b/tests/test_rate_limits.py @@ -1,6 +1,7 @@ """ `!play` is the expensive path — every call starts a yt-dlp process on a small -host — so its limits must hold, not just exist. +host — and `!lyrics` calls out to two third-party APIs, so their limits must +hold, not just exist. The command tests call ``.callback()``, which skips discord.py's limiter. These run the two steps ``Command.prepare`` runs before every invocation: the @@ -13,6 +14,7 @@ import pytest from discord.ext import commands +from cogs.lyrics import Lyrics from cogs.music import Music PLAY = Music.play @@ -70,3 +72,14 @@ async def test_one_play_at_a_time_per_user(): await limit.acquire(ctx_for(2, 1)) # another user may play await limit.release(first) await limit.acquire(ctx_for(1, 2)) # free again once done + + +def test_lyrics_is_limited_per_user(): + """Each lookup can reach LRCLIB and Genius up to twice; spam must not.""" + lyrics = Lyrics.lyrics + lyrics._buckets._cache.clear() + for second in (0, 1, 2): + lyrics._prepare_cooldowns(ctx_for(1, second)) + with pytest.raises(commands.CommandOnCooldown): + lyrics._prepare_cooldowns(ctx_for(1, 3)) + lyrics._prepare_cooldowns(ctx_for(2, 3)) # someone else is unaffected diff --git a/utils/embeds.py b/utils/embeds.py index 34fae84..c827d97 100644 --- a/utils/embeds.py +++ b/utils/embeds.py @@ -63,6 +63,8 @@ def queue_embed(queue: list[Track], current: Optional[Track], page: int = 1, inline=False, ) + total_pages = max(1, (len(queue) + per_page - 1) // per_page) + page = max(1, min(page, total_pages)) start = (page - 1) * per_page page_items = queue[start:start + per_page] if page_items: @@ -75,7 +77,6 @@ def queue_embed(queue: list[Track], current: Optional[Track], page: int = 1, value = "*Queue is empty*" embed.add_field(name="⏭ Up Next", value=value, inline=False) - total_pages = max(1, (len(queue) + per_page - 1) // per_page) embed.set_footer(text=f"Page {page}/{total_pages} • {len(queue)} tracks in queue") return embed diff --git a/utils/errors.py b/utils/errors.py index bc9e990..f8380fe 100644 --- a/utils/errors.py +++ b/utils/errors.py @@ -83,7 +83,7 @@ async def handle(ctx: commands.Context, error: Exception) -> None: # and stays in the log — a stack trace in the channel helps nobody — but # saying nothing at all is worse. A TypeError in !lyrics was logged and # never answered, and from the channel the bot simply looked dead. - log.warning("Error in command %s: %s", ctx.command, error, exc_info=error) + log.error("Error in command %s: %s", ctx.command, error, exc_info=error) await _reply(ctx, "Something went wrong on my side. It has been logged.") diff --git a/utils/now_playing_view.py b/utils/now_playing_view.py index 431a99f..0905d6d 100644 --- a/utils/now_playing_view.py +++ b/utils/now_playing_view.py @@ -81,7 +81,7 @@ async def _announce(self, interaction: discord.Interaction, f"{outcome.message} — {interaction.user.mention}")) async def _edit_in_place(self, interaction: discord.Interaction, - outcome: controls.Outcome) -> None: + outcome: controls.Outcome) -> None: if not outcome.ok: return await _private_error(interaction, outcome.message) await interaction.response.edit_message(embed=self.embed(), view=self) diff --git a/utils/player.py b/utils/player.py index fd43677..151ac94 100644 --- a/utils/player.py +++ b/utils/player.py @@ -240,7 +240,7 @@ def _prefetch_delay(self, track: Track) -> Optional[float]: idle that long — the stream would then be dead by the time we wanted it. Live streams have no end to count back from, so they prefetch now. """ - duration = (track or {}).get("duration") + duration = track.get("duration") if not duration: return None remaining = duration - self.elapsed - PREFETCH_LEAD_SECONDS @@ -343,7 +343,7 @@ def go_previous(self) -> bool: def set_volume(self, vol: float) -> None: self.volume = vol vc = self.voice - if vc and vc.source and isinstance(vc.source, discord.PCMVolumeTransformer): + if vc and isinstance(vc.source, discord.PCMVolumeTransformer): vc.source.volume = vol # ── The single playback loop ────────────────────────────────────── @@ -477,7 +477,7 @@ async def _advance(self) -> tuple[Optional[Track], bool]: return self.current, False if self.autoplay and prev: - nxt = await media.related(prev, loop=self.bot.loop) + nxt = await media.related(prev) if nxt: self.current = nxt return self.current, False