feat(plugins): add geometry plugin API - #1
Closed
Travis-Gilbert wants to merge 1 commit into
Closed
Conversation
📝 WalkthroughWalkthroughThis PR adds a generalized plugin system with extensible backend registration and implements a complete geometry-based spatial indexing feature across all service layers, enabling designation and querying of geometry properties via point/WKB/WKT encodings with optional S2-backed spatial indexing. ChangesGeometry Spatial Plugin Implementation
🎯 4 (Complex) | ⏱️ ~60 minutes
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 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 |
|
Analysis CompleteGenerated ECC bundle from 1 commits | Confidence: 50% View Pull Request #2Repository Profile
Changed Files (16)
Top hotspots
Top directories
Analysis Depth Readiness (commit-history, 7%)ECC Tools uses this to decide whether recommendations should stay at commit-history/setup guidance or expand into CI, security, harness, reference-set, AI-routing, and team backlog work.
Reference Set Readiness (0/7, 0%)
Likely Future Issues (1)
Suggested Follow-up Work (1)
Copy-ready bodies test: add regression coverage for crates/rustyred-core/src/executor.rs + crates/rustyred-core/src/fulltext.rs ## Summary
- Add regression coverage for the recently touched code paths before more changes stack on top.
## Why
- Backfill regression coverage before another change set lands on the touched code paths.
## Touched paths
- `crates/rustyred-core/src/executor.rs`
- `crates/rustyred-core/src/fulltext.rs`
## Validation
- Add or extend focused tests that exercise the touched paths.
- Run the affected test suite and verify the new coverage closes the gap.Generated Instincts (16)
After merging, import with: Files
|
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: cf33004146
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Comment on lines
+395
to
+396
| if !node.labels.iter().any(|node_label| node_label == label) { | ||
| continue; |
There was a problem hiding this comment.
When an already-indexed node is upserted with the same id but without this designated label, this branch just skips the index instead of removing the old entry. Since the graph store replaces the node record on upsert, the geometry index keeps the previous geometry/cells and contains/intersects/within can still return that node id even though it no longer matches the designation; remove the node from the index when it no longer qualifies.
Useful? React with 👍 / 👎.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 8
🧹 Nitpick comments (4)
crates/rustyred-core/src/executor.rs (1)
526-532: 💤 Low valuePlugin dispatch logic is duplicated between executors.
Both
InMemoryRustyredExecutor::execute_requestandStoreBackedRustyredExecutor::execute_requestcontain nearly identical plugin lookup and dispatch code. Consider extracting a helper function to reduce duplication.That said, the logic is correct and the duplication is localized. This can be addressed in a follow-up refactor.
Also applies to: 561-571
🤖 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 `@crates/rustyred-core/src/executor.rs` around lines 526 - 532, Duplicate plugin dispatch logic exists in InMemoryRustyredExecutor::execute_request and StoreBackedRustyredExecutor::execute_request: extract the shared lookup/dispatch into a single helper (e.g., a free function or impl method) that takes &self, command_name &str and request.args, calls crate::plugin::builtin_plugin_registry().operation(&command_name), constructs crate::plugin::PluginOperationContext { command: operation.command, state_hash: self.state_hash() } and invokes (operation.handler)(context, args); replace the duplicated blocks in both execute_request implementations with calls to this helper to remove duplication while preserving behavior.crates/rustyred-core/src/geometry.rs (3)
167-216: 💤 Low valueConsider validating coordinate ranges during encoding/decoding.
PointEncodervalidates finiteness (lines 186-194) but does not enforce thatlat ∈ [-90, 90]orlon ∈ [-180, 180]. Range validation only occurs in S2 functions (lines 701-713), meaning invalid coordinates can be stored but will fail silently during spatial indexing or querying.If this is intentional to support non-Earth coordinate systems, consider documenting the behavior. Otherwise, adding optional range validation during encode/decode would prevent silent indexing failures.
41-41: 💤 Low valueDocument valid resolution range.
The
resolutionfield isu8(0-255), butlevel_from_resolution(lines 651-653) clamps the computed S2 level to 1-30. Resolutions above 15 effectively map to the maximum level 30, making higher values redundant. Consider documenting the effective range (e.g., 0-15) to guide users.Also applies to: 651-653
🤖 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 `@crates/rustyred-core/src/geometry.rs` at line 41, The resolution field (pub resolution: u8) accepts 0–255 but values above ~15 are redundant because level_from_resolution clamps the computed S2 level to 1–30; update the doc comment for the resolution field to state the effective/expected range (e.g., 0–15) and explicitly describe how values map to S2 levels (including that values >15 will map to the maximum level via level_from_resolution), and add a cross-reference to the level_from_resolution function name so callers understand the clamping behavior.
29-29: 💤 Low valueClarify purpose of
GeometryEncoding::Subgraph.The
Subgraphvariant (line 29) exists but has no encoder implementation. The error message at lines 316-319 states it is "declared for consumers," but the intended use case is not documented. Consider adding a doc comment explaining whenSubgraphshould be used and how consumers are expected to handle it.Also applies to: 316-319
🤖 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 `@crates/rustyred-core/src/geometry.rs` at line 29, Add a doc comment to the GeometryEncoding::Subgraph enum variant explaining its intended purpose and how consumers should handle it (e.g., that it is a marker used when encoding is delegated to external consumers and therefore has no internal encoder implementation); also update the error text emitted around the panic in the encoder path (referenced at the code that panics when encountering Subgraph at runtime) to reference the doc guidance so consumers know they must implement handling for Subgraph encodings. Ensure the documentation mentions expected consumer responsibilities and any API contract (e.g., "no encoder provided, consumers must supply encoding/decoding for Subgraph").
🤖 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 `@crates/rustyred-core/src/plugin.rs`:
- Around line 201-203: The function builtin_plugin_registry currently constructs
a new PluginRegistry via PluginRegistry::with_builtin_plugins on every call;
change it to initialize a single static registry (e.g., using
std::sync::LazyLock or once_cell::sync::Lazy) and return a &'static
PluginRegistry (or a reference to the static) instead of allocating each time,
or alternatively keep returning an owned PluginRegistry but move the static
clone from the prebuilt registry; update callers that assume ownership
accordingly (look for calls to builtin_plugin_registry,
PluginRegistry::with_builtin_plugins, and places in executor.rs that call it) so
hot-path allocations and repeated BTreeMap/Arc population are eliminated.
In `@crates/rustyred-mcp/src/lib.rs`:
- Around line 2432-2446: The MCP schema for
"rustyred.spatial.designate_geometry" incorrectly advertises "subgraph" as a
valid encoding while the server rejects GeometryEncoding::Subgraph; remove
"subgraph" from the encoding enum in the JSON schema (the encoding field in the
tool_write call) and update the corresponding MCP parser that currently accepts
or maps "subgraph" to ensure it no longer recognizes or documents that value
(search for handlers referencing GeometryEncoding::Subgraph or parsing
"subgraph" in the MCP parser and remove that branch), keeping the default "wkb"
unchanged so clients only see supported encodings.
- Around line 881-953: The geometry tools are currently exposed unconditionally
in tool_definitions() and call_tool() while ProductMcpBackend only implements
geometry methods under the geometry feature; wrap the geometry tool
registrations and the corresponding match arms in call_tool() (the arms handling
"rustyred.spatial.designate_geometry", "rustyred.spatial.contains",
"rustyred.spatial.intersects", "rustyred.spatial.within" and their
"rustyred.graph.*" aliases) with #[cfg(feature = "geometry")], and mirror the
same gating wherever tool_definitions() lists these geometry tools (also update
the other ranges mentioned), so the tools are omitted from tools/list and calls
fall back only when the feature is enabled and ProductMcpBackend implements
them.
In `@crates/rustyred-server/src/grpc/service.rs`:
- Around line 388-392: The code reads a user-provided "resolution" into a u8 but
currently clamps it to u8::MAX, allowing values outside S2's supported range;
update the parse/validation for the `resolution` variable (the let resolution =
... assignment) to enforce S2 bounds by validating against the expected
resolution range (0..=30) before casting, returning an error or defaulting to a
safe value when out of range; reference the S2 mapping helpers
(`level_from_resolution` and `MAX_S2_LEVEL` in geometry.rs) to determine the
valid upper bound and ensure you use that bound (not u8::MAX) when clamping or
validating.
In `@crates/rustyred-server/src/router.rs`:
- Around line 2650-2670: Batch and root_batch handlers bypass the post-write
indexing used in the /command path, so GraphNodeUpsert in batched commands
doesn't trigger maybe_index_node_spatially/fulltext/geometry. Modify the
implementation so execute_graph_store_command (or a small helper it returns)
also reports back mutated Node records for GraphNodeUpsert (e.g., return a
list/optional Node records parsed via
serde_json::from_value::<NodeWriteBody>(...) and NodeWriteBody::into_record),
and update the batch and root_batch call sites that invoke
execute_graph_store_command to inspect that return and run the same post-write
hook (state.observability.record_mutation(); state.maybe_index_node_spatially/
maybe_index_node_fulltext/ maybe_index_node_geometry) for each returned node;
alternatively, centralize the node_for_hooks logic into
execute_graph_store_command so all callers (including batch/root_batch) get the
same indexing behavior.
In `@crates/rustyred-server/src/state.rs`:
- Around line 375-382: The geometry index code inserts and looks up entries
under the raw tenant_id, causing alias mismatches; update all places interacting
with geometry_indexes (e.g., the insertion code using geometry_indexes.lock()
and any lookup code at the other noted ranges) to canonicalize the tenant key by
running the tenant_id through sanitize_tenant_segment(...) (or the same helper
the graph store uses) before calling .entry(...) or .get(...)/.remove(...);
ensure you replace tenant_id.to_string() with the sanitized value consistently
for insertions and lookups so both graph store and geometry indexes use the same
canonical tenant key.
- Around line 394-401: The loop over tenant_map currently skips when a node no
longer has a label, leaving stale geometry entries; update the branch inside for
((label, _property), index) in tenant_map.iter_mut() so that when
!node.labels.iter().any(|node_label| node_label == label) you call
index.remove(&node.id) to delete the old entry (instead of continue), and keep
the existing upsert_from_properties handling (match
index.upsert_from_properties(&node.id, &node.properties) { Ok(()) => {}, Err(_)
=> index.remove(&node.id), }) so removals happen both when the label is absent
and when upsert fails.
---
Nitpick comments:
In `@crates/rustyred-core/src/executor.rs`:
- Around line 526-532: Duplicate plugin dispatch logic exists in
InMemoryRustyredExecutor::execute_request and
StoreBackedRustyredExecutor::execute_request: extract the shared lookup/dispatch
into a single helper (e.g., a free function or impl method) that takes &self,
command_name &str and request.args, calls
crate::plugin::builtin_plugin_registry().operation(&command_name), constructs
crate::plugin::PluginOperationContext { command: operation.command, state_hash:
self.state_hash() } and invokes (operation.handler)(context, args); replace the
duplicated blocks in both execute_request implementations with calls to this
helper to remove duplication while preserving behavior.
In `@crates/rustyred-core/src/geometry.rs`:
- Line 41: The resolution field (pub resolution: u8) accepts 0–255 but values
above ~15 are redundant because level_from_resolution clamps the computed S2
level to 1–30; update the doc comment for the resolution field to state the
effective/expected range (e.g., 0–15) and explicitly describe how values map to
S2 levels (including that values >15 will map to the maximum level via
level_from_resolution), and add a cross-reference to the level_from_resolution
function name so callers understand the clamping behavior.
- Line 29: Add a doc comment to the GeometryEncoding::Subgraph enum variant
explaining its intended purpose and how consumers should handle it (e.g., that
it is a marker used when encoding is delegated to external consumers and
therefore has no internal encoder implementation); also update the error text
emitted around the panic in the encoder path (referenced at the code that panics
when encountering Subgraph at runtime) to reference the doc guidance so
consumers know they must implement handling for Subgraph encodings. Ensure the
documentation mentions expected consumer responsibilities and any API contract
(e.g., "no encoder provided, consumers must supply encoding/decoding for
Subgraph").
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 8e7dfb51-1b2d-462b-a1af-6bf910734bc6
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (15)
crates/rustyred-core/Cargo.tomlcrates/rustyred-core/src/executor.rscrates/rustyred-core/src/fulltext.rscrates/rustyred-core/src/geometry.rscrates/rustyred-core/src/lib.rscrates/rustyred-core/src/plugin.rscrates/rustyred-core/src/spatial.rscrates/rustyred-mcp/Cargo.tomlcrates/rustyred-mcp/src/lib.rscrates/rustyred-server/Cargo.tomlcrates/rustyred-server/src/grpc/service.rscrates/rustyred-server/src/openapi.rscrates/rustyred-server/src/router.rscrates/rustyred-server/src/state.rsvendor/proto/rustyred/v1/rustyred.proto
Comment on lines
+201
to
+203
| pub fn builtin_plugin_registry() -> PluginRegistry { | ||
| PluginRegistry::with_builtin_plugins() | ||
| } |
There was a problem hiding this comment.
Recreating the plugin registry on every call is inefficient.
builtin_plugin_registry() allocates a new PluginRegistry, populates three BTreeMaps, wraps plugins in Arc, and iterates through all registrations—every time it's called. The executor calls this on every request (see executor.rs lines 526 and 561), and backend factories call it on each designation. This adds measurable overhead on hot paths.
Consider using std::sync::LazyLock (stable since Rust 1.80) or once_cell::sync::Lazy to initialize the registry once:
♻️ Proposed fix using LazyLock
+use std::sync::LazyLock;
+
+static BUILTIN_REGISTRY: LazyLock<PluginRegistry> = LazyLock::new(PluginRegistry::with_builtin_plugins);
+
pub fn builtin_plugin_registry() -> PluginRegistry {
- PluginRegistry::with_builtin_plugins()
+ BUILTIN_REGISTRY.clone()
}Alternatively, return &'static PluginRegistry if callers don't need ownership:
pub fn builtin_plugin_registry() -> &'static PluginRegistry {
&BUILTIN_REGISTRY
}🤖 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 `@crates/rustyred-core/src/plugin.rs` around lines 201 - 203, The function
builtin_plugin_registry currently constructs a new PluginRegistry via
PluginRegistry::with_builtin_plugins on every call; change it to initialize a
single static registry (e.g., using std::sync::LazyLock or
once_cell::sync::Lazy) and return a &'static PluginRegistry (or a reference to
the static) instead of allocating each time, or alternatively keep returning an
owned PluginRegistry but move the static clone from the prebuilt registry;
update callers that assume ownership accordingly (look for calls to
builtin_plugin_registry, PluginRegistry::with_builtin_plugins, and places in
executor.rs that call it) so hot-path allocations and repeated BTreeMap/Arc
population are eliminated.
Comment on lines
+881
to
+953
| "rustyred.spatial.designate_geometry" | "rustyred.graph.spatial.designate_geometry" => { | ||
| if let Some(error) = require_write_tool(config, context, name) { | ||
| return Ok(error); | ||
| } | ||
| let label = required_str(&arguments, "label", name)?; | ||
| let property = required_str(&arguments, "property", name)?; | ||
| let encoding = arguments | ||
| .get("encoding") | ||
| .and_then(Value::as_str) | ||
| .unwrap_or("wkb"); | ||
| let resolution = arguments | ||
| .get("resolution") | ||
| .and_then(Value::as_u64) | ||
| .unwrap_or(9) | ||
| .min(u8::MAX as u64) as u8; | ||
| backend.designate_geometry_property(label, property, encoding, resolution)?; | ||
| json!({ | ||
| "tenant": tenant, | ||
| "designated": { | ||
| "label": label, | ||
| "property": property, | ||
| "encoding": encoding, | ||
| "resolution": resolution | ||
| } | ||
| }) | ||
| } | ||
| "rustyred.spatial.contains" | "rustyred.graph.spatial.contains" => { | ||
| let label = required_str(&arguments, "label", name)?; | ||
| let property = required_str(&arguments, "property", name)?; | ||
| let lat = required_f64(&arguments, "lat", name)?; | ||
| let lon = required_f64(&arguments, "lon", name)?; | ||
| let node_ids = backend.spatial_contains_point(label, property, lat, lon)?; | ||
| json!({ | ||
| "tenant": tenant, | ||
| "node_ids": node_ids, | ||
| "stats": { "returned": node_ids.len() } | ||
| }) | ||
| } | ||
| "rustyred.spatial.intersects" | "rustyred.graph.spatial.intersects" => { | ||
| let label = required_str(&arguments, "label", name)?; | ||
| let property = required_str(&arguments, "property", name)?; | ||
| let encoding = arguments | ||
| .get("encoding") | ||
| .and_then(Value::as_str) | ||
| .unwrap_or("wkt"); | ||
| let geometry = arguments.get("geometry").ok_or_else(|| { | ||
| McpError::invalid_params("rustyred.spatial.intersects requires geometry") | ||
| })?; | ||
| let node_ids = | ||
| backend.spatial_intersects_geometry(label, property, encoding, geometry)?; | ||
| json!({ | ||
| "tenant": tenant, | ||
| "node_ids": node_ids, | ||
| "stats": { "returned": node_ids.len() } | ||
| }) | ||
| } | ||
| "rustyred.spatial.within" | "rustyred.graph.spatial.within" => { | ||
| let label = required_str(&arguments, "label", name)?; | ||
| let property = required_str(&arguments, "property", name)?; | ||
| let encoding = arguments | ||
| .get("encoding") | ||
| .and_then(Value::as_str) | ||
| .unwrap_or("wkt"); | ||
| let geometry = arguments.get("geometry").ok_or_else(|| { | ||
| McpError::invalid_params("rustyred.spatial.within requires geometry") | ||
| })?; | ||
| let node_ids = backend.spatial_within_geometry(label, property, encoding, geometry)?; | ||
| json!({ | ||
| "tenant": tenant, | ||
| "node_ids": node_ids, | ||
| "stats": { "returned": node_ids.len() } | ||
| }) | ||
| } |
There was a problem hiding this comment.
Gate these geometry tools behind the geometry feature.
tool_definitions() and call_tool() expose the geometry operations unconditionally, but ProductMcpBackend only overrides the geometry methods when crates/rustyred-server/src/state.rs is built with #[cfg(feature = "geometry")]. In non-geometry builds, tools/list advertises working tools that immediately fall back to unsupported_operation, which breaks the feature-disabled contract.
Also applies to: 2305-2446
🤖 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 `@crates/rustyred-mcp/src/lib.rs` around lines 881 - 953, The geometry tools
are currently exposed unconditionally in tool_definitions() and call_tool()
while ProductMcpBackend only implements geometry methods under the geometry
feature; wrap the geometry tool registrations and the corresponding match arms
in call_tool() (the arms handling "rustyred.spatial.designate_geometry",
"rustyred.spatial.contains", "rustyred.spatial.intersects",
"rustyred.spatial.within" and their "rustyred.graph.*" aliases) with
#[cfg(feature = "geometry")], and mirror the same gating wherever
tool_definitions() lists these geometry tools (also update the other ranges
mentioned), so the tools are omitted from tools/list and calls fall back only
when the feature is enabled and ProductMcpBackend implements them.
Comment on lines
+2432
to
+2446
| tools.push(tool_write( | ||
| "rustyred.spatial.designate_geometry", | ||
| "Designate a node geometry property for geometry predicate search.", | ||
| json!({ | ||
| "type": "object", | ||
| "properties": { | ||
| "tenant": { "type": "string" }, | ||
| "label": { "type": "string" }, | ||
| "property": { "type": "string" }, | ||
| "encoding": { "type": "string", "enum": ["point", "wkb", "wkt", "subgraph"], "default": "wkb" }, | ||
| "resolution": { "type": "integer", "default": 9 } | ||
| }, | ||
| "required": ["label", "property"] | ||
| }), | ||
| )); |
There was a problem hiding this comment.
Do not advertise subgraph as a valid MCP geometry encoding.
The schema accepts "subgraph", but the core geometry layer explicitly rejects GeometryEncoding::Subgraph as an unsupported scalar encoding. That leaves clients with a documented input that the backend cannot honor. Drop it from the MCP enum and the server-side MCP parser so the contract stays honest.
🤖 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 `@crates/rustyred-mcp/src/lib.rs` around lines 2432 - 2446, The MCP schema for
"rustyred.spatial.designate_geometry" incorrectly advertises "subgraph" as a
valid encoding while the server rejects GeometryEncoding::Subgraph; remove
"subgraph" from the encoding enum in the JSON schema (the encoding field in the
tool_write call) and update the corresponding MCP parser that currently accepts
or maps "subgraph" to ensure it no longer recognizes or documents that value
(search for handlers referencing GeometryEncoding::Subgraph or parsing
"subgraph" in the MCP parser and remove that branch), keeping the default "wkb"
unchanged so clients only see supported encodings.
Comment on lines
+388
to
+392
| let resolution = args | ||
| .get("resolution") | ||
| .and_then(Value::as_u64) | ||
| .unwrap_or(9) | ||
| .min(u8::MAX as u64) as u8; |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check if GeometryIndex or designate_geometry_property validates resolution bounds
rg -nC3 'resolution' --type rust -g '*geometry*'Repository: Travis-Gilbert/RustyRed-Graph-Database
Length of output: 6459
Clamp/validate resolution against S2’s supported range (0–30) instead of allowing up to 255
crates/rustyred-server/src/grpc/service.rs currently clamps resolution to u8::MAX (255). In crates/rustyred-core/src/geometry.rs, the S2 path maps this via level_from_resolution(resolution) using resolution.saturating_mul(2).clamp(1, MAX_S2_LEVEL), so out-of-range values are silently coerced to the maximum level—changing coverage/granularity unexpectedly.
🛠️ Proposed fix to validate S2 resolution bounds
let resolution = args
.get("resolution")
.and_then(Value::as_u64)
.unwrap_or(9)
- .min(u8::MAX as u64) as u8;
+ .min(30) as u8;📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| let resolution = args | |
| .get("resolution") | |
| .and_then(Value::as_u64) | |
| .unwrap_or(9) | |
| .min(u8::MAX as u64) as u8; | |
| let resolution = args | |
| .get("resolution") | |
| .and_then(Value::as_u64) | |
| .unwrap_or(9) | |
| .min(30) as u8; |
🤖 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 `@crates/rustyred-server/src/grpc/service.rs` around lines 388 - 392, The code
reads a user-provided "resolution" into a u8 but currently clamps it to u8::MAX,
allowing values outside S2's supported range; update the parse/validation for
the `resolution` variable (the let resolution = ... assignment) to enforce S2
bounds by validating against the expected resolution range (0..=30) before
casting, returning an error or defaulting to a safe value when out of range;
reference the S2 mapping helpers (`level_from_resolution` and `MAX_S2_LEVEL` in
geometry.rs) to determine the valid upper bound and ensure you use that bound
(not u8::MAX) when clamping or validating.
Comment on lines
+1472
to
+1563
| "/v1/tenants/{tenant_id}/graph/spatial/designate_geometry": { | ||
| "post": { | ||
| "tags": ["graph"], | ||
| "summary": "Designate a geometry index", | ||
| "description": "Registers a node label and property holding geometry (point, WKB, or WKT) for the S2-cover geometry index. Served by the spatial plugin-operation route. Upserts of matching nodes refresh the cover.", | ||
| "parameters": [tenant_parameter.clone()], | ||
| "requestBody": { | ||
| "required": true, | ||
| "content": { | ||
| "application/json": { | ||
| "schema": { "$ref": "#/components/schemas/SpatialGeometryDesignateRequest" } | ||
| } | ||
| } | ||
| }, | ||
| "responses": { | ||
| "200": { "$ref": "#/components/responses/GeometryDesignationResponse" }, | ||
| "400": { "$ref": "#/components/responses/GraphStoreError" }, | ||
| "401": { "$ref": "#/components/responses/Unauthorized" }, | ||
| "403": { "$ref": "#/components/responses/Forbidden" }, | ||
| "503": { "$ref": "#/components/responses/StoreUnavailable" } | ||
| } | ||
| } | ||
| }, | ||
| "/v1/tenants/{tenant_id}/graph/spatial/within": { | ||
| "post": { | ||
| "tags": ["graph"], | ||
| "summary": "Run a geometry within query", | ||
| "description": "Returns node ids whose designated geometry lies within the query geometry. Two-phase: S2 cover broad phase, then an exact geo predicate narrow phase.", | ||
| "parameters": [tenant_parameter.clone()], | ||
| "requestBody": { | ||
| "required": true, | ||
| "content": { | ||
| "application/json": { | ||
| "schema": { "$ref": "#/components/schemas/SpatialGeometryQueryRequest" } | ||
| } | ||
| } | ||
| }, | ||
| "responses": { | ||
| "200": { "$ref": "#/components/responses/GeometryIdsResponse" }, | ||
| "400": { "$ref": "#/components/responses/GraphStoreError" }, | ||
| "401": { "$ref": "#/components/responses/Unauthorized" }, | ||
| "403": { "$ref": "#/components/responses/Forbidden" }, | ||
| "503": { "$ref": "#/components/responses/StoreUnavailable" } | ||
| } | ||
| } | ||
| }, | ||
| "/v1/tenants/{tenant_id}/graph/spatial/intersects": { | ||
| "post": { | ||
| "tags": ["graph"], | ||
| "summary": "Run a geometry intersects query", | ||
| "description": "Returns node ids whose designated geometry intersects the query geometry. Two-phase: S2 cover broad phase, then an exact geo predicate narrow phase.", | ||
| "parameters": [tenant_parameter.clone()], | ||
| "requestBody": { | ||
| "required": true, | ||
| "content": { | ||
| "application/json": { | ||
| "schema": { "$ref": "#/components/schemas/SpatialGeometryQueryRequest" } | ||
| } | ||
| } | ||
| }, | ||
| "responses": { | ||
| "200": { "$ref": "#/components/responses/GeometryIdsResponse" }, | ||
| "400": { "$ref": "#/components/responses/GraphStoreError" }, | ||
| "401": { "$ref": "#/components/responses/Unauthorized" }, | ||
| "403": { "$ref": "#/components/responses/Forbidden" }, | ||
| "503": { "$ref": "#/components/responses/StoreUnavailable" } | ||
| } | ||
| } | ||
| }, | ||
| "/v1/tenants/{tenant_id}/graph/spatial/contains": { | ||
| "post": { | ||
| "tags": ["graph"], | ||
| "summary": "Run a geometry contains-point query", | ||
| "description": "Returns node ids whose designated polygon or multipolygon geometry contains the given lat/lon point. Two-phase: S2 cell lookup broad phase, then an exact geo contains narrow phase.", | ||
| "parameters": [tenant_parameter.clone()], | ||
| "requestBody": { | ||
| "required": true, | ||
| "content": { | ||
| "application/json": { | ||
| "schema": { "$ref": "#/components/schemas/SpatialContainsRequest" } | ||
| } | ||
| } | ||
| }, | ||
| "responses": { | ||
| "200": { "$ref": "#/components/responses/GeometryIdsResponse" }, | ||
| "400": { "$ref": "#/components/responses/GraphStoreError" }, | ||
| "401": { "$ref": "#/components/responses/Unauthorized" }, | ||
| "403": { "$ref": "#/components/responses/Forbidden" }, | ||
| "503": { "$ref": "#/components/responses/StoreUnavailable" } | ||
| } | ||
| } | ||
| }, |
There was a problem hiding this comment.
Gate geometry OpenAPI paths/schemas by feature to avoid false API contracts.
Line 1472 through Line 2901 and Line 3633 through Line 3639 add geometry routes/schemas/assertions unconditionally, but geometry is described as optional/feature-gated. This can publish endpoints in /openapi.json that do not exist in non-geometry builds.
As per coding guidelines, this conflicts with the PR objective that geometry support is optional and feature-gated across crates.
Suggested direction
+let geometry_enabled = cfg!(feature = "geometry");
+
+// Build `paths` and `components.schemas/responses` as mutable JSON objects,
+// then conditionally insert geometry entries only when geometry_enabled.
+if geometry_enabled {
+ // insert /graph/spatial/designate_geometry, /within, /intersects, /contains
+ // insert SpatialGeometryDesignateRequest, SpatialContainsRequest,
+ // SpatialGeometryQueryRequest, GeometryIdsResponseBody,
+ // GeometryDesignationResponseBody, GeometryIdsResponse, GeometryDesignationResponse
+}Also applies to: 1932-1947, 2836-2901, 3633-3639
Comment on lines
+2650
to
+2670
| let node_for_hooks = matches!( | ||
| RustyredCommand::from_name(command_name), | ||
| Ok(RustyredCommand::GraphNodeUpsert) | ||
| ) | ||
| .then(|| { | ||
| serde_json::from_value::<NodeWriteBody>(args.clone()) | ||
| .ok() | ||
| .map(NodeWriteBody::into_record) | ||
| }) | ||
| .flatten(); | ||
| let response = execute_graph_store_command(&mut store, command_name, args); | ||
| if response.ok { | ||
| if let Some(node) = node_for_hooks { | ||
| state.observability.record_mutation(); | ||
| state.maybe_index_node_spatially(tenant_id, &node); | ||
| state.maybe_index_node_fulltext(tenant_id, &node); | ||
| #[cfg(feature = "geometry")] | ||
| state.maybe_index_node_geometry(tenant_id, &node); | ||
| } | ||
| } | ||
| response |
There was a problem hiding this comment.
Reuse this post-write indexing hook for batch graph commands.
This hook only fixes the /command path. batch and root_batch still send RUSTYRED.GRAPH.NODE.UPSERT through execute_graph_store_command directly (Lines 3171-3190), so batched node upserts leave the spatial/fulltext/geometry indexes stale until a rebuild.
🤖 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 `@crates/rustyred-server/src/router.rs` around lines 2650 - 2670, Batch and
root_batch handlers bypass the post-write indexing used in the /command path, so
GraphNodeUpsert in batched commands doesn't trigger
maybe_index_node_spatially/fulltext/geometry. Modify the implementation so
execute_graph_store_command (or a small helper it returns) also reports back
mutated Node records for GraphNodeUpsert (e.g., return a list/optional Node
records parsed via serde_json::from_value::<NodeWriteBody>(...) and
NodeWriteBody::into_record), and update the batch and root_batch call sites that
invoke execute_graph_store_command to inspect that return and run the same
post-write hook (state.observability.record_mutation();
state.maybe_index_node_spatially/ maybe_index_node_fulltext/
maybe_index_node_geometry) for each returned node; alternatively, centralize the
node_for_hooks logic into execute_graph_store_command so all callers (including
batch/root_batch) get the same indexing behavior.
Comment on lines
+375
to
+382
| let mut indexes = self | ||
| .geometry_indexes | ||
| .lock() | ||
| .map_err(|_| StoreAccessError::internal("geometry index lock poisoned"))?; | ||
| indexes | ||
| .entry(tenant_id.to_string()) | ||
| .or_default() | ||
| .insert((label.to_string(), property.to_string()), index); |
There was a problem hiding this comment.
Canonicalize geometry-index tenant keys the same way as the graph store.
The RedCore store/cache paths normalize tenants with sanitize_tenant_segment(...), but geometry indexes are inserted and looked up under the raw tenant_id. That lets two equivalent tenant spellings hit the same graph store while reading/writing different geometry-index buckets, so a designation done through one alias is invisible to queries or upserts done through the other.
Also applies to: 388-392, 1133-1144
🤖 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 `@crates/rustyred-server/src/state.rs` around lines 375 - 382, The geometry
index code inserts and looks up entries under the raw tenant_id, causing alias
mismatches; update all places interacting with geometry_indexes (e.g., the
insertion code using geometry_indexes.lock() and any lookup code at the other
noted ranges) to canonicalize the tenant key by running the tenant_id through
sanitize_tenant_segment(...) (or the same helper the graph store uses) before
calling .entry(...) or .get(...)/.remove(...); ensure you replace
tenant_id.to_string() with the sanitized value consistently for insertions and
lookups so both graph store and geometry indexes use the same canonical tenant
key.
Comment on lines
+394
to
+401
| for ((label, _property), index) in tenant_map.iter_mut() { | ||
| if !node.labels.iter().any(|node_label| node_label == label) { | ||
| continue; | ||
| } | ||
| match index.upsert_from_properties(&node.id, &node.properties) { | ||
| Ok(()) => {} | ||
| Err(_) => index.remove(&node.id), | ||
| } |
There was a problem hiding this comment.
Remove stale geometry entries when a node drops the designated label.
When an upsert removes label from a node, this branch just skips the index instead of deleting the old entry. The geometry index can then keep returning a node that no longer matches the designation.
Suggested fix
for ((label, _property), index) in tenant_map.iter_mut() {
if !node.labels.iter().any(|node_label| node_label == label) {
+ index.remove(&node.id);
continue;
}
match index.upsert_from_properties(&node.id, &node.properties) {
Ok(()) => {}
Err(_) => index.remove(&node.id),📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| for ((label, _property), index) in tenant_map.iter_mut() { | |
| if !node.labels.iter().any(|node_label| node_label == label) { | |
| continue; | |
| } | |
| match index.upsert_from_properties(&node.id, &node.properties) { | |
| Ok(()) => {} | |
| Err(_) => index.remove(&node.id), | |
| } | |
| for ((label, _property), index) in tenant_map.iter_mut() { | |
| if !node.labels.iter().any(|node_label| node_label == label) { | |
| index.remove(&node.id); | |
| continue; | |
| } | |
| match index.upsert_from_properties(&node.id, &node.properties) { | |
| Ok(()) => {} | |
| Err(_) => index.remove(&node.id), | |
| } |
🤖 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 `@crates/rustyred-server/src/state.rs` around lines 394 - 401, The loop over
tenant_map currently skips when a node no longer has a label, leaving stale
geometry entries; update the branch inside for ((label, _property), index) in
tenant_map.iter_mut() so that when !node.labels.iter().any(|node_label|
node_label == label) you call index.remove(&node.id) to delete the old entry
(instead of continue), and keep the existing upsert_from_properties handling
(match index.upsert_from_properties(&node.id, &node.properties) { Ok(()) => {},
Err(_) => index.remove(&node.id), }) so removals happen both when the label is
absent and when upsert fails.
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.
This PR introduces a new plugin system for RustyRed and wires through a geometry index API. It expands the graph server’s spatial capabilities in line with our MCP- and Theseus-facing product direction.
Changes
This change adds a general-purpose plugin registry to rustyred-core and threads it through the executors, providing a first-class extension point for spatial, full-text, and arbitrary operations. The new plugin.rs module defines RustyRedPlugin, capability metadata, backend registrations, and an operation registry keyed by normalized command names. Core plugins are registered by default (spatial, fulltext, operations), including an example RUSTYRED.PLUGIN.ECHO operation that exercises the pipeline end to end.
On the indexing side, the PR introduces an optional geometry feature in rustyred-core, rustyred-mcp, and rustyred-server, backed by a new geometry.rs module. This module defines GeometryDesignation, GeometryEncoding, and GeometryIndex, which provides S2-based broad-phase cover plus precise geo predicates (contains, intersects, within) for polygon/multipolygon and related geometries. The AppState now owns an in-memory geometry_indexes map per tenant; designate_geometry_property seeds these indexes from existing nodes, and maybe_index_node_geometry keeps them up to date on node upserts and committed graph transactions.
The spatial/fulltext configuration flow is refactored to route through the plugin registry where appropriate. Full-text and spatial backends now first consult builtin_plugin_registry() (in fulltext.rs and spatial.rs), allowing environment-driven backend selection (e.g., Tantivy, S2) to be satisfied by built-in or future plugins. The MCP adapter (rustyred-mcp/src/lib.rs) is extended with geometry-aware methods (designate_geometry_property, spatial_contains_point, spatial_intersects_geometry, spatial_within_geometry), and the tooling surface is updated so MCP tools like rustyred.spatial.contains, .intersects, .within, and .designate_geometry are exposed with appropriate schemas and write guards.
On the gRPC and HTTP surfaces, the server now exposes plugin operations in a more flexible way while keeping geometry concerns separate and feature-gated. The GraphDatabaseService::execute_plugin_operation method delegates any geometry-related operations to execute_geometry_plugin_operation (when the geometry feature is enabled), and otherwise dispatches to the core plugin registry. This gives us a clean path for using ExecutePluginOperation for both geometry topology queries and generic plugins across storage modes (backed stores and in-memory). The OpenAPI spec and router are updated to document and route the new /v1/tenants/{tenant_id}/graph/spatial/designate_geometry, /within, /intersects, and /contains endpoints, including request/response schemas (SpatialGeometryDesignateRequest, SpatialGeometryQueryRequest, SpatialContainsRequest, GeometryIdsResponseBody, and GeometryDesignationResponseBody).
Finally, the rustyred-mcp and rustyred-server crates gain new feature flags (geometry, s2) wired through to rustyred-core, aligning the geometry plugin, S2 backend, and MCP/HTTP/GRPC surfaces under consistent build-time configuration. Existing call paths like node upsert, graph transactions, and graph cache invalidation are extended minimally to ensure geometry indexes stay in sync with the underlying graph store without changing existing behavior for installations that don’t enable the new features.
Testing
There are several new and extended unit/integration tests added in this PR across the core, MCP, and server layers. In rustyred-core, tests exercise plugin registry behavior (registration, capability enumeration, built-in plugin discovery) and ensure that JSON-based executor calls round-trip plugin operations like RUSTYRED.PLUGIN.ECHO. In rustyred-mcp, tests verify that geometry-related tools are correctly surfaced in the tool definitions, including the new rustyred.spatial.contains, .intersects, .within, and .designate_geometry entries, and that the tool set respects the write-vs-read distinction. In rustyred-server, there are OpenAPI tests confirming that the new spatial geometry endpoints and schemas are registered, as well as gRPC tests that: (1) designate a geometry property, (2) upsert a node with WKT polygon geometry, (3) run a contains-point plugin operation via gRPC, and (4) validate both the geometry result and a follow-up registry-backed RUSTYRED.PLUGIN.ECHO call. These tests together cover the plugin registry wiring, geometry index designation and updates, and both MCP and gRPC/HTTP surfaces for the new geometry capabilities.
Suggested manual tests
Smoke test plugin echo over gRPC: Use the ExecutePluginOperation RPC with operation = "RUSTYRED.PLUGIN.ECHO" and a small JSON payload to confirm the plugin registry path works end to end in your dev environment.
End-to-end geometry designation and contains: For a test tenant, upsert a Parcel node with a simple WKT polygon, call /graph/spatial/designate_geometry to register Parcel.geom, then hit /graph/spatial/contains (or the corresponding plugin operation) with a point inside the polygon and verify the expected node id is returned.
Intersects/within variants: Run intersects and within queries against overlapping and non-overlapping geometries to confirm the broad-phase/narrow-phase behavior seems plausible for real-world municipal parcels or boundaries.
Feature-flag sanity check: Start the server with and without the geometry,s2 features enabled and confirm that geometry endpoints and plugin operations either work correctly or fail with the documented “requires building with –features geometry,s2” error.
MCP tool discovery and usage: From a client using the MCP interface, list available tools and confirm the geometry/special spatial tools are visible only when geometry is enabled, then attempt a simple rustyred.spatial.contains call.
Suggested automated tests
Geometry indexing regression suite: Add focused tests around GeometryIndex for a range of encodings (WKT, WKB, point, subgraph) and resolutions, asserting both expected matches and “no match” behavior for boundary conditions and invalid geometries.
Tenant isolation of geometry indexes: Add tests that create two tenants with overlapping label/property names and ensure the geometry indexes remain tenant-scoped (no cross-tenant leakage in contains/intersects/within results).
Performance/scale tests for geometry queries: Introduce benchmarks or load tests that exercise contains/intersects/within for large numbers of polygons to validate S2-cover broad-phase performance and to catch any pathological cases before production-scale data.
MCP geometry error-path tests: Extend MCP tests to cover bad encodings, missing geometry payloads, and out-of-range resolutions to ensure consistent GraphStoreError codes/messages for clients.
Plugin registry extensibility tests: Add tests that register a custom RustyRedPlugin with additional operations and backends, then exercise them via both the in-memory executor and gRPC ExecutePluginOperation to guard against future regressions in plugin registration or normalization rules.
Summary by CodeRabbit
Release Notes
New Features
Refactor