Refactor the RDF action into enterprise/server#1118
Conversation
Signed-off-by: Juan Cruz Viotti <jv@jviotti.com>
|
This pull request is abnormally large and would use a significant amount of tokens to review. If you still wish to review it, comment "augment review" and we will review it. |
jviotti
force-pushed
the
refactor-rdf-enterprise-action
branch
from
ba8142a to
a211451
Compare
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
All reported issues were addressed across 9 files (changes from recent commits).
Tip: Review your code locally with the cubic CLI to iterate faster.
Re-trigger cubic
There was a problem hiding this comment.
6 issues found across 7 files (changes from recent commits).
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="enterprise/server/include/sourcemeta/one/enterprise_server_action_jsonschema_rdf_v1.h">
<violation number="1" location="enterprise/server/include/sourcemeta/one/enterprise_server_action_jsonschema_rdf_v1.h:56">
P2: The `mcpResponseSchema` argument is stored in `rpc_response_schema_` but never consumed. The `mcp()` method returns tool-success payloads (`mcp_make_tool_success`) directly without evaluating them against the configured response schema, so the stored value is dead configuration. This creates an unfulfilled contract: the routing layer passes a response schema, but the action silently skips validation, allowing future payload changes to drift from the published MCP tool contract unnoticed. If response schema enforcement is intended, validate each returned payload before wrapping it in the MCP success result; otherwise, remove the unused member and its constructor handling to avoid claiming a guarantee that is not provided.</violation>
</file>
<file name="enterprise/server/include/sourcemeta/one/enterprise_server_action_mcp_v1.h">
<violation number="1" location="enterprise/server/include/sourcemeta/one/enterprise_server_action_mcp_v1.h:71">
P1: The `allowed_origin_` value loaded from metapack metadata is only validated to be lowercase via `assert()`. Because `assert` is stripped in release builds (NDEBUG), a stale or manually modified metapack carrying a mixed-case origin (e.g., `HTTPS://EXAMPLE.COM`) will silently be accepted. Since every inbound `Origin` header is lowercased before comparison, a mixed-case `allowed_origin_` will cause **all browser requests to be rejected** in production. Consider replacing the `assert` with an eager call to `sourcemeta::core::to_lowercase(this->allowed_origin_)` so the invariant holds regardless of build type or metapack provenance.</violation>
<violation number="2" location="enterprise/server/include/sourcemeta/one/enterprise_server_action_mcp_v1.h:340">
P2: The `resources/list` handler rescans and reauthorizes the entire catalog for every page request. `for_each(0, count())` checks `authentication.admits(...)` on every entry, then counts `admitted` to determine whether the current item falls in the requested page window. For a catalog with N visible entries and P pages, this is O(P*N) total work, making MCP catalog browsing disproportionately expensive for large indexes. Consider whether `SearchView` can expose an authenticated iterator, a pre-filtered count, or a way to terminate `for_each` early so that deep pages do not require re-walking the full catalog from index 0.</violation>
<violation number="3" location="enterprise/server/include/sourcemeta/one/enterprise_server_action_mcp_v1.h:545">
P2: Unhandled tool exceptions are returned verbatim to MCP clients, which can leak internal diagnostic details. The `catch (const std::exception &error)` block in `on_tools_call` forwards `error.what()` directly via `mcp_make_tool_error`, whereas the batch dispatch loop in `on_message` maps unexpected exceptions to a generic internal error. Tool actions may throw filesystem, database, or network exceptions whose messages include internal paths or backend details. Expected input errors should be modeled explicitly, and truly unexpected exceptions should be logged server-side and surfaced to the client as a generic error message instead.</violation>
<violation number="4" location="enterprise/server/include/sourcemeta/one/enterprise_server_action_mcp_v1.h:661">
P2: The `process_one` method checks `mcp_is_request_method` and returns `Method not found` *before* the `schema_evaluate_fast` call that is documented to enforce MCP's stricter null-id rule. This means a null-id request with an unknown method bypasses the null-id validation and receives `-32601` instead of `-32600`. Since the adjacent comment states "Sourcemeta One follows MCP's tighter rule and rejects null-id requests here," the method lookup should probably come after the schema (or a stand-alone null-id) check, or the null-id rule should be validated earlier so it applies uniformly regardless of whether the method is known.</violation>
<violation number="5" location="enterprise/server/include/sourcemeta/one/enterprise_server_action_mcp_v1.h:677">
P2: The MCP dispatch in `process_one` does not guard resource and tool method handlers against the capability flags advertised during initialization. The server reads `.resources` and `.tools` from the metapack metadata in `on_initialize` and includes them in the negotiated capabilities, but `process_one` unconditionally routes to `resources/list`, `resources/read`, `tools/list`, and `tools/call` even when those capabilities are `false`. Per the MCP spec, capabilities established during initialization must be respected for the lifetime of the session. If the metapack ever sets either flag to `false`, the server will advertise the feature as unavailable while still serving it. Consider adding capability checks before dispatching to these handlers, or ensure the metapack generation invariant guarantees these are always `true` for this action class and document it explicitly.</violation>
</file>
Tip: Review your code locally with the cubic CLI to iterate faster.
Re-trigger cubic
| // https://datatracker.ietf.org/doc/html/rfc6454#section-4 | ||
| assert(sourcemeta::core::is_lowercase(this->allowed_origin_)); | ||
| } | ||
|
|
There was a problem hiding this comment.
P1: The allowed_origin_ value loaded from metapack metadata is only validated to be lowercase via assert(). Because assert is stripped in release builds (NDEBUG), a stale or manually modified metapack carrying a mixed-case origin (e.g., HTTPS://EXAMPLE.COM) will silently be accepted. Since every inbound Origin header is lowercased before comparison, a mixed-case allowed_origin_ will cause all browser requests to be rejected in production. Consider replacing the assert with an eager call to sourcemeta::core::to_lowercase(this->allowed_origin_) so the invariant holds regardless of build type or metapack provenance.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At enterprise/server/include/sourcemeta/one/enterprise_server_action_mcp_v1.h, line 71:
<comment>The `allowed_origin_` value loaded from metapack metadata is only validated to be lowercase via `assert()`. Because `assert` is stripped in release builds (NDEBUG), a stale or manually modified metapack carrying a mixed-case origin (e.g., `HTTPS://EXAMPLE.COM`) will silently be accepted. Since every inbound `Origin` header is lowercased before comparison, a mixed-case `allowed_origin_` will cause **all browser requests to be rejected** in production. Consider replacing the `assert` with an eager call to `sourcemeta::core::to_lowercase(this->allowed_origin_)` so the invariant holds regardless of build type or metapack provenance.</comment>
<file context>
@@ -0,0 +1,705 @@
+ // only need to fold the inbound header. Catch a stale metapack that
+ // somehow carried a mixed-case origin through.
+ // https://datatracker.ietf.org/doc/html/rfc6454#section-4
+ assert(sourcemeta::core::is_lowercase(this->allowed_origin_));
+ }
+
</file context>
| sourcemeta::core::to_lowercase(this->allowed_origin_); |
| this->rpc_request_schema_ = std::get<std::string_view>(value); | ||
| } else if (key == "mcpResponseSchema") { | ||
| this->rpc_response_schema_ = std::get<std::string_view>(value); | ||
| } else if (key == "errorSchema") { |
There was a problem hiding this comment.
P2: The mcpResponseSchema argument is stored in rpc_response_schema_ but never consumed. The mcp() method returns tool-success payloads (mcp_make_tool_success) directly without evaluating them against the configured response schema, so the stored value is dead configuration. This creates an unfulfilled contract: the routing layer passes a response schema, but the action silently skips validation, allowing future payload changes to drift from the published MCP tool contract unnoticed. If response schema enforcement is intended, validate each returned payload before wrapping it in the MCP success result; otherwise, remove the unused member and its constructor handling to avoid claiming a guarantee that is not provided.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At enterprise/server/include/sourcemeta/one/enterprise_server_action_jsonschema_rdf_v1.h, line 56:
<comment>The `mcpResponseSchema` argument is stored in `rpc_response_schema_` but never consumed. The `mcp()` method returns tool-success payloads (`mcp_make_tool_success`) directly without evaluating them against the configured response schema, so the stored value is dead configuration. This creates an unfulfilled contract: the routing layer passes a response schema, but the action silently skips validation, allowing future payload changes to drift from the published MCP tool contract unnoticed. If response schema enforcement is intended, validate each returned payload before wrapping it in the MCP success result; otherwise, remove the unused member and its constructor handling to avoid claiming a guarantee that is not provided.</comment>
<file context>
@@ -0,0 +1,497 @@
+ this->response_schema_ = std::get<std::string_view>(value);
+ } else if (key == "mcpRequestSchema") {
+ this->rpc_request_schema_ = std::get<std::string_view>(value);
+ } else if (key == "mcpResponseSchema") {
+ this->rpc_response_schema_ = std::get<std::string_view>(value);
+ } else if (key == "errorSchema") {
</file context>
| } | ||
| if (method == sourcemeta::core::MCP_METHOD_TOOLS_LIST) { | ||
| return this->on_tools_list(version, request_json); | ||
| } |
There was a problem hiding this comment.
P2: The MCP dispatch in process_one does not guard resource and tool method handlers against the capability flags advertised during initialization. The server reads .resources and .tools from the metapack metadata in on_initialize and includes them in the negotiated capabilities, but process_one unconditionally routes to resources/list, resources/read, tools/list, and tools/call even when those capabilities are false. Per the MCP spec, capabilities established during initialization must be respected for the lifetime of the session. If the metapack ever sets either flag to false, the server will advertise the feature as unavailable while still serving it. Consider adding capability checks before dispatching to these handlers, or ensure the metapack generation invariant guarantees these are always true for this action class and document it explicitly.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At enterprise/server/include/sourcemeta/one/enterprise_server_action_mcp_v1.h, line 677:
<comment>The MCP dispatch in `process_one` does not guard resource and tool method handlers against the capability flags advertised during initialization. The server reads `.resources` and `.tools` from the metapack metadata in `on_initialize` and includes them in the negotiated capabilities, but `process_one` unconditionally routes to `resources/list`, `resources/read`, `tools/list`, and `tools/call` even when those capabilities are `false`. Per the MCP spec, capabilities established during initialization must be respected for the lifetime of the session. If the metapack ever sets either flag to `false`, the server will advertise the feature as unavailable while still serving it. Consider adding capability checks before dispatching to these handlers, or ensure the metapack generation invariant guarantees these are always `true` for this action class and document it explicitly.</comment>
<file context>
@@ -0,0 +1,705 @@
+ if (method == sourcemeta::core::MCP_METHOD_INITIALIZE) {
+ return this->on_initialize(request_json);
+ }
+ if (method == sourcemeta::core::MCP_METHOD_TOOLS_LIST) {
+ return this->on_tools_list(version, request_json);
+ }
</file context>
| assert(id != nullptr); | ||
| const auto method{sourcemeta::core::jsonrpc_method(request_json)}; | ||
| if (!sourcemeta::core::mcp_is_request_method(method)) { | ||
| return sourcemeta::core::jsonrpc_make_error_method_not_found(*id); |
There was a problem hiding this comment.
P2: The process_one method checks mcp_is_request_method and returns Method not found before the schema_evaluate_fast call that is documented to enforce MCP's stricter null-id rule. This means a null-id request with an unknown method bypasses the null-id validation and receives -32601 instead of -32600. Since the adjacent comment states "Sourcemeta One follows MCP's tighter rule and rejects null-id requests here," the method lookup should probably come after the schema (or a stand-alone null-id) check, or the null-id rule should be validated earlier so it applies uniformly regardless of whether the method is known.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At enterprise/server/include/sourcemeta/one/enterprise_server_action_mcp_v1.h, line 661:
<comment>The `process_one` method checks `mcp_is_request_method` and returns `Method not found` *before* the `schema_evaluate_fast` call that is documented to enforce MCP's stricter null-id rule. This means a null-id request with an unknown method bypasses the null-id validation and receives `-32601` instead of `-32600`. Since the adjacent comment states "Sourcemeta One follows MCP's tighter rule and rejects null-id requests here," the method lookup should probably come after the schema (or a stand-alone null-id) check, or the null-id rule should be validated earlier so it applies uniformly regardless of whether the method is known.</comment>
<file context>
@@ -0,0 +1,705 @@
+ }
+ const auto *id{sourcemeta::core::jsonrpc_request_id(request_json)};
+ assert(id != nullptr);
+ const auto method{sourcemeta::core::jsonrpc_method(request_json)};
+ if (!sourcemeta::core::mcp_is_request_method(method)) {
+ return sourcemeta::core::jsonrpc_make_error_method_not_found(*id);
</file context>
| std::uint64_t admitted{0}; | ||
| this->search_view_.for_each( | ||
| 0, this->search_view_.count(), | ||
| [this, credential, &authentication, &resources, &admitted, |
There was a problem hiding this comment.
P2: The resources/list handler rescans and reauthorizes the entire catalog for every page request. for_each(0, count()) checks authentication.admits(...) on every entry, then counts admitted to determine whether the current item falls in the requested page window. For a catalog with N visible entries and P pages, this is O(P*N) total work, making MCP catalog browsing disproportionately expensive for large indexes. Consider whether SearchView can expose an authenticated iterator, a pre-filtered count, or a way to terminate for_each early so that deep pages do not require re-walking the full catalog from index 0.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At enterprise/server/include/sourcemeta/one/enterprise_server_action_mcp_v1.h, line 340:
<comment>The `resources/list` handler rescans and reauthorizes the entire catalog for every page request. `for_each(0, count())` checks `authentication.admits(...)` on every entry, then counts `admitted` to determine whether the current item falls in the requested page window. For a catalog with N visible entries and P pages, this is O(P*N) total work, making MCP catalog browsing disproportionately expensive for large indexes. Consider whether `SearchView` can expose an authenticated iterator, a pre-filtered count, or a way to terminate `for_each` early so that deep pages do not require re-walking the full catalog from index 0.</comment>
<file context>
@@ -0,0 +1,705 @@
+ const auto &authentication{this->dispatcher().authentication()};
+ auto resources{sourcemeta::core::JSON::make_array()};
+ std::uint64_t admitted{0};
+ this->search_view_.for_each(
+ 0, this->search_view_.count(),
+ [this, credential, &authentication, &resources, &admitted,
</file context>
| credential); | ||
| } catch (const std::exception &error) { | ||
| return sourcemeta::core::mcp_make_tool_error(id, error.what()); | ||
| } |
There was a problem hiding this comment.
P2: Unhandled tool exceptions are returned verbatim to MCP clients, which can leak internal diagnostic details. The catch (const std::exception &error) block in on_tools_call forwards error.what() directly via mcp_make_tool_error, whereas the batch dispatch loop in on_message maps unexpected exceptions to a generic internal error. Tool actions may throw filesystem, database, or network exceptions whose messages include internal paths or backend details. Expected input errors should be modeled explicitly, and truly unexpected exceptions should be logged server-side and surfaced to the client as a generic error message instead.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At enterprise/server/include/sourcemeta/one/enterprise_server_action_mcp_v1.h, line 545:
<comment>Unhandled tool exceptions are returned verbatim to MCP clients, which can leak internal diagnostic details. The `catch (const std::exception &error)` block in `on_tools_call` forwards `error.what()` directly via `mcp_make_tool_error`, whereas the batch dispatch loop in `on_message` maps unexpected exceptions to a generic internal error. Tool actions may throw filesystem, database, or network exceptions whose messages include internal paths or backend details. Expected input errors should be modeled explicitly, and truly unexpected exceptions should be logged server-side and surfaced to the client as a generic error message instead.</comment>
<file context>
@@ -0,0 +1,705 @@
+ return instance->mcp(version, id,
+ arguments == nullptr ? empty_arguments : *arguments,
+ credential);
+ } catch (const std::exception &error) {
+ return sourcemeta::core::mcp_make_tool_error(id, error.what());
+ }
</file context>
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Benchmark Index (community)
Details
| Benchmark suite | Current: a211451 | Previous: 608acda | Ratio |
|---|---|---|---|
Add one schema (0 existing) |
318 ms |
306 ms |
1.04 |
Add one schema (100 existing) |
28 ms |
23 ms |
1.22 |
Add one schema (1000 existing) |
85 ms |
70 ms |
1.21 |
Add one schema (10000 existing) |
888 ms |
600 ms |
1.48 |
Update one schema (1 existing) |
20 ms |
16 ms |
1.25 |
Update one schema (101 existing) |
29 ms |
23 ms |
1.26 |
Update one schema (1001 existing) |
84 ms |
70 ms |
1.20 |
Update one schema (10001 existing) |
698 ms |
599 ms |
1.17 |
Cached rebuild (1 existing) |
7 ms |
5 ms |
1.40 |
Cached rebuild (101 existing) |
9 ms |
6 ms |
1.50 |
Cached rebuild (1001 existing) |
29 ms |
19 ms |
1.53 |
Cached rebuild (10001 existing) |
250 ms |
162 ms |
1.54 |
Index 100 schemas |
530 ms |
369 ms |
1.44 |
Index 1000 schemas |
1461 ms |
1244 ms |
1.17 |
Index 10000 schemas |
13621 ms |
11320 ms |
1.20 |
Index 10000 schemas (custom meta-schema) |
16398 ms |
13541 ms |
1.21 |
Index 10000 schemas ($ref fan-out) |
16514 ms |
13942 ms |
1.18 |
This comment was automatically generated by workflow using github-action-benchmark.
There was a problem hiding this comment.
Benchmark Index (enterprise)
Details
| Benchmark suite | Current: a211451 | Previous: 608acda | Ratio |
|---|---|---|---|
Add one schema (0 existing) |
395 ms |
395 ms |
1 |
Add one schema (100 existing) |
107 ms |
106 ms |
1.01 |
Add one schema (1000 existing) |
159 ms |
160 ms |
0.99 |
Add one schema (10000 existing) |
770 ms |
792 ms |
0.97 |
Update one schema (1 existing) |
97 ms |
97 ms |
1 |
Update one schema (101 existing) |
105 ms |
104 ms |
1.01 |
Update one schema (1001 existing) |
161 ms |
161 ms |
1 |
Update one schema (10001 existing) |
768 ms |
796 ms |
0.96 |
Cached rebuild (1 existing) |
8 ms |
8 ms |
1 |
Cached rebuild (101 existing) |
10 ms |
11 ms |
0.91 |
Cached rebuild (1001 existing) |
31 ms |
31 ms |
1 |
Cached rebuild (10001 existing) |
251 ms |
259 ms |
0.97 |
Index 100 schemas |
655 ms |
498 ms |
1.32 |
Index 1000 schemas |
1585 ms |
1630 ms |
0.97 |
Index 10000 schemas |
14787 ms |
14430 ms |
1.02 |
Index 10000 schemas (custom meta-schema) |
16963 ms |
17585 ms |
0.96 |
Index 10000 schemas ($ref fan-out) |
16663 ms |
17055 ms |
0.98 |
This comment was automatically generated by workflow using github-action-benchmark.
1 participant
Signed-off-by: Juan Cruz Viotti jv@jviotti.com