Conversation
Add a new MCP (Model Context Protocol) server module that exposes Tolgee
functionality as MCP tools over streamable HTTP at /mcp/developer.
Includes a Kotlin DSL (toolSchema {}) for defining tool input schemas
with compile-time safety, replacing raw JSON strings. Tools cover
projects, languages, namespaces, keys, translations, tags, batch
operations, and BigMeta.
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
- Extract ProjectContextService from ProjectAuthorizationInterceptor (scopes, PAK validation, bypass, holder population) - Extract FeatureCheckService from FeatureAuthorizationInterceptor - Split ToolEndpointSpec into separate file from McpSecurityContext - McpSecurityContext now reuses interceptor logic via shared services and mirrors the HTTP interceptor chain order - Simplify McpSecurityContext.checkOrgRole to use OrganizationRoleService.isUserOfRole Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Verify the duplicated HTTP interceptor chain in McpSecurityContext (token type, rate limiting, read-only mode, org role, features, activity tracking, project context) with mocked dependencies. Also test buildSpec error handling for missing @AllowApiAccess and unsupported @RequiresSuperAuthentication. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Move all MCP source and test files from the standalone backend/mcp/ module into backend/app/ to enable integration tests that need the full Spring Boot context. Reorganize test packages: existing unit tests move to tools/spec/, BuildSpecErrorTest moves to tools/. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
The base class tests the full ToolEndpointSpec interceptor chain, not just security. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Integration tests that send real JSON-RPC calls to the MCP server via McpSyncClient. Covers authentication, project tools, key tools, translation tools, and language tools. DB-level assertions verify data persistence via KeyService, LanguageService, and TranslationService. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
McpTagToolsTest: tag_key, list_tags, list_tags with search filter. McpBigMetaToolsTest: store_big_meta with async DB verification. McpBatchToolsTest: machine_translate and get_batch_job_status with fake MT providers. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Fix incorrect set_translation description (claimed to create keys but actually errors), remove phantom language filtering claim from get_translations, clarify BigMeta purpose, add cross-references between related tools, and document result limits. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Short blurb in README with link; detailed Claude Code connection instructions (claude mcp add command, PAT setup) in DEVELOPMENT.md. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
…gration tests The class handles more than security (activity tracking, PostHog events, rate limiting, read-only mode), so McpRequestContext better reflects its purpose. Also adds McpServerIntegrationTest with 8 tests covering activity tracking, PostHog events, auth, read-only mode, and rate limiting. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Add list_keys tool that returns up to 100 keys sorted by ID, with totalKeys count and a hint to use search_keys when there are more. Also add a default branch to AbstractMcpTest.createTestDataWithPat() which fixes the branch_id null constraint in all key-related tests. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
📝 WalkthroughWalkthroughAdds a Model Context Protocol (MCP) server at /mcp/developer, integrates MCP SDK, wires MCP transport/server/config, implements request-context, endpoint spec and schema builders, provides nine MCP tool providers, centralizes project/feature authorization, extends services, and adds comprehensive unit/integration tests and test infrastructure. Changes
Sequence Diagram(s)sequenceDiagram
participant Client as MCP Client
participant Server as MCP Server (/mcp/developer)
participant McpReq as McpRequestContext
participant Auth as AuthenticationFacade
participant Rate as RateLimitService
participant ProjCtx as ProjectContextService
participant Tool as Tool Handler
participant Service as Domain Service
Client->>Server: callTool(name, args, X-API-Key)
Server->>McpReq: executeAs(spec, projectId) { block }
McpReq->>Auth: validate token type / scopes
Auth-->>McpReq: auth result
McpReq->>Rate: apply rate limit (per-user)
Rate-->>McpReq: allowed / rate-limited
McpReq->>ProjCtx: setup(projectId, scopes, permissions, isWrite)
ProjCtx->>Auth: check permissions / pak validation
ProjCtx-->>McpReq: context holders populated
McpReq->>McpReq: feature checks & read-only guard
McpReq->>Tool: execute block()
Tool->>Service: perform domain operation
Service-->>Tool: result
Tool-->>McpReq: return CallToolResult
McpReq-->>Server: respond to client
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
⚔️ Resolve merge conflicts (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 |
BuildProperties requires the build-info Gradle plugin which isn't available in all CI modules, causing bean resolution failures. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
ProjectAuthorizationInterceptor now delegates to ProjectContextService, and FeatureAuthorizationInterceptor uses FeatureCheckService. Update tests to use the new constructor signatures with real service instances. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
- Merge create_key + batch_create_keys into single create_keys tool - Add delete_keys tool with destructive operation warning - Replace get_project_stats with get_project_language_statistics - Convert get_key, update_key, machine_translate, tag_keys to name-based lookups - Add languages filter to get_translations - Add branch parameter to store_big_meta - Update all MCP tests for new tool signatures Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Extract the repeated key-name-to-ID resolution pattern from BatchMcpTools, KeyMcpTools, and TagMcpTools into a shared KeyService.resolveKeysByName() extension in McpToolUtils. Also remove unused imports and fields. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Branch tools use @ConditionalOnClass to only register when the EE module is present, and requiredFeatures + ProjectFeatureGuard for runtime checks. Includes WithoutEe test to verify tools are absent in OSS builds. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
- Move AbstractMcpTest to :testing module (io.tolgee package) for cross-module access - Move McpBranchToolsTest to :ee-test module to avoid compilation failure in runWithoutEeTests - Centralize MCP SDK version in settings.gradle version catalog (libs.mcpBom, libs.mcp, libs.mcpSpringWebmvc) - Fix ktlint issues (import order in McpToolUtils, line length in TranslationMcpTools) Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 13
🤖 Fix all issues with AI agents
In `@backend/app/src/main/kotlin/io/tolgee/mcp/McpRequestContext.kt`:
- Around line 43-50: The non-global-route branch silently skips project/org
authorization when projectId is null; update McpRequestContext initialization so
that when spec.isGlobalRoute is false and projectId is null you fail fast (e.g.
throw a clear exception or call requireNotNull) instead of skipping
projectContextService.setup; change the logic around projectContextService.setup
in McpRequestContext (the isGlobalRoute / projectId check) to explicitly require
projectId for non-global routes and include a descriptive message referencing
projectId, so downstream methods like checkOrgRole and checkFeatures cannot be
bypassed.
In `@backend/app/src/main/kotlin/io/tolgee/mcp/tools/BatchMcpTools.kt`:
- Around line 70-82: The handler in BatchMcpTools.kt silently defaults required
parameters keyNames and targetLanguageTags to empty lists (via
request.arguments.getStringList(...)? : emptyList()), which can lead to
confusing behavior; update the mcpRequestContext.executeAs block to explicitly
validate that keyNames and targetLanguageTags are non-null and non-empty (use
the values from request.arguments.getStringList and check isNullOrEmpty()), and
if either is missing return a clear errorResult like "Missing required
parameter: keyNames" or "Missing required parameter: targetLanguageTags" before
calling keyService.resolveKeysByName or proceeding further.
- Around line 42-57: After retrieving the job view with
batchJobService.getView(jobId) inside the mcpRequestContext.executeAs block,
validate that the fetched job belongs to the requested projectId (compare
projectId to view.batchJob.projectId or use a batchJobService method that
fetches by both projectId and jobId) and return a 404/forbidden or equivalent
error response if they don’t match; update the handler around variables
projectId, jobId, mcpRequestContext.executeAs, and view.batchJob to perform this
check before constructing and returning the result.
In `@backend/app/src/main/kotlin/io/tolgee/mcp/tools/BranchMcpTools.kt`:
- Around line 29-42: Remove the redundant explicit feature checks: delete the
projectFeatureGuard.checkEnabled(Feature.BRANCHING) calls inside the
BranchMcpTools endpoint handlers that correspond to the tool specs (e.g., the
handler using listBranchesSpec and the two other BranchMcpTools specs that set
requiredFeatures = arrayOf(Feature.BRANCHING)); McpRequestContext.executeAs
already enforces requiredFeatures via checkFeatures, so simply remove those
explicit calls from the handler bodies to avoid duplicate validation.
In `@backend/app/src/main/kotlin/io/tolgee/mcp/tools/KeyMcpTools.kt`:
- Around line 137-156: The response currently uses keys.size (the number of keys
requested) which is misleading; update keyService.importKeys to return the
number of keys actually created (or a result object containing createdCount),
then in KeyMcpTools (inside the createKeysSpec block) capture that return value
when calling keyService.importKeys(keys, projectHolder.projectEntity, branch)
and use the returned createdCount in the JSON response passed to textResult
(e.g., {"created": true, "keyCount": createdCount}); adjust the
keyService.importKeys signature/implementation and any callers accordingly so
the reported keyCount reflects actual creations rather than the requested keys
list size.
In `@backend/app/src/main/kotlin/io/tolgee/mcp/tools/LanguageMcpTools.kt`:
- Around line 70-76: The handler in LanguageMcpTools constructs a
LanguageRequest with required fields but currently uses safe default empty
strings for name and tag, allowing invalid entities; update the construction of
LanguageRequest in the relevant method to fail fast by either using non-null
assertions (e.g., replace the `?: ""` uses for `name` and `tag` with `!!`) or
validate and return an errorResult(...) when those arguments are null, ensuring
required fields (name, tag) are not silently defaulted; keep the existing
`projectId` handling style and adjust any surrounding null-check logic
accordingly.
In `@backend/app/src/main/kotlin/io/tolgee/mcp/tools/TranslationMcpTools.kt`:
- Around line 104-135: The code currently defaults missing translations to
emptyMap() in the SetTranslationsWithKeyDto creation
(getStringMap("translations") ?: emptyMap()), which allows a silent no-op;
update the executeAs handler in TranslationMcpTools (the block that builds
SetTranslationsWithKeyDto) to validate that the "translations" argument is
present and non-empty before proceeding — if missing or empty, immediately call
errorResult with a clear message (e.g. "Missing or empty 'translations'
parameter") instead of continuing to
keyService.find/translationService.setForKey; keep using
SetTranslationsWithKeyDto, but only after the presence check to avoid performing
a pointless write.
- Around line 43-49: The code in TranslationMcpTools uses
request.arguments.getString("keyName") ?: "" which silently substitutes an empty
keyName and passes it into keyService.find; update both places (the key lookup
at the key variable and the set_translation usage) to explicitly validate that
request.arguments.getString("keyName") is non-null and non-blank and
return/throw a clear error (e.g., BadRequest/IllegalArgumentException or a
proper MCP error response) when missing instead of defaulting to "", then call
keyService.find with the validated value. Locate the occurrences by searching
for request.arguments.getString("keyName") and the surrounding keyService.find
call inside the TranslationMcpTools class and adjust the control flow to
validate before invoking keyService.find.
In `@backend/app/src/test/kotlin/io/tolgee/mcp/McpAuthenticationTest.kt`:
- Around line 92-134: The test method `callTool fails with expired PAT` uses a
no-op assertion `assertThat(e).isNotNull()` which doesn't verify the expected
failure; replace it with a precise assertion that the failure is the expected
authentication error (for example assert that the caught exception is an
instance of the specific exception thrown by `client.initialize()` or
`callTool`, or use an `assertThrows` around `client.initialize()`/`callTool`),
or assert on the exception message/HTTP status to confirm expired PAT; locate
this in `McpAuthenticationTest.kt` near the
`client.initialize()`/`callTool(client, "list_projects")` call and update the
catch block accordingly to assert the specific exception type/message instead of
a null check.
- Around line 59-90: The test method `callTool fails without auth` is masking
failures by catching any Exception and asserting the caught exception is
non-null; update the test to assert the precise expected behavior: either (A) if
an exception is expected, remove the try/catch and use JUnit's assertThrows or
AssertJ's assertThatThrownBy around the call to
`client.initialize()`/`callTool(client, "list_projects")` asserting the specific
exception type/message, or (B) if the client should return an error result
instead of throwing, remove the catch and assert directly that `result.isError`
is true (and validate error details), then finally close the `client` in a
finally block; reference `callTool`, `McpClient.sync(...).build()`, and the test
method name when making the change.
In `@backend/app/src/test/kotlin/io/tolgee/mcp/McpServerIntegrationTest.kt`:
- Around line 106-135: The catch block uses assertThat(e).isNotNull() which
always passes; change the test to explicitly assert the expected authentication
failure instead of asserting non-null: for example, replace the try/catch with
an assertThrows or assertThatThrownBy around the client.initialize() /
callTool(...) sequence and assert the specific exception type or message (e.g.,
an authentication/authorization exception) or, if using the returned result,
assert result.isError is true and validate the error code/message; apply the
same change to the expired PAT test so both `tool call with invalid PAT returns
error` and the expired PAT test validate the specific auth failure rather than
any exception.
In `@backend/app/src/test/kotlin/io/tolgee/mcp/tools/McpProjectToolsTest.kt`:
- Around line 55-70: The test
McpProjectToolsTest::`get_project_language_statistics returns per-language
stats` currently allows passing when the returned JSON array is empty because
the field assertions are inside an if(json.size() > 0) guard; update the test to
assert the array is non-empty (or assert its size equals the expected language
count from the test fixture) before checking element fields. Specifically, after
calling callToolAndGetJson(client, "get_project_language_statistics",
mapOf("projectId" to data.projectId)) assert json.isArray and assert json.size()
> 0 (or compare to expectedLanguagesCount), then proceed to check that json[0]
has "languageTag", "translatedPercentage", "reviewedPercentage", and
"untranslatedPercentage".
In `@settings.gradle`:
- Around line 78-80: Update the pinned MCP BOM version in the dependency
catalog: change the library declaration for 'mcpBom' (the call library('mcpBom',
'io.modelcontextprotocol.sdk', 'mcp-bom')) to use version '0.17.0' instead of
'0.17.2' so the build resolves from Maven Central.
🧹 Nitpick comments (16)
backend/security/src/main/kotlin/io/tolgee/security/ProjectContextService.kt (1)
60-101: RedundantcanBypasscheck when scopes are empty and bypass is already granted.When
scopes.isEmpty()succeeds andbypassedis set totrue(line 80), the code falls through into therequiredScopesblock (line 83), re-computesmissing(which will be all required scopes sincescopesis empty), and callscanBypassagain. Consider short-circuiting:♻️ Proposed fix
bypassed = true } - if (requiredScopes != null) { + if (!bypassed && requiredScopes != null) { val missing = requiredScopes.toSet() - scopesbackend/security/src/test/kotlin/io/tolgee/security/authorization/ProjectAuthorizationInterceptorTest.kt (1)
135-148: Consider resetting the additional mocks for consistency.
projectHolder,organizationHolder,activityHolder, andtracingContextare not included inresetMocks. While these mocks don't accumulate stateful interactions that would cause test interference, including them would be consistent with the existing pattern and prevent subtle issues if future tests addverify()calls on them.♻️ Proposed fix
fun resetMocks() { Mockito.reset( authenticationFacade, authentication, organizationService, securityService, requestContextService, + projectHolder, + organizationHolder, + activityHolder, + tracingContext, project, projectDto, userAccount, apiKey, ) }backend/app/src/main/kotlin/io/tolgee/mcp/ToolEndpointSpec.kt (2)
33-45:Arrayfields in adata classbreakequals/hashCode.Kotlin data classes use referential equality for arrays, so two
ToolEndpointSpecinstances with identical array contents won't be considered equal. If this type is ever used in collections, assertions, or deduplication, it will silently misbehave.If structural equality isn't needed (specs are only constructed and passed around), this is fine as-is. Otherwise, consider switching to
List<Scope>/List<Feature>or overridingequals/hashCode.
67-102: Non-global routes should validate the presence of a permission annotation in buildSpecFromMethod.
ProjectAuthorizationInterceptor.getRequiredScopesthrows at runtime when neither@UseDefaultPermissionsnor@RequiresProjectPermissionsis present. The MCPbuildSpecFromMethoddoesn't replicate this guard; a misconfigured MCP tool referencing a non-global project route could silently skip authorization —ProjectContextService.setupwould be called withrequiredScopes=nullanduseDefaultPermissions=false, bypassing all scope checks.Add validation for defense in depth:
Proposed validation
+ val isGlobalRoute = AnnotationUtils.getAnnotation(method, IsGlobalRoute::class.java) != null + val useDefaultPermissions = AnnotationUtils.getAnnotation(method, UseDefaultPermissions::class.java) != null + val requiredScopes = AnnotationUtils.getAnnotation(method, RequiresProjectPermissions::class.java)?.scopes + + if (!isGlobalRoute && requiredScopes == null && !useDefaultPermissions) { + error("MCP tool $mcpOperation references ${method.name} which is not a global route but lacks both `@UseDefaultPermissions` and `@RequiresProjectPermissions`") + } + return ToolEndpointSpec( mcpOperation = mcpOperation, - requiredScopes = AnnotationUtils.getAnnotation(method, RequiresProjectPermissions::class.java)?.scopes, - useDefaultPermissions = AnnotationUtils.getAnnotation(method, UseDefaultPermissions::class.java) != null, - isGlobalRoute = AnnotationUtils.getAnnotation(method, IsGlobalRoute::class.java) != null, + requiredScopes = requiredScopes, + useDefaultPermissions = useDefaultPermissions, + isGlobalRoute = isGlobalRoute,backend/app/src/main/kotlin/io/tolgee/mcp/tools/McpToolUtils.kt (1)
184-195:getStringListandgetStringMapskip element-level type checks, unlikegetLongList.Due to JVM type erasure,
this[key] as? List<String>only checksListat runtime, not the element types. If the deserialized JSON contains unexpected element types, you'll get a deferredClassCastExceptionat the usage site rather than a clean null/empty result.getLongListalready handles this correctly withmapNotNulland per-element casting.Consider applying the same defensive pattern for consistency:
Proposed fix
-@Suppress("UNCHECKED_CAST") -fun Map<String, Any?>.getStringList(key: String): List<String>? = this[key] as? List<String> +fun Map<String, Any?>.getStringList(key: String): List<String>? = + (this[key] as? List<*>)?.mapNotNull { it as? String }backend/app/src/test/kotlin/io/tolgee/mcp/tools/McpLanguageToolsTest.kt (1)
44-60:list_namespacestest is in the wrong test class.This test exercises the
list_namespacestool (provided byNamespaceMcpTools), but it lives inMcpLanguageToolsTest. Consider moving it to a dedicatedMcpNamespaceToolsTestto keep test organization aligned with the tool providers.backend/app/src/test/kotlin/io/tolgee/mcp/McpWithoutEeTest.kt (1)
9-20:eePresentcheck may be unnecessary given@WithoutEeTest.Since the class is annotated with
@WithoutEeTest, the EE module should never be on the classpath when this test runs. TheeePresentflag and the conditional logic inbranch tools are only available with EE modulewould then always take theelsebranch. This effectively means theif (eePresent)path is dead code under normal execution.If
@WithoutEeTestguarantees no EE, you can simplify to always assert absence:Simplified assertion
- if (eePresent) { - assertThat(toolNames).containsAll(eeBranchTools) - } else { - assertThat(toolNames).doesNotContainAnyElementsOf(eeBranchTools) - } + assertThat(toolNames).doesNotContainAnyElementsOf(eeBranchTools)Also applies to: 69-83
backend/app/src/test/kotlin/io/tolgee/mcp/tools/spec/ExecutionOrderTest.kt (1)
21-30:organizationRoleServiceis included in theinOrderverifier but never verified.
organizationRoleServiceis passed toMockito.inOrder(...)at Line 26 but noinOrder.verify(organizationRoleService)call appears in the verification block. This doesn't cause a failure, but it's misleading — it suggests the test intends to verify its ordering relative to other collaborators. Either add the missing verification step (e.g., between step 4 and step 5), or remove it from theinOrderconstructor to keep the test's intent clear.backend/app/src/main/kotlin/io/tolgee/mcp/McpConfig.kt (1)
46-52: The@Suppress("unused")dependency-ordering trick could use a brief comment.The
mcpServerparameter exists solely to force Spring to initialize theMcpSyncServerbean before exposing the router. A one-line comment would save the next reader from wondering why it's there.Proposed clarification
`@Bean` fun mcpRouterFunction( transportProvider: WebMvcStreamableServerTransportProvider, - `@Suppress`("unused") mcpServer: McpSyncServer, + `@Suppress`("unused") mcpServer: McpSyncServer, // injected to ensure server is initialized before exposing transport ): RouterFunction<ServerResponse> { return transportProvider.routerFunction }backend/app/src/test/kotlin/io/tolgee/mcp/tools/McpKeyToolsTest.kt (1)
128-132: Minor: NPE on Line 132 shadows the assertion failure on Line 131.If
keyService.findreturns null, theassertThat(key).isNotNull()on Line 131 will fail with a clear message. However, AssertJ's soft assertion behavior isn't in play here — the!!on Line 132 will never be reached if Line 131 fails. That said, a safer pattern avoids the!!:Suggested improvement
- val key = keyService.find(data.projectId, "new.name", null) - assertThat(key).isNotNull() - assertThat(key!!.name).isEqualTo("new.name") + val key = keyService.find(data.projectId, "new.name", null) + assertThat(key).isNotNull + assertThat(key?.name).isEqualTo("new.name")backend/app/src/main/kotlin/io/tolgee/mcp/tools/TagMcpTools.kt (1)
36-52:list_tagssilently truncates at 1000 results with no indication to the caller.If a project has more than 1000 tags, the excess tags are dropped. Consider either returning a
hasMoreflag (likelist_keysdoes) or documenting the limit in the tool description.Suggested description update
server.addTool( "list_tags", - "List all tags used in a Tolgee project", + "List tags used in a Tolgee project (up to 1000 results)", toolSchema {backend/app/src/main/kotlin/io/tolgee/mcp/tools/ProjectMcpTools.kt (1)
79-111:create_project: fallback values for required fields could produce confusing errors.Lines 94-95: When
namedefaults to""andorganizationIddefaults to0, the resulting errors from downstream validation may be unclear to the MCP caller (e.g., "organization not found" for ID 0, or a validation error for empty name). Since both fields arerequired = truein the schema, this is unlikely to occur in practice, but failing fast with a clear message would be preferable.Suggested improvement
- val dto = - CreateProjectRequest( - name = args.getString("name") ?: "", - organizationId = args.getLong("organizationId") ?: 0, - languages = languages, - baseLanguageTag = args.getString("baseLanguageTag"), - ) + val name = args.getString("name") ?: return@executeAs errorResult("Missing required field: name") + val organizationId = args.getLong("organizationId") ?: return@executeAs errorResult("Missing required field: organizationId") + val dto = + CreateProjectRequest( + name = name, + organizationId = organizationId, + languages = languages, + baseLanguageTag = args.getString("baseLanguageTag"), + )backend/app/src/main/kotlin/io/tolgee/mcp/tools/BigMetaMcpTools.kt (1)
38-57:relatedKeysInOrdercan be null despite being required, and would be passed tobigMetaService.store.If
getList("relatedKeysInOrder")returnsnull(e.g., due to a client bug or schema validation gap),relatedKeyswill benull,dto.relatedKeysInOrderwill be set tonull, andbigMetaService.storewill be called with a potentially invalid DTO. The response would still report"stored": true, "keyCount": 0.Consider adding an early return with an error for this case:
Suggested guard
val relatedKeys = request.arguments .getList("relatedKeysInOrder") ?.map { k -> RelatedKeyDto( keyName = k.getString("keyName") ?: "", namespace = k.getString("namespace"), branch = branch, ) }?.toMutableList() + if (relatedKeys.isNullOrEmpty()) { + return@executeAs errorResult("relatedKeysInOrder is required and must not be empty") + } val dto = BigMetaDto() dto.relatedKeysInOrder = relatedKeysbackend/app/src/main/kotlin/io/tolgee/mcp/tools/LanguageMcpTools.kt (1)
36-54: No pagination signal forlist_languages— unlikelist_keys.
list_keys(inKeyMcpTools.kt, lines 69-80) providestotalKeys,hasMore, and ahintwhen results are truncated.list_languagessilently caps at 1000 with no indication to the caller. While 1000 languages is unlikely to be exceeded, keeping the response shape consistent across tools helps AI assistants handle truncation uniformly.ee/backend/tests/src/test/kotlin/io/tolgee/ee/mcp/McpBranchToolsTest.kt (1)
24-31: DoublesaveTestDatacall — consider consolidating.
createTestDataWithPat()already callstestDataService.saveTestData(base.root)internally (seeAbstractMcpTest.ktline 69). Line 28 here saves again after settinguseBranching = true. This works but is potentially fragile — ifsaveTestDatais not idempotent for all entity types, it could cause issues. Consider either parameterizingcreateTestDataWithPatto accept a project customizer block, or extracting a variant that defers the save.backend/app/src/main/kotlin/io/tolgee/mcp/tools/KeyMcpTools.kt (1)
192-233:update_keyrequiresnewNameeven when only updating description — consider making it optional.The schema declares
newNameasrequired = true(line 200) with the hint "provide the current name if unchanged". This forces the AI assistant to always know and supply the current name, even when only updating the description or namespace. MakingnewNameoptional and defaulting to the existing name when absent would simplify the tool's usage for partial updates.
backend/app/src/main/kotlin/io/tolgee/mcp/tools/BranchMcpTools.kt
Outdated
Show resolved
Hide resolved
JanCizmar
left a comment
JanCizmar
left a comment
There was a problem hiding this comment.
Awesome job thanks a lot! The code reads well.
I am a bit scared about the security ascpect. There are proper tests for everything, but maybe I would ask Claude to generate tests methods without mocking that would actually check the permissions for all the MCP Tools individually, so we are sure it works end-to-end. Also I would test that correct responses are returned from endpoints when Auth fails. So for multiple endpoints I woudl test if feature checks work, org role, project scopes, rate limits, etc.
backend/security/src/main/kotlin/io/tolgee/security/authorization/FeatureCheckService.kt
Outdated
Show resolved
Hide resolved
backend/app/src/main/kotlin/io/tolgee/mcp/tools/BatchMcpTools.kt
Outdated
Show resolved
Hide resolved
backend/app/src/main/kotlin/io/tolgee/mcp/tools/McpToolUtils.kt
Outdated
Show resolved
Hide resolved
backend/app/src/main/kotlin/io/tolgee/mcp/tools/BigMetaMcpTools.kt
Outdated
Show resolved
Hide resolved
| assertThatThrownBy { | ||
| buildSpec(TestController::class.java, "superAuth", "bad_tool") | ||
| }.isInstanceOf(IllegalArgumentException::class.java) | ||
| .hasMessageContaining("requires super authentication") |
There was a problem hiding this comment.
no. These are startup-time validation errors. The specs are built at the starup of the app, so that they don't do reflections in runtime. So it's like a message to the developer, that "you're trying to build an mcp tool for the wrong method"
backend/app/src/test/kotlin/io/tolgee/mcp/McpAuthenticationTest.kt
Outdated
Show resolved
Hide resolved
backend/app/src/main/kotlin/io/tolgee/mcp/tools/BatchMcpTools.kt
Outdated
Show resolved
Hide resolved
…ools
Replace all `getLong("projectId")!!` with `getProjectId()` helper that
throws ProjectNotSelectedException. Also fail fast in McpRequestContext
when a non-global route is called without projectId, preventing silent
bypass of authorization checks.
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Prevents cross-project information disclosure where a user with access to one project could query batch job status from another project by guessing job IDs. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
…llbacks Add require* helpers (requireString, requireLong, requireStringList, requireStringMap, requireList) that throw BadRequestException with REQUEST_VALIDATION_ERROR when a required parameter is missing. Replace all silent fallbacks (?: "", ?: emptyList(), ?: emptyMap(), !!) for required parameters across all MCP tools. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
… value Replace weak try/catch with assertThrows in McpAuthenticationTest. Add REQUEST_VALIDATION_ERROR to Message enum (needed by require* helpers). Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
…pping Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Aligns naming with ProjectFeatureGuard and drops misleading "Service" suffix since it's a @component, not a @service. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
…riptions Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Add optional page parameter (0-based) to list_projects, list_keys,
search_keys, list_tags, list_languages, and list_branches. All paginated
responses now return a consistent shape: {items, page, totalPages, totalItems}.
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
…ping Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Remove languageService dependency from TranslationMcpTools. Use translationService.findForKeyByLanguages and new getAllByKeyId instead of manually fetching all languages and filtering. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
The test expected null projectId to silently skip setup, but the code correctly throws ProjectNotSelectedException. Updated test to match. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Use buildSpec with Class.forName to derive specs from BranchController annotations instead of manually constructing ToolEndpointSpec. Fixes allowedTokenType (was ONLY_PAT, now correctly ANY). Also change delete_branch to accept branchName instead of branchId. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Keep security-critical fields required (mcpOperation, requiredScopes, allowedTokenType, isWriteOperation) while defaulting the rest. Simplify existing manual constructors in tests. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
The hardcoded list of 22 tool names breaks whenever tools are added or removed. The existing listTools-returns-tools test already verifies tools are returned. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Add createMcpClientWithPak/createTestDataWithPak to AbstractMcpTest. Rename createMcpClient to createMcpClientWithPat, McpTestData to McpPatTestData. Switch project-scoped tool tests to use PAK auth. Keep PAT for global-route tests (list_projects, create_project). Add PAK auth tests to McpAuthenticationTest. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
When authenticated with a Project API Key (PAK), the project is encoded in the token — requiring callers to also pass projectId is redundant. McpRequestContext.executeAs() now resolves projectId from the PAK when not explicitly provided, matching how REST endpoints resolve the project via ProjectContextService. All tool schemas mark projectId as optional. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Verifies that per-tool scope gating (KEYS_VIEW, KEYS_CREATE, KEYS_EDIT, KEYS_DELETE, LANGUAGES_EDIT, TRANSLATIONS_EDIT) and PAK-vs-PAT token restrictions work end-to-end through the real security chain without mocking. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Fix all issues with AI agents
In `@backend/app/src/main/kotlin/io/tolgee/mcp/tools/BatchJobMcpTools.kt`:
- Around line 42-60: The call to batchJobService.getView(jobId) inside
mcpRequestContext.executeAs can throw NotFoundException and currently
propagates; wrap the getView call (and the subsequent project ownership check)
in a try-catch that catches NotFoundException and returns an MCP error result
via return@executeAs errorResult("Batch job not found"), preserving the existing
ownership check that uses projectHolder and the current errorResult("Batch job
$jobId does not belong to this project") for mismatched projects; ensure the
rest of the response-building (id, status, type, progress, totalItems,
errorMessage) remains inside the try block so only missing-job errors are
converted to an MCP error result.
In
`@backend/security/src/main/kotlin/io/tolgee/security/authorization/OrganizationFeatureGuard.kt`:
- Around line 23-31: The checkOneOfFeaturesEnabled function currently treats an
empty features array as false and throws; update checkOneOfFeaturesEnabled to
early-return (no-op) when features.isEmpty to match checkFeaturesEnabled
behavior: add a guard at the start of checkOneOfFeaturesEnabled using
features.isEmpty, then only evaluate features.any {
enabledFeaturesProvider.isFeatureEnabled(organizationId, it) } and throw
BadRequestException(Message.FEATURE_NOT_ENABLED, features.toList()) when the
array is non-empty and none are enabled.
In `@ee/backend/tests/src/test/kotlin/io/tolgee/ee/mcp/McpBranchToolsTest.kt`:
- Around line 24-31: Add an `@AfterEach` teardown to McpBranchToolsTest that
resets enabledFeaturesProvider.forceEnabled to null; locate the test class
McpBranchToolsTest and add a method annotated with `@AfterEach` (e.g., tearDown or
cleanup) that sets enabledFeaturesProvider.forceEnabled = null to avoid leaking
the forced feature flag set in the `@BeforeEach`.
🧹 Nitpick comments (11)
backend/data/src/main/kotlin/io/tolgee/service/key/KeyService.kt (1)
549-562: N+1 query pattern: each name triggers a separate DB call.
find()is called in a loop for every name, issuing a separate query per key. For small MCP batches this is likely acceptable, but consider a batch repository query (e.g.,findAllByProjectIdAndNameInAndNamespace) if usage grows.backend/testing/src/main/kotlin/io/tolgee/AbstractMcpTest.kt (1)
122-124:callToolAndGetJsonwill throw an unhelpful error if the tool returns no content or non-text content.
result.content().first()throwsNoSuchElementExceptionon empty content, and the cast toTextContentthrowsClassCastExceptionfor other content types. Adding a descriptive assertion or check would make test failures easier to diagnose.Proposed improvement
fun callToolAndGetJson( client: McpSyncClient, name: String, arguments: Map<String, Any?> = emptyMap(), ): JsonNode { val result = callTool(client, name, arguments) - val textContent = result.content().first() as McpSchema.TextContent + val content = result.content() + assertThat(content).isNotEmpty + val first = content.first() + assertThat(first).isInstanceOf(McpSchema.TextContent::class.java) + val textContent = first as McpSchema.TextContent return objectMapper.readTree(textContent.text()) }backend/app/src/test/kotlin/io/tolgee/mcp/tools/spec/ExecutionOrderTest.kt (1)
21-30:organizationRoleServiceis included ininOrderbut never verified.Line 26 adds
organizationRoleServiceto theinOrderverifier, but noinOrder.verify(organizationRoleService)call appears in the verification block (lines 64–79). Either the org-role check step is missing from the assertions, or the mock should be removed from theinOrderlist to avoid confusion about what this test actually guarantees.backend/app/src/main/kotlin/io/tolgee/mcp/tools/BranchMcpTools.kt (1)
29-34:Class.forNamein companion object is a startup risk if the class layout changes.These reflective lookups run at class-load time. If the fully-qualified class names change (e.g., package reorganization in the EE module), this will fail with
ClassNotFoundExceptionat startup. The@ConditionalOnClassguard only checks forBranchServiceImpl, not forBranchControllerorCreateBranchModel. A mismatch would cause a hard failure.Consider adding a brief comment documenting this coupling, or unifying the conditional check to cover all three classes.
backend/app/src/main/kotlin/io/tolgee/mcp/McpRequestContext.kt (1)
101-123: Silent return whenorganizationOrNullisnullis correct but consider a defensive log.Both
checkOrgRole(line 104) andcheckFeatures(line 114) silently return viaorganizationOrNull?.id ?: returnwhen the organization is not set. This mirrors the HTTP interceptors, but for MCP it could mask configuration issues where a non-global tool unexpectedly has no org context. A debug/trace-level log on these early returns would help troubleshoot authorization misconfigurations without changing behavior.backend/app/src/main/kotlin/io/tolgee/mcp/tools/TagMcpTools.kt (1)
57-91:tag_keysuses all-or-nothing error semantics — confirm this is intentional.If any key name in the request is not found, the entire operation returns an error and no keys are tagged (line 75-76). This is a safe default for consistency, but the tool description says "Add tags to translation keys" without mentioning partial-failure behavior. Consider documenting this in the tool description so AI assistants know to verify key names before calling.
Proposed description update
server.addTool( "tag_keys", - "Add tags to translation keys. Creates tags if they don't exist.", + "Add tags to translation keys. Creates tags if they don't exist. All specified key names must exist — the operation fails entirely if any key is not found.", toolSchema {backend/app/src/main/kotlin/io/tolgee/mcp/tools/KeyMcpTools.kt (1)
222-251:delete_keysall-or-nothing semantics should be documented in the tool description.Similar to the
tag_keystool,delete_keysfails entirely if any key name is not found. The current description doesn't mention this — an AI assistant might not know to pre-validate key existence. Consider adding this to the tool description.Suggested update
"delete_keys", "Delete translation keys from a Tolgee project. " + - "IMPORTANT: This is a destructive operation. The AI assistant should always confirm with the user before calling this tool.", + "IMPORTANT: This is a destructive operation. The AI assistant should always confirm with the user before calling this tool. " + + "All specified key names must exist — the operation fails entirely if any key is not found.",backend/app/src/main/kotlin/io/tolgee/mcp/ToolEndpointSpec.kt (2)
33-45:data classwithArrayfields breaksequals/hashCodecontracts.
ToolEndpointSpecis adata classcontainingArray<Scope>?,Array<out Feature>?, etc. Kotlindata classgeneratesequals/hashCodeusing reference equality for arrays, so two structurally identical specs won't be equal. If this type is ever compared, used as a map key, or deduplicated, it will silently misbehave.Options: switch arrays to
List/Set, overrideequals/hashCode, or convert to a plainclassif structural equality isn't needed.
93-101:isWriteOperationfallback uses directischecks — won't match meta-annotations.The fallback at line 98 checks
it is PostMapping || …on raw annotations. If a controller method uses a composed annotation (e.g., a custom annotation meta-annotated with@PostMapping), this won't detect it. The rest of the builder usesAnnotationUtils.getAnnotationwhich handles meta-annotations. Consider usingAnnotationUtils.findAnnotationfor consistency, or document this as intentional.ee/backend/tests/src/test/kotlin/io/tolgee/ee/mcp/McpBranchToolsTest.kt (1)
86-119:delete_branchtest relies oncreate_branchsucceeding — consider using service-level setup.If
create_branchfails, this test fails with a misleading error. For robustness, consider creating the branch directly viabranchServicein the arrange step, isolating the delete behavior being tested. That said, this is a common integration test pattern and acceptable here.backend/app/src/main/kotlin/io/tolgee/mcp/tools/SchemaBuilder.kt (1)
6-7: Consider restricting mutability ofpropertiesandrequiredFields.These are
valbut their underlying collections are mutable and publicly accessible, meaning callers could modify them directly afterbuild()is called, or add entries bypassing the convenience methods. Since this is an internal builder, it's low-risk, but using a backingprivatemutable field with a public read-only view (or just making themprivate) would be cleaner.
backend/security/src/main/kotlin/io/tolgee/security/authorization/OrganizationFeatureGuard.kt
Show resolved
Hide resolved
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Early-return when features is empty to match checkFeaturesEnabled behavior instead of incorrectly throwing FEATURE_NOT_ENABLED. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@backend/app/src/test/kotlin/io/tolgee/mcp/McpTokenTypeEnforcementTest.kt`:
- Around line 52-91: The test `PAK rejected when accessing different project`
builds project_a inline but omits creating the default "main" branch which other
helpers (e.g., createTestDataWithPak/createTestDataWithPat) add; add a default
branch for project_a (mirror how other test builders add a branch named "main"
and mark it default) before saving test data so MCP tool
resolution/authorization follows the same path—look for the project builder
block that adds languages/permissions and insert an addBranch { name = "main";
isDefault = true } (or the equivalent branch-creation call used elsewhere) for
project_a.
In `@ee/backend/tests/src/test/kotlin/io/tolgee/ee/mcp/McpBranchToolsTest.kt`:
- Around line 25-32: The test currently calls createTestDataWithPak() (which
already saves via testDataService.saveTestData in AbstractMcpTest) and then
mutates data.testData.projectBuilder.self.useBranching before calling
saveTestData again; change this to set useBranching before the single save by
either adding a parameter to createTestDataWithPak(useBranching: Boolean) so the
builder is created with useBranching=true inside that method (and thus saved
once), or update createTestDataWithPak's construction path to enable branching
on the projectBuilder.self before testDataService.saveTestData is invoked; keep
the rest of setup (enabledFeaturesProvider.forceEnabled and
createMcpClientWithPak) unchanged.
🧹 Nitpick comments (6)
backend/app/src/main/kotlin/io/tolgee/mcp/McpRequestContext.kt (2)
101-123: Silent early-return incheckOrgRole/checkFeatureswhen org is absent on global routes is intentional but fragile.Both
checkOrgRole(Line 104) andcheckFeatures(Line 114) silently return whenorganizationOrNullisnull. For global routes this is correct (no project → no org). However, if a non-global route'sprojectContextService.setupever fails to populateorganizationHolder(e.g., due to a future refactor), these checks are silently bypassed.Consider adding a defensive assertion inside
executeAsafterprojectContextService.setupto confirmorganizationHolderwas populated for non-global routes:🛡️ Suggested defensive check
projectContextService.setup( resolvedProjectId, spec.requiredScopes, spec.useDefaultPermissions, spec.isWriteOperation, ) + check(organizationHolder.organizationOrNull != null) { + "organizationHolder was not populated after project context setup for '${spec.mcpOperation}'" + } }
138-141:businessEventDatais populated unconditionally, but downstream guards prevent spurious events.
setupActivityreturns early whenspec.activityTypeis null, butemitPostHogEventalways writes toactivityHolder.businessEventData["mcp"]andactivityHolder.businessEventData["mcp_operation"]. However,BusinessEventReportingActivityListenerchecksevent.activityRevision.type?.nameand returns early if the activity type is null, preventing event emission. The unnecessary map population when no event will be emitted is harmless but could be optimized to only populatebusinessEventDatawhenspec.activityTypeis set.backend/app/src/main/kotlin/io/tolgee/mcp/tools/KeyMcpTools.kt (3)
234-253:delete_keysuses all-or-nothing semantics — consider if partial deletes would be more useful.If any key name is not found, the entire operation fails with an error (Line 241–242) and no keys are deleted. This is a safe default, but an AI assistant trying to clean up keys may be blocked by a single typo or already-deleted key. Consider whether a partial-success response (reporting both deleted and not-found keys) would be more practical for the MCP use case.
Also, similar to
create_keys,keyCounthere accurately reflects deletions since the all-or-nothing check ensures all resolved keys exist — that's consistent at least.
44-253: Theregistermethod is ~210 lines long with 6 inline tool definitions.Each tool registration is self-contained, so this reads linearly, but extracting each tool into a private method (e.g.,
registerListKeys(server),registerSearchKeys(server), etc.) would improve navigability and testability of individual tools.
194-222:update_key:newNameis required even when only updating namespace or description.The schema (Line 190) marks
newNameas required, and the description says "provide the current name if unchanged." This forces the AI assistant to always echo back the current name even when only changing the description. A more ergonomic API would makenewNameoptional and default to the current name server-side, reducing the chance of accidental renames.♻️ Suggested approach
- string("newName", "New key name (provide the current name if unchanged)", required = true) + string("newName", "Optional: new key name (defaults to current name if omitted)")Then in the handler:
val dto = EditKeyDto( - name = request.arguments.requireString("newName"), + name = request.arguments.getString("newName") ?: key.name, namespace = request.arguments.getString("newNamespace"), description = request.arguments.getString("newDescription"), )ee/backend/tests/src/test/kotlin/io/tolgee/ee/mcp/McpBranchToolsTest.kt (1)
48-57: Consider importingMcpErrorand usingassertThrowsidiomatically.The inline fully-qualified references to
org.junit.jupiter.api.assertThrowsandio.modelcontextprotocol.spec.McpErrorreduce readability. A top-level import would keep this block cleaner.✏️ Suggested cleanup
Add to imports:
import io.modelcontextprotocol.spec.McpError import org.junit.jupiter.api.assertThrowsThen simplify the test body:
- assertThat( - org.junit.jupiter.api - .assertThrows<io.modelcontextprotocol.spec.McpError> { - callTool(client, "list_branches", mapOf("projectId" to data.projectId)) - }.message, - ).contains("feature_not_enabled") + val error = assertThrows<McpError> { + callTool(client, "list_branches", mapOf("projectId" to data.projectId)) + } + assertThat(error.message).contains("feature_not_enabled")
backend/app/src/test/kotlin/io/tolgee/mcp/McpTokenTypeEnforcementTest.kt
Show resolved
Hide resolved
|
@JanCizmar added 2 tests without mocks: |
2 participants
Summary by CodeRabbit
New Features
Documentation
Tests