Send structured banned payload for localized client handling

[ZainKazmiii]

Fixes #640

Summary

• replace hardcoded English ban notice text with a structured banned payload
• send UTC ISO-8601 expires_at plus reason so clients can localize presentation
• keep ban enforcement behavior unchanged (server still aborts banned sessions)

Validation

• python -m py_compile server/exceptions.py server/lobbyconnection.py tests/integration_tests/test_login.py tests/integration_tests/test_server.py tests/unit_tests/test_lobbyconnection.py
• python -m pytest tests/unit_tests/test_lobbyconnection.py -k abort_connection_if_banned -q (blocked in this runner: No module named pytest)

Test updates

• switched ban-related integration/unit expectations from HTML notice text to the structured banned payload format

Summary by CodeRabbit

• Bug Fixes

  • Ban notifications are now a structured "banned" response with an ISO‑8601 UTC expires_at and a separate reason for clearer, machine‑readable ban info.
  • Lobby connections now abort with a consistent ban-block reason and send the structured ban payload to clients.
  • Message packing now accepts additional buffer types for more robust serialization.

• Tests

  • Integration and unit tests updated to validate the new structured ban payload and serialization behavior.

[coderabbitai]

Note

Reviews paused

It looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the reviews.auto_review.auto_pause_after_reviewed_commits setting.

Use the following commands to manage reviews:

• @coderabbitai resume to resume automatic reviews.
• @coderabbitai review to trigger a single review.

Use the checkboxes below for quick actions:

• ▶️ Resume reviews
• 🔍 Trigger review

📝 Walkthrough

Walkthrough

Replaces human-readable ban notices with a structured "banned" payload: BanError.to_payload() returns UTC ISO-8601 expires_at and reason; LobbyConnection sends that payload and aborts with a fixed message. Tests and protocol annotation updated.

Changes

Ban message to structured payload migration

