feat(mcp): scope completion to authorized Assets and sanitize tool errors - #121
Conversation
…rors Publish MCP completion for the released Prompt argument and both Asset resource templates. Every suggestion comes from one authorized Asset delivery call for the current identity, an already-resolved argument narrows the next one, results are capped with total/hasMore, and a delivery failure yields no suggestion instead of an error so completion cannot become a second existence channel. Keep the downstream cause off the MCP boundary. The annotation runtime appends the root cause message to the tool error it returns, so an unresolved host or request URL reached the caller. The boundary now logs the cause and rethrows a cause-free failure carrying only the already sanitized gateway message. Reconcile the assistant-and-mcp spec and test matrix, which still claimed the surface was one search tool and that the inbound bearer was forwarded downstream. Verified with gradlew :apps:mcp:test (46 tests) and gradlew compileJava. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…mmit The spec and test matrix now describe the completion surface and the sanitized error boundary introduced in 2f6b0f8, so the reconciliation marker moves to that commit. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
Warning Review limit reached
Next review available in: 32 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Plus Run ID: ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (3)
📝 WalkthroughWalkthroughAdds MCP completion support for authorized asset and release values, enables completion capabilities, and introduces sanitized cause-free failure handling across asset, knowledge, resource, and prompt operations with corresponding tests and architecture documentation. ChangesMCP completion and failure handling
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant MCPClient
participant AssetCompletionAdapter
participant McpApiAuthorization
participant AssetDeliveryApiClient
MCPClient->>AssetCompletionAdapter: Request completion for an argument
AssetCompletionAdapter->>McpApiAuthorization: Resolve authorized identity
McpApiAuthorization-->>AssetCompletionAdapter: Authorized context
AssetCompletionAdapter->>AssetDeliveryApiClient: Search readable assets
AssetDeliveryApiClient-->>AssetCompletionAdapter: Asset and release summaries
AssetCompletionAdapter-->>MCPClient: Filtered paged completions
Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
@CodeRabbit review |
✅ Action performedReview finished.
|
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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 `@apps/mcp/src/main/java/com/orgmemory/mcp/AssetCompletionAdapter.java`:
- Around line 138-150: Update the completion flow so authorized() receives the
typed prefix and passes it as the query argument to assets.search when
non-empty, while retaining null for an empty prefix. Verify
AssetDeliveryApiClient.search supports prefix narrowing without removing the
existing title/namespace/slug/versionLabel filtering performed by matches() or
breaking AssetCompletionAdapterTests.
In `@apps/mcp/src/main/java/com/orgmemory/mcp/AssetDeliveryResources.java`:
- Around line 37-58: Update getAsset and getAssetRelease so
JsonProcessingException is caught inside each lambda before invoking
McpFailureBoundary.sanitized. Convert the checked serialization failure into
AssetDeliveryGatewayException, while preserving the existing authorization,
asset lookup, release lookup, and sanitized boundary behavior.
🪄 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: ASSERTIVE
Plan: Pro Plus
Run ID: c0aa0a7a-064d-4787-bae6-c3ca8b21c680
⛔ Files ignored due to path filters (2)
docs/specs/domains/assistant-and-mcp.mdis excluded by!docs/**docs/tests/domains/assistant-and-mcp.mdis excluded by!docs/**
📒 Files selected for processing (11)
ARCHITECTURE.mdapps/mcp/src/main/java/com/orgmemory/mcp/AssetCompletionAdapter.javaapps/mcp/src/main/java/com/orgmemory/mcp/AssetDeliveryResources.javaapps/mcp/src/main/java/com/orgmemory/mcp/AssetDeliveryTools.javaapps/mcp/src/main/java/com/orgmemory/mcp/KnowledgeSearchTool.javaapps/mcp/src/main/java/com/orgmemory/mcp/McpFailureBoundary.javaapps/mcp/src/main/java/com/orgmemory/mcp/ReleasedPromptAdapter.javaapps/mcp/src/main/resources/application.ymlapps/mcp/src/test/java/com/orgmemory/mcp/AssetCompletionAdapterTests.javaapps/mcp/src/test/java/com/orgmemory/mcp/McpToolErrorSurfaceTests.javaapps/mcp/src/test/java/com/orgmemory/mcp/OrgMemoryMcpContextTests.java
📜 Review details
🧰 Additional context used
📓 Path-based instructions (5)
**/*
📄 CodeRabbit inference engine (CLAUDE.md)
**/*: Always read the repository guidance and relevant sections ofARCHITECTURE.md; before changing a domain, read its specification, test-coverage document, and binding decision filenames.
Treat the repository as the engineering system of record; current repository and runtime evidence take precedence over chat or Northstar.
Readdocs/guidelines/agent-safety.mdbefore retrieval, AI, MCP, permission, upload, graph, or export work. Never commit secrets or customer data.
Files:
apps/mcp/src/test/java/com/orgmemory/mcp/McpToolErrorSurfaceTests.javaapps/mcp/src/main/java/com/orgmemory/mcp/McpFailureBoundary.javaapps/mcp/src/main/java/com/orgmemory/mcp/KnowledgeSearchTool.javaARCHITECTURE.mdapps/mcp/src/main/java/com/orgmemory/mcp/ReleasedPromptAdapter.javaapps/mcp/src/main/resources/application.ymlapps/mcp/src/main/java/com/orgmemory/mcp/AssetCompletionAdapter.javaapps/mcp/src/main/java/com/orgmemory/mcp/AssetDeliveryResources.javaapps/mcp/src/test/java/com/orgmemory/mcp/AssetCompletionAdapterTests.javaapps/mcp/src/main/java/com/orgmemory/mcp/AssetDeliveryTools.javaapps/mcp/src/test/java/com/orgmemory/mcp/OrgMemoryMcpContextTests.java
**/*.{java,gradle,gradle.kts,properties,yml,yaml}
📄 CodeRabbit inference engine (CLAUDE.md)
Before using unfamiliar Spring Boot 4, Spring Modulith 2, Spring AI 2, or Gradle APIs, consult current official documentation, Context7, and the relevant project verification skill.
Files:
apps/mcp/src/test/java/com/orgmemory/mcp/McpToolErrorSurfaceTests.javaapps/mcp/src/main/java/com/orgmemory/mcp/McpFailureBoundary.javaapps/mcp/src/main/java/com/orgmemory/mcp/KnowledgeSearchTool.javaapps/mcp/src/main/java/com/orgmemory/mcp/ReleasedPromptAdapter.javaapps/mcp/src/main/resources/application.ymlapps/mcp/src/main/java/com/orgmemory/mcp/AssetCompletionAdapter.javaapps/mcp/src/main/java/com/orgmemory/mcp/AssetDeliveryResources.javaapps/mcp/src/test/java/com/orgmemory/mcp/AssetCompletionAdapterTests.javaapps/mcp/src/main/java/com/orgmemory/mcp/AssetDeliveryTools.javaapps/mcp/src/test/java/com/orgmemory/mcp/OrgMemoryMcpContextTests.java
**/*.{java,sql}
📄 CodeRabbit inference engine (CLAUDE.md)
Keep
ddl-auto=validateand pair every persisted-model change with a Flyway migration.
Files:
apps/mcp/src/test/java/com/orgmemory/mcp/McpToolErrorSurfaceTests.javaapps/mcp/src/main/java/com/orgmemory/mcp/McpFailureBoundary.javaapps/mcp/src/main/java/com/orgmemory/mcp/KnowledgeSearchTool.javaapps/mcp/src/main/java/com/orgmemory/mcp/ReleasedPromptAdapter.javaapps/mcp/src/main/java/com/orgmemory/mcp/AssetCompletionAdapter.javaapps/mcp/src/main/java/com/orgmemory/mcp/AssetDeliveryResources.javaapps/mcp/src/test/java/com/orgmemory/mcp/AssetCompletionAdapterTests.javaapps/mcp/src/main/java/com/orgmemory/mcp/AssetDeliveryTools.javaapps/mcp/src/test/java/com/orgmemory/mcp/OrgMemoryMcpContextTests.java
**/*.{java,gradle,gradle.kts}
📄 CodeRabbit inference engine (CLAUDE.md)
Use the testing harness; a terminating clean test is the JVM context gate, and
bootRunis not verification. IDE inspection applies only to edited backend Java.
Files:
apps/mcp/src/test/java/com/orgmemory/mcp/McpToolErrorSurfaceTests.javaapps/mcp/src/main/java/com/orgmemory/mcp/McpFailureBoundary.javaapps/mcp/src/main/java/com/orgmemory/mcp/KnowledgeSearchTool.javaapps/mcp/src/main/java/com/orgmemory/mcp/ReleasedPromptAdapter.javaapps/mcp/src/main/java/com/orgmemory/mcp/AssetCompletionAdapter.javaapps/mcp/src/main/java/com/orgmemory/mcp/AssetDeliveryResources.javaapps/mcp/src/test/java/com/orgmemory/mcp/AssetCompletionAdapterTests.javaapps/mcp/src/main/java/com/orgmemory/mcp/AssetDeliveryTools.javaapps/mcp/src/test/java/com/orgmemory/mcp/OrgMemoryMcpContextTests.java
ARCHITECTURE.md
📄 CodeRabbit inference engine (CLAUDE.md)
Keep
ARCHITECTURE.mdlimited to implemented facts, current project-wide facts, and commands; do not use it for intended or unimplemented behavior.
Files:
ARCHITECTURE.md
🧠 Learnings (2)
📚 Learning: 2026-07-26T05:46:47.443Z
Learnt from: kl3inIT
Repo: kl3inIT/OrgMemory PR: 61
File: apps/mcp/src/main/java/com/orgmemory/mcp/McpSecurityConfiguration.java:50-52
Timestamp: 2026-07-26T05:46:47.443Z
Learning: In OrgMemory, treat the `apps/mcp` and `apps/api` as independent protocol adapter modules. When adjusting OAuth/wire-level scopes, do not introduce a shared Java constant or create a code dependency from `apps/mcp` to `apps/api` solely to deduplicate scope values. Instead, keep OAuth/scope constants adapter-local (e.g., in the relevant adapter/security configuration classes) and ensure cross-adapter consistency via automated realm/OAuth/authorization tests, rather than via shared wiring-level constants or cross-module references.
Applied to files:
apps/mcp/src/main/java/com/orgmemory/mcp/McpFailureBoundary.javaapps/mcp/src/main/java/com/orgmemory/mcp/KnowledgeSearchTool.javaapps/mcp/src/main/java/com/orgmemory/mcp/ReleasedPromptAdapter.javaapps/mcp/src/main/java/com/orgmemory/mcp/AssetCompletionAdapter.javaapps/mcp/src/main/java/com/orgmemory/mcp/AssetDeliveryResources.javaapps/mcp/src/main/java/com/orgmemory/mcp/AssetDeliveryTools.java
📚 Learning: 2026-07-26T05:46:49.308Z
Learnt from: kl3inIT
Repo: kl3inIT/OrgMemory PR: 61
File: apps/mcp/src/main/java/com/orgmemory/mcp/McpTransportConfiguration.java:34-43
Timestamp: 2026-07-26T05:46:49.308Z
Learning: In the MCP module, authentication should be captured at servlet-time in `McpTransportConfiguration` (via the servlet `contextExtractor`) and stored in `McpTransportContext` under `AUTHENTICATION_CONTEXT_KEY`. Any authorization code (e.g., `McpApiAuthorization`) must consume this stored `Authentication` from `McpTransportContext` rather than re-reading it from `SecurityContextHolder`. Preserve the existing validation behavior: treat missing authentication as `null`, and only proceed when the consumed authentication is non-null and `authentication.isAuthenticated()` is true.
Applied to files:
apps/mcp/src/main/java/com/orgmemory/mcp/McpFailureBoundary.javaapps/mcp/src/main/java/com/orgmemory/mcp/KnowledgeSearchTool.javaapps/mcp/src/main/java/com/orgmemory/mcp/ReleasedPromptAdapter.javaapps/mcp/src/main/java/com/orgmemory/mcp/AssetCompletionAdapter.javaapps/mcp/src/main/java/com/orgmemory/mcp/AssetDeliveryResources.javaapps/mcp/src/main/java/com/orgmemory/mcp/AssetDeliveryTools.java
🪛 ast-grep (0.45.0)
apps/mcp/src/test/java/com/orgmemory/mcp/McpToolErrorSurfaceTests.java
[warning] 35-35: Avoid LDAP injections
Context: search.search(any(), any(), any())
Note: [CWE-90] Improper Neutralization of Special Elements used in an LDAP Query ('LDAP Injection'). Security best practice.
(ldap-injection-java)
[warning] 39-40: Avoid building a URL host from untrusted input
Context: "I/O error on GET request for "http://"
+ INTERNAL_HOST
Note: [CWE-20] Improper Input Validation.
(tainted-url-host)
apps/mcp/src/main/java/com/orgmemory/mcp/AssetCompletionAdapter.java
[warning] 140-141: Avoid LDAP injections
Context: assets.search(
authorization.require(context), null, type)
Note: [CWE-90] Improper Neutralization of Special Elements used in an LDAP Query ('LDAP Injection'). Security best practice.
(ldap-injection-java)
apps/mcp/src/test/java/com/orgmemory/mcp/AssetCompletionAdapterTests.java
[warning] 97-97: Avoid LDAP injections
Context: assets.search("Bearer exchanged-api-token", null, "PROMPT_TEMPLATE")
Note: [CWE-90] Improper Neutralization of Special Elements used in an LDAP Query ('LDAP Injection'). Security best practice.
(ldap-injection-java)
[warning] 152-152: Avoid LDAP injections
Context: assets.search("Bearer exchanged-api-token", null, null)
Note: [CWE-90] Improper Neutralization of Special Elements used in an LDAP Query ('LDAP Injection'). Security best practice.
(ldap-injection-java)
[warning] 188-188: Avoid LDAP injections
Context: assets.search("Bearer exchanged-api-token", null, "PROMPT_TEMPLATE")
Note: [CWE-90] Improper Neutralization of Special Elements used in an LDAP Query ('LDAP Injection'). Security best practice.
(ldap-injection-java)
apps/mcp/src/main/java/com/orgmemory/mcp/AssetDeliveryTools.java
[warning] 44-45: Avoid LDAP injections
Context: assets.search(
authorization.require(context), query, type)
Note: [CWE-90] Improper Neutralization of Special Elements used in an LDAP Query ('LDAP Injection'). Security best practice.
(ldap-injection-java)
🪛 PMD (7.26.0)
apps/mcp/src/main/java/com/orgmemory/mcp/McpFailureBoundary.java
[Low] 29-32: InvalidLogMessageFormat (Error Prone): Too many arguments, expected 1 argument but found 2
(InvalidLogMessageFormat (Error Prone))
[Medium] 33-33: PreserveStackTrace (Best Practices): Thrown exception does not preserve the stack trace of exception 'failure' on all code paths
(PreserveStackTrace (Best Practices))
🔇 Additional comments (10)
apps/mcp/src/main/resources/application.yml (1)
46-46: LGTM!apps/mcp/src/main/java/com/orgmemory/mcp/McpFailureBoundary.java (1)
23-35: LGTM!apps/mcp/src/main/java/com/orgmemory/mcp/AssetDeliveryTools.java (1)
44-50: LGTM!Also applies to: 63-68, 81-87, 100-111, 124-140, 153-160, 173-179, 192-205
apps/mcp/src/main/java/com/orgmemory/mcp/KnowledgeSearchTool.java (1)
34-47: LGTM!apps/mcp/src/main/java/com/orgmemory/mcp/ReleasedPromptAdapter.java (1)
22-25: LGTM!Also applies to: 43-67
apps/mcp/src/test/java/com/orgmemory/mcp/AssetCompletionAdapterTests.java (2)
42-147: LGTM!
149-183: LGTM!apps/mcp/src/test/java/com/orgmemory/mcp/McpToolErrorSurfaceTests.java (1)
32-55: LGTM!apps/mcp/src/test/java/com/orgmemory/mcp/OrgMemoryMcpContextTests.java (1)
41-42: LGTM!Also applies to: 53-61, 100-111
ARCHITECTURE.md (1)
68-71: LGTM!
Merging origin/main brought 5058cd8, which renamed the chat model provider type used by GraphRagRuntimeConfiguration — a source path this spec lists. The described behavior is unchanged, so only the reconciliation marker moves to the merge commit. The mirrored test matrix is untouched: none of its source paths moved. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The boundary caught only the two gateway exceptions, so a Jackson failure while writing a resource representation went around it. That path is worse than the tool path: the stateless resource callback returns "Error invoking resource method: <method> in <bean class>. Cause: <root cause message>", which publishes the declaring class and whatever Jackson put in the message, including the failing record and property chain. Jackson 3 exceptions are unchecked, so nothing forced this to be handled. The boundary now maps them to one fixed message and logs the detail. Deliberately narrow: an unexpected RuntimeException still propagates rather than being swallowed, which a test pins. Found while checking a CodeRabbit finding. Its stated reason was wrong -- it argued from Jackson 2 checked exceptions -- but a real hole sat next to it. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
What
Closes the two MCP gaps that were reachable under the current stateless transport.
1. Permission-scoped completion (
completioncapability wasfalse)released_promptand both Asset resource templates now answercompletion/complete:release_idonly offers releases of the chosenasset_id(CompleteRequest.context());onboardingstill finds the right release;total/hasMore;2. Sanitized tool errors
AbstractSyncMcpToolMethodCallbackbuilds the returned error text ase.getMessage() + "\n" + rootCause.getMessage(). Our gateway messages were already sanitized, but the root cause was not: anUnknownHostExceptionorResourceAccessExceptionpublished the internal API host and request URL to the MCP caller.McpFailureBoundarynow wraps every tool, prompt, and resource call: it logs the cause server-side and rethrows a cause-free failure carrying only the sanitized gateway message. The gateway clients keep their cause for diagnosis.Deliberately not done
Blocked by
protocol: STATELESS, each needing a separate transport decision rather than a quiet change here:tools/list/prompts/list/resources/list— the stateless annotation factory registers specifications once at startup;Docs
docs/specs/domains/assistant-and-mcp.mdwas stale on two counts and is corrected against the code: it claimed the surface was onesearch_knowledgetool (it is nine tools, two resource templates, one prompt) and that the inbound bearer was forwarded downstream (McpApiAuthorizationexchanges it and never forwards).Verification
gradlew :apps:mcp:test— 14 classes / 46 tests, 0 failures. New: 7 completion tests, 1 error-surface regression test that fails without the boundary fix;OrgMemoryMcpContextTests(a@SpringBootTest) proves the enabled capability wires three completion specifications.gradlew compileJava(whole repo) — pass.clean testnot run: the change is confined toapps/mcp, a leaf app no module depends on.Known limits
release_idsuggests only each Asset current release;/api/asset-deliveryexposes no release history.🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Bug Fixes
Documentation