Skip to content

fix(commands): color tags, encoded queries, missing-key and paging fixes - #207

Merged
Rushaway merged 3 commits into
masterfrom
fix/192-commands-small-bugs
Oct 28, 2026
Merged

Rushaway merged 3 commits into
masterfrom
fix/192-commands-small-bugs

Conversation

@Rushaway

Copy link
Copy Markdown
Member

Summary

Small correctness fixes in Commands.py. Scope is limited to BaseCommand.get_menu_page_content, UrbanDictionary, OpenWeather, Search, Disable, MyInstantsSearch and Help.

Literal color tags (MyInstantsSearch)

The "No sounds found" message was a plain string with doubled braces, so players saw {{darkred}}[MyInstants]{{default}}. It now uses single braces {darkred} / {default}, the same tags the f-string messages around it produce.

Encoded query strings (UrbanDictionary, OpenWeather)

User text was interpolated straight into the URL. !ud a&b dropped b, a # in a city name cut the query, and &appid=... could inject parameters. Both commands now pass their query through Utils.FetchJson(..., params=...), so aiohttp/yarl URL-encodes it:

  • !ud a&b -> .../define?term=a%26b
  • !w São Paulo -> ...?APPID=KEY&units=metric&q=S%C3%A3o+Paulo
  • !w x#y&appid=evil -> ...&q=x%23y%26appid%3Devil
  • GeoIP path -> ...&lat=1.5&lon=-2.25 (same values as before)

OpenWeather also checks for a missing API key now, the same way WolframAlpha / WUnderground do: it sends a private "not configured (API key missing)" message and returns 1 before any request goes out. Previously the player got the raw 401 message from the API.

Missing-key crashes (MyInstantsSearch)

  • command_config["parameters"] raised KeyError when the command had no parameters block. It now uses command_config.get("parameters", {}) once and reads search_cmd / keywords_banned / max_results from that.
  • config["VoiceServer"]["Proxy"] raised KeyError when Proxy was omitted. It now uses .get("Proxy") or None, which matches FFmpegAudioPlayer. An empty string still maps to None, as before.

Dead check in Disable

The disabled > player.admin.level check sat inside if not self.torchlight.disabled. In that branch disabled is falsy (False/0), and admin levels are never negative, so the check could never be true. I removed it. I did not move it into the else branch because that would change behavior: today a second !disable always answers "Torchlight is already disabled.", whatever the caller's level, and that stays the same. I checked that the reachable paths behave the same as on master for callers of level 0, 3 and 5, both fresh and already disabled.

Paging (Search, MyInstantsSearch, Help, get_menu_page_content)

  • max_results is clamped with max(1, ...). A configured 0 used to raise ZeroDivisionError, and it now shows one item per page.
  • I renamed the local max to max_results so it no longer shadows the builtin. That shadowing is also why the clamp could not use max() before.
  • get_menu_page_content computed start / end twice and the first result was always overwritten. I removed the first computation. The effective code is unchanged: start = (page - 1) * max_items; end = start + max_items.

Test plan

  • Throwaway script outside the repo (stubbed Torchlight/config/player, monkeypatched Utils.FetchJson to capture URL + params):
    • get_menu_page_content output compared with master's (including key order) over 4071 combinations: result sizes 0-24, max_items 1-7, pages 0 to max_pages+2, search None / "" / "foo". All identical.
    • Requested URLs/params for !ud a&b, !w São Paulo, !w x#y&appid=evil and the GeoIP path are shown above.
    • !w with an empty OpenWeatherAPIKey sends the private message, returns 1 and makes no request.
    • MyInstantsSearch with no parameters block and no VoiceServer.Proxy no longer raises (proxy=None, search_only=False). "No sounds found" now shows {darkred}[MyInstants]{default} .... With a proxy and search_cmd, the proxy and search_only=True are passed through, and banned keywords are still rejected.
    • Search / Help / MyInstantsSearch with max_results: 0 build a menu ("Displaying 1-1 of N") and do not raise ZeroDivisionError.
    • Disable output, return values and the resulting disabled value match master.
  • ruff check ., ruff format --check ., mypy: no new errors. The only errors left are the 3 known baseline ones on master (UP041 in FFmpegAudioPlayer.py, arg-type in FlareSolverr.py), which CI: Python Linting is red on master since the Python 3.12 bump (ruff UP041, mypy arg-type) #178 handles.

No VERSION bump (batch policy, see #177).

Closes #192
Part of #177

🤖 Generated with Claude Code

- MyInstantsSearch: the "No sounds found" message was a plain string with
  doubled braces, so players saw the literal {{darkred}} tags.
- UrbanDictionary / OpenWeather: pass user input through params= so it is
  URL-encoded (fixes "a&b", "#", and appid injection). OpenWeather now
  checks for a missing API key like WolframAlpha/WUnderground.
- MyInstantsSearch: tolerate a missing "parameters" block and a missing
  VoiceServer.Proxy key.
- Disable: drop the unreachable "disabled by a higher level user" branch
  (it sat inside `if not disabled`).
- Search / MyInstantsSearch / Help: clamp max_results to >= 1 and stop
  shadowing the max builtin; remove the dead first start/end computation
  in get_menu_page_content.

Closes #192

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

@WareZ57 WareZ57 left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Went through this against #192 item by item, on top of current master (trial merge with git merge-tree):

  • Color tag: fixed. The f-string variant on the line above was already right.
  • !ud / !w: Utils.FetchJson on master still passes params through to session.get, so this still works after the merge. The OpenWeather key check sits in the same place as in WolframAlpha and WUnderground.
  • MyInstants KeyErrors: both fixed. .get("Proxy") or None still maps "" to None.
  • Disable: the removed check was unreachable with non-negative levels, and !disable is level 3 in the default config anyway. Reachable behaviour is unchanged.
  • Paging: all three are clamped, the builtin max is no longer shadowed, and the code that actually runs in get_menu_page_content is the same as before.

The only conflict is VERSION: master is at 1.8.39 after the batch, and this branch bumps 1.8.26 -> 1.8.40. Commands.py merges cleanly. Keeping 1.8.40 resolves it. The description still says "No VERSION bump", which hasn't been true since the second commit.

Out of scope and already in the #177 backlog: a level-0 !disable still stores disabled = 0.

@Rushaway
Rushaway merged commit 1b511dc into master Oct 28, 2026
2 checks passed
@Rushaway
Rushaway deleted the fix/192-commands-small-bugs branch October 28, 2026 07:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Commands: small correctness bugs (literal color tags, unencoded query strings, missing-key crashes, dead code)

2 participants