Repository navigation
Receive GG Requestz game requests and fulfil them from the plugin catalogues - #41
Merged
Merged
Conversation
Owner
Author
|
CI settled on 180c572. All four jobs fail on exactly the two pre-existing
Both failures are the ones already on Bisected to Everything this PR adds passes on both platforms: 137 new tests, none skipped, |
The Hub has everything needed to fulfil a game request -- search plugins over Archive.org, homebrew, itch.io and No-Intro, an importer, three library backends -- and no way to learn that a request exists. GG Requestz already POSTs a `game_request` event on approval, merged upstream, and Romarr already answers it from torrent indexers. This answers the same webhook from the curated catalogues, which makes the two alternatives chosen by where the operator points REQUEST_WEBHOOK_URL rather than a new protocol. `webhook.py` is what a request means: the payload, a sqlite log keyed on request_id, the IGDB-name-to-RomM-slug translation, and the match. `fulfil` takes its search and its import as callables, which is the seam that lets the decision be tested without a plugin subprocess or a library server -- and the decision is the part worth testing, because a wrong ROM in somebody's library is worse than an unfulfilled request. So an igdb_id decides when a source states one, otherwise the title must match exactly under the same normaliser the search listing already groups by, two candidate platforms with nothing to choose between them is a refusal, and every refusal is recorded as a sentence. `webhook_server.py` is the socket. `http.server` rather than FastAPI: one POST route with no path parameters, no content negotiation and nothing to browse does not justify Starlette, anyio, h11 and a settings stack in the process that holds the library credential, and pydantic -- already a dependency -- is where the validation argument mostly lay. The handler claims the request id and answers 202 in milliseconds because GG Requestz allows five seconds and an import is a multi-gigabyte download; a single worker thread does the rest. The token in the URL is documented as a shared secret in a URL and not as authentication, because GG Requestz sends no signature and no auth header and that side is fixed. What is done about it is proportionate: loopback by default, a 24-character minimum, no default value, 401 for everything else including a wrong path, and the token kept out of this program's own log -- BaseHTTPRequestHandler.log_request would otherwise write the request line, and the request line is the secret. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
`rom-hub webhook serve|url|log|forget`. The wiring is the only new thing here: `_fulfil_request` hands `webhook.fulfil` the same `search_all` fan-out `_cmd_search` uses and the same `run_import` pipeline `_cmd_import` uses, so a fulfilled request goes through exactly the code an operator would have typed. Everything is opened per request and closed again. A receiver can sit idle for days, and a backend token, an httpx client and a sqlite connection held across that are three things that can be stale by the time they are used; an import is measured in minutes, so opening them costs nothing worth saving. `serve` refuses before it binds, and the order matters. A receiver that answers 202 and can never import is worse than one that will not start -- GG Requestz would record five seconds of success per request and the library would stay empty -- so the token, then the backend's IMPORT capability, are checked first. A short token is refused in `webhook_settings` rather than in the server's constructor so the operator hears about the thing they can fix instead of about RomM. No search plugin at all is a warning and not a refusal: setting the endpoint up before installing plugins is a reasonable order to work in. `url` prints the endpoint to paste into REQUEST_WEBHOOK_URL, because the path, the port and the token are three things that can be got wrong independently and every one of them produces the same opaque 401. It also prints, in full, that the token is not authentication. `log` and `forget` are what make a recorded outcome actionable. A NO_MATCH is the interesting state -- it becomes fulfillable the moment a plugin covering that platform is installed -- and without `forget` the duplicate guard would mean the only way to retry was to invent a new request id. `requests.db` is its own file rather than a table in `jobs.db`: a request can end with no job at all, it has to outlive every retry, and sharing the file would have `rom_hub.jobs`'s migration reaching a table it does not own. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
docs/DESIGN.md gains "The request receiver": why `http.server` rather than FastAPI, why the handler cannot wait for the import, why the URL token is documented as a shared secret and not as authentication, why idempotency is a primary key rather than a heuristic, and why a near miss is refused. The dependency argument is the one worth having on the record, and it is the same argument the `libtorrent` decision already made in this file rather than a new one: a daemon-shaped dependency does not fit a command-shaped program, and the validation half of the case does not survive the fact that pydantic is already here. Two things are stated because they are unflattering and would otherwise look like oversights. No search plugin in this repository emits an igdb_id, so the stronger match key does not fire today and the title is what matches. And the receiver is the one listening socket in a project whose security model is built on plugins having none -- so it gets its own paragraph in README's security section rather than only a mention in the feature section. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Self-review of my own claim. Both the module docstring and the design note say
the token is compared with `hmac.compare_digest`, and only the `?token=` form
actually was -- the path-segment form, which is the one an operator will
normally use, was a plain `==`. That returns as soon as two bytes differ, on the
only gate this endpoint has.
Fixing it naively introduces a worse bug, which is why this is a helper with the
reasoning attached rather than a one-word change. `BaseHTTPRequestHandler`
decodes the request line as latin-1, so `POST /requests/café` yields a
non-ASCII `str` and `hmac.compare_digest` raises TypeError on those. That
TypeError would be raised inside `authorised`, before the handler's own `try`,
escape into `handle_error` -- which this class deliberately silences -- and
leave the sender with a reset connection and no status code at all. So both
sides are encoded to bytes first.
Two tests come with it: a raw latin-1 request line that must get a 401 and
leave the server still answering, and the `ROM_HUB_WEBHOOK_PATH=/` case, whose
normalised path is the empty string and is the one configuration where the route
and the prefix comparison can disagree. Also `/{token}extra` and
`/{token}/more`, which the old suffix-equality check happened to refuse and a
prefix check has to be made to refuse deliberately.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
BlizzHacker
force-pushed
the
feat/request-webhook
branch
from
October 3, 2026 09:09
180c572 to
762ecfe
Compare
|
Important
This repository does not receive automatic reviews because it has fewer than 10 stars. ⚙️ Run configuration
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. Comment |
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.
What this closes
ROM Hub could already fulfil a game request and had no way to learn that one
exists. Search plugins over Archive.org, homebrew, itch.io and No-Intro; an
importer; three library backends; and a pure CLI with no HTTP server anywhere.
GG Requestz already emits the trigger. It POSTs a
game_requestevent towhatever
REQUEST_WEBHOOK_URLnames when a request is approved, mergedupstream, nothing to change on its side. This receives it.
This is the standalone case and deliberately the smaller one. With ROMarr in
front,
REQUEST_WEBHOOK_URLshould point at ROMarr, which fulfils from trackersand from these plugins. This is for the operator who is not running ROMarr:
GG Requestz posts to a single URL, so without this the Hub cannot be that URL at
all.
The HTTP layer:
http.server, not FastAPIArgued rather than assumed, because the brief asked for the argument either way.
What FastAPI would cost. Starlette, anyio, h11, click and a settings
stack: a dependency tree heavier than this project's entire runtime requirement list
(
pydantic,httpx,pyseccomp,python-socketio), added to the process thatholds the library credential. This project's whole security posture is that
plugins are subprocesses with no token and no sockets; adding five packages to
the privileged half of that arrangement for one route is the wrong direction.
What it would buy. One POST route with no path parameters, no content
negotiation, no schema generation and no docs page anyone will browse. The
validation argument is the strongest one and it does not survive the fact that
pydanticis already a dependency and is what parses the payload here.http.serversupplies the socket, the request line and the headers, which isthe whole remainder.
This is the same trade
docs/DESIGN.mdalready made and wrote down forlibtorrent, on the grounds that a daemon-shaped dependency does not fit acommand-shaped program. It goes the same way here for the same stated reason
rather than a new one. What
a framework would have supplied is written down instead: a
Content-Lengthcaprefused on the header, a
411for the chunked bodyhttp.servercannot read, abounded queue with a
503when it is full, a JSON body on every answerincluding the refusals, and a drain-after-refusal so a rejected sender sees the
status code rather than a connection reset.
The four constraints
No new dependency.
pyproject.tomlis unchanged. Nothing was added.The sender cannot authenticate. GG Requestz posts
Content-Type: application/jsonand nothing else, so the token is a path segment(
/requests/<token>) or a?token=parameter, compared withhmac.compare_digest. It is documented as a shared secret in a URL and not asauthentication, in README, in
docs/DESIGN.md, inrom-hub webhook url's ownoutput and in the module docstring. Loopback by default; a 24-character minimum;
no default token;
401for everything else including a wrong path, because a404would enumerate paths.BaseHTTPRequestHandler.log_requestis overriddento nothing, because it writes the request line and the request line is the
secret.
Five seconds. The handler claims the
request_idand answers202; asingle worker thread does the search and the import. Measured live below at 2 to
30 ms. The test that proves it holds the fulfilment callable open on a
threading.Eventand asserts the202came back anyway.The same request arrives more than once.
INSERT ... ON CONFLICT DO NOTHINGagainstrequest_idas a sqlite primary key, claimed on the acceptorthread before the
202so a re-approval landing mid-import sees the row. NotINSERT OR REPLACE: the re-dispatched payload is the original one, so replacingwould reset a
FULFILLEDrow and import a second copy.rom-hub webhook forgetis the deliberate undo, because a
NO_MATCHbecomes fulfillable the moment aplugin covering it is installed.
Reuse, and the matching rule
webhook.fulfiltakes its search and its import as callables. The CLI passesdispatcher.search_all(withfanout_limit,prepare_assets,prepare_secretsand the census catalogue, exactly as_cmd_searchdoes) andimporter.run_import(withPluginProcess,JobQueueanddownloads_dir,exactly as
_cmd_importdoes). Grouping isgrouping.group_results; the titlenormaliser is
romnames.normalise_title; the platform vocabulary comes fromplayability's own three tables rather than a second list. Nothing wasreimplemented.
Matching refuses rather than guesses, because a wrong ROM in a library is worse
than an unfulfilled request:
igdb_iddecides when a result states one.listing already groups by.
Chrono Trigger 2does not answerChrono Trigger.refusal, not a coin toss.
rom-hub streamnamed instead.platformsnarrows the search.webhook.PLATFORM_NAMESmaps IGDB platformnames to RomM slugs, and a test asserts every target is a slug
rom_hub.playabilityknows, so a typo cannot become a filter no pluginrecognises. An untranslatable name is reported on the row and the search runs
unfiltered.
Tests
3312 passing, up from 3178. 134 new across three files, covering every case the
brief listed plus the ones that came out of writing it. Nothing skips on either
platform, so the
ci_gate.py junitskip allowlist is untouched.Coverage went up, not down: 85.90 % to 86.57 % on Windows, measured on a
worktree of
masterfor the before figure.webhook.pyis 99 %,webhook_server.py96 %.What I verified live, and what I did not
Live, against a real
rom-hub webhook servesubprocess on loopback, driven byreal
http.clientrequests.ROMM_URLpointed at a closed loopback portthroughout; nothing in this work touched the RomM on 8080.
Verified live:
401for a wrong token, a missing token and an unrelated path;400for amalformed body with the server still serving afterwards;
413on the declaredlength;
202for a valid payload;202 duplicatefor a repeat; the query-tokenform.
subprocess:
Chrono Trigger (USA)was chosen andChrono Trigger 2: Not The Same Game, returned by the same plugin in the same response, was not.NO_MATCHrecorded for a title nothing indexed. An untranslatable platformname reported on the row with the search still run unfiltered. An
updaterequest recorded
IGNOREDwith the reason.webhook log,webhook log --json,webhook forget, andforgettwice giving a clean error.run_importgenuinely entered (job rows 1, 2 and 3 exist injobs.db) andfailing at the closed library endpoint with the real message on the request
row.
Not verified live: a successful upload into a real library server. The last
link,
run_importwriting to RomM, Gaseous or Retrom, was exercised onlyagainst the closed endpoint and against the existing suite's mock transports. I
did not stand up a disposable RomM for it:
scripts/proof-stack.compose.ymlexists for that and needs Docker plus a socket.io scan the receiver does not
change. The receiver hands
run_importaSearchResultand nothing more, sothat path is the same one
rom-hub importalready proves indocs/PROOF.md,but I am not claiming to have watched a requested ROM land in a library.
Also not verified: anything on Linux. The suite, the coverage figure and the
live run are all Windows. CI covers Linux.
Two other things worth knowing:
contrib/test-backendsdoes not exist in this repository. The brief said toread it; the equivalent here is
scripts/proof-stack.compose.ymlplusscripts/proof-stack-bootstrap.sh, which is what I read instead.master. Aconcurrent session in this checkout committed and pushed my README edits along
with its own branding work, then reframed them from "an alternative to Romarr"
to "Answering requests without ROMarr". I rebased onto that, dropped the two
duplicate commits, and reconciled
docs/DESIGN.mdand the CLI help text tomatch
master's positioning rather than contradict it. So this PR's diff iscode, tests and
docs/DESIGN.md; the README half of the documentation landedahead of it.
masteris currently red, for an unrelated reasontests/test_catalog.py::test_the_catalog_lists_every_plugin_that_ships_in_this_repoand
::test_directory_is_in_sync_with_the_catalogfail onorigin/masteritself. Bisected to
dfd3063("catalog: drop archive-org-torrent, which cannotbe installed"), which removed the entry from
catalog/plugins.jsonwhileplugins-dev/archive-org-torrent/still ships anddocs/PLUGINS.mdwas notre-rendered.
f0ba953and96866a1are green;dfd3063is not.CI on this PR will fail on those two and only those two. I left them alone
deliberately: fixing the catalog and re-rendering the directory is a separate
change, and another session appears to be mid-way through splitting
torrents.jsonout. Everything else is green: 3279 passed with--ignore=tests/test_catalog.py.🤖 Generated with Claude Code