feat(mcp): server module foundation + compatibility spike - #539
Conversation
Deploying profiletailors with
|
| Latest commit: |
0b72134
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://c2534ce4.profiletailors-com.pages.dev |
| Branch Preview URL: | https://feature-mcp-server-01-founda.profiletailors-com.pages.dev |
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughSummary by CodeRabbit
WalkthroughThis change establishes the MCP server foundation with Spring AI dependencies, feature-gated WebFlux transport, a Modulith-aligned bounded context, authenticated ChangesMCP Server Foundation
Estimated code review effort: 3 (Moderate) | ~25 minutes Sequence Diagram(s)sequenceDiagram
participant MCPClient
participant McpSecurityConfiguration
participant MCPEndpoint
MCPClient->>McpSecurityConfiguration: POST /api/mcp
McpSecurityConfiguration->>MCPEndpoint: authenticated request
MCPEndpoint-->>MCPClient: MCP response
McpSecurityConfiguration-->>MCPClient: 401 with Bearer challenge
Suggested labels: Poem
🚥 Pre-merge checks | ✅ 7 | ❌ 3❌ Failed checks (3 warnings)
✅ Passed checks (7 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 |
|
Review the following changes in direct dependencies. Learn more about Socket for GitHub.
|
Deploying app-profile-tailors with
|
| Latest commit: |
0b72134
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://eb7a5fab.app-profile-tailors.pages.dev |
| Branch Preview URL: | https://feature-mcp-server-01-founda.app-profile-tailors.pages.dev |
There was a problem hiding this comment.
Actionable comments posted: 19
🤖 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 `@openspec/changes/mcp-server/design.md`:
- Around line 299-307: Update the ApplicationError mapping in
openspec/changes/mcp-server/design.md:299-307 to use the canonical uppercase
codes, including WORKSPACE_ACCESS_DENIED, and align the corresponding documented
error-code names in openspec/changes/mcp-server/spec.md:51-67 and
openspec/changes/mcp-server/specs/mcp-server/spec.md:40-77 with that mapper
contract.
- Around line 256-280: The workspace_context flow must bind and consume each
context before token issuance. Update the Keycloak protocol-mapper behavior
described in the flow to require matching authenticated sub, validate issuer,
audience, and expiration, reject already-consumed jti values, and atomically
record valid jti values as single-use before emitting workspace_id.
In `@openspec/changes/mcp-server/proposal.md`:
- Around line 1-5: Update openspec/changes/mcp-server/proposal.md (lines 1-5),
openspec/changes/mcp-server/design.md (lines 1-5),
openspec/changes/mcp-server/spec.md (lines 1-5),
openspec/changes/mcp-server/specs/mcp-server/spec.md (lines 1-4),
openspec/changes/mcp-server/specs/iam/spec.md (lines 1-4), and
openspec/changes/mcp-server/specs/oauth-mcp-client-registration/spec.md (lines
1-4) to use the mandated section sequence: Overview, Changes, Usage,
Troubleshooting, References; preserve each document’s existing content under the
appropriate sections.
- Line 50: Correct the configuration filename references: in
openspec/changes/mcp-server/proposal.md lines 50-50, replace application.yml
with application.yaml; in openspec/changes/mcp-server/design.md lines 137-137,
update the section label to application.yaml.
In `@openspec/changes/mcp-server/specs/workspace-scoped-oauth/spec.md`:
- Around line 62-68: Update the “Authentication Failures” requirement and
missing-token scenario to specify the exact RFC 9728 401 contract: require a
WWW-Authenticate header with Bearer realm="mcp" and resource_metadata="<url>",
and state that the referenced Protected Resource Metadata identifies Keycloak.
- Around line 8-19: Update the “Pre-Flow Workspace Injection” requirement to
define replay-resistant validation for signed workspace context or pre-flow
tokens: specify the trusted issuer and signing key, audience, expiry, nonce with
one-time use, and binding to the OAuth client and authorization request. Require
Keycloak to verify all claims and reject invalid, expired, replayed, or
cross-client contexts before emitting workspace_id, while preserving the
existing authorized-workspace requirement.
- Line 3: Restructure the document using the required heading order: Overview,
Changes, Usage, Troubleshooting, and References, replacing the current
Purpose/Requirements structure. Add blank lines before and after every Markdown
heading to satisfy the MD022 requirements, while preserving the existing content
under the appropriate sections.
In `@openspec/changes/mcp-server/spikes/SPIKE_OUTCOME.md`:
- Line 328: Add the text language tag to the fenced Keycloak mapper
configuration block in SPIKE_OUTCOME.md, preserving its existing contents so the
Markdown satisfies MD040.
- Around line 443-450: Update the T11 entry in the PR task inheritance table to
reference the existing Section 1.4 instead of nonexistent Section 1.5; do not
add a new section.
- Around line 1-16: Rename openspec/changes/mcp-server/spikes/SPIKE_OUTCOME.md
to a lowercase kebab-case Markdown filename, update every reference to the
renamed file, and organize its content into Overview, Changes, Usage,
Troubleshooting, and References. In openspec/changes/mcp-server/tasks.md, add or
map the same five required sections; keep both documents in English and follow
the repository documentation rules.
- Around line 371-373: Update the workspace-context flow description near the
SPA JWS forwarding text to call workspace_context a parameter added to
Keycloak’s authorization request, not an authorization request parameter. Align
the validation description with the selected authenticator/session-note
validation path and preserve the existing JWS verification and workspace_id
binding behavior.
- Around line 339-346: Update the workspace context flow around the
`workspaceContext` block so `workspace_id` is never copied from parsed,
unverified input. Have the authenticator validate the JWS using the Profile
Tailors JWKS, issuer, expiry, audience/workspace binding, and replay checks,
then store the validated workspace ID in `session.note`; make the mapper copy
claims only from those verified results.
- Around line 163-179: Record CIMD as experimental in the MCP spike outcome and
status state: update the CIMD section of SPIKE_OUTCOME.md to document Keycloak
26 support behind --features=cimd, remove the unsupported/fallback-only
conclusion, and revise state.yaml’s keycloak_cimd value from
not_supported_fallback_to_preregistered to an experimental status that reflects
feature-gated support.
In `@openspec/changes/mcp-server/tasks.md`:
- Around line 39-40: Update every task heading in the task list, including “Task
1,” to include one blank line before its following list content, resolving the
markdownlint MD022 violations consistently throughout the document.
- Around line 79-87: Align the PR1 gate with the staged implementation: in
openspec/changes/mcp-server/tasks.md lines 79-87, move authenticated Inspector
coverage and full RFC 9728 discovery-header acceptance to PR2/PR3, retaining
only startup, endpoint exposure, and placeholder unauthenticated 401 validation
for PR1; update openspec/changes/mcp-server/spikes/SPIKE_OUTCOME.md lines
404-417 to state the same reduced PR1 acceptance criteria.
- Around line 49-57: Update Task 2 in tasks.md and the related MCP
client-registration documentation to use Keycloak’s actual
`/realms/{realm}/clients-registrations/default` endpoint instead of
`/oauth2/register`. If retaining `/oauth2/register`, document concrete
deployment-proxy evidence showing how it maps to the Keycloak endpoint, and
ensure the DCR verification and recorded request/response paths match.
In
`@server/smp/src/main/kotlin/com/profiletailors/smp/mcp/infrastructure/McpSecurityConfiguration.kt`:
- Around line 44-45: Update mcpPathMatcher in McpSecurityConfiguration to derive
its patterns from the configured Spring AI MCP endpoint property instead of
hardcoding "/api/mcp" and "/api/mcp/**". Ensure the matcher protects both the
configured endpoint and its subpaths.
In `@server/smp/src/main/resources/application.yaml`:
- Around line 82-87: The MCP configuration in
server/smp/src/main/resources/application.yaml lines 82-87 must enforce a single
canonical resource/audience value: derive mcp.required-audience from
mcp.resource-uri or add startup validation that rejects mismatches. Update
.env.example lines 120-123 to document one canonical environment variable, or
explicitly document the equality requirement and ensure the application
validates it.
In
`@server/smp/src/test/kotlin/com/profiletailors/smp/mcp/infrastructure/McpWiringTest.kt`:
- Around line 103-120: Add mandatory Cucumber BDD coverage for the
unauthenticated POST /api/mcp behavior alongside the existing McpWiringTest.
Create a feature under the test resources features directory and matching BDD
step definitions, tagging the scenario with the MCP domain tag, `@smoke`, and
`@fast`; verify a 401 response with a WWW-Authenticate header beginning with
Bearer.
🪄 Autofix (Beta)
✅ Autofix completed
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Organization UI (inherited)
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: e94d9216-dedd-4f3f-aae4-92c01936c323
📒 Files selected for processing (22)
.env.examplegradle/libs.versions.tomlopenspec/changes/mcp-server/design.mdopenspec/changes/mcp-server/proposal.mdopenspec/changes/mcp-server/spec.mdopenspec/changes/mcp-server/specs/iam/spec.mdopenspec/changes/mcp-server/specs/mcp-server/spec.mdopenspec/changes/mcp-server/specs/oauth-mcp-client-registration/spec.mdopenspec/changes/mcp-server/specs/workspace-scoped-oauth/spec.mdopenspec/changes/mcp-server/spikes/SPIKE_OUTCOME.mdopenspec/changes/mcp-server/state.yamlopenspec/changes/mcp-server/tasks.mdserver/smp/build.gradle.ktsserver/smp/src/main/kotlin/com/profiletailors/smp/mcp/McpBoundedContext.ktserver/smp/src/main/kotlin/com/profiletailors/smp/mcp/ModuleMetadata.ktserver/smp/src/main/kotlin/com/profiletailors/smp/mcp/application/McpApplicationMarker.ktserver/smp/src/main/kotlin/com/profiletailors/smp/mcp/domain/McpDomainMarker.ktserver/smp/src/main/kotlin/com/profiletailors/smp/mcp/infrastructure/McpConfiguration.ktserver/smp/src/main/kotlin/com/profiletailors/smp/mcp/infrastructure/McpSecurityConfiguration.ktserver/smp/src/main/kotlin/com/profiletailors/smp/mcp/infrastructure/oauth/.gitkeepserver/smp/src/main/resources/application.yamlserver/smp/src/test/kotlin/com/profiletailors/smp/mcp/infrastructure/McpWiringTest.kt
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
|
Note Autofix is a beta feature. Expect some limitations and changes as we gather feedback and continue to improve it. ❌ Failed to clone repository into sandbox. Please try again. |
|
Note Autofix is a beta feature. Expect some limitations and changes as we gather feedback and continue to improve it. The branch was updated while autofix was in progress. Please try again. |
|
Note Autofix is a beta feature. Expect some limitations and changes as we gather feedback and continue to improve it. Fixes Applied SuccessfullyFixed 13 file(s) based on 19 unresolved review comments. Files modified:
Commit: The changes have been pushed to the Time taken: |
5c04c4a to
7358be1
Compare
There was a problem hiding this comment.
Actionable comments posted: 12
🤖 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 `@gradle/libs.versions.toml`:
- Line 65: Remove the standalone “Spring AI — MCP server (PR 1)” comment from
the dependency catalog, leaving the following aliases unchanged; do not replace
it unless a concise, English rationale documents a non-obvious dependency
constraint.
In `@openspec/changes/mcp-server/design.md`:
- Around line 256-269: Update the OAuth flow description around the
workspace_context exchange so the signed JWS is never placed in the
authorization URL or other front-channel browser parameters. Replace the direct
workspace_context query parameter with an opaque, single-use context reference
or Pushed Authorization Request, while preserving Keycloak mapper validation and
workspace_id propagation into the token.
- Around line 194-195: Define the security-boundary workspace-access denial
contract: in openspec/changes/mcp-server/design.md lines 194-195, specify that
the access-denied handler returns HTTP 403 with a body containing the canonical
workspace-access error code. In openspec/changes/mcp-server/spec.md lines 51-52
and openspec/changes/mcp-server/specs/mcp-server/spec.md lines 40-41, exclude
pre-dispatch workspace denials from the tool error taxonomy and restrict
ApplicationError failures to post-dispatch tool failures. In
openspec/changes/mcp-server/specs/mcp-server/spec.md lines 63-66, require this
security-boundary 403 response instead of a tool-level CallToolResult.
In `@openspec/changes/mcp-server/spikes/SPIKE_OUTCOME.md`:
- Around line 120-161: Align the DCR documentation and tracking with one
verified Keycloak contract: in
openspec/changes/mcp-server/spikes/SPIKE_OUTCOME.md:120-161, select the tested
endpoint and make its payload representation consistent; remove the stale
/oauth2/register ownership claim at
openspec/changes/mcp-server/spikes/SPIKE_OUTCOME.md:189-191; update Task 2 at
openspec/changes/mcp-server/tasks.md:54-57 and the removed-item table at
openspec/changes/mcp-server/tasks.md:403-411; mark DCR supported in
openspec/changes/mcp-server/state.yaml:21 only after an executable test passes
against that exact endpoint and payload.
- Around line 202-264: Update
openspec/changes/mcp-server/spikes/SPIKE_OUTCOME.md:202-264 to remove any
“rfc8707: verified” status and state that verification requires inspecting a
real access token for the MCP URI in aud; retain the audience-mapper fallback
and explicitly limit PR 2 to enforcing the configured app.mcp.resource-uri when
applicable. Update openspec/changes/mcp-server/tasks.md:59-67 so Task 3
acceptance and verification require the same configured-resource-uri scope and
real-token evidence. Update openspec/changes/mcp-server/state.yaml:23 to remove
the verified RFC 8707 status until that evidence exists.
- Around line 349-355: The documented mcp browser flow must define a real
Keycloak authenticator provider rather than a “SPI-free” authenticator. Update
the flow description and implementation guidance to use either a Java
AuthenticatorFactory-based provider or a deployed script provider, and configure
the protocol mapper to read workspace_id from the user session note
(session.note), not user session attributes.
In `@openspec/changes/mcp-server/state.yaml`:
- Line 18: Update the spike_document value in state.yaml to reference the
renamed lowercase kebab-case file, replacing SPIKE_OUTCOME.md with
spike-outcome.md while preserving the existing directory path.
In `@openspec/changes/mcp-server/tasks.md`:
- Around line 95-97: Update the verification tasks in the MCP backend plan to
route all repository checks and Gradle invocations through the appropriate just
recipes instead of direct ./gradlew commands. Apply this consistently to the
TDD, Acceptance, and Verification entries, documenting an approved exception
only where no suitable just recipe exists.
- Around line 319-327: Add a minimal PR1 Cucumber feature for the /api/mcp
endpoint covering endpoint exposure, feature-gate behavior, and unauthenticated
401 responses with the WWW-Authenticate header. Update the relevant BDD task or
feature scope while keeping detailed tool, workspace-isolation, and OAuth
discovery scenarios deferred to PR4.
- Around line 179-187: Keep McpWorkspaceContextResolver and its tests free of
ServerWebExchange, Jwt, and direct RequestContextStore dependencies by moving
HTTP/JWT extraction and context-store writes into an infrastructure adapter.
Pass the validated workspace value through a framework-independent application
port or CQRS boundary, while preserving JWT-derived workspace resolution and
silently ignoring X-Workspace-Id for /api/mcp traffic. Update tests to target
the adapter and application boundary separately.
- Around line 329-337: Update the Task 27 `McpToolsBddSteps.kt` WebTestClient
request configuration for `POST /api/mcp` to use `Accept: application/json,
text/event-stream` and `Content-Type: application/json` when sending JSON-RPC
bodies. Remove the JSON:API media type while preserving the existing
authorization, response capture, database reset, and workspace seeding behavior.
In
`@server/smp/src/main/kotlin/com/profiletailors/smp/mcp/infrastructure/McpSecurityConfiguration.kt`:
- Around line 55-79: Replace HttpStatusServerEntryPoint and
PlaceholderWwwAuthenticateFilter with a single ServerAuthenticationEntryPoint
configured in exceptionHandling. Have its commence implementation set the
WWW-Authenticate Bearer realm header before assigning HttpStatus.UNAUTHORIZED
and completing the response, then remove the filter registration and nested
filter class.
🪄 Autofix (Beta)
❌ Autofix failed (check again to retry)
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: Repository YAML (base), Organization UI (inherited)
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 1667225e-07ab-4d1b-a293-0d0b5676b4e7
📒 Files selected for processing (22)
.env.examplegradle/libs.versions.tomlopenspec/changes/mcp-server/design.mdopenspec/changes/mcp-server/proposal.mdopenspec/changes/mcp-server/spec.mdopenspec/changes/mcp-server/specs/iam/spec.mdopenspec/changes/mcp-server/specs/mcp-server/spec.mdopenspec/changes/mcp-server/specs/oauth-mcp-client-registration/spec.mdopenspec/changes/mcp-server/specs/workspace-scoped-oauth/spec.mdopenspec/changes/mcp-server/spikes/SPIKE_OUTCOME.mdopenspec/changes/mcp-server/state.yamlopenspec/changes/mcp-server/tasks.mdserver/smp/build.gradle.ktsserver/smp/src/main/kotlin/com/profiletailors/smp/mcp/McpBoundedContext.ktserver/smp/src/main/kotlin/com/profiletailors/smp/mcp/ModuleMetadata.ktserver/smp/src/main/kotlin/com/profiletailors/smp/mcp/application/McpApplicationMarker.ktserver/smp/src/main/kotlin/com/profiletailors/smp/mcp/domain/McpDomainMarker.ktserver/smp/src/main/kotlin/com/profiletailors/smp/mcp/infrastructure/McpConfiguration.ktserver/smp/src/main/kotlin/com/profiletailors/smp/mcp/infrastructure/McpSecurityConfiguration.ktserver/smp/src/main/kotlin/com/profiletailors/smp/mcp/infrastructure/oauth/.gitkeepserver/smp/src/main/resources/application.yamlserver/smp/src/test/kotlin/com/profiletailors/smp/mcp/infrastructure/McpWiringTest.kt
|
Note Autofix is a beta feature. Expect some limitations and changes as we gather feedback and continue to improve it. ❌ Failed to clone repository into sandbox. Please try again. |
|
Note Docstrings generation - SUCCESS |
Slice test proving: 1. The mcp bounded context is registered as a Spring Modulith module. 2. McpConfiguration + McpSecurityConfiguration beans load when spring.ai.mcp.server.enabled=true. 3. POST /api/mcp without an Authorization header returns 401 with a WWW-Authenticate: Bearer ... header (placeholder contents; the RFC 9728 resource_metadata URL lands in PR 2 via ResourceMetadataController). Spring AI's STATELESS WebFlux transport bean is wired automatically by auto-configuration; no @mcptool beans exist yet so tools/list returns an empty array (verified via WARN log 'No tool methods found' on startup). Tagged @tag("postgres"); uses Testcontainers PostgreSQL since the test boots the full SMP application context. Marked @testcontainers(disabledWithoutDocker=true) so the test gracefully skips when Docker is unavailable.
Captures the design decisions and acceptance criteria for the mcp-server change. Mirrors the stacked-PR strategy (PR1 foundation, PR2 security, PR3 tools, PR4 verification) and the OAuth split (SMP = Resource Server, Keycloak = Authorization Server). These artifacts travel with PR 1 to preserve traceability between design and implementation.
Docstrings generation was requested by @yacosta738. The following files were modified: * `server/smp/src/main/kotlin/com/profiletailors/smp/mcp/infrastructure/McpSecurityConfiguration.kt` These files were ignored: * `server/smp/src/test/kotlin/com/profiletailors/smp/mcp/infrastructure/McpWiringTest.kt` These file types are not supported: * `.env.example` * `gradle/libs.versions.toml` * `openspec/changes/mcp-server/design.md` * `openspec/changes/mcp-server/proposal.md` * `openspec/changes/mcp-server/spec.md` * `openspec/changes/mcp-server/specs/iam/spec.md` * `openspec/changes/mcp-server/specs/mcp-server/spec.md` * `openspec/changes/mcp-server/specs/oauth-mcp-client-registration/spec.md` * `openspec/changes/mcp-server/specs/workspace-scoped-oauth/spec.md` * `openspec/changes/mcp-server/spikes/SPIKE_OUTCOME.md` * `openspec/changes/mcp-server/state.yaml` * `openspec/changes/mcp-server/tasks.md` * `server/smp/src/main/resources/application.yaml`
The listing query uses a 30-day forward window. Hardcoded dates far in the future fell outside this range, causing 'expected 2 but was 1' in the List scenario. Now seeds and feature files use relative offsets (+7days, +14days) resolved at runtime.
06ae6c2 to
d2ad02d
Compare
|



Summary
PR 1 of the stacked PRs strategy for the MCP server feature (issue #157).
This PR delivers the compatibility spike + module skeleton that unblocks PR 2 (security + OAuth discovery).
What's in this PR
1. Compatibility spike outcome (
SPIKE_OUTCOME.md)Comprehensive documentation of the technical spike validating:
@McpToolannotation API with Kotlin coroutinesresourceparameter handling verified2. Spring AI 2.0.0 integration
springAi = "2.0.0"togradle/libs.versions.tomlspring-ai-bomandspring-ai-starter-mcp-server-webfluxdependenciesserver/smp/build.gradle.kts3.
mcpbounded contextdomain/application/infrastructure/infrastructure/oauthMcpConfiguration(placeholder, gated bySMP_MCP_ENABLED)McpSecurityConfiguration(placeholder — returns 401 for/api/mcp)McpBoundedContext+ModuleMetadatamarkers for Spring Modulith discovery4. Configuration
5. Acceptance test:
McpWiringTestVerifies that
POST /api/mcpreturns 401 +WWW-Authenticate: Bearer realm="mcp"when the server is enabled.What's NOT in this PR (deferred to PR 2)
@McpToolbeansPre-merge checklist
just backend-checkpassesjust backend-testpassesjust backend-bdd-fastpassesjust ci-localpasses (gitleaks, lint, tests, build)@McpToolbeans registered (no tools yet)SMP_MCP_ENABLED=falseby default (safe to deploy)Verification
Stacked PRs
This PR is the foundation. Subsequent PRs will branch from it:
feature/mcp-server-02-security→ base PR 1feature/mcp-server-03-tools→ base PR 2feature/mcp-server-04-verification→ base PR 3Each PR's base branch must be re-retargeted to
mainafter the previous PR merges.Watchpoints for PR 2
McpSecurityConfigurationis a placeholder — PR 2 must replace with full JWT chain/api/mcp/**only (don't leak into REST endpoints)References
openspec/changes/mcp-server/{proposal,spec,design,tasks,spikes/SPIKE_OUTCOME.md}