Add kind discriminator for MCP resource server tools and resources#3550
Open
sahandilshan wants to merge 1 commit into
Open
Add kind discriminator for MCP resource server tools and resources#3550sahandilshan wants to merge 1 commit into
sahandilshan wants to merge 1 commit into
Conversation
Contributor
Author
|
Console UI counterpart: #3551 |
12 tasks
sahandilshan
added
the
breaking change
Contributor
📝 WalkthroughWalkthroughThe PR adds ActionKind handling across OpenAPI contracts, HTTP request/response models, resource service validation, storage persistence and filtering, and declarative resource export/import. Action lists can now be filtered by kind, and MCP action and resource validation rules now enforce kind and handle constraints. ChangesMCP ActionKind Rollout
Estimated code review effort🎯 5 (Critical) | ⏱️ ~90+ minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 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 |
Bundle ReportBundle size has no change ✅ Affected Assets, Files, and Routes:view changes for bundle: console-esmAssets Changed:
|
![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: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
backend/internal/resource/declarative_resource_test.go (1)
154-187: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winKeep the default server fixture kind-free.
This server has no MCP
Type, so it defaults to CUSTOM; assertingActionKindToolhere codifies a state thatparseToResourceServerrejects for non-MCP servers. DropKindfrom this baseline test and keep MCP kind coverage in the round-trip test below.Proposed test fix
Handle: "read", Description: "Read action", Permission: "test-server:resource1:read", - Kind: providers.ActionKindTool, }, }, } @@ assert.Equal(s.T(), serverID, dto.ID) assert.Equal(s.T(), "Test Server", dto.Name) assert.Len(s.T(), dto.Resources, 1) assert.Len(s.T(), dto.Resources[0].Actions, 1) - assert.Equal(s.T(), providers.ActionKindTool, dto.Resources[0].Actions[0].Kind) + assert.Empty(s.T(), dto.Resources[0].Actions[0].Kind) }🤖 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 `@backend/internal/resource/declarative_resource_test.go` around lines 154 - 187, The baseline GetResourceByID test is incorrectly asserting an MCP tool kind on a server fixture that should remain kind-free. Update the test around exporter.GetResourceByID to remove the ActionKindTool expectation from the default resource/action setup, since parseToResourceServer should not require or preserve a kind for non-MCP servers. Keep Action kind coverage in the dedicated round-trip/MCP test instead, and only assert the server/resource fields that are valid for the default CUSTOM case.backend/internal/resource/handler.go (1)
299-322: 🔒 Security & Privacy | 🟡 MinorFile: docs/content/guides/guides/resource-servers.mdx
🔴 Documentation Required
This PR introduces user-facing API and schema changes that are not yet reflected in the documentation. Please update the relevant files to cover the following:
- API Reference: Add the new
kindquery parameter to theGET /resource-servers/{id}/actionsendpoint indocs/content/apis.mdx, specifying allowed values (tool,resource).- Action Payloads: Document the new
kindfield in action create request/response schemas, including MCP-specific semantics.- Guides: Update
docs/content/guides/guides/resource-servers.mdxto clarify that thehandlefield is now required when creating actions and explain related validation logic.Missing documentation:
docs/content/apis.mdx: UpdateGET /resource-servers/{id}/actionssection to include thekindparameter.docs/content/guides/guides/resource-servers.mdx: Clarify thehandlerequirement and update the action creation example.🤖 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 `@backend/internal/resource/handler.go` around lines 299 - 322, The new `kind` query filter in `HandleActionListAtResourceServerRequest` is not documented, and the resource-server guide still reflects the old action creation rules. Update `docs/content/apis.mdx` for `GET /resource-servers/{id}/actions` to list the `kind` parameter with allowed values `tool` and `resource`, and revise `docs/content/guides/guides/resource-servers.mdx` to state that `handle` is required when creating actions and to update the action creation example and validation notes accordingly.Source: Path instructions
🤖 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 `@backend/internal/resource/declarative_resource_test.go`:
- Around line 414-459: The MCP parser tests are using YAML fixtures that are now
invalid because they omit the required server handle, so the failures are no
longer isolated to action kind validation. Update the fixtures in
TestParseToResourceServer_MCPActionRequiresKind and
TestParseToResourceServer_MCPActionWithKindSucceeds to include a valid MCP
server handle so parseToResourceServer is exercising the intended action-kind
behavior. Keep the existing assertions around providers.ActionKindResource,
providers.ActionKindTool, and the “requires a valid kind” error so the tests
continue to validate the correct path.
In `@backend/internal/resource/declarative_resource.go`:
- Around line 288-309: The MCP validation in parseToResourceServer is missing a
required server handle check, so type MCP can still pass through with an empty
handle before nested action validation. Add an early validation in
parseToResourceServer for MCP resource servers to reject empty rs.Handle before
the action loop, using the existing resource server type checks and error style
alongside rs.Type, rs.Handle, and ProcessResourceServer so permissions are never
derived without a handle.
---
Outside diff comments:
In `@backend/internal/resource/declarative_resource_test.go`:
- Around line 154-187: The baseline GetResourceByID test is incorrectly
asserting an MCP tool kind on a server fixture that should remain kind-free.
Update the test around exporter.GetResourceByID to remove the ActionKindTool
expectation from the default resource/action setup, since parseToResourceServer
should not require or preserve a kind for non-MCP servers. Keep Action kind
coverage in the dedicated round-trip/MCP test instead, and only assert the
server/resource fields that are valid for the default CUSTOM case.
In `@backend/internal/resource/handler.go`:
- Around line 299-322: The new `kind` query filter in
`HandleActionListAtResourceServerRequest` is not documented, and the
resource-server guide still reflects the old action creation rules. Update
`docs/content/apis.mdx` for `GET /resource-servers/{id}/actions` to list the
`kind` parameter with allowed values `tool` and `resource`, and revise
`docs/content/guides/guides/resource-servers.mdx` to state that `handle` is
required when creating actions and to update the action creation example and
validation notes accordingly.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 0ab091ac-74e4-4adc-9cee-4a8d9e85840d
⛔ Files ignored due to path filters (2)
backend/tests/mocks/resourcemock/ResourceServiceInterface_mock.gois excluded by!**/*_mock.gobackend/tests/mocks/resourcemock/resourceStoreInterface_mock.gois excluded by!**/*_mock.go
📒 Files selected for processing (21)
api/resource.yamlbackend/internal/authzen/service.gobackend/internal/authzen/service_test.gobackend/internal/resource/ResourceServiceInterface_mock_test.gobackend/internal/resource/composite_store.gobackend/internal/resource/composite_store_test.gobackend/internal/resource/declarative_resource.gobackend/internal/resource/declarative_resource_test.gobackend/internal/resource/file_based_store.gobackend/internal/resource/file_based_store_test.gobackend/internal/resource/handler.gobackend/internal/resource/handler_test.gobackend/internal/resource/model.gobackend/internal/resource/resourceStoreInterface_mock_test.gobackend/internal/resource/service.gobackend/internal/resource/service_test.gobackend/internal/resource/store.gobackend/internal/resource/store_constants.gobackend/internal/resource/store_test.gobackend/pkg/thunderidengine/providers/model.gobackend/pkg/thunderidengine/providers/model_test.go
Comment on lines
+414
to
+459
| func TestParseToResourceServer_MCPActionRequiresKind(t *testing.T) { | ||
| yamlData := []byte(` | ||
| id: "rs1" | ||
| name: "Test Server" | ||
| type: "MCP" | ||
| ouId: "ou1" | ||
| resources: | ||
| - name: "Users" | ||
| handle: "users" | ||
| actions: | ||
| - name: "Read" | ||
| handle: "read" | ||
| `) | ||
|
|
||
| dto, err := parseToResourceServer(yamlData) | ||
|
|
||
| assert.Error(t, err) | ||
| assert.Nil(t, dto) | ||
| assert.Contains(t, err.Error(), "read") | ||
| assert.Contains(t, err.Error(), "requires a valid kind") | ||
| } | ||
|
|
||
| func TestParseToResourceServer_MCPActionWithKindSucceeds(t *testing.T) { | ||
| yamlData := []byte(` | ||
| id: "rs1" | ||
| name: "Test Server" | ||
| type: "MCP" | ||
| ouId: "ou1" | ||
| resources: | ||
| - name: "Users" | ||
| handle: "users" | ||
| actions: | ||
| - name: "Read" | ||
| handle: "read" | ||
| kind: "resource" | ||
| - name: "Create" | ||
| handle: "create" | ||
| kind: "tool" | ||
| `) | ||
|
|
||
| dto, err := parseToResourceServer(yamlData) | ||
|
|
||
| assert.NoError(t, err) | ||
| assert.NotNil(t, dto) | ||
| assert.Equal(t, providers.ActionKindResource, dto.Resources[0].Actions[0].Kind) | ||
| assert.Equal(t, providers.ActionKindTool, dto.Resources[0].Actions[1].Kind) |
Contributor
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Add MCP handles to parser fixtures.
These MCP YAML fixtures omit handle; after enforcing the required MCP server handle, the “requires kind” test would fail for the wrong reason and the success test would encode an invalid MCP document.
Proposed fixture fix
id: "rs1"
name: "Test Server"
type: "MCP"
+handle: "test-server"
ouId: "ou1"
resources:
@@
id: "rs1"
name: "Test Server"
type: "MCP"
+handle: "test-server"
ouId: "ou1"
resources:📝 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
| func TestParseToResourceServer_MCPActionRequiresKind(t *testing.T) { | |
| yamlData := []byte(` | |
| id: "rs1" | |
| name: "Test Server" | |
| type: "MCP" | |
| ouId: "ou1" | |
| resources: | |
| - name: "Users" | |
| handle: "users" | |
| actions: | |
| - name: "Read" | |
| handle: "read" | |
| `) | |
| dto, err := parseToResourceServer(yamlData) | |
| assert.Error(t, err) | |
| assert.Nil(t, dto) | |
| assert.Contains(t, err.Error(), "read") | |
| assert.Contains(t, err.Error(), "requires a valid kind") | |
| } | |
| func TestParseToResourceServer_MCPActionWithKindSucceeds(t *testing.T) { | |
| yamlData := []byte(` | |
| id: "rs1" | |
| name: "Test Server" | |
| type: "MCP" | |
| ouId: "ou1" | |
| resources: | |
| - name: "Users" | |
| handle: "users" | |
| actions: | |
| - name: "Read" | |
| handle: "read" | |
| kind: "resource" | |
| - name: "Create" | |
| handle: "create" | |
| kind: "tool" | |
| `) | |
| dto, err := parseToResourceServer(yamlData) | |
| assert.NoError(t, err) | |
| assert.NotNil(t, dto) | |
| assert.Equal(t, providers.ActionKindResource, dto.Resources[0].Actions[0].Kind) | |
| assert.Equal(t, providers.ActionKindTool, dto.Resources[0].Actions[1].Kind) | |
| func TestParseToResourceServer_MCPActionRequiresKind(t *testing.T) { | |
| yamlData := []byte(` | |
| id: "rs1" | |
| name: "Test Server" | |
| type: "MCP" | |
| handle: "test-server" | |
| ouId: "ou1" | |
| resources: | |
| - name: "Users" | |
| handle: "users" | |
| actions: | |
| - name: "Read" | |
| handle: "read" | |
| `) | |
| dto, err := parseToResourceServer(yamlData) | |
| assert.Error(t, err) | |
| assert.Nil(t, dto) | |
| assert.Contains(t, err.Error(), "read") | |
| assert.Contains(t, err.Error(), "requires a valid kind") | |
| } | |
| func TestParseToResourceServer_MCPActionWithKindSucceeds(t *testing.T) { | |
| yamlData := []byte(` | |
| id: "rs1" | |
| name: "Test Server" | |
| type: "MCP" | |
| handle: "test-server" | |
| ouId: "ou1" | |
| resources: | |
| - name: "Users" | |
| handle: "users" | |
| actions: | |
| - name: "Read" | |
| handle: "read" | |
| kind: "resource" | |
| - name: "Create" | |
| handle: "create" | |
| kind: "tool" | |
| `) | |
| dto, err := parseToResourceServer(yamlData) | |
| assert.NoError(t, err) | |
| assert.NotNil(t, dto) | |
| assert.Equal(t, providers.ActionKindResource, dto.Resources[0].Actions[0].Kind) | |
| assert.Equal(t, providers.ActionKindTool, dto.Resources[0].Actions[1].Kind) |
🤖 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 `@backend/internal/resource/declarative_resource_test.go` around lines 414 -
459, The MCP parser tests are using YAML fixtures that are now invalid because
they omit the required server handle, so the failures are no longer isolated to
action kind validation. Update the fixtures in
TestParseToResourceServer_MCPActionRequiresKind and
TestParseToResourceServer_MCPActionWithKindSucceeds to include a valid MCP
server handle so parseToResourceServer is exercising the intended action-kind
behavior. Keep the existing assertions around providers.ActionKindResource,
providers.ActionKindTool, and the “requires a valid kind” error so the tests
continue to validate the correct path.
Comment on lines
+288
to
+309
| // Enforce the action kind discriminator rules per resource server type (mirrors the REST path). | ||
| // MCP resource servers require a valid kind on each nested action; API/CUSTOM resource servers | ||
| // must not carry a kind. | ||
| for i := range rs.Resources { | ||
| for j := range rs.Resources[i].Actions { | ||
| action := rs.Resources[i].Actions[j] | ||
| if rs.Type == providers.ResourceServerTypeMCP { | ||
| if !action.Kind.IsValid() { | ||
| return nil, fmt.Errorf( | ||
| "action %q in resource server '%s' requires a valid kind (tool|resource)", | ||
| action.Handle, rs.Name, | ||
| ) | ||
| } | ||
| } else if action.Kind != "" { | ||
| return nil, fmt.Errorf( | ||
| "action %q in resource server '%s' must not specify a kind", | ||
| action.Handle, rs.Name, | ||
| ) | ||
| } | ||
| } | ||
| } | ||
|
|
Contributor
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Reject MCP declarative servers without a handle.
parseToResourceServer now enforces MCP action kind, but still accepts type: MCP with an empty server handle; then ProcessResourceServer derives permissions without the required server handle. Add the MCP handle check before validating nested actions.
Proposed fix
if rs.Type == "" {
rs.Type = providers.ResourceServerTypeCustom
}
+ if rs.Type == providers.ResourceServerTypeMCP && rs.Handle == "" {
+ return nil, fmt.Errorf("resource server '%s' requires a non-empty handle for MCP type", rs.Name)
+ }
// Enforce the action kind discriminator rules per resource server type (mirrors the REST path).📝 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
| // Enforce the action kind discriminator rules per resource server type (mirrors the REST path). | |
| // MCP resource servers require a valid kind on each nested action; API/CUSTOM resource servers | |
| // must not carry a kind. | |
| for i := range rs.Resources { | |
| for j := range rs.Resources[i].Actions { | |
| action := rs.Resources[i].Actions[j] | |
| if rs.Type == providers.ResourceServerTypeMCP { | |
| if !action.Kind.IsValid() { | |
| return nil, fmt.Errorf( | |
| "action %q in resource server '%s' requires a valid kind (tool|resource)", | |
| action.Handle, rs.Name, | |
| ) | |
| } | |
| } else if action.Kind != "" { | |
| return nil, fmt.Errorf( | |
| "action %q in resource server '%s' must not specify a kind", | |
| action.Handle, rs.Name, | |
| ) | |
| } | |
| } | |
| } | |
| if rs.Type == "" { | |
| rs.Type = providers.ResourceServerTypeCustom | |
| } | |
| if rs.Type == providers.ResourceServerTypeMCP && rs.Handle == "" { | |
| return nil, fmt.Errorf("resource server '%s' requires a non-empty handle for MCP type", rs.Name) | |
| } | |
| // Enforce the action kind discriminator rules per resource server type (mirrors the REST path). | |
| // MCP resource servers require a valid kind on each nested action; API/CUSTOM resource servers | |
| // must not carry a kind. | |
| for i := range rs.Resources { | |
| for j := range rs.Resources[i].Actions { | |
| action := rs.Resources[i].Actions[j] | |
| if rs.Type == providers.ResourceServerTypeMCP { | |
| if !action.Kind.IsValid() { | |
| return nil, fmt.Errorf( | |
| "action %q in resource server '%s' requires a valid kind (tool|resource)", | |
| action.Handle, rs.Name, | |
| ) | |
| } | |
| } else if action.Kind != "" { | |
| return nil, fmt.Errorf( | |
| "action %q in resource server '%s' must not specify a kind", | |
| action.Handle, rs.Name, | |
| ) | |
| } | |
| } | |
| } |
🤖 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 `@backend/internal/resource/declarative_resource.go` around lines 288 - 309,
The MCP validation in parseToResourceServer is missing a required server handle
check, so type MCP can still pass through with an empty handle before nested
action validation. Add an early validation in parseToResourceServer for MCP
resource servers to reject empty rs.Handle before the action loop, using the
existing resource server type checks and error style alongside rs.Type,
rs.Handle, and ProcessResourceServer so permissions are never derived without a
handle.
Codecov Report❌ Patch coverage is 📢 Thoughts on this report? Let us know! |
Model MCP tools and resources as actions distinguished by a kind
("tool"|"resource") stored in ACTION.PROPERTIES, surfaced as a flat
Action.Kind field (mirroring the RESOURCE_SERVER delimiter pattern).
- kind is optional on create for all resource server types; MCP-type
servers default an omitted kind to "tool"; API/CUSTOM may set a kind
but are not required to. A provided kind must be "tool" or "resource".
- kind is immutable after create (the update path does not accept it).
- MCP-gated cross-entity handle/permission collision guard, on the REST
and declarative-import paths.
- kind on the create request/response; GET .../actions?kind= filter
(Postgres and SQLite variants) with a matching filtered count.
- Declarative import/export preserve kind; OpenAPI updated.
Refs thunder-id#3465
sahandilshan
force-pushed
the
mcp-rs-kind-backend
branch
from
1c9fd23 to
d814ba4
Compare
sahandilshan
removed
the
breaking change
Contributor
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 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 `@api/resource.yaml`:
- Around line 1168-1173: Update the example UUID values in the resource schema
so they are valid UUIDs composed only of hex characters and hyphens, since the
current examples in the booking/resource definitions use invalid characters and
conflict with the documented uuid format. Replace the affected example ids in
the resource examples block and the other referenced example sections with
schema-valid UUIDs, keeping the surrounding fields and structure unchanged.
- Around line 1238-1247: Update the invalid-kind example in the resource
validation examples so it matches a GET query-parameter failure, not a malformed
request body. In the relevant example blocks for the invalid-kind response,
change the description text to refer to an invalid query parameter or query
validation issue, and keep the error key/value structure consistent with the
surrounding resource validation examples. Use the invalid-kind example entry to
locate and mirror the same fix in both places.
In `@backend/internal/resource/declarative_resource_test.go`:
- Around line 190-253: The MCP round-trip test only covers nested resource
actions, but server-level actions are still dropped because GetResourceByID only
exports actions via GetActionList tied to each resource and ResourceServer has
no top-level Actions field. Update the declarative export/import path in
GetResourceByID and the related providers.ResourceServer model so server-scoped
MCP actions are preserved, and extend this test to assert that top-level actions
survive the export-to-YAML and parseToResourceServer import round-trip alongside
the existing resource-level action checks.
In `@backend/internal/resource/service.go`:
- Around line 1076-1079: The filtered action listing in GetActionList drops the
kind query parameter when building pagination links, so self/next/prev can point
to an unfiltered endpoint. Update the baseURL and link generation in
GetActionList, preserving the kind value alongside limit/offset, and make the
same fix in the pagination link assembly used later in the function so filtered
pages stay consistent.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: c63c508e-0b57-4dfe-b982-f036a965133e
⛔ Files ignored due to path filters (2)
backend/tests/mocks/resourcemock/ResourceServiceInterface_mock.gois excluded by!**/*_mock.gobackend/tests/mocks/resourcemock/resourceStoreInterface_mock.gois excluded by!**/*_mock.go
📒 Files selected for processing (21)
api/resource.yamlbackend/internal/authzen/service.gobackend/internal/authzen/service_test.gobackend/internal/resource/ResourceServiceInterface_mock_test.gobackend/internal/resource/composite_store.gobackend/internal/resource/composite_store_test.gobackend/internal/resource/declarative_resource.gobackend/internal/resource/declarative_resource_test.gobackend/internal/resource/file_based_store.gobackend/internal/resource/file_based_store_test.gobackend/internal/resource/handler.gobackend/internal/resource/handler_test.gobackend/internal/resource/model.gobackend/internal/resource/resourceStoreInterface_mock_test.gobackend/internal/resource/service.gobackend/internal/resource/service_test.gobackend/internal/resource/store.gobackend/internal/resource/store_constants.gobackend/internal/resource/store_test.gobackend/pkg/thunderidengine/providers/model.gobackend/pkg/thunderidengine/providers/model_test.go
✅ Files skipped from review due to trivial changes (3)
- backend/pkg/thunderidengine/providers/model_test.go
- backend/internal/resource/ResourceServiceInterface_mock_test.go
- backend/internal/resource/resourceStoreInterface_mock_test.go
🚧 Files skipped from review as they are similar to previous changes (14)
- backend/internal/resource/store_constants.go
- backend/pkg/thunderidengine/providers/model.go
- backend/internal/resource/file_based_store_test.go
- backend/internal/resource/file_based_store.go
- backend/internal/resource/composite_store.go
- backend/internal/resource/handler.go
- backend/internal/resource/model.go
- backend/internal/authzen/service_test.go
- backend/internal/authzen/service.go
- backend/internal/resource/declarative_resource.go
- backend/internal/resource/composite_store_test.go
- backend/internal/resource/handler_test.go
- backend/internal/resource/store.go
- backend/internal/resource/store_test.go
Comment on lines
+1168
to
+1173
| - id: "9c6d3g0g-7e8b-4d82-b454-4d2ccg87gh5e" | ||
| name: "Create Booking" | ||
| description: "Create a new booking" | ||
| handle: "create" | ||
| permission: "create" | ||
| - id: "ad7e4h1h-8f9c-4e93-c565-5e3ddf98hi6f" |
Contributor
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Use schema-valid UUIDs in the new examples.
The new id example values contain non-hex characters (g, h, i, j), so they do not satisfy the documented format: uuid. Some OpenAPI tooling validates examples and will flag these payloads as invalid.
Also applies to: 1188-1194, 1718-1723, 1738-1738
🤖 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 `@api/resource.yaml` around lines 1168 - 1173, Update the example UUID values
in the resource schema so they are valid UUIDs composed only of hex characters
and hyphens, since the current examples in the booking/resource definitions use
invalid characters and conflict with the documented uuid format. Replace the
affected example ids in the resource examples block and the other referenced
example sections with schema-valid UUIDs, keeping the surrounding fields and
structure unchanged.
Comment on lines
+1238
to
+1247
| invalid-kind: | ||
| summary: Invalid kind parameter value | ||
| value: | ||
| code: "RES-1001" | ||
| message: | ||
| key: "error.resourceservice.invalid_request_format" | ||
| defaultValue: "Invalid request format" | ||
| description: | ||
| key: "error.resourceservice.invalid_request_format_description" | ||
| defaultValue: "The request body is malformed or contains invalid data" |
Contributor
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Fix the invalid-kind error examples.
These are GET query-validation failures, but the example description says the request body is malformed. That makes the contract misleading for clients diagnosing a bad kind parameter.
Also applies to: 1782-1791
🤖 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 `@api/resource.yaml` around lines 1238 - 1247, Update the invalid-kind example
in the resource validation examples so it matches a GET query-parameter failure,
not a malformed request body. In the relevant example blocks for the
invalid-kind response, change the description text to refer to an invalid query
parameter or query validation issue, and keep the error key/value structure
consistent with the surrounding resource validation examples. Use the
invalid-kind example entry to locate and mirror the same fix in both places.
Comment on lines
+190
to
+253
| func (s *ResourceServerExporterTestSuite) TestGetResourceByID_MCPExportImportRoundTrip() { | ||
| ctx := context.Background() | ||
| serverID := "rs-mcp" | ||
|
|
||
| server := &providers.ResourceServer{ | ||
| ID: serverID, | ||
| Name: "Booking MCP", | ||
| Handle: "booking-mcp", | ||
| Identifier: "booking-mcp", | ||
| Type: providers.ResourceServerTypeMCP, | ||
| OUID: "ou1", | ||
| Delimiter: ":", | ||
| } | ||
|
|
||
| resources := []providers.Resource{ | ||
| { | ||
| ID: "res1", | ||
| Name: "User Management", | ||
| Handle: "user-mgmt", | ||
| Parent: nil, | ||
| Permission: "booking-mcp:user-mgmt", | ||
| }, | ||
| } | ||
|
|
||
| actions := &ActionList{ | ||
| TotalResults: 1, | ||
| Actions: []providers.Action{ | ||
| { | ||
| ID: "act1", | ||
| Name: "Create User", | ||
| Handle: "create_user", | ||
| Permission: "booking-mcp:user-mgmt:create_user", | ||
| Kind: providers.ActionKindTool, | ||
| }, | ||
| }, | ||
| } | ||
|
|
||
| resourceID := "res1" | ||
| s.mockService.EXPECT().GetResourceServer(ctx, serverID).Return(server, nil) | ||
| s.mockService.EXPECT().GetAllResourceList(ctx, serverID).Return(resources, nil) | ||
| s.mockService.EXPECT(). | ||
| GetActionList(ctx, serverID, &resourceID, providers.ActionKind(""), serverconst.MaxPageSize, 0). | ||
| Return(actions, nil) | ||
|
|
||
| result, _, err := s.exporter.GetResourceByID(ctx, serverID) | ||
| assert.Nil(s.T(), err) | ||
|
|
||
| dto, ok := result.(*providers.ResourceServer) | ||
| assert.True(s.T(), ok) | ||
|
|
||
| // Marshal the exported DTO to YAML, then re-import it. This guards the export->import | ||
| // lossless guarantee: type and handle must survive so the kind-vs-type import validation | ||
| // accepts the nested action carrying a kind. | ||
| yamlBytes, marshalErr := yaml.Marshal(dto) | ||
| assert.NoError(s.T(), marshalErr) | ||
|
|
||
| imported, parseErr := parseToResourceServer(yamlBytes) | ||
| s.Require().NoError(parseErr) | ||
| s.Require().NotNil(imported) | ||
| assert.Equal(s.T(), providers.ResourceServerTypeMCP, imported.Type) | ||
| assert.Equal(s.T(), "booking-mcp", imported.Handle) | ||
| assert.Len(s.T(), imported.Resources, 1) | ||
| assert.Len(s.T(), imported.Resources[0].Actions, 1) | ||
| assert.Equal(s.T(), providers.ActionKindTool, imported.Resources[0].Actions[0].Kind) |
Contributor
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy lift
Server-level MCP actions are still unrepresented in declarative export/import.
This round-trip only covers actions under a resource. In the supplied GetResourceByID implementation, export walks GetAllResourceList and only calls GetActionList(..., &res.ID, ...), while providers.ResourceServer has no top-level Actions field. That means the new “resource is optional grouping namespace” case can’t round-trip: actions attached directly to the resource server will be dropped on export/import.
🤖 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 `@backend/internal/resource/declarative_resource_test.go` around lines 190 -
253, The MCP round-trip test only covers nested resource actions, but
server-level actions are still dropped because GetResourceByID only exports
actions via GetActionList tied to each resource and ResourceServer has no
top-level Actions field. Update the declarative export/import path in
GetResourceByID and the related providers.ResourceServer model so server-scoped
MCP actions are preserved, and extend this test to assert that top-level actions
survive the export-to-YAML and parseToResourceServer import round-trip alongside
the existing resource-level action checks.
Comment on lines
+1076
to
+1079
| // If kind is non-empty, only actions of that kind are returned. | ||
| func (rs *resourceService) GetActionList( | ||
| ctx context.Context, | ||
| resourceServerID string, resourceID *string, limit, offset int, | ||
| resourceServerID string, resourceID *string, kind providers.ActionKind, limit, offset int, |
Contributor
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Preserve kind in pagination links.
Line 1079 adds filtered listing, but the baseURL built below never carries that filter. A GET .../actions?kind=tool page will return filtered data while self/next/prev links point at the unfiltered endpoint, so following pagination links changes the result set.
Also applies to: 1127-1141
🤖 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 `@backend/internal/resource/service.go` around lines 1076 - 1079, The filtered
action listing in GetActionList drops the kind query parameter when building
pagination links, so self/next/prev can point to an unfiltered endpoint. Update
the baseURL and link generation in GetActionList, preserving the kind value
alongside limit/offset, and make the same fix in the pagination link assembly
used later in the function so filtered pages stay consistent.
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.
Purpose
Model MCP tools and resources as first-class primitives in resource servers so the product (and the Console) can speak MCP's language, per #3465. An MCP tool and an MCP resource are both actions distinguished by a
kind(tool|resource); a Thunder resource acts as an optional grouping namespace. Thunder stores only what authorization needs — the permission plus thekinddiscriminator — while the MCP server remains the owner of per-primitive definitions (URI, input schema, content). API- and CUSTOM-type resource servers are unchanged.Approach
kindis stored inACTION.PROPERTIESJSONB and surfaced as a flat field, mirroring the existingRESOURCE_SERVER.delimiterpattern (read pathresolveActionProperties, write pathbuildActionPropertiesJSON; written asNULLwhen empty so non-MCP actions are byte-for-byte unchanged). TheActionKindtype andAction.Kindfield live inpkg/thunderidengine/providersalongside the domain types.kindis optional on create for all resource server types:kinddefaults totool; a providedkindmust betool|resource.kindmay be set if desired but is not required; omitted means no kind (unchanged behavior).kind→400for any type.kindis immutable after create — the update path does not accept it; the stored value is preserved.kindon the create request/response;GET /resource-servers/{id}/actions?kind=tool|resourcefilter (PostgresPROPERTIES->>'kind'/ SQLitejson_extract, with a matching filtered count). Declarative import/export preservekind(MCP import also applies the default).api/resource.yamldocuments the field, the filter, and behavior.kindnever participates in permission derivation or exact-string RBAC matching.Related Issues
Related PRs
Checklist
breaking changelabel added.Security checks
Summary by CodeRabbit
New Features
toolorresource) across the app.Bug Fixes
Documentation