refactor!: make the server production-ready on FastMCP 4 - #9
Open
devmadou wants to merge 56 commits into
Open
Conversation
…Lab/McpDiffusion into feat/refacto-clean-archi
Drops the unused tasks extra. No source changes were required: the lifespan decorator, tool registration and http_app all survive the major bump. Verified with the full suite plus a live tool call through an in-memory client.
Project instructions in CLAUDE.md, topic rules under .claude/rules/ (python, git, error handling, logging), shared permissions in .claude/settings.json, and the FastMCP documentation MCP server in .mcp.json so contributors get it without local setup. Personal and reference files stay out of git: settings.local.json, TODO.md and CLAUDE-example.md are ignored.
`# Fixme:` is ours to fix; `# Business rule:` marks a question only the owner of the search and data semantics can answer. Preserve current behaviour and flag it rather than deciding.
…lues Settings are constructed in server.py and passed down. `get_settings()` is gone: no service or client reaches for configuration, each takes the specific value it needs. Elasticsearch queries no longer block the event loop. Replaces hand-rolled infrastructure with what FastMCP already ships: - core/middleware.py -> SlidingWindowRateLimitingMiddleware, keyed per client - @log_tool -> LoggingMiddleware + TimingMiddleware + ErrorHandlingMiddleware - TrustedHostMiddleware -> http_app(host_origin_protection=...) Fixes found on the way: - the title relevance boost was discarded by every INSEE search tool, which overwrote its `should` clauses with the collection filters - _truncate could compute a negative tail, and a zero-length tail appended the whole document instead of nothing - the server crashed on shutdown, awaiting a synchronous close() - `must_not` was threaded through three tools while always empty Configuration is not backward compatible. Renamed: TLS_VERIFY -> ES_TLS_VERIFY, GLOBAL_REQUEST_MIN -> RATE_LIMIT_MAX_REQUESTS, FORWARDED_ALLOW_IPS -> TRUSTED_PROXY_HOSTS, ENABLE_* -> ENABLE_*_TOOLS. ALLOWED_HOSTS and TRUSTED_PROXY_HOSTS now take JSON lists. TZ and ES_HOST_LOCAL are removed, nothing read them. TRUSTED_PROXY_HOSTS defaults to 127.0.0.1 rather than "*", so a deployment behind a proxy must name it or every client shares one rate-limit bucket.
A failed call now always reaches the caller as is_error with a message it can act on. Three mechanisms coexisted before: raising, returning an error inside a successful payload, and per-item statuses. Only the last one was right, and it stays where it belongs. RMES failures were the worst case. A timeout, a syntax error or an unreachable endpoint returned is_error=False with the reason tucked into an `error` field, so a failed query looked like a success to the caller. - AppToolError replaces fail(). Being a class, `raise` is visible at the call site, so the eight dead `raise` statements that followed fail() are gone and cannot come back. - mask_error_details is on. Deliberate messages pass through untouched; anything else is a bug and is replaced by a generic message. - ErrorHandlingMiddleware no longer transforms. Its default promoted our errors to JSON-RPC protocol errors labelled "Internal error", losing is_error. - One vocabulary. RMES had six codes of its own; they map onto the shared set. - An empty result is no longer an error: search_melodi_modalities returns an empty list and a count. - get_insee_document no longer copies raw exception text into its results. That text is a returned value, so masking could never redact it. Also fixes a crash: _fetch_html referenced a variable removed when the client gained a base_url, so every insee.fr timeout, 404 and network error raised NameError instead of the intended message. Function names now lead with a verb: _build_failed_document, _match_exact, _match_prefix, _strip_graph_base, compute_current_date_iso. BREAKING CHANGE: the `error` field is removed from RMES_list_graphs, RMES_describe_resource and RMES_run_sparql. A client reading `output.error` must instead check is_error on the call result and read the message.
120 findings across the package, most of them rules we had written down but had no way to enforce. - UP045 x26: `Optional[X]` where python.md mandates `X | None` - I001 x11: unsorted imports, including one I introduced by hand - E501 x3, plus 59 in data/indicators.py which is literal statistics awaiting a live source and is ignored per-file until then - F401 x1: register_extras_send_feedback is imported and never registered, so send_feedback is not exposed at all. Marked with an explicit noqa and a note, rather than deleted, until we decide to wire it up or drop it. B904 flagged 20 raises inside except clauses without `from exc`. Adding it turned out to buy nothing: Python preserves the original as __context__ either way, so the cause is in the traceback regardless and `from exc` only changes the wording. What was actually missing is that ErrorHandlingMiddleware was discarding tracebacks, so no cause reached the log at all. include_traceback is now on, `from exc` is dropped, and B904 is ignored with the reason recorded. python.md now explains that the formatter collapses anything fitting on one line and never splits it, so a trailing comma is what keeps a literal exploded. Without that the multiline rule reads as broken. Quotes need no rule: the formatter normalises them. error.md's chaining rule now states its real weight - server-side hygiene, nothing crossing the wire - instead of implying it preserves lost information.
error.md still told the reader to write `raise ... from exc` after we removed every one of them and ignored B904. It now says not to, and why, so nobody adds them back believing the ignore was an oversight. It also read as forbidding the Elasticsearch client's own max_retries and retry_on_timeout, which RetryMiddleware does not replace: one retries the HTTP connection, the other the MCP request. The rule now separates them. uvicorn moves to module scope. It is a declared dependency, so importing it inside __main__ bought no isolation and was the only import in a function body, against python.md.
The description promised `mainIndicators` with a per-indicator link to pass to
`get_insee_document`, plus `lastArticles` and `keyGraphics`. The tool returns
`indicators` and `count` and no links at all, so the model was instructed to
perform a chain it could not perform, on every call. get_insee_document
pointed at those same links, and now points at the search tools instead.
Cleanup around it, behaviour unchanged at 59 indicators:
- DICT_KV becomes KEY_INDICATORS, which is python.md's own example of the
distinction
- the header row {"cle": "clé", ...} is deleted along with the runtime filter
that removed it by comparing three literals on every call
- trailing whitespace lives in the data rather than in a .strip() per read
- the mapping moves to services/insee_indicators.py, so it is testable without
an MCP server
- the tool is sync: it reads a literal and performs no I/O
The figures themselves are untouched. They are frozen literals that assert
their own dates, which is a decision for the data owners, so it is recorded as
a `# Business rule:` along with the promised-but-never-built output shape - the
best evidence of what the tool was originally meant to be.
Tool descriptions came from a separate `config/tool_metadata.py`, so they drifted
from the code they described: five routing hints pointed at tools that never
existed (`query_insee_rmes`, `search_chiffres_clefs_insee`, `get_MELODI_datasets`)
and `search_insee_documents` documented a `chiffre_clef` parameter its signature
had lost. Descriptions now live in the docstring beside the function, so a rename
cannot leave the prose behind.
Cross-tool routing moved to the server `instructions`, assembled from the enabled
families -- a deployment with `ENABLE_RMES_TOOLS=false` no longer advertises RMES
workflows. Per-tool guidance keeps its WHEN TO USE / WHEN NOT TO USE shape.
Tool parameters are flat: `params: XxxInput` nested every argument one level deep
for no gain. The Input models became reusable `Annotated` aliases, which also
removed the duplication four `# Fixme:` markers asked about.
Names now say what a value is, not what the caller does with it -- `ResultCount`
became `NumberOfResults`, `PublicationYearFilter` became `YearOfReference`, and
enums took the `Choice` suffix so the plain name was free for the alias.
Every parameter description, default, bound, example and enum is byte-identical
to before; only names and nesting changed.
BREAKING CHANGE: three tools are renamed and several parameters with them.
Connected clients calling the old names fail.
RMES_list_graphs -> search_rmes_graphs
contains / category / expand -> graph_uri_substring / graph_category / expand_graphs
RMES_describe_resource -> describe_rmes_resource
uri / graph -> resource_uri / graph_uri
RMES_run_sparql -> run_rmes_sparql
full_sparql_query / timeout -> sparql_query / timeout_seconds
search_insee_* geo_niveau -> geo_level
get_insee_document list_of_url -> document_urls
include_sommaire -> include_table_of_contents
get_melodi_observations list_of_year -> years
dict_of_columns_and_values -> column_filters
number_of_results -> number_of_observations
search_melodi_datasets french_query -> query
number_of_results -> number_of_datasets
search_melodi_modalities columns_id -> column_ids
french_query -> query
number_of_results -> number_of_modalities
`Depends()` and `CurrentContext()` appear as argument defaults, which B008 flags because a shared mutable default would leak between calls. These build a dependency marker rather than a value: FastMCP resolves them per request, so the default is never what the function receives. Configured once here rather than repeating a `noqa` at every injection site.
Melodi's data access lived in one module that the tools wired by hand: each tool pulled a raw client out of the lifespan context, passed an index name down on every call, and translated backend failures itself. Each backend now sits behind a service, built once at startup with its client and index already bound: services/melodi/index_service.py MelodiIndexService -- Elasticsearch services/melodi/api_service.py MelodiApiService -- Melodi REST API services/elasticsearch_failures.py one failure boundary for both searches Tools declare the service they need and FastMCP injects it, so no Melodi tool imports `elasticsearch` or `httpx` any more, and none carries an index name. Query construction moves onto the `elasticsearch.dsl` builders insee already uses; every generated body is identical to the raw dict it replaces, except that the DSL omits an empty `filter` clause, which is a no-op in Elasticsearch. The tool contract -- names, input schemas, descriptions, output schemas -- is byte-identical to the previous commit. Also corrects four failures the caller could not act on: - Elasticsearch `ApiError` escaped the boundary entirely, because it does not subclass `TransportError`. A missing index surfaced as a generic error. It is now mapped by status and marked non-retryable, so the model reports it instead of rephrasing the query. - The Elasticsearch host and port travelled to the client inside the exception message. Only the exception type is sent now; the full cause stays in the server log. - Melodi answers HTTP 400 for an unknown dataset, column and modality alike, yet the message only suggested checking modality codes. It now names every remedy and quotes the upstream detail that distinguishes them. - Values were forced through `str()`, so a field arriving as a list became the literal "['GEO', 'SEX']". Malformed data now fails validation rather than reaching the caller as text.
`core/` and `infra/` were junk drawers: neither name states a membership rule, so unrelated files accumulated. `core/` held an error type, a 186-line instructions document, logging setup and a rate-limit helper. `infra/` held the lifespan, three client accessors and the dependency providers -- and imported `services/`, inverting the hierarchy its own name claimed. Both are gone, along with `config/`, whose single module now names itself: settings.py errors.py instructions.py lifespan.py logging.py rate_limiting.py dependencies.py The three client accessors fold into `dependencies.py`, so one module holds everything a tool can inject. They keep their explicit-`ctx` signatures and sit under their own heading until insee and rmes move to `Depends`. `data/`, `models/`, `services/` and `tools/` stay: each one can state what belongs in it. Pure relocation. The tool contract -- names, input schemas, descriptions, output schemas -- is byte-identical, as are the generated Elasticsearch queries and every `# Fixme:` and `# Business rule:` marker. The Fixme about untyped dependency functions moves from `infra/__init__.py` into `dependencies.py`, where it now names the cause: the lifespan context is an untyped mapping, so the return annotations are asserted, never checked.
It sat inside the package at `src/mcpdiffusion/.env.example`, which shipped it in the wheel and put the documented configuration surface where nobody looks for it. Nothing referenced that path except README. `docker-compose-dev.yaml` reads `mcp-diffusion.env` and is unaffected, and `.gitignore` ignores `*.env` but not `*.env.example`, so the example stays tracked.
insee's data access lived in three modules the tools wired by hand: each search tool assembled its own query, threaded an index name down on every call, and translated Elasticsearch failures itself. Each backend now sits behind a service, built once at startup with its client and index already bound: services/insee/index_service.py InseeIndexService -- Elasticsearch services/insee/document_service.py InseeDocumentService -- insee.fr pages Tools declare the service they need and FastMCP injects it, so no insee tool imports `elasticsearch` or `httpx` any more, and none carries an index name. `build_key_indicators` moves into the homepage tool, its only caller, and `services/insee_indicators.py` goes with it. The one query builder driven by three booleans -- `must_not_rapides`, `must_only_rapides`, `chiffre_clef` -- becomes one builder per search. Those flags encoded which tool was calling and allowed combinations that mean nothing. The clause lists they passed around as a bare tuple are now a frozen `QueryClauses` with named `must`, `filter` and `should`. Every generated Elasticsearch body is identical to the one it replaces, checked through the registered tools rather than in isolation. Also corrects three failures the caller could not act on: - `search_insee_chiffrecle` reported "INSEE documents search backend unreachable", copy-pasted from the documents tool. Each search now names itself. - Elasticsearch `ApiError` escaped the boundary entirely, because it does not subclass `TransportError`. A missing index surfaced as a generic error. All three searches now share the boundary that maps it by status. - `document_urls` entries were typed `object` and coerced with `str()`, though the model has always declared `list[str]`. `es_index_produits` becomes `es_index_publications`, so the setting and the parameter reading it finally share one name. The value stays "produit": that is the real index. Deployments setting ES_INDEX_PRODUITS must rename it to ES_INDEX_PUBLICATIONS -- an unknown variable is ignored, not rejected. The only change to the tool contract is `DocumentResult.status`, now `Literal["success", "error"]` rather than `str`. That adds an enum to the output schema without changing any value a caller receives.
rmes was one 502-line module holding the graph taxonomy, SPARQL transport, a
module-level cache and the three tool operations, with every tool passing the
endpoint down on each call.
It splits along the seam it actually has -- domain versus transport -- rather
than the index/api seam the other two sources use:
data/rmes_graph_categories.py which families exist, as static data
services/rmes/graph_taxonomy.py what matching a family means
services/rmes/graph_store_service.py RmesGraphStoreService: queries and cache
Tools declare the service through `Depends` and build their own output, so no
rmes tool imports `httpx` any more and none carries the endpoint. Every tool in
the server now takes its service the same way, and the last explicit-`ctx`
accessor in `dependencies.py` goes with it.
The category families were declared twice: `GraphCategoryChoice` listed the
keys and `CATEGORY_DEFS` defined them, free to drift apart. The families are
now static data -- no classes, no lambdas, just the label, description and the
test each one declares -- the enum is derived from their keys, and the taxonomy
turns the declarations into matchers. Adding a family makes it selectable.
`_execute_sparql` returned a dict whose meaning depended on magic keys --
`format`, `data`, `_meta.limit_added`, `_meta.hint`. It returns a frozen
`SparqlResponse` with those as fields, so the turtle and JSON shapes are
visible in the type rather than discovered at the call site.
Five `# Fixme:` markers are resolved by code, not deletion:
- The graph cache was a module-level dict, and a second request arriving during
the long listing re-ran it. It is now instance state behind an `asyncio.Lock`
with a second freshness check inside, so ten concurrent callers run the query
once.
- The summary categorised every row, then the expansion pass categorised them
all again. Rows are grouped once and the expansion only decides whether the
grouping is reported.
- `CategoryMatcher` was `Any`; it is `Callable[[str], bool]`.
- `_CategoryRule` was a hand-written `__slots__` class; it is a frozen
dataclass.
- The logger asking for a naming convention was never called by any code, in
this module or the one it replaces. Both are gone.
Four values move out of the code and into the documented configuration: the
graph namespace and the listing's timeout, row cap and cache lifetime. The two
RMES URLs now say which is which: `RMES_SPARQL_ENDPOINT_URL` is posted to,
`RMES_GRAPH_BASE_URI` is only ever a prefix. Deployments setting RMES_ENDPOINT
must rename it -- an unknown variable is ignored, not rejected.
Fixes a defect in the Turtle branch: `result.get("limit_added") and max_rows`
yields `False` when the caller supplied their own LIMIT, which Pydantic then
coerced to `0` on an `int | None` field. A CONSTRUCT or DESCRIBE query reported
`limit_added: 0` where the JSON branch reports `null`, and no limit of zero was
ever added. Both branches now report `null` -- the only change to a value a
caller receives.
Verified against the live endpoint: eleven of twelve recorded cases match
exactly, the twelfth being that fix. They cover 703 graphs across 12 families,
filtering by substring and by family, the expanded listing, a 43-property
resource description, SELECT, ASK, CONSTRUCT, an added LIMIT, an empty query
and a non-SPARQL one.
Commit messages carried `Co-Authored-By` and a session link. They say who typed the change rather than what it does, so they do not belong in the history the changelog is read from. The rule states that it overrides tooling defaults, because the assistant harness instructs the opposite.
Every table in `data/` serves exactly one source, but they sat flat in the package and only the rmes one carried a prefix. They now group the way `services/`, `tools/` and `models/` already do -- folder is the source, filename is what it holds: data/insee/ geography.py themes.py indicators.py data/rmes/ graph_categories.py The `rmes_` prefix goes with the move, since the folder says it. `pyproject.toml` carried a per-file ruff ignore for the long literal tables in `data/indicators.py`. Moving the file orphaned it and surfaced 52 line-length errors in a file that had been exempt, so the path moves with it. Two comments naming the old paths are corrected, including the one inside the `# Business rule:` marker on `get_insee_homepage` -- the path only, not the rule. Pure relocation: the tool contract is byte-identical, and the homepage tool still returns all 59 indicators.
`ES_HOST` had no default, so the server refused to start without it even when the insee.fr and Melodi tools were both disabled. CLAUDE.md says only rmes works without Elasticsearch; the code disagreed, and a rmes-only deployment was impossible. The host is now genuinely optional, and a settings validator demands it only when a family that searches is enabled, naming the flags that would relax the requirement instead of failing on a bare field name. The lifespan built every client and service unconditionally, so it could not express that. It splits into one module per source, each an async context manager that yields its own fragment of the lifespan context and closes its own clients: lifespan/elasticsearch.py the client insee.fr and Melodi share lifespan/insee.py scraper and services lifespan/melodi.py api client and services lifespan/rmes.py sparql client and service `AsyncExitStack` replaces the unconditional `finally`, so a family that was never built is never torn down, and the two families that need Elasticsearch nest under it -- the dependency is structural rather than a comment, and no optional client is handed to a parameter that requires one. `build_lifespan` went from seventeen parameters to one. It takes the whole `Settings` because it is the composition root, the one place whose job is to read configuration; the per-source builders still take narrow values, which is where `python.md` means it. That resolves the `# Fixme:` asking for exactly this. In rmes-only mode no Elasticsearch client is constructed at all, checked by counting constructor calls rather than reading the code.
`dependencies.py` held the providers for all three sources, so adding a source meant editing a file every other source already depends on. It becomes a package with one module per source, matching how `data/`, `services/`, `tools/` and `lifespan/` already group: dependencies/insee.py dependencies/melodi.py dependencies/rmes.py Nothing is re-exported from the package `__init__`: a tool imports from its own source's module, so a new source adds a file rather than editing a shared one.
The deployment passed no TRUSTED_PROXY_HOSTS, so uvicorn kept its default of trusting only 127.0.0.1. Behind the Ingress the peer is a cluster IP, never that, so uvicorn ignored the forwarded caller address and reported the Ingress as the client of every request. The rate limiter keys on that address. Every caller therefore shared one bucket: a hundred requests a minute for the whole world combined, and one busy client could lock out the rest, while the code read as though the limit were per caller. Trusting any peer is safe only while nothing but the Ingress can reach the pod. A NetworkPolicy restricting traffic to the Ingress namespace is what makes that true; without one, a pod inside the cluster could reach the server directly and claim any caller address.
The Host and Origin guard was configured in a way that enforced the opposite of what it looked like. `host_origin_protection="auto"` leaves the decision to a heuristic that defers to existing handling for reverse-proxy deployments, while `allowed_hosts` was left at its `["*"]` default, which accepts any Host. The check ran and approved everyone: a request claiming `Host: evil.example.com` reached the session manager untouched. Meanwhile `allowed_origins` was never passed and had no setting, so the Origin half ran on a default nobody chose and could not adjust. A browser client on another origin was refused with no way to permit it. So the rule that was written did nothing and the rule that was not written did the refusing. Protection is now enforced rather than inferred, the Ingress hostname is declared, and the browser origins are a setting: empty today, because no browser client calls this server. A request claiming another hostname is now answered 421, and a cross-origin browser request 403. Local development is unaffected: with nothing set, `ALLOWED_HOSTS` stays `["*"]`.
added 26 commits
September 8, 2026 12:10
`ErrorCode` was a `Literal`, which Python never checks. A typo was accepted in
silence and reached the caller as an invented code: `AppToolError("TOTALLY_MADE_UP",
...)` produced `[TOTALLY_MADE_UP] oops` with nothing to catch it.
It is a `StrEnum` now, and all twenty-four raises pass a member. A typo is an
`AttributeError` at the line that wrote it, and an editor marks it before the
code runs. No runtime coercion is added: validating the code inside `__init__`
would raise while an `except` block was already handling a failure, replacing
the real error with a confusing one. Referencing a member fails earlier and
more clearly.
`UNKNOWN` becomes `INTERNAL_ERROR`. Every other code says where the fault is
-- the caller's input, a backend, the network -- while `UNKNOWN` described our
ignorance, which reads as an admission rather than information. The caller
learns something useful from `INTERNAL_ERROR`: the fault is in this server, so
rephrasing will not help.
That code had never been used: the one place needing it wrote `"[UNKNOWN] ..."`
as a literal, because it reports a per-URL failure in the result rather than
raising. It now builds the prefix from the same vocabulary, so the two cannot
drift apart, and that branch is exercised by forcing an unexpected failure
rather than assumed to work.
The module is `error.py`, matching the rule file that governs it.
Message text is unchanged throughout, which matters because the message is the
error contract: every insee and Melodi failure message, both live rmes error
paths, and the tool contract are identical.
…gnature The bare * marker was in only three signatures, and every caller already named its arguments, so it duplicated a convention the layout rules already carry. Ruff FBT003 replaces the one case it genuinely guarded: a bare boolean passed positionally to AppToolError. FBT001 and FBT002 stay off, because a tool's boolean parameter is a named field of its JSON schema.
run_rmes_sparql's schema bounds were applied inside the shared execute path, so the graph listing was silently capped at 60s no matter what RMES_GRAPH_LISTING_TIMEOUT_SECONDS was set to, and describe_rmes_resource drew its own budget from a tool it does not expose. The ceiling now sits in run_rmes_sparql, next to the max_rows clamp, and describe_rmes_resource carries its own timeout and row limit.
…igurable ES_MAX_RETRIES and INSEE_DOCUMENT_MAX_MARKDOWN_CHARS were hardcoded constants, though both depend on the deployment: how reliable that Elasticsearch is, and how much context the calling model has. Both reach their clients through the lifespan rather than being read where they are used.
The two clients announced themselves as McpDiffusion/0.1 and MCP-RMeS/2.0: two product names and two versions, neither matching pyproject.toml. Both now send McpDiffusion/0.1.0. insee.fr keeps its browser string, which it needs to be served the real markup.
Both files carried a Fixme claiming these values belong in settings. They do not: they bound the published tool schema, so an env-driven value would advertise a different contract per deployment, and they are read at import time, before any Settings instance exists. The banner said "Constants", which explained nothing about why they sit beside the schema they shape.
…ctured send_feedback was registered until d00e9a7 removed the else branch that carried it. It was the only tool registered exclusively there, so it went unnoticed while SKILL.md kept telling clients to call it. It now records to the server log instead of appending to a file inside the container, which was lost on every restart, blocked the event loop, and was tracked by git despite holding user-submitted content. Client text is JSON-encoded into the message so an embedded newline cannot forge a second log line, and both fields are length-bounded in the schema. Registration is gated on ENABLE_FEEDBACK_TOOL, matching the three data sources, and the handshake instructions name the tool only when it is enabled. feedback.md stays on disk but is no longer tracked.
error.py and the Elasticsearch failure translator sat at the package root and in services/ respectively, though both belong to one concern and are used by every layer. errors/ now holds them, with a membership rule the root never had: does it define or produce an AppToolError. The translator also gains a home it lacked. It was never a service -- it orchestrates nothing and holds no client -- so services/ only ever housed it for want of anywhere better.
rate_limiting.py was named after the feature that consumes the function rather than what the function does: it resolves the caller's address and knows nothing about windows or buckets. It is now utils/client_host.py, named for its subject, and the docstring says why the middleware context argument goes unused. The package root is left holding only what server.py needs to boot.
enable_inseefr_tools was the only identifier using "inseefr". Five directories, all five tool names and every other setting already say "insee", so the flag was the outlier rather than the convention. A comment now carries what the longer name was reaching for: INSEE is the institution and owns all three sources, while this flag is only the website. ENABLE_INSEEFR_TOOLS becomes ENABLE_INSEE_TOOLS. No manifest sets it, so nothing breaks silently, but it belongs in the release notes.
The guidance sections point at each other -- the Melodi entries route to search_insee_documents, the insee.fr ones to run_rmes_sparql -- so a deployment with a family disabled was telling the model to call tools that were never registered. A global rule now says the tool list is what exists, rather than trying to keep prose and configuration in step. The insee.fr bullet that routed to two sources in one sentence is split, one per source.
get_insee_document took an unbounded list, and each entry costs a fetch of insee.fr plus a full extraction. A caller could ask for hundreds in one call and hold the request open while insee.fr absorbed the load. The bound sits in the schema rather than in a runtime check, so the model is told the limit instead of discovering it through an error.
ThemeChoice and ThemeConjonctureChoice listed by hand the same labels that the tables in data/insee/themes.py already hold, so a theme added to one was invisible to the other. Both are now derived from those tables, following what rmes already does with GraphCategoryChoice. The enum values are unchanged; only their order follows the tables now. The curated indicator entries gain a TypedDict naming their three keys. They stay plain dicts, because data/ holds plain data and the consuming layer is what turns it into typed objects.
describe_rmes_resource interpolates the resource and graph URIs into `<...>`, so a URI carrying `>` closed the brackets and the rest of the string ran as query text. Both parameters are now checked against the IRI grammar, which forbids those characters anyway, so nothing legitimate is refused. Not an escalation -- run_rmes_sparql already accepts arbitrary SPARQL from the same caller. What it fixes is the diagnosis: a malformed URI now fails naming the parameter, instead of coming back as a syntax error from RMES.
MAX_DOCUMENT_URLS bounds the list in the schema, so the note asking for it was stale. The one below it opened with "on top of that" and lost its antecedent, and claimed the sequential fetching blocks the event loop -- awaiting in a loop is serial, not blocking. Reworded to say what is actually wrong with it.
The check looked for LIMIT anywhere in the query, so one inside a subquery -- or the word inside a string literal -- counted as the caller's own and no cap was added. The outer query then returned as many rows as the endpoint would give. A LIMIT that bounds the whole query is the last thing in it, with only OFFSET allowed to follow, so the check is anchored to the tail. This bounds what comes back, not what RMES computes: a subquery with no limit of its own is still evaluated in full upstream. The per-query timeout stays the only guard on that.
…lter
An observation whose `dimensions` was null crashed the filter: the `{}` fallback
only applies when the key is absent, not when its value is null. That raised an
AttributeError, which is not a ToolError, so the caller lost the entire batch to
a generic internal error over a single row.
Reading the year moves into its own function, total for every shape of missing
period. The open question -- whether TIME_PERIOD is always a string, which we do
not control -- stays marked, with a note that coercing it would hide a change in
the upstream format rather than surface it.
A batch took the sum of its URLs: each was fetched only once the previous one had been rendered. They now go out together, bounded by MAX_DOCUMENT_URLS on the schema rather than a second limit of its own. Rendering one URL moves into its own method, which returns a failure as a result instead of raising, so one bad URL still costs the caller nothing but that entry and the order still matches the URLs given.
Turning a page into markdown costs 80-1000 ms of CPU, and it ran on the event loop. During a batch the server served nothing else: a probe scheduled every 10 ms got zero turns until the whole batch finished. Rendering and table-of-contents parsing now run in a worker thread. The batch itself costs about 7% more wall-clock; in exchange the loop stays responsive, with a worst observed stall of 56 ms. Sharing TRAFILATURA_OPTIONS across threads was checked first: extract() never writes to it, and concurrent runs return byte-identical output to serial ones.
The return annotations are correct; what the note described -- that `ctx.lifespan_context` is an untyped mapping, so they are asserted rather than checked -- follows from the FastMCP lifespan contract and holds for the Melodi and RMES modules just the same. Only insee.py said so, which made the three read as though one of them were different.
Two notes sat on this table. Whether static reference data belongs in the source tree was settled when data/ was reorganised by source, so that one goes. The other is real but not ours to answer: the ids are insee.fr's own, transcribed by hand, and nothing in this repo can check them. A wrong id searches the wrong theme without failing, which makes it a Business rule -- preserved, flagged, left to whoever owns the site's taxonomy.
"I wonder whether the API supports filtering" is now answered by testing it. It does filter, but `TIME_PERIOD=2025` matches only periods starting on that date: on DS_DECES_MORTALITE_SERIES, which holds annual and monthly rows together, it returns the yearly row and January while skipping August entirely. Moving the filter upstream would look like an obvious speed-up and would silently drop most of a monthly dataset, so fetching everything is deliberate rather than pending. The note on the value's type shrinks to the one thing that is not obvious from the code: str() is absent on purpose, because a coerced value would match nothing instead of failing.
The build context carried the whole working tree, .venv and .git included, to the daemon on every build; a .dockerignore now excludes it. uv was pulled from the :latest tag, so the tool could change between two builds of the same commit. `uv sync` used --frozen, which installs a stale lock without complaint: a dependency added to pyproject.toml but never locked would be missing from the image and surface as an ImportError at runtime. --locked fails the build instead. The image also gains a healthcheck, so compose can wait for the server rather than guess, OCI labels tracing it back to this repository, and the venv on PATH so `python` means the right one when exec-ing into a container. Cache mounts keep uv's downloads between builds without storing them in the image.
Compose could not start the project. It referenced an image nobody built, ran no Elasticsearch though the insee.fr and Melodi tools require one, expected a network created by hand, and read an env file that does not exist. Anyone cloning the repo had no way to run it. It now builds the server, starts Elasticsearch with the INSEE indexes, restores the snapshot and waits for each step to be healthy before the next. The restore skips itself when the data is already there, so a restart costs seconds rather than a minute. Elasticsearch is built from the published amd64 image rather than run from it: the snapshot is copied into the official multi-arch base, so it runs native on arm64 and amd64 with no emulation. Setting path.repo there also removes the dependency on configuration baked into an image we do not own. ES_HOST defaults to the bundled service but yields to one set in .env, which is what lets an existing cluster be used with `up mcpdiffusion inspector`. The example env file no longer ships a value that would be wrong inside a container.
Fifty-five commits are hard to review as a list. This groups them by what they address -- architecture, FastMCP adoption, bugs, security, configuration, performance, build -- and states the before and after for each. It also records what was deliberately not done: the eight questions left for whoever owns the data semantics, and the four known gaps, so neither reads later as something that was missed.
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
1 participant
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.
Restructuring the whole server, adopting the FastMCP 4 APIs, and fixing what the code
review turned up. Full breakdown in
docs/report.md.Breaking
Three RMES tools renamed — connected clients calling the old names will fail:
RMES_describe_resourcedescribe_rmes_resourceRMES_list_graphssearch_rmes_graphsRMES_run_sparqlrun_rmes_sparqlThree environment variables renamed:
ES_INDEX_PRODUITSES_INDEX_PUBLICATIONSRMES_ENDPOINTRMES_SPARQL_ENDPOINT_URLENABLE_INSEEFR_TOOLSENABLE_INSEE_TOOLSThe Kubernetes
env:block needs the first two.What changed
Architecture. 23 mostly-flat modules became 58 grouped by responsibility. The
helpers/,core/,infra/andconfig/drawers are gone.FastMCP 4. Dependency injection replaces service-locator lookups, built-in rate limiting and
host protection replace hand-rolled versions, and
mask_error_detailsis on.About 20 bugs, each reproduced before being fixed. Elasticsearch
ApiErrorescaped thefailure boundary entirely. A SPARQL injection was possible through a resource URI. Every caller
shared one rate-limit bucket behind the ingress. A
LIMITinside a subquery left the outer queryunbounded.
Configuration. 7 variables became 31, all typed and documented.
Performance. Documents now fetch concurrently, and rendering moved off the event loop — it
previously blocked every other request for the length of a batch.
Build. ruff adopted.
docker compose upworks from a clean clone, which it did not before.Verification
The auto-generated test suite does not collect and was not relied on. Instead: the tool contract
was diffed at every step, error messages were diffed byte-for-byte, live calls were made to all
three upstreams, and the full stack was verified from an empty volume — 51 seconds cold start,
three indexes restored, twelve tools answering.
Left open
Eight
# Business rule:markers record questions only the data owners can answer.Known gaps: no readiness or liveness probe on the MCP Kubernetes deployment, no Elasticsearch
credentials support, and
README.mdandSKILL.mdawait regeneration.