feat: add the Hall of Deaths - #23
Merged
Merged
Conversation
W3 from the family server roadmap, and the first feature built on the event bus. Every death now gets a one-line obituary. It is announced in game to everyone online and kept on a new dashboard page with a leaderboard. This is deliberately a complete vertical slice: an event arrives, something visible happens in the game, and a record survives for later. Anything else that reacts to play follows the same shape, so it doubles as the worked example for the rest of the roadmap. api/epitaphs.py Classifies a vanilla death cause into one of eighteen categories and writes a line for it. Several lines per category, so the same death does not read the same way twice in an evening. Matching order matters and is tested: "was blown up by Creeper" is an explosion rather than a mob kill, and "walked into a cactus while trying to escape Creeper" is a cactus death even though it names a mob. Where the message names a culprit it is extracted and used, with the weapon dropped, because the lines read better naming the culprit alone. Selection is seeded by player, cause and timestamp, so a given death always produces the same epitaph while different deaths vary. Writing sits behind a small interface. The default writer runs offline, costs nothing, returns instantly and needs no API key, which is the right default for a server running on a Pi in a family's house. A writer backed by a language model implements write() and drops in; the docs note that handlers run on the follower thread, so a network call belongs on a queue rather than inline. api/hall_of_deaths.py Subscribes to death events, writes the epitaph, persists the record, then announces it. Persisting comes first on purpose: announcing needs the game server to be reachable, and a death is worth keeping even when the announcement cannot be delivered. Records carry `announced` so the difference is visible. Announcement text is JSON-encoded rather than interpolated, so an epitaph containing a quote cannot break the tellraw or inject extra components, and overlong lines are truncated deliberately rather than silently by the server. Leaderboard entries carry favourite_cause_count alongside favourite_cause. A smoke test showed the leaderboard claiming "mostly drowning" for a player whose four deaths were four different categories tied at one. A favourite only means something once it has happened more than once, so the count is exposed, ties resolve deterministically by count then name, and the dashboard falls back to the player's latest death when there is no real pattern. Elsewhere - New page at /deaths with recent obituaries, summary tiles and the leaderboard. - GET /api/deaths and GET /api/deaths/leaderboard, both requiring players.view, with matching OpenAPI paths and schema. - getDeaths() and getDeathsLeaderboard() in web/src/services/api.js. - config/deaths.conf.example: in-game announcements can be turned off while keeping the dashboard, and the colour and retention are configurable. - docs/HALL_OF_DEATHS.md, linked from the docs index. A player typing a fake death message in chat does not create an entry, because the event bus matches chat before any other pattern. There is a test for exactly that, since it is the first thing either child will try. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Dependency Review✅ No vulnerabilities or license issues or OpenSSF Scorecard issues found.Scanned FilesNone |
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
Five new alerts, all legitimate: - Unused imports: Death in tests/api/test_hall_of_deaths.py and dataclasses.field in api/hall_of_deaths.py. - An empty except in the retention-days config parse now states why: a typo should leave the default in place rather than stop the server starting. - The EpitaphWriter protocol method used a bare ellipsis, which reads as an ineffectual statement. A docstring says the same thing. - Two asserts constructed or called into objects inline. Asserts vanish under -O, so the work is hoisted out of them. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Copilot review overview
🟡 Changes recommended
Unresolved moderate issues affect persistence, event processing, culprit extraction, API documentation, and dashboard behavior.
Get a fresh assessment by requesting another Copilot review.
Review effort: Lite
Findings: 5
Open (6)
What changed in this PR
Adds the Hall of Deaths vertical slice: death classification, epitaphs, persistence, announcements, APIs, and a dashboard leaderboard.
Changes:
- Added configurable epitaph generation, storage, announcements, and leaderboard logic.
- Added secured REST/OpenAPI endpoints and the
/deathsReact dashboard. - Added event-bus integration, tests, configuration, and documentation.
| File | Reviewed changes and findings |
|---|---|
web/src/services/api.js |
Death API client methods. |
web/src/services/__tests__/api.test.js |
API client tests. |
web/src/pages/HallOfDeaths.jsx |
Dashboard UI. moderate, 2 votes: Error state also renders the empty-hall message. moderate, 3 votes: Earlier filter requests can overwrite newer results. nit, 2 votes: Filter input lacks an accessible name. |
web/src/pages/__tests__/HallOfDeaths.test.jsx |
Dashboard tests. |
web/src/components/Layout.jsx |
Navigation entry. |
web/src/App.jsx |
Lazy-loaded /deaths route. |
tests/api/test_hall_of_deaths.py |
Hall, API, and integration tests. |
tests/api/test_epitaphs.py |
Epitaph classification tests. |
docs/INDEX.md |
Documentation index link. |
docs/HALL_OF_DEATHS.md |
Feature documentation. |
config/deaths.conf.example |
Death feature configuration example. |
CHANGELOG.md |
Release notes. nit, 1 vote: Duplicate ### Added heading in the Unreleased section. |
api/server.py |
REST endpoints and event-bus wiring. moderate, 1 vote: Synchronous RCON announcement delivery can block event processing; enqueue or otherwise bound delivery. |
api/openapi.yaml |
Death API schemas and paths. moderate, 1 vote: Leaderboard schema omits the implemented causes map. |
api/hall_of_deaths.py |
Persistence, announcements, statistics, and leaderboard. moderate, 3 votes: Successful announcements do not update the persisted announced value. moderate, 2 votes: Synchronous RCON work can block the follower thread. |
api/epitaphs.py |
Death classification and epitaph generation. moderate, 2 votes: a skull from ... is captured as the culprit phrase. moderate, 1 vote: Generic was killed by magic phrases are treated as named attackers. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
All six Copilot findings. Each was verified against the code; all six were real, and the first two were the serious ones. Announcing blocked the log follower thread Event bus handlers run on the thread that follows the server log, and the production announcer makes a network call: RCON with a five second connection timeout, then a shell fallback with a thirty second one. An unreachable game server could therefore stall the follower for up to thirty-five seconds per death, holding up every other event behind it. The docs shipped in this branch already said a network call belongs on a queue rather than inline, and then the code did it inline anyway. Deaths are now queued and processed on a worker thread, started by start_event_capture(). The handler enqueues and returns. drain() waits for the queue and stop_worker() gives queued deaths a chance to be written; without a worker the handler stays synchronous, which is what the tests use. `announced` was always false in storage The record was written before the announcement was attempted, so the stored value never reflected what happened, and the field was decorative in /api/deaths. The announcement now runs first and the record is written once with the real value. Storing first and patching afterwards would have meant either rewriting the file or appending a duplicate. Nothing is lost by waiting: the death is already in the event log by then, and the announcer has a bounded timeout. Epitaphs named the projectile instead of the mob "was shot by a skull from Wither" recorded "a skull from Wither" as the culprit, so the epitaph credited the skull. Indirect forms now resolve to what fired them, and ordinary names are left alone. Dashboard - Typing in the player filter fired a request per keystroke with no ordering guard, so a slow response for "S" could overwrite the results for "Si". The filter is debounced through the existing useDebounce hook and stale responses are discarded by request id. - A failed request left the list empty, which rendered "NOBODY HAS DIED YET" underneath the error and reported a backend failure as good news. The empty state now excludes the error case. - The filter input had no accessible name once its placeholder disappeared. It has an aria-label. Both dashboard fixes have tests that were confirmed to fail without them. docs/HALL_OF_DEATHS.md described the old ordering and has been corrected, with the reasoning for the worker and for announcing before storing. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Owner
Author
|
All six Copilot findings are fixed in eb006e4. The five CodeQL alerts were already fixed in 000f365, before those threads were filed. Each was checked against the code first; all eleven were real. The two that mattered: Announcing blocked the log follower thread. Worse than the diff suggests. The production announcer goes through RCON with a five second connection timeout, then falls back to
The other four: epitaphs credited the projectile rather than the mob for Both dashboard fixes have tests that I confirmed fail without them, rather than assuming they cover the bug.
🤖 Generated with Claude Code |
11 tasks
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
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.
Description
W3 from the family server roadmap, and the
first feature built on the event bus from #22.
Every death now gets a one-line obituary. It is announced in game to everyone
online and kept on a new dashboard page with a leaderboard.
This is deliberately a complete vertical slice: an event arrives, something
visible happens in the game, and a record survives for later. Anything else that
reacts to play follows the same shape, so it doubles as the worked example for
the rest of the roadmap.
api/epitaphs.pyClassifies a vanilla death cause into one of eighteen categories and writes a
line for it, with several lines per category so the same death does not read the
same way twice in an evening.
Matching order matters and is tested.
was blown up by Creeperis an explosionrather than a mob kill, and
walked into a cactus while trying to escape Creeperis a cactus death even though it names a mob. Where a culprit is named it is
extracted and used, with the weapon dropped, because the lines read better
naming the culprit alone.
Selection is seeded by player, cause and timestamp, so a given death always
produces the same epitaph while different deaths vary.
api/hall_of_deaths.pySubscribes to death events, writes the epitaph, persists the record, then
announces it. Persisting comes first on purpose: announcing needs the game
server to be reachable, and a death is worth keeping even when the announcement
cannot be delivered. Records carry
announcedso the difference is visible.Elsewhere
/deathswith recent obituaries, summary tiles and the leaderboard.GET /api/deathsandGET /api/deaths/leaderboard, both requiringplayers.view, with matching OpenAPI paths and schema.config/deaths.conf.example. In-game announcements can be turned off whilekeeping the dashboard; colour and retention are configurable.
Notes for review
A smoke test caught a real flaw in the leaderboard. It claimed "mostly
drowning" for a player whose four deaths were four different categories, all
tied at one. A favourite only means something once it has happened more than
once, so entries now carry
favourite_cause_count, ties resolve deterministicallyby count then name rather than by dict insertion order, and the dashboard shows
the player's latest death instead when there is no real pattern.
A player typing a fake death message in chat does not create an entry. The
event bus matches chat before any other pattern, so
<Silas> Jonah was slain by Zombiestays chat. There is a test for exactly that, since it is the first thingeither child will try.
Epitaph writing is behind a small interface, and the default writer is
offline. It costs nothing, returns instantly and needs no API key, which is the
right default for a server running on a Pi in a family's house. A
language-model-backed writer implements
write()and drops in. The docs notethat handlers run synchronously on the follower thread, so a network call belongs
on a queue rather than inline; that, plus key handling and the kid-safety
guardrails, is W1 rather than this PR.
Announcement text is JSON-encoded, not interpolated, so an epitaph containing
a quote cannot break the
tellrawor inject extra components. Overlong lines aretruncated deliberately, because the server would otherwise truncate them silently.
Type of Change
Checklist
Verification run locally:
make test-apimake test-webmake lintpasses,docker compose configvalidates, Black is clean, and theproduction web build succeeds.
Related Issues
🤖 Generated with Claude Code