Layer / File(s)	Summary
BanError payload structure server/exceptions.py	BanError adds to_payload() converting ban_expiry to UTC and returning {"command":"banned","expires_at":<ISO UTC>,"reason":...}. Module imports updated to use timezone instead of humanize/datetime_now.
LobbyConnection ban exception handler server/lobbyconnection.py	on_message_received sends e.to_payload() on BanError and aborts with a fixed "Banned user blocked from lobby session" reason instead of using the exception message.
QDataStream pack signature widen server/protocol/qdatastream.py	pack_block signature widened to accept `bytes
Integration test ban response assertions tests/integration_tests/test_login.py, tests/integration_tests/test_server.py	Tests updated to expect "banned" command payloads with reason and ISO-8601 expires_at parsed as UTC (permanent ban year >= 2500). Non-ASCII title test input adjusted; hosting ban test narrowed to field-level checks.
Unit test ban payload validation tests/unit_tests/test_lobbyconnection.py	Removes unused re import. test_abort_connection_if_banned now asserts BanError.value.to_payload() fields (command, reason, expires_at) instead of matching formatted message text.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20 minutes

Poem

🐰 I hopped through code to change the sound,
Ban messages now wear timestamps round,
No English fluff, just ISO light,
UTC ticks keep expiry right,
A tidy payload — neat and sound.

🚥 Pre-merge checks | ✅ 3 | ❌ 2

❌ Failed checks (2 warnings)

Check name	Status	Explanation	Resolution
Out of Scope Changes check	⚠️ Warning	The update to QDataStreamProtocol.pack_block to accept bytearray is a minor, unrelated change not tied to the ban payload objectives, appearing out-of-scope.	Either justify the bytearray change or move it to a separate pull request focused on protocol improvements.
Docstring Coverage	⚠️ Warning	Docstring coverage is 20.00% which is insufficient. The required threshold is 80.00%.	Write docstrings for the functions missing them to satisfy the coverage threshold.

✅ Passed checks (3 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title clearly summarizes the main change: replacing hardcoded English ban text with a structured payload for client-side localization.
Linked Issues check	✅ Passed	The pull request fully implements issue #640's requirements: replaces ban notice with a structured 'banned' payload, uses ISO-8601 UTC timestamps in expires_at, includes reason field, and removes humanize dependency.

_{✏️ Tip: You can configure your own custom pre-merge checks in the settings.}

✨ Finishing Touches

🧪 Generate unit tests (beta)

• Create PR with unit tests

---

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

[coderabbitai]

Actionable comments posted: 3

🤖 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 `@server/exceptions.py`:
- Around line 5-7: Add one additional blank line between the top import
statement (from datetime import timezone) and the class declaration (class
ClientError) so there are two blank lines separating imports and top-level class
definitions to satisfy flake8 E302/isort formatting rules.

In `@tests/integration_tests/test_login.py`:
- Around line 1-2: The import order in the test module is incorrect for isort:
move the datetime import before time so that "from datetime import datetime,
timezone" appears above "from time import time"; update the top-of-file imports
accordingly (keeping the same imported symbols: time, datetime, timezone) to
satisfy isort --check --diff.

In `@tests/integration_tests/test_server.py`:
- Line 724: The test title string "title": "ÇÒÖL GÃMÊ" contains
confusable/ambiguous Unicode characters flagged by Ruff; replace that value with
a clear non-ASCII but unambiguous title (for example "ÇÖL GÁMÉ" or another
equivalent) so the test intent remains but RUF001 is avoided, i.e., update the
literal where "title": "ÇÒÖL GÃMÊ" appears in
tests/integration_tests/test_server.py to the chosen unambiguous non-ASCII
string.

🪄 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

Run ID: 51ea1e81-1dac-4b40-a3b2-b2b8a704f097

📥 Commits

Reviewing files that changed from the base of the PR and between e36de71 and 23dcd55.

📒 Files selected for processing (5)

• server/exceptions.py
• server/lobbyconnection.py
• tests/integration_tests/test_login.py
• tests/integration_tests/test_server.py
• tests/unit_tests/test_lobbyconnection.py

[coderabbitai]

🧹 Nitpick comments (1)

server/protocol/qdatastream.py (1)

74-74: ⚖️ Poor tradeoff

Clarify immutability contract between pack_message and pack_block.

• A search shows QDataStreamProtocol.pack_block(...) is only called at line 95 in QDataStreamProtocol.pack_message; there are no other call sites that would require pack_block to accept bytearray.
• If the intended contract is “convert mutable buffer to immutable bytes before calling pack_block”, add bytes(msg) at line 95 and narrow pack_block back to bytes; otherwise, update the description to match the current “accept bytes | bytearray and return packed bytes” approach.

🤖 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 `@server/protocol/qdatastream.py` at line 74, The signature of
QDataStreamProtocol.pack_block currently accepts bytes | bytearray but is only
called from QDataStreamProtocol.pack_message; change the contract to require
immutable bytes by converting the mutable buffer at the call site: in
pack_message call pack_block with bytes(msg) (i.e., ensure msg is converted to
bytes before invoking pack_block) and update pack_block's type annotation to
accept only bytes, removing bytearray from its parameter type.

🤖 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.

Nitpick comments:
In `@server/protocol/qdatastream.py`:
- Line 74: The signature of QDataStreamProtocol.pack_block currently accepts
bytes | bytearray but is only called from QDataStreamProtocol.pack_message;
change the contract to require immutable bytes by converting the mutable buffer
at the call site: in pack_message call pack_block with bytes(msg) (i.e., ensure
msg is converted to bytes before invoking pack_block) and update pack_block's
type annotation to accept only bytes, removing bytearray from its parameter
type.

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: 066a843e-fb42-49ea-a65c-6347d464a999

📥 Commits

Reviewing files that changed from the base of the PR and between c8b3265 and aa10c6e.

⛔ Files ignored due to path filters (1)

• Pipfile.lock is excluded by !**/*.lock

📒 Files selected for processing (1)

• server/protocol/qdatastream.py

[ZainKazmiii]

Quick note on the latest CodeRabbit qdatastream nitpick:

I intentionally kept pack_block accepting bytes | bytearray and kept the call site unchanged because a prior bytes-conversion variant caused the qstream matchmaker test path to fail on this PR line.

Current head 7db962f1630329d1bbe39e015c458fa7eb998f74 is green across checks after restoring that behavior. If maintainers want a stricter immutable-bytes contract, I can do that in a follow-up that preserves runtime behavior and keeps CI green.

[ZainKazmiii]

Addressed in 7e722a6. I clarified the pack_block contract in qdatastream.py: it intentionally accepts �ytes | bytearray because pack_message builds a mutable �ytearray buffer and passes it through. I kept behavior unchanged here because forcing �ytes(msg) previously correlated with the qstream regression we just fixed.

[ZainKazmiii]

Follow-up on the qdatastream immutability discussion: this is now addressed in head commit 41d82aa.

• pack_message now calls pack_block(bytes(msg))
• pack_block now requires bytes

This keeps the contract explicit and aligns with the CodeRabbit note. Current PR checks are green on this head.

[ZainKazmiii]

@coderabbitai review

Current head (41d82aa) now calls pack_block(bytes(msg)) and keeps pack_block(block: bytes), so the immutability nitpick should be resolved on latest commit.

[coderabbitai]

@ZainKazmiii Thanks for the update — confirming that pack_message now wraps with bytes(msg) before passing to pack_block, and pack_block is typed strictly as bytes, looks like the right resolution. I'll kick off a fresh review against the latest head now.

✅ Actions performed

Review triggered.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.