feat(python): add an MCP server for authoring GeoLibre projects - #1734
Merged
Conversation
Adds `geolibre-mcp`, a headless stdio MCP server that writes real `.geolibre.json` projects from an AI client. No browser, no running app, and no bundled web build: it composes projects with the same builders the Python package already uses, so anything it writes opens unchanged in the desktop app, the web app, and the Jupyter widget. Layering, so nothing is duplicated: - `project.py` keeps *building* pieces (a layer, a plugin-state blob). - New `authoring.py` *applies* them to a whole project: add/remove/reorder and restyle layers, classify a choropleth, move the camera, compose the legend/colorbar/swipe controls, and summarize a project back. - `Map` now delegates its split-map, legend, colorbar, and choropleth composition to `authoring.py` instead of holding its own copy, so the widget and the MCP server cannot drift. - `to_html` splits into a module-level `render_project_html()` the server can call without a widget; `Map.to_html` delegates to it. The server itself is `mcp/server.py` (21 tools), the only module that imports the `mcp` SDK, kept behind the optional `geolibre[mcp]` extra. `mcp/workspace.py` confines every read and write to roots given via `--root` or `GEOLIBRE_MCP_ROOTS`, mirroring the sidecar's `GEOLIBRE_CONVERSION_ROOTS`; symlinks out of a root are refused, writes are limited to `.json`/`.html`, and an existing file needs `overwrite`. Verified end to end: a client spawned the server over stdio, listed the tools, and built a choropleth of 52 US states fetched from a live URL plus COG, XYZ, legend, colorbar, and swipe layers; the resulting project round-trips through the app's own `parseProject` with its symbology and plugin state intact, and a workspace escape is refused over the wire. `tests/test_mcp_server.py` skips itself without the SDK, so publish-python.yml installs `mcp` explicitly rather than shipping the server untested.
Contributor
|
Note Reviews pausedIt 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 Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughGeoLibre adds a shared widget-free authoring layer and a workspace-confined MCP server. It adds project editing tools, HTML export, optional packaging, CI coverage, tests, and documentation. ChangesMCP authoring
Estimated code review effort: 5 (Critical) | ~120 minutes Sequence Diagram(s)sequenceDiagram
participant MCPClient
participant MCPServer
participant Workspace
participant Authoring
MCPClient->>MCPServer: Invoke an authoring tool
MCPServer->>Workspace: Resolve a confined project path
Workspace-->>MCPServer: Return the validated path
MCPServer->>Authoring: Load and mutate the project
Authoring-->>MCPServer: Return a project summary
MCPServer->>Workspace: Save the project
MCPServer-->>MCPClient: Return the tool result
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
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 |
Contributor
🔍 Cloudflare PR preview
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 6
🤖 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 `@CLAUDE.md`:
- Around line 86-87: Update the test reference in the MCP server documentation
to use the full repository-relative path python/tests/test_mcp_server.py,
replacing the shortened tests/test_mcp_server.py reference; leave the
surrounding workflow and SDK guidance unchanged.
In `@python/src/geolibre/authoring.py`:
- Around line 289-296: Update the values comprehension in the column-loading
logic to require that each feature’s properties value is a dictionary before
calling `.get(column)`, matching the existing `layer_properties` guard. Preserve
the current ValueError when no valid feature contains the requested column.
- Around line 91-94: Update the project-writing function containing the shown
Path logic to serialize into a temporary file in the destination’s parent
directory, then atomically replace the destination using os.replace. Preserve
parent-directory creation, UTF-8 encoding, JSON formatting, and the existing
Path return value; ensure temporary-file cleanup if replacement fails.
In `@python/src/geolibre/mcp/workspace.py`:
- Around line 112-116: Update the suffix validation around target.name so each
accepted filename has a non-empty stem before the matched suffix, rejecting bare
dotfiles such as “.json” and “.html” while preserving the existing allowed
suffixes and WorkspaceError behavior.
- Around line 48-55: Update the workspace constructor’s root validation around
_roots_from_env and the resolved list to reject an empty candidate set before
assigning self.roots. Raise WorkspaceError using the same fail-fast behavior as
invalid directory roots, while preserving existing expansion, resolution, and
validation for non-empty roots.
In `@python/tests/test_mcp_server.py`:
- Around line 85-88: Remove the unused is_dir() conditional in
test_workspace_rejects_traversal_out_of_a_root and initialize Workspace directly
with [tmp_path]. Keep the existing traversal assertion and expected
WorkspaceError unchanged.
🪄 Autofix
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: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: e10a808a-e4dd-4a8e-aeef-bd3c8bb373c9
📒 Files selected for processing (15)
.github/workflows/publish-python.ymlCLAUDE.mddocs/mcp.mddocs/python.mdmkdocs.ymlpython/README.mdpython/pyproject.tomlpython/src/geolibre/authoring.pypython/src/geolibre/geolibre.pypython/src/geolibre/mcp/__init__.pypython/src/geolibre/mcp/__main__.pypython/src/geolibre/mcp/server.pypython/src/geolibre/mcp/workspace.pypython/tests/test_authoring.pypython/tests/test_mcp_server.py
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
Contributor
Code reviewBugs
Security
Performance
Quality
CLAUDE.md
Beyond the two inline notes above, I cross-checked every |
…nted render_mode An audit of the tool surface found 6 of 21 tools with no test at the MCP layer, and two more (add_tiles_layer, remove_layer) covered only on their error paths. Some were covered one layer down in test_authoring.py, but the argument plumbing in the tool wrappers was not. Filling the gap turned up a real defect: add_vector_layer documented render_mode as "geojson" or "vector-tiles", but project.vector_layer accepts "geojson" or "tiles". A model following the docstring would have hit a ValueError on every call. The docstring now names the real values, and a test asserts the undocumented spelling is rejected, so the two cannot drift again. Adds happy-path coverage for every add_* tool (parametrized over the builder each one plumbs, asserting the resulting layer type), plus style_layer merge semantics, remove_layer, set_basemap, add_legend from both a preset and paired lists, add_colorbar, and the legend/colorbar coexistence in their shared settings blob.
Contributor
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
python/src/geolibre/mcp/server.py (1)
90-93: 🔒 Security & Privacy | 🟠 Major | ⚡ Quick winOther (CWE-73)
Reachability: External
Restrict existing project paths to project extensions before reading/writing.
edit()accepts the MCP-controlledpath, confines only the path and existence, then applies the mutation and writesauthoring.save_project(file, project)back to the same file. Becauseauthoring.load_project()accepts any JSON object and seedslayerswhen it is absent, a mutating tool can replace non-project JSON files such aspackage.jsoninside a workspace root.Apply the same extension check used by
create_projectbefore everyauthoring.load_project()/authoring.save_project()call. Use the stricter.geolibre.jsonsuffix in existing files unless.jsonis intentionally required and documented.🤖 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 `@python/src/geolibre/mcp/server.py` around lines 90 - 93, The edit flow around the project-loading generator must restrict existing files to the project extension before any read or write. Reuse the extension validation used by create_project, requiring the stricter .geolibre.json suffix, before authoring.load_project and authoring.save_project in the relevant edit path.
🤖 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.
Outside diff comments:
In `@python/src/geolibre/mcp/server.py`:
- Around line 90-93: The edit flow around the project-loading generator must
restrict existing files to the project extension before any read or write. Reuse
the extension validation used by create_project, requiring the stricter
.geolibre.json suffix, before authoring.load_project and authoring.save_project
in the relevant edit path.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 50f60fa3-13f9-4014-ad03-c1a0fd23c879
📒 Files selected for processing (2)
python/src/geolibre/mcp/server.pypython/tests/test_mcp_server.py
Contributor
Code reviewBugs
Security
Performance
Quality
CLAUDE.md
Overall the layering ( |
Contributor
🔍 GitHub Pages PR preview
|
- authoring.save_project: write to a temporary file in the destination's directory and os.replace it into place, so an interrupted write cannot truncate an existing project (the MCP edit path rewrites the whole file on every tool call). - authoring.column_values: GeoJSON permits `"properties": null`, which made `.get(column)` raise AttributeError instead of the documented ValueError. Guard with isinstance, matching layer_properties. - mcp/workspace.Workspace: refuse an empty root set at construction. A separators-only GEOLIBRE_MCP_ROOTS filtered down to no roots and every later relative-path resolve raised IndexError instead of WorkspaceError. - mcp/workspace.resolve_output: require a non-empty stem before the suffix, so a bare dotfile named exactly `.json` or `.html` is refused as the comment already claimed. - mcp/server.edit: run the destination through the same extension allowlist create_project writes through, and check the loaded JSON actually looks like a GeoLibre project. load_project accepts any JSON object and seeds `layers`, so a mutating tool could previously rewrite an unrelated `package.json` inside a workspace root as a map. - mcp/server.create_project: route the initial zoom through authoring.set_view so it is clamped to [0, 24] like every other camera-mutating path. - mcp/server.add_geojson_layer: resolve the destination before fetching or reading `data`, so a bad path fails without paying for the download first. - mcp/workspace: note that `.geolibre.json` in PROJECT_SUFFIXES is a subset of `.json` and is listed to name the convention in the client-facing error. - Tests for each of the above, and drop the dead `is_dir()` conditional in test_workspace_rejects_traversal_out_of_a_root. - CLAUDE.md: use the repository-relative `python/tests/test_mcp_server.py`. - docs/mcp.md: document the two new write guards.
Member
Author
|
Addressed the outside-diff finding on
I did not narrow the allowlist to |
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
python/src/geolibre/mcp/server.py (1)
835-836: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winWrite HTML exports atomically when overwrite is enabled.
destination.write_texttruncates an existing export before the full HTML is written. A write failure or interruption can destroy the prior export. Reuse or extract the same temporary-file-and-os.replacehelper used byauthoring.save_project.🤖 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 `@python/src/geolibre/mcp/server.py` around lines 835 - 836, Update the HTML export write flow around destination.write_text to use the existing temporary-file-and-os.replace atomic-write helper from authoring.save_project, particularly when overwrite is enabled. Ensure the temporary file is fully written before replacing destination so failed or interrupted writes preserve the previous export.
🤖 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 `@python/src/geolibre/mcp/server.py`:
- Around line 251-253: Update the validation flow around workspace.resolve and
the edit operation to validate the complete destination before loading data:
enforce the same allowed suffix, file-type, and project-location checks used by
edit. Perform these checks before either reading a local file or fetching a URL,
while preserving the existing path-confinement and existence validation.
---
Outside diff comments:
In `@python/src/geolibre/mcp/server.py`:
- Around line 835-836: Update the HTML export write flow around
destination.write_text to use the existing temporary-file-and-os.replace
atomic-write helper from authoring.save_project, particularly when overwrite is
enabled. Ensure the temporary file is fully written before replacing destination
so failed or interrupted writes preserve the previous export.
🪄 Autofix
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: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 1bcb9f83-7ca6-4ded-b67c-1c3c5a095b26
📒 Files selected for processing (7)
CLAUDE.mddocs/mcp.mdpython/src/geolibre/authoring.pypython/src/geolibre/mcp/server.pypython/src/geolibre/mcp/workspace.pypython/tests/test_authoring.pypython/tests/test_mcp_server.py
Contributor
Code reviewThis PR adds a headless Bugs
Security
Performance
Quality
CLAUDE.md
|
- render_project_html: require app_url to be an http(s) URL with a host, and post the project to that exact origin instead of "*". The MCP export_html tool takes app_url straight from a tool call, so a model reading untrusted content could be steered into posting a project — inlined features, layer URLs, camera — to an attacker's origin. Verified the default host (web.geolibre.app) serves the embed with no cross-origin redirect, so the strict targetOrigin does not break the default export. - authoring.save_project: chmod the temporary file to the destination's mode before os.replace. NamedTemporaryFile creates at 0600, so the atomic write added in c97c1f8 would have narrowed a project the user had made group/world readable on its first edit. - mcp/server.add_geojson_layer: load `data` inside the edit context, so the destination clears every check edit() makes — confinement, extension, and that the file really is a project — before the fetch or read is paid for. The previous pre-check only covered confinement and existence. - mcp/server.add_geojson_layer: the docstring said `https://` URL; load_geojson accepts plain `http://` too. - Tests for each of the above.
Contributor
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
python/src/geolibre/mcp/server.py (1)
829-835: 🔒 Security & Privacy | 🟠 Major | ⚡ Quick winSensitive Data Exposure (CWE-200): Exposure of Sensitive Information to an Unauthorized Actor
Reachability: External · Exploitability: Moderate
Reachability path
● Entry python/src/geolibre/mcp/server.py:834 render_project_html │ ▼ ● Sink python/src/geolibre/geolibre.pyValidate the export source as GeoLibre project shape before rendering it.
export_htmlonly checks workspace existence and project-file suffix;authoring.load_projectaccepts any JSON object and seedslayerswhen absent. That can export unrelated workspace JSON, embed it in the exported page, and send it to the selected app origin. Call_require_project(file, project)beforerender_project_html()and add a behavioral test thatexport_htmlrejects a non-project JSON file such aspackage.json.🤖 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 `@python/src/geolibre/mcp/server.py` around lines 829 - 835, In export_html, validate the loaded project with _require_project(file, project) immediately after authoring.load_project and before render_project_html. Add a behavioral test confirming export_html rejects a non-project JSON file such as package.json.Source: Coding guidelines
🤖 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.
Outside diff comments:
In `@python/src/geolibre/mcp/server.py`:
- Around line 829-835: In export_html, validate the loaded project with
_require_project(file, project) immediately after authoring.load_project and
before render_project_html. Add a behavioral test confirming export_html rejects
a non-project JSON file such as package.json.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: b62891c4-8631-4b91-a0d1-6dc049e45638
📒 Files selected for processing (6)
python/src/geolibre/authoring.pypython/src/geolibre/geolibre.pypython/src/geolibre/mcp/server.pypython/tests/test_authoring.pypython/tests/test_mcp_server.pypython/tests/test_scripting.py
Contributor
Code reviewBugs
Security
Performance
Quality
CLAUDE.md
|
- mcp/server._require_project: drop the `or project.get("layers")` fallback.
load_project normalizes `layers` onto whatever it loaded, and a top-level
`layers` array is not exclusive to this format (map/style configs use one
too), so the fallback let an unrelated config pass the guard the marker
check exists to enforce. Requires `mapView` or `basemapStyleUrl` now, both
of which every project this package or the app writes carries.
- authoring.set_view: clear `mapView.bbox` when `center` or `zoom` is set.
fit_bounds records the bbox to describe the camera it computed; moving the
camera by hand left it describing a different extent, which the app reads
for the status bar's BBox readout. Bearing and pitch leave it alone, since
neither changes the extent. `bbox` is optional in the schema and StatusBar
renders "—" without it.
- Tests for both, plus the set_view tool docstring.
- Credential redaction no longer strips the first-party map controls. Both implementations blanked `plugins.settings` wholesale, so the documented compose-then-export flow (add_legend/add_colorbar/add_swipe -> export_html) produced a page with no legend, colorbar, or swipe — and Map.to_html and Map.save_project lost them too. A new PUBLISHABLE_PLUGIN_SETTINGS allowlist keeps the two first-party control plugins, whose state is known and carries no credentials, and still drops every external plugin's free-form blob, so the no-secret guarantee the wholesale wipe existed for is unchanged. Mirrored in packages/core/src/credentials.ts so the app's share/export path does not diverge from the Python one. - project.load_featurecollection: cap literal GeoJSON text at 50 MB. Only the URL and file forms were bounded, so the "Capped at 50 MB" the MCP tool documents was not true for the one form an MCP client supplies directly. - mcp/server.create_project: `overwrite=True` now means "replace the project there", not "replace whatever is there". PROJECT_SUFFIXES admits any `.json`, so an agent retrying after an "already exists" error could destroy an unrelated config with none of the safety net edit() applies. - authoring.describe_project: report `swipe` only when its plugin is also in activePluginIds. A settings blob left behind by a deactivated control was reported as a live control. The legend and colorbar are drawn from settings alone, so they stay settings-only. - authoring.column_values: distinguish a column absent from every feature from one present but null in all of them; only the first is worth retrying with a different name. - Tests for each, including an export-keeps-its-controls test on both the MCP tool and Map.to_html, which is what the previous test missed.
Contributor
Code reviewBugs
Security
Performance
Quality
CLAUDE.md
|
Both findings were holes in the round-5 fixes. - PUBLISHABLE_PLUGIN_SETTINGS is now plugin id -> allowed sub-keys, not a flat id allowlist, and the components plugin lists only `legend` and `colorbar`. Its `html` sub-key holds a custom panel the user authored by hand, so it can carry anything — including a URL with a token — and keeping the whole blob would have let that survive an export. The swipe blob is structured throughout (layer ids, orientation, position, flags) and is still kept whole. What survives now also goes through the same credential sweep layer configuration gets, rather than being trusted verbatim; a blob with an unexpected shape is dropped rather than passed through. Mirrored in packages/core/src/credentials.ts so the two implementations still agree. - mcp/server.create_project: a ValueError from load_project now refuses the overwrite instead of skipping the guard. A file that cannot be read as a project at all — malformed, a JSON array, over MAX_PROJECT_BYTES — is the case to refuse hardest, not the one where the check falls away. The error says to delete the file if it is unwanted, so a corrupted project is not a dead end. - Tests for each, on both the Python and TypeScript sides.
Contributor
|
All inline comments posted. Here's the final summary. Code reviewBugs
Security
Performance
Quality
CLAUDE.md
|
- authoring.set_view: reject non-finite camera values. `json.loads` turns an
out-of-range literal like `1e400` into `inf` without raising, and
`json.dumps` writes it back as a bare `Infinity` token — not valid JSON per
RFC 8259, so the app's JSON.parse rejects the whole project. center, zoom,
bearing, and pitch all go through a `_finite` check now, matching
fit_bounds. save_project also passes allow_nan=False as a backstop, so no
other path can write a project the app cannot read.
- authoring.remove_layer: drop the removed layer's id from a swipe control's
leftLayers/rightLayers. The split kept a dangling reference with nothing
surfacing it. The basemap pseudo-id is not a layer, so it is untouched.
- mcp/server._build_layer: check `style` against the builder's whole
signature, not just the arguments the tool forwards. vector_layer takes
`picker`/`ingest_mode` and vector_tiles_layer takes a singular
`source_layer`, none of which the tools expose — so `style={"picker": False}`
bound to real behavior while the docstring promised style overrides. This
also catches the positional case up front, leaving the TypeError branch as a
backstop.
- authoring.save_project: use stat.S_IMODE when carrying the destination's
mode over, so only the permission bits are copied.
- Tests for each.
Contributor
Code reviewReviewed the full diff (docs, Bugs
Security
Performance
Quality
CLAUDE.md
Overall this is a well-scoped, defensively coded PR with strong test coverage (workspace confinement, SSRF, credential redaction parity, HTML-export injection surfaces are all exercised on both the TS and Python sides). I did not find any correctness or security defects worth blocking on. |
- geolibre-mcp now points at a package-level `geolibre.mcp:main` wrapper that
translates a missing `mcp` extra into an actionable line ("Install it with:
pip install \"geolibre[mcp]\"") and exit code 1, instead of a raw
ModuleNotFoundError traceback. `python -m geolibre.mcp` goes through the same
wrapper. A dependency missing from underneath an installed SDK still
propagates, since that is a broken environment rather than a missing extra
and its traceback is the useful thing to show.
- Tests for both branches, plus a real check that `geolibre-mcp --help` still
works through the new entry point.
Contributor
Code reviewBugs
Security
Performance
Quality
CLAUDE.md
|
- authoring.add_layer: enforce the reserved "__basemap__" name at creation, not only on rename. The round-4 guard covered update_layer but a client could still create a layer wearing the sentinel, which resolve_layer_ids would then treat as the basemap for any name-based reference. The check is now one _reject_reserved_name helper both paths call. - packages/core/src/credentials.ts: cast the redacted settings to Record<string, unknown>, matching the field's declared type, rather than Record<string, never> — which claims no property may have a value and only type-checked because the value flowed through `unknown`. - docs/mcp.md: document `app_url` as a trust boundary on export_html. The exported page posts the project to exactly that origin, and the caller is a model that may be acting on content it has read. Credentials are already stripped so this is not a key leak, but the rest of the project travels, and the residual risk is worth stating rather than leaving implicit. - Test that add_layer refuses the reserved name.
Contributor
|
Waiting for the background review agents to complete before synthesizing the final report. |
The server was registered and healthy, all 21 tools loaded, and a model asked to "make me a choropleth of US state population density" still wrote geopandas + matplotlib + folium by hand and never called it. The descriptions said what each tool does, never when to use it. `create_project` read "Create a new, empty GeoLibre project file" -- true, and useless to a model deciding whether this is the right path for a map request. Since create_project is the entry point, nothing downstream could fire either. Rewrites the server instructions and the four entry-point descriptions (create_project, add_geojson_layer, classify_layer, export_html) to lead with the user intents they serve -- "make me a map of X", "a choropleth of Y", "show this on a map" -- and to say plainly when hand-written plotting code is the better answer instead (a static figure for a paper, a projection GeoLibre cannot render). Measured, not assumed. Same prompt, same clean directory, before and after: 0 geolibre calls before, 11 after, in the intended order -- list_catalog, create_project, add_geojson_layer, classify_layer, set_view, add_legend, export_html, describe_project -- producing a valid project with 51 states classified into 7 quantile breaks and a 127 KB standalone page.
Comment on lines
+2034
to
+2048
| # Layer objects are resolved to ids here (authoring.py works on plain | ||
| # project dicts and knows nothing about the Layer handle); the rest of | ||
| # the validation and state building is shared with the MCP server. | ||
| left = self._coerce_layer_ids(left_layers) | ||
| right = self._coerce_layer_ids(right_layers) | ||
| clamped = min(100.0, max(0.0, float(position))) | ||
| state = _project.swipe_state( | ||
| left_layers=left, | ||
| right_layers=right, | ||
| orientation=orientation, | ||
| position=clamped, | ||
| ) | ||
|
|
||
| def mutate(p: dict[str, Any]) -> None: | ||
| _project.set_plugin_state( | ||
| self._update_project( | ||
| lambda p: _authoring.add_swipe( | ||
| p, | ||
| _project.SWIPE_PLUGIN_ID, | ||
| state, | ||
| position=control_position, | ||
| left_layers=left, | ||
| right_layers=right, | ||
| orientation=orientation, | ||
| position=position, | ||
| control_position=control_position, | ||
| ) | ||
|
|
||
| self._update_project(mutate) | ||
|
|
||
| def _update_components_state( | ||
| self, key: str, entry_state_builder: Callable[[Any], dict[str, Any]] | ||
| ) -> None: | ||
| """Merge one feature's state into the Components plugin settings. | ||
|
|
||
| The Components plugin (legend / colorbar / html) stores all its features | ||
| under a single settings blob keyed by feature name, so a new legend must | ||
| be merged in without dropping an existing colorbar (and vice versa). | ||
|
|
||
| Args: | ||
| key: The feature key (``"legend"`` or ``"colorbar"``). | ||
| entry_state_builder: Called with the feature's current state (or | ||
| ``None``) and returns its new state. | ||
| """ | ||
|
|
||
| def mutate(p: dict[str, Any]) -> None: | ||
| plugins = _project.ensure_plugins_block(p) | ||
| current = plugins["settings"].get(_project.COMPONENTS_PLUGIN_ID) | ||
| components = dict(current) if isinstance(current, dict) else {} | ||
| components[key] = entry_state_builder(components.get(key)) | ||
| # The legend/colorbar restore from their settings blob alone, so the | ||
| # plugin is configured but not added to activePluginIds (activating | ||
| # it would also mount the full Components toolbar). | ||
| _project.set_plugin_state( | ||
| p, | ||
| _project.COMPONENTS_PLUGIN_ID, | ||
| components, | ||
| activate=False, | ||
| ) | ||
|
|
||
| self._update_project(mutate) | ||
| ) |
Contributor
There was a problem hiding this comment.
Validation order changed here: before this refactor, split_map checked orientation/control_position before coercing the layer arguments (see the removed lines in the diff). Now _coerce_layer_ids runs first, and the orientation/control_position checks happen later, inside authoring.add_swipe.
Concretely, m.split_map(123, orientation="bad") used to raise "orientation must be one of [...], got 'bad'" and now raises "Layer reference must be a layer id string, a Layer, or a list of those; got 123" instead — the wrong argument gets blamed first when both are invalid. Low impact (both still raise ValueError), but any code/docs relying on the old precedence would see a different message/exception path. Confidence: medium.
Comment on lines
+449
to
+475
| def add_tile_layer( | ||
| path: str, | ||
| name: str, | ||
| url: str, | ||
| tile_size: int = 256, | ||
| attribution: str | None = None, | ||
| index: int | None = None, | ||
| ) -> dict[str, Any]: | ||
| """Add a raster XYZ tile layer from a `{z}/{x}/{y}` URL template. | ||
|
|
||
| This is how you add OpenStreetMap-style raster basemaps and imagery | ||
| services. A vector *style* (as opposed to tiles) belongs in | ||
| `set_basemap` instead. | ||
|
|
||
| Args: | ||
| path: Path to the `.geolibre.json` file. | ||
| name: The layer's display name. | ||
| url: Tile URL template containing `{z}`, `{x}`, and `{y}`. | ||
| tile_size: Tile edge in pixels, usually 256. | ||
| attribution: Attribution text to credit the source. | ||
| index: Draw-order position; appended on top when omitted. | ||
|
|
||
| Returns: | ||
| The new layer's id and the project's updated layer count. | ||
| """ | ||
| layer = _project.tile_layer(name, url, tile_size=tile_size, attribution=attribution) | ||
| return add(path, layer, index) |
Contributor
There was a problem hiding this comment.
add_tile_layer (raster XYZ) is the only add_*_layer tool that doesn't accept a style parameter or route through _build_layer — it calls _project.tile_layer(...) directly. Every sibling tool (add_vector_layer, add_raster_layer, add_tiles_layer, ...) lets the caller set style overrides (opacity, etc.) at creation time via _build_layer. Here a client has to create the layer and then make a separate style_layer/update_layer call to set anything beyond tile_size/attribution. Given how closely-named add_tile_layer and add_tiles_layer are, this asymmetry is easy to miss and looks like drift rather than a deliberate design choice. Confidence: medium.
Comment on lines
+61
to
+70
| def call_error(server, tool, /, **arguments): | ||
| """Invoke a tool expected to fail and return its error text. | ||
|
|
||
| A tool that raises surfaces as a ``ToolError`` from this in-process entry | ||
| point; over the wire the same failure reaches the client as an ``isError`` | ||
| result. | ||
| """ | ||
| with pytest.raises(ToolError) as excinfo: | ||
| asyncio.run(server.call_tool(tool, arguments)) | ||
| return str(excinfo.value) |
Contributor
There was a problem hiding this comment.
call_error only asserts that the in-process server.call_tool(...) raises ToolError; it never inspects CallToolResult.is_error/isError. Every workspace-escape/refusal test in this file goes through this helper, so none of them actually verify the PR description's specific claim that "a workspace escape is refused over the wire as an isError result" — the docstring here concedes as much ("over the wire the same failure reaches the client as an isError result", asserted only in a comment). Contrast with call above, which does check result.is_error on the success path. If the SDK's in-process vs. wire error surfacing ever diverges, this suite stays green while the actual client-facing contract breaks. Consider adding at least one test that drives a tool call through the wire-facing path (or asserts on result.is_error/result.content the way call does) to pin down the documented behavior. Confidence: medium-high.
Comment on lines
+77
to
+92
| it("keeps the first-party map controls so an export still renders them", () => { | ||
| const original = credentialProject(); | ||
| original.plugins!.settings = { | ||
| external: { arbitraryName: "plugin-secret" }, | ||
| "maplibre-gl-components": { legend: { A: "#112233" } }, | ||
| "maplibre-gl-swipe": { position: 50 }, | ||
| }; | ||
| const { project, redactedPaths } = redactProjectCredentials(original); | ||
| const settings = project.plugins!.settings; | ||
|
|
||
| assert.deepEqual(settings["maplibre-gl-components"], { legend: { A: "#112233" } }); | ||
| assert.deepEqual(settings["maplibre-gl-swipe"], { position: 50 }); | ||
| // An unknown plugin's blob is free-form and can hold a key, so it still goes. | ||
| assert.ok(!("external" in settings)); | ||
| assert.ok(redactedPaths.includes("plugins.settings")); | ||
| }); |
Contributor
There was a problem hiding this comment.
This test exercises legend and maplibre-gl-swipe (the null allow-all case) but never exercises the colorbar sub-key, which is the other entry in PUBLISHABLE_PLUGIN_SETTINGS["maplibre-gl-components"] (credentials.ts:53). A regression that drops/renames colorbar in the allow-list wouldn't be caught by this suite — the exact class of silent-export-breakage bug this PR fixes for legend. Worth adding a colorbar assertion alongside the existing legend one here. Confidence: medium.
Contributor
Code reviewBugs
Security
Performance
Quality
CLAUDE.md
|
4 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.
Adds
geolibre-mcp, a headless stdio MCP server that authors real.geolibre.jsonprojects from an AI client. No browser, no running app, no bundled web build. It composes projects with the same builders the Python package already uses, so anything it writes opens unchanged in the desktop app, the web app, and the Jupyter widget.Layering
The point of the structure is that nothing gets duplicated:
project.pykeeps building pieces (a layer, a plugin-state blob).authoring.pyapplies them to a whole project: add/remove/reorder and restyle layers, classify a choropleth, move the camera, compose the legend/colorbar/swipe controls, summarize a project back.Mapnow delegates its split-map, legend, colorbar, and choropleth composition toauthoring.pyrather than holding its own copy, so the widget and the MCP server cannot drift apart.to_htmlsplits into a module-levelrender_project_html()the server can call without a widget;Map.to_htmldelegates to it.mcp/server.pyis the only module that imports the SDK, behind the optionalgeolibre[mcp]extra, so the rest of the package is unaffected.Tools (21)
Lifecycle:
create_project,describe_project,list_catalog.Layers:
add_geojson_layer,add_vector_layer,add_raster_layer,add_tile_layer,add_tiles_layer,add_ogc_layer,add_3d_tiles_layer.Editing:
update_layer,remove_layer,style_layer,classify_layer,list_layer_properties.Framing:
set_view,set_basemap,add_legend,add_colorbar,add_swipe.Export:
export_html.Layers are addressed by id or display name, so a client can work from what
describe_projectshowed it.describe_projectreports inlined GeoJSON as a feature count and never echoes it back.Confinement
mcp/workspace.pyresolves every path against roots given via--root(repeatable) orGEOLIBRE_MCP_ROOTS, mirroring the sidecar'sGEOLIBRE_CONVERSION_ROOTS. Paths outside them are refused, as is a symlink inside a root pointing out of it. Writes are limited to.jsonand.html, and replacing an existing file needs an explicitoverwrite. Remote GeoJSON fetches reuse the existing SSRF guard (every redirect hop checked, 50 MB cap).Verification
Beyond the unit tests, a real client spawned the server as a subprocess over stdio, initialized, listed the 21 tools, and built a choropleth of 52 US states fetched from a live URL plus COG, XYZ, legend, colorbar, and swipe layers. The resulting project round-trips through the app's own
parseProjectwith its graduated symbology (5 stops) and plugin state intact:A workspace escape is refused over the wire as an
isErrorresult.Notes
set_view(bbox=...)is approximate by construction: a saved project stores center/zoom and the app applies those verbatim on load rather than fitting a stored bbox, so the server resolves the box to a camera itself against an assumed pane size. Documented indocs/mcp.md; passcenter/zoomfor exact framing.tests/test_mcp_server.pyskips itself without the SDK, sopublish-python.ymlinstallsmcpexplicitly. Dropping that line would ship the server untested (green but hollow).Testing
python -m pytest python/tests— 248 passed, 3 skipped, on 3.11 and 3.12 (the SDK needs >= 3.10, under the repo's 3.11 floor).pre-commit run --files <changed>— clean.Summary by CodeRabbit
New Features
Bug Fixes
Documentation
Tests