Fix Code Mode MCP server function results - #489
Open
theianjones wants to merge 9 commits into
Open
Conversation
…cp-server # Conflicts: # apps/crm/src/server-fns/crm.ts
Contributor
WalkthroughThis PR introduces a comprehensive Model Context Protocol (MCP) implementation across WODsmith: CRM exposes CRUD MCP tools and routes, a standalone Cloudflare Worker (wodsmith-code-mode-mcp) runs sandboxed user JavaScript via MCP tools (search/execute) with KV-backed session auth, and WODsmith Start publishes a competition operation catalog and RPC entrypoint for calling those operations with session-aware dispatch. ChangesCRM MCP Server
Cloudflare Workers Code Mode MCP
WODsmith Start MCP Integration
Documentation and Project Metadata
Estimated code review effort 🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 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 |
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
14 issues found across 38 files
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="apps/wodsmith-code-mode-mcp/src/auth.ts">
<violation number="1" location="apps/wodsmith-code-mode-mcp/src/auth.ts:75">
P1: Handle invalid KV JSON defensively; an unparseable session record currently throws and can turn auth checks into 500 responses.</violation>
<violation number="2" location="apps/wodsmith-code-mode-mcp/src/auth.ts:76">
P2: Validate `expiresAt` before comparing; malformed session payloads can bypass the expiration check.</violation>
</file>
<file name="apps/crm/src/mcp.ts">
<violation number="1" location="apps/crm/src/mcp.ts:182">
P1: Defaulting missing update source to "Outreach" can break valid Meeting interaction updates. Require an explicit source for update actions instead of silently changing type.</violation>
</file>
<file name="apps/crm/src/routes/api/mcp.ts">
<violation number="1" location="apps/crm/src/routes/api/mcp.ts:7">
P3: This route reintroduces MCP handler wiring that already exists in `routes/mcp.ts`; duplicate endpoint logic increases maintenance drift risk.</violation>
</file>
<file name="apps/crm/src/lib/mcp-handler.ts">
<violation number="1" location="apps/crm/src/lib/mcp-handler.ts:17">
P2: Close the MCP server after each request. This handler connects a server but never disposes it, which can leak resources across requests.</violation>
</file>
<file name="apps/wodsmith-code-mode-mcp/src/mcp/outbound.ts">
<violation number="1" location="apps/wodsmith-code-mode-mcp/src/mcp/outbound.ts:13">
P2: `json()` incorrectly spreads `ResponseInit.headers`; this breaks valid `HeadersInit` inputs like `Headers` or header tuples.</violation>
<violation number="2" location="apps/wodsmith-code-mode-mcp/src/mcp/outbound.ts:76">
P2: Raw internal error messages are returned on 500 responses, which can leak backend implementation details.</violation>
</file>
<file name="apps/crm/src/server-fns/crm.ts">
<violation number="1" location="apps/crm/src/server-fns/crm.ts:877">
P2: Document records are removed before storage cleanup, and storage deletion failures are swallowed, which can leave orphaned files in R2.</violation>
<violation number="2" location="apps/crm/src/server-fns/crm.ts:1267">
P2: Outreach interaction creation now silently skips the Person relation when that field is missing instead of failing fast.</violation>
</file>
<file name="apps/wodsmith-start/src/mcp/competition-operations.ts">
<violation number="1" location="apps/wodsmith-start/src/mcp/competition-operations.ts:696">
P2: Non-2xx `Response` objects are thrown before serialized payload decoding, so server-function error payloads are lost and callers only get a generic status error.</violation>
<violation number="2" location="apps/wodsmith-start/src/mcp/competition-operations.ts:703">
P1: Serialized responses are only decoded for `application/json`; `application/x-ndjson` serialized responses are not handled and can return an undecoded `Response` instead of operation data.</violation>
<violation number="3" location="apps/wodsmith-start/src/mcp/competition-operations.ts:705">
P1: The custom cross-JSON decoder only supports a small subset of node types, so valid TanStack/Seroval serialized values can throw `Unsupported serialized server function result`.</violation>
</file>
<file name="apps/wodsmith-start/src/mcp/rpc.ts">
<violation number="1" location="apps/wodsmith-start/src/mcp/rpc.ts:46">
P2: Set an explicit `POST` method when constructing the credential request. The current default `GET` changes request semantics versus the existing MCP server-function context and can break method-sensitive middleware/logic.</violation>
</file>
<file name="apps/wodsmith-code-mode-mcp/src/mcp/executor.ts">
<violation number="1" location="apps/wodsmith-code-mode-mcp/src/mcp/executor.ts:158">
P2: Validate successful operation responses before reading `payload.result`; otherwise unexpected 200-body shapes are silently converted into `undefined` results.</violation>
</file>
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
| ) | ||
| if (!sessionStr) return null | ||
|
|
||
| const session = JSON.parse(sessionStr) as KVSession |
Contributor
There was a problem hiding this comment.
P1: Handle invalid KV JSON defensively; an unparseable session record currently throws and can turn auth checks into 500 responses.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At apps/wodsmith-code-mode-mcp/src/auth.ts, line 75:
<comment>Handle invalid KV JSON defensively; an unparseable session record currently throws and can turn auth checks into 500 responses.</comment>
<file context>
@@ -0,0 +1,140 @@
+ )
+ if (!sessionStr) return null
+
+ const session = JSON.parse(sessionStr) as KVSession
+ if (Date.now() >= session.expiresAt) {
+ await env.KV_SESSION.delete(getSessionKey(parts.userId, sessionId))
</file context>
| if (interaction.action === "create") { | ||
| const result = await createInteraction({ | ||
| ...interaction, | ||
| source: interaction.source ?? "Outreach", |
Contributor
There was a problem hiding this comment.
P1: Defaulting missing update source to "Outreach" can break valid Meeting interaction updates. Require an explicit source for update actions instead of silently changing type.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At apps/crm/src/mcp.ts, line 182:
<comment>Defaulting missing update source to "Outreach" can break valid Meeting interaction updates. Require an explicit source for update actions instead of silently changing type.</comment>
<file context>
@@ -0,0 +1,413 @@
+ if (interaction.action === "create") {
+ const result = await createInteraction({
+ ...interaction,
+ source: interaction.source ?? "Outreach",
+ title: requireValue(interaction.title, "interaction title"),
+ campaignId,
</file context>
| const contentType = response.headers.get("Content-Type") ?? "" | ||
| const isSerialized = response.headers.has("x-tss-serialized") | ||
|
|
||
| if (isSerialized && contentType.includes("application/json")) { |
Contributor
There was a problem hiding this comment.
P1: Serialized responses are only decoded for application/json; application/x-ndjson serialized responses are not handled and can return an undecoded Response instead of operation data.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At apps/wodsmith-start/src/mcp/competition-operations.ts, line 703:
<comment>Serialized responses are only decoded for `application/json`; `application/x-ndjson` serialized responses are not handled and can return an undecoded `Response` instead of operation data.</comment>
<file context>
@@ -0,0 +1,774 @@
+ const contentType = response.headers.get("Content-Type") ?? ""
+ const isSerialized = response.headers.has("x-tss-serialized")
+
+ if (isSerialized && contentType.includes("application/json")) {
+ const payload = await response.json()
+ const unwrapped = fromSimpleCrossJson(payload as CrossJsonNode)
</file context>
|
|
||
| if (isSerialized && contentType.includes("application/json")) { | ||
| const payload = await response.json() | ||
| const unwrapped = fromSimpleCrossJson(payload as CrossJsonNode) |
Contributor
There was a problem hiding this comment.
P1: The custom cross-JSON decoder only supports a small subset of node types, so valid TanStack/Seroval serialized values can throw Unsupported serialized server function result.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At apps/wodsmith-start/src/mcp/competition-operations.ts, line 705:
<comment>The custom cross-JSON decoder only supports a small subset of node types, so valid TanStack/Seroval serialized values can throw `Unsupported serialized server function result`.</comment>
<file context>
@@ -0,0 +1,774 @@
+
+ if (isSerialized && contentType.includes("application/json")) {
+ const payload = await response.json()
+ const unwrapped = fromSimpleCrossJson(payload as CrossJsonNode)
+ return unwrapServerFunctionResponse(unwrapped)
+ }
</file context>
| if (!sessionStr) return null | ||
|
|
||
| const session = JSON.parse(sessionStr) as KVSession | ||
| if (Date.now() >= session.expiresAt) { |
Contributor
There was a problem hiding this comment.
P2: Validate expiresAt before comparing; malformed session payloads can bypass the expiration check.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At apps/wodsmith-code-mode-mcp/src/auth.ts, line 76:
<comment>Validate `expiresAt` before comparing; malformed session payloads can bypass the expiration check.</comment>
<file context>
@@ -0,0 +1,140 @@
+ if (!sessionStr) return null
+
+ const session = JSON.parse(sessionStr) as KVSession
+ if (Date.now() >= session.expiresAt) {
+ await env.KV_SESSION.delete(getSessionKey(parts.userId, sessionId))
+ return null
</file context>
| const field = fields.get(fieldName) | ||
| if (field) await upsertFieldValue(entryId, field.id, value) | ||
| } | ||
| const contactField = fields.get(source === "Meeting" ? "Contact" : "Person") |
Contributor
There was a problem hiding this comment.
P2: Outreach interaction creation now silently skips the Person relation when that field is missing instead of failing fast.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At apps/crm/src/server-fns/crm.ts, line 1267:
<comment>Outreach interaction creation now silently skips the Person relation when that field is missing instead of failing fast.</comment>
<file context>
@@ -1012,390 +1065,525 @@ export async function getCrmData() {
- const field = fields.get(fieldName)
- if (field) await upsertFieldValue(entryId, field.id, value)
- }
+ const contactField = fields.get(source === "Meeting" ? "Contact" : "Person")
+ if (contactField) {
+ await setRelation(entryId, contactField.id, clean(data.contactId))
</file context>
Suggested change
| const contactField = fields.get(source === "Meeting" ? "Contact" : "Person") | |
| const contactField = | |
| source === "Meeting" | |
| ? fields.get("Contact") | |
| : requireField(fields, "Person", "Outreach") |
| response: ServerFnResult | Response | unknown, | ||
| ): Promise<unknown> { | ||
| if (response instanceof Response) { | ||
| if (!response.ok) { |
Contributor
There was a problem hiding this comment.
P2: Non-2xx Response objects are thrown before serialized payload decoding, so server-function error payloads are lost and callers only get a generic status error.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At apps/wodsmith-start/src/mcp/competition-operations.ts, line 696:
<comment>Non-2xx `Response` objects are thrown before serialized payload decoding, so server-function error payloads are lost and callers only get a generic status error.</comment>
<file context>
@@ -0,0 +1,774 @@
+ response: ServerFnResult | Response | unknown,
+): Promise<unknown> {
+ if (response instanceof Response) {
+ if (!response.ok) {
+ throw new Error(`Server function failed with status ${response.status}`)
+ }
</file context>
| headers.set("Cookie", credential.cookie) | ||
| } | ||
|
|
||
| return new Request("https://wodsmith-mcp.internal/rpc", { headers }) |
Contributor
There was a problem hiding this comment.
P2: Set an explicit POST method when constructing the credential request. The current default GET changes request semantics versus the existing MCP server-function context and can break method-sensitive middleware/logic.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At apps/wodsmith-start/src/mcp/rpc.ts, line 46:
<comment>Set an explicit `POST` method when constructing the credential request. The current default `GET` changes request semantics versus the existing MCP server-function context and can break method-sensitive middleware/logic.</comment>
<file context>
@@ -0,0 +1,81 @@
+ headers.set("Cookie", credential.cookie)
+ }
+
+ return new Request("https://wodsmith-mcp.internal/rpc", { headers })
+}
+
</file context>
| throw new Error(payload.error || "WODsmith operation failed with status " + response.status); | ||
| } | ||
|
|
||
| return payload.result; |
Contributor
There was a problem hiding this comment.
P2: Validate successful operation responses before reading payload.result; otherwise unexpected 200-body shapes are silently converted into undefined results.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At apps/wodsmith-code-mode-mcp/src/mcp/executor.ts, line 158:
<comment>Validate successful operation responses before reading `payload.result`; otherwise unexpected 200-body shapes are silently converted into `undefined` results.</comment>
<file context>
@@ -0,0 +1,199 @@
+ throw new Error(payload.error || "WODsmith operation failed with status " + response.status);
+ }
+
+ return payload.result;
+}
+
</file context>
| export const Route = createFileRoute("/api/mcp")({ | ||
| server: { | ||
| handlers: { | ||
| GET: ({ request }) => handleMcpRequest(request), |
Contributor
There was a problem hiding this comment.
P3: This route reintroduces MCP handler wiring that already exists in routes/mcp.ts; duplicate endpoint logic increases maintenance drift risk.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At apps/crm/src/routes/api/mcp.ts, line 7:
<comment>This route reintroduces MCP handler wiring that already exists in `routes/mcp.ts`; duplicate endpoint logic increases maintenance drift risk.</comment>
<file context>
@@ -0,0 +1,12 @@
+export const Route = createFileRoute("/api/mcp")({
+ server: {
+ handlers: {
+ GET: ({ request }) => handleMcpRequest(request),
+ POST: ({ request }) => handleMcpRequest(request),
+ DELETE: ({ request }) => handleMcpRequest(request),
</file context>
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 19
🧹 Nitpick comments (9)
apps/crm/src/server-fns/auth.ts (1)
5-6: ⚡ Quick winUse the exact
@latmarker syntax.These comments are written as
// `@lat`: ..., which won't match the repo's required//@lat: [[section-id]]format for lat tooling.As per coding guidelines
**/*.{ts,tsx,js,jsx,py,rs,go,h,c}: Use//@lat: [[section-id]](for JS/TS/Rust/Go/C) or#@lat: [[section-id]](for Python) comments to tie source code to concepts documented in lat.md/.Also applies to: 66-67, 101-102
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/crm/src/server-fns/auth.ts` around lines 5 - 6, The inline marker comments use the wrong backtick-wrapped form; replace them with the exact tooling syntax by changing the comment above the SESSION_COOKIE export (and the other similar occurrences in this file) from // `@lat`: [[auth]] to the required form // `@lat`: [[auth]] so the lat tooling can match the section id (ensure every instance like the ones near SESSION_COOKIE and the other two occurrences use // `@lat`: [[section-id]] rather than backticks).apps/wodsmith-start/src/mcp/competition-operations.ts (1)
1-1: ⚡ Quick winDrop the new
server-onlyimport.This repo explicitly moved away from adding
server-onlyin TypeScript files, so this just adds another dependency on the stubbed alias without buying anything here.Based on learnings, this repository no longer wants new `server-only` imports in TypeScript files.Suggested change
-import "server-only" -🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/wodsmith-start/src/mcp/competition-operations.ts` at line 1, Remove the unnecessary top-level import "server-only" from the file: delete the import "server-only" statement so the file no longer depends on that stubbed alias; ensure there are no other references to server-only in functions or exports (e.g., any usage around competition-operations module) and run typechecks to confirm nothing else requires the removed import.apps/wodsmith-start/src/mcp/rpc.ts (1)
1-1: ⚡ Quick winDrop the new
server-onlyimport.This repo no longer wants new TypeScript files to depend on
server-only, and this file already only ships through the server entrypoint.Based on learnings, this repository no longer wants new `server-only` imports in TypeScript files.Suggested change
-import "server-only" -🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/wodsmith-start/src/mcp/rpc.ts` at line 1, Remove the top-level import "server-only" from this file — delete the import line so the module no longer depends on server-only (the file already only ships via the server entrypoint); verify no other code in mcp/rpc.ts requires server-only and run typecheck to ensure no missing types or references.apps/wodsmith-code-mode-mcp/test/mcp/server-protocol.test.ts (1)
65-67: ⚡ Quick winAdd the required
//@lat:references beside each spec.Both
it(...)sections are missing the per-spec//@lat: [[section-id]]annotation that this repo requires next to the relevant test block.As per coding guidelines, "
**/*.{test.ts,test.js,test.py,spec.ts,spec.js,spec.py}: Reference test specifications with exactly one//@lat:or#@lat:comment placed next to the relevant test, not at the top of the file, using format//@lat: [[section-id]]or#@lat: [[section-id]]."Also applies to: 123-125
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/wodsmith-code-mode-mcp/test/mcp/server-protocol.test.ts` around lines 65 - 67, The two test specs are missing the per-spec locator comments required by repo guidelines; add a single line comment using the exact format "// `@lat`: [[section-id]]" immediately adjacent to each it(...) test declaration (for example next to the it("initializes, lists tools, and calls the search tool", ...) spec and the other it(...) spec referenced in the review) so each test block has exactly one // `@lat`: [[section-id]] placed next to the corresponding it(...) line.apps/wodsmith-code-mode-mcp/src/index.ts (1)
23-42: 💤 Low valueConsider adding
@lat:comment to reference MCP routing documentation.This entrypoint implements MCP request routing. As per coding guidelines, source code should reference relevant lat.md sections using
//@lat: [[section-id]]comments.As per coding guidelines: Use
//@lat: [[section-id]]comments to tie source code to concepts documented in lat.md/🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/wodsmith-code-mode-mcp/src/index.ts` around lines 23 - 42, Add a "// `@lat`: [[section-id]]" comment above the MCP routing entrypoint to reference the MCP routing documentation: place it near the export default or directly above the async fetch function in this file so readers can link the implementation to the lat.md section; reference the fetch handler and the isMcpPath/handleMcpRequest symbols when choosing the appropriate section-id for MCP routing.apps/wodsmith-code-mode-mcp/src/mcp/truncate.ts (1)
14-22: 💤 Low valueConsider adding
@lat:comment to reference MCP result formatting documentation.This utility formats and truncates MCP results. As per coding guidelines, source code should reference relevant lat.md sections using
//@lat: [[section-id]]comments.As per coding guidelines: Use
//@lat: [[section-id]]comments to tie source code to concepts documented in lat.md/🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/wodsmith-code-mode-mcp/src/mcp/truncate.ts` around lines 14 - 22, Add a // `@lat`: [[section-id]] comment referencing the MCP result formatting documentation above the truncateMcpText utility so the source is tied to lat.md; locate the truncateMcpText function and add a one-line comment (e.g., // `@lat`: [[mcp-result-formatting]] or the correct section id) immediately above the function declaration to indicate which lat.md section documents the formatting/truncation behavior.apps/wodsmith-code-mode-mcp/src/mcp/outbound.ts (1)
20-81: 💤 Low valueConsider adding
@lat:comment to reference MCP documentation.This file implements outbound operation dispatch, which is documented in
lat.md/mcp.md. As per coding guidelines, source code should reference relevant lat.md sections using//@lat: [[section-id]]comments.As per coding guidelines: Use
//@lat: [[section-id]]comments to tie source code to concepts documented in lat.md/🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/wodsmith-code-mode-mcp/src/mcp/outbound.ts` around lines 20 - 81, Add a `@lat` reference to link this outbound operation dispatch implementation to the MCP docs: insert a line comment like // `@lat`: [[mcp.outbound.operation]] (or the canonical section id from lat.md/mcp.md) immediately above the handleMcpOperationOutbound function declaration so the source clearly ties to the documented section for operation dispatch.apps/wodsmith-code-mode-mcp/src/mcp/handler.ts (1)
32-51: 💤 Low valueConsider adding
@lat:comment to reference MCP handler documentation.This request handler orchestrates MCP session lifecycle and request routing. As per coding guidelines, source code should reference relevant lat.md sections using
//@lat: [[section-id]]comments.As per coding guidelines: Use
//@lat: [[section-id]]comments to tie source code to concepts documented in lat.md/🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/wodsmith-code-mode-mcp/src/mcp/handler.ts` around lines 32 - 51, Add a `// `@lat`: [[mcp-handler]]` comment referencing the MCP handler docs near the MCP orchestration code so the source ties to the lat.md section; place the comment immediately above the createWodsmithMcpServer/createMcpHandler block (around the createWodsmithMcpServer, createMcpHandler and server.close usage) so readers can find the documented MCP session lifecycle and routing guidance.apps/wodsmith-code-mode-mcp/src/wodsmith-service.ts (1)
21-32: ⚡ Quick winUse named object parameters for functions with more than one parameter.
The function has 4 parameters but doesn't use an object parameter. As per coding guidelines, functions with more than one parameter should use named object parameters.
♻️ Refactor to use object parameter
export async function callCompetitionOperation( - env: Env, - credential: SessionCredential, - operation: string, - input: unknown, + { + env, + credential, + operation, + input, + }: { + env: Env + credential: SessionCredential + operation: string + input: unknown + }, ): Promise<unknown> { return getWodsmithApp(env).callCompetitionOperation({ credential, operation, input, }) }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/wodsmith-code-mode-mcp/src/wodsmith-service.ts` around lines 21 - 32, Change callCompetitionOperation to accept a single named parameter object instead of four positional args: replace the signature with one parameter (e.g., { env, credential, operation, input }: { env: Env; credential: SessionCredential; operation: string; input: unknown }) and update the body to call getWodsmithApp(env).callCompetitionOperation({ credential, operation, input }). Update all call sites to pass a single object with keys env, credential, operation, and input. Leave types Env and SessionCredential as-is and ensure imports/exports for callCompetitionOperation remain consistent.
🤖 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/crm/src/lib/mcp-handler.ts`:
- Line 6: The inline lat marker comment uses incorrect backtick/quote syntax;
replace the existing ``// `@lat`: [[auth]]`` with the proper single-line marker
format `// `@lat`: [[auth]]` so it follows the required convention (update the
comment in mcp-handler.ts where the current `@lat` marker appears).
In `@apps/crm/src/mcp.ts`:
- Around line 343-377: The audience update logic currently coerces missing
audience arrays to [] which causes accidental deletion; change the handling
around shouldUpdateAudience and the call to updateCampaignAudience so that
input.audienceGymIds and input.audienceContactIds are passed through as-is
(preserve undefined) instead of defaulting to []—or alternatively add an
explicit validation in the handler to reject partial audience updates; update
references: shouldUpdateAudience, updateCampaignAudience, and the create/update
branches that currently use audienceGymIds ?? [] / audienceContactIds ?? [] so
the lower layer receives undefined when the caller omitted a field.
- Line 209: Replace the incorrect backticked annotation comments like // `@lat`:
[[architecture]] with the canonical format // `@lat`: [[architecture]] (and
likewise for the other occurrences noted) so the docs tooling can match them;
search for the literal string "`@lat`" in this file (e.g., the lines currently
containing // `@lat`: [[...]]), remove the backticks and ensure each comment
uses // `@lat`: [[section-id]] consistently.
- Around line 348-389: The handler currently performs
createCampaign/updateCampaign, updateCampaignAudience, and
applyCampaignInteractionActions as separate calls causing partial commits;
replace this orchestration with a single atomic CRM helper (e.g.,
executeCampaignMutationAtomically or performCampaignTransaction) that opens a
DB/CRM transaction, runs the appropriate flow (createCampaign or updateCampaign
with requireValue checks, updateCampaignAudience when shouldUpdateAudience, and
applyCampaignInteractionActions), and commits only if all steps succeed or rolls
back on any error; then have the handler call that helper and return
jsonResponse({ campaign, audience, interactions }) so the entire operation
succeeds or fails as one unit.
- Around line 241-267: The update branch for the "manage_gym" tool enforces
full-replacement by calling requireValue for fields like name before calling
updateGym, which contradicts the optional fields in gymSchema; change the update
path to only require the id (use requireValue(input.id, "id")) and pass the rest
of input through as a partial so updateGym performs a true partial update (do
not call requireValue for name on updates), or alternatively make a distinct
update schema that marks required fields explicitly; apply the same fix pattern
to the other tools mentioned (the analogous update branches for contacts,
interactions, and campaigns referenced in the comment).
- Around line 166-175: The delete branch in the interactions loop forwards only
interaction.id to deleteInteraction, allowing deletion outside the current
campaign; change the logic in the loop that calls deleteInteraction (inside the
interactions processing for-loop) to require and/or validate the current
campaign id (e.g., use the current campaignId from the surrounding
manage_campaign context) and pass it to deleteInteraction (or call a
campaign-scoped method) so the delete is scoped to the current campaign; also
validate that requireValue(interaction.id, "interaction id") belongs to the
current campaign before pushing the result to results to prevent cross-campaign
deletes.
In `@apps/crm/src/server-fns/crm.ts`:
- Around line 1375-1383: The createCampaign path is writing data.audienceGymIds
and data.audienceContactIds directly via setRelations (using audienceGymsField
and audienceContactsField) without the same Company/People ID validation
performed in updateCampaignAudience; replicate or call the same validation logic
from updateCampaignAudience (or extract it to a shared helper) to filter/reject
invalid ids before invoking setRelations for audienceGymIds and
audienceContactIds so create cannot persist non-Company/non-People audience
members.
- Around line 840-855: deleteEntryFromObject currently deletes only the entry
row (and docs) but leaves inbound references in other entries
(setRelation/setRelations store linked ids in entryFieldsTable.value), causing
stale "Unknown ..." links; before the delete in deleteEntryFromObject call
update entryFieldsTable to remove any occurrences of entryId from relation
fields (the JSON/array stored in entryFieldsTable.value) and clear or null the
corresponding relation metadata so other entries no longer reference the deleted
id, then proceed with deleteDocumentsForEntries, assertEntryInObject and the
delete on entriesTable; locate and modify the code around deleteEntryFromObject
and use the same column identifiers (entryFieldsTable.value, entriesTable.id,
entriesTable.objectId) so the update targets inbound references created by
setRelation/setRelations.
- Around line 1178-1181: The code unconditionally calls setRelation(entryId,
companyField.id, clean(data.companyId)) which can persist invalid or non-Company
relations; before calling setRelation (in the block handling companyField and
the similar block at 1204-1207) validate data.companyId using the same guard
used in updateCampaignAudience — e.g., call assertEntriesInObject or otherwise
verify the target entry exists and has type "Company" (or is present in the
validated entries map) and only then pass the cleaned id to setRelation; update
the logic around companyField, setRelation, clean, and entryId to perform this
check and bail out or skip the relation write if validation fails.
- Around line 1262-1277: The companyId and contactId relations are not validated
before calling setRelation, allowing dangling links; mirror the campaignId
pattern used with assertCampaignEntry by cleaning the ids (using
clean(data.companyId) and clean(data.contactId)), calling the appropriate
existence checks (e.g., assertCompanyEntry(companyId) and
assertContactEntry/contact person assertion for person ids) when the cleaned id
is present, then pass the validated id to setRelation (same for the duplicate
block around the other occurrence noted). Use the existing symbols companyField,
contactField, setRelation, clean and assertCampaignEntry as a template to add
the corresponding assert* checks for company and contact/person ids.
In `@apps/wodsmith-code-mode-mcp/src/auth.ts`:
- Around line 107-116: The code currently stores the entire incoming Cookie
header in the SessionCredential (credential: { kind: "cookie", cookie }), which
forwards unrelated cookies downstream; instead build and forward only the
session cookie using the extracted value from getCookieFromHeader (i.e. set
credential.cookie to a single cookie string like
`${SESSION_COOKIE_NAME}=${value}` or equivalent), keeping the existing
decodeSessionToken(parts) logic intact and ensuring only the session cookie (not
the full Cookie header) is included in the returned credential.
- Around line 69-78: The KV payload is parsed without validation causing crashes
on malformed JSON; update the session-loading logic around
generateSessionId/getSessionKey and env.KV_SESSION.get to try/catch JSON.parse
(and validate the resulting object matches KVSession shape, e.g., has
expiresAt), and on parse/validation failure delete the KV entry via
env.KV_SESSION.delete(getSessionKey(parts.userId, sessionId)) and return null so
malformed/partial writes are treated as invalid sessions rather than throwing.
In `@apps/wodsmith-code-mode-mcp/src/mcp/executor.ts`:
- Around line 135-143: The call function currently defaults input to {} which
causes an empty input object to be sent for zero-arg operations; change the
signature to keep input undefined by default (remove input = {}) and build the
request body so that you only add the input property when input !== undefined
(e.g., create payload = { operation } and conditionally payload.input = input),
then JSON.stringify that payload before sending; this preserves true-optional
semantics for wodsmith.call and avoids sending { input: {} } for
read-only/no-arg operations.
In `@apps/wodsmith-code-mode-mcp/src/mcp/outbound.ts`:
- Line 65: The call in outbound.ts passes input ?? {} into
callCompetitionOperation so the handler never receives undefined; add the
required linkage marker comment immediately above or beside the call to
callCompetitionOperation to satisfy the lat.md guideline. Specifically, insert
the comment "// `@lat`:
[[apps/wodsmith-start/src/mcp/competition-operations.ts#callCompetitionOperation]]"
adjacent to the call to callCompetitionOperation(...) in
apps/wodsmith-code-mode-mcp/src/mcp/outbound.ts so the reference is clear and
tied to that invocation.
In `@apps/wodsmith-start/src/mcp/competition-operations.ts`:
- Around line 93-104: The new MCP contract types (CompetitionOperationMode and
CompetitionOperationSpec) and the exported operation catalog/entrypoints are
missing the required source-to-doc linkage comments; add the appropriate `//
`@lat`: [[section-id]]` comment lines immediately above the exported symbols
(e.g., above `export type CompetitionOperationMode`, `export interface
CompetitionOperationSpec`, and above the catalog/entrypoint export declarations
referenced later in the file) so each exported type and public entrypoint has a
matching `// `@lat`:` tag linking it to the corresponding section in lat.md.
- Around line 73-79: The current CrossJsonNode union and the fromSimpleCrossJson
/ x-tss-serialized handling are a partial ad-hoc deserializer that only supports
tags 0/1/2/9/10 and will throw on other seroval/TanStack Start variants; replace
this custom logic by using seroval's full cross-JSON deserialization pipeline:
import and call fromCrossJSON (or the equivalent seroval entry) with
getDefaultSerovalPlugins() (and any Start/TanStack plugins) to deserialize
x-tss-serialized payloads instead of switching on CrossJsonNode.t, updating
references to CrossJsonNode and fromSimpleCrossJson to use the seroval result so
all seroval tags are supported and no manual tag-switching remains.
In `@apps/wodsmith-start/src/mcp/rpc.ts`:
- Around line 24-35: Add the required traceability comments for the RPC surface
by inserting `// `@lat`: [[section-id]]` refs above the RPC types/entrypoints;
specifically add a `// `@lat`:` comment above the McpOperationCall interface and
above the WodsmithMcpOperationsRpc declaration (which references
listCompetitionOperationSpecs and callCompetitionOperation) so the new request
shape and worker entrypoint are tied back to lat.md; ensure the same pattern is
applied for the related block covering lines 61-81 (the other RPC
types/functions) using the appropriate section-id(s).
In `@apps/wodsmith-start/src/utils/auth.ts`:
- Around line 540-545: Add a `// `@lat`:` reference comment above the
withSessionOverride function to link it to its design doc; place a single-line
comment like `// `@lat`: [[session-override]]` immediately before the exported
function declaration `withSessionOverride` (replace `[[session-override]]` with
the exact section id from lat.md if different) so the source is tied to the
documented concept.
In `@apps/wodsmith-start/test/mcp/competition-operations.test.ts`:
- Around line 97-167: Each new test case (the it blocks named "generates stable
unique ids for organizer and cohost competition functions", "executes compiled
TanStack server functions through the server entrypoint", and "unwraps
serialized Response values from compiled TanStack server functions") needs a
single adjacent annotation comment of the form // `@lat`: [[section-id]] placed
next to each respective it(...) declaration; add one unique // `@lat`:
[[section-id]] line per test (adjacent to the it call) so each spec is linked
per the test-guideline format.
---
Nitpick comments:
In `@apps/crm/src/server-fns/auth.ts`:
- Around line 5-6: The inline marker comments use the wrong backtick-wrapped
form; replace them with the exact tooling syntax by changing the comment above
the SESSION_COOKIE export (and the other similar occurrences in this file) from
// `@lat`: [[auth]] to the required form // `@lat`: [[auth]] so the lat tooling
can match the section id (ensure every instance like the ones near
SESSION_COOKIE and the other two occurrences use // `@lat`: [[section-id]] rather
than backticks).
In `@apps/wodsmith-code-mode-mcp/src/index.ts`:
- Around line 23-42: Add a "// `@lat`: [[section-id]]" comment above the MCP
routing entrypoint to reference the MCP routing documentation: place it near the
export default or directly above the async fetch function in this file so
readers can link the implementation to the lat.md section; reference the fetch
handler and the isMcpPath/handleMcpRequest symbols when choosing the appropriate
section-id for MCP routing.
In `@apps/wodsmith-code-mode-mcp/src/mcp/handler.ts`:
- Around line 32-51: Add a `// `@lat`: [[mcp-handler]]` comment referencing the
MCP handler docs near the MCP orchestration code so the source ties to the
lat.md section; place the comment immediately above the
createWodsmithMcpServer/createMcpHandler block (around the
createWodsmithMcpServer, createMcpHandler and server.close usage) so readers can
find the documented MCP session lifecycle and routing guidance.
In `@apps/wodsmith-code-mode-mcp/src/mcp/outbound.ts`:
- Around line 20-81: Add a `@lat` reference to link this outbound operation
dispatch implementation to the MCP docs: insert a line comment like // `@lat`:
[[mcp.outbound.operation]] (or the canonical section id from lat.md/mcp.md)
immediately above the handleMcpOperationOutbound function declaration so the
source clearly ties to the documented section for operation dispatch.
In `@apps/wodsmith-code-mode-mcp/src/mcp/truncate.ts`:
- Around line 14-22: Add a // `@lat`: [[section-id]] comment referencing the MCP
result formatting documentation above the truncateMcpText utility so the source
is tied to lat.md; locate the truncateMcpText function and add a one-line
comment (e.g., // `@lat`: [[mcp-result-formatting]] or the correct section id)
immediately above the function declaration to indicate which lat.md section
documents the formatting/truncation behavior.
In `@apps/wodsmith-code-mode-mcp/src/wodsmith-service.ts`:
- Around line 21-32: Change callCompetitionOperation to accept a single named
parameter object instead of four positional args: replace the signature with one
parameter (e.g., { env, credential, operation, input }: { env: Env; credential:
SessionCredential; operation: string; input: unknown }) and update the body to
call getWodsmithApp(env).callCompetitionOperation({ credential, operation, input
}). Update all call sites to pass a single object with keys env, credential,
operation, and input. Leave types Env and SessionCredential as-is and ensure
imports/exports for callCompetitionOperation remain consistent.
In `@apps/wodsmith-code-mode-mcp/test/mcp/server-protocol.test.ts`:
- Around line 65-67: The two test specs are missing the per-spec locator
comments required by repo guidelines; add a single line comment using the exact
format "// `@lat`: [[section-id]]" immediately adjacent to each it(...) test
declaration (for example next to the it("initializes, lists tools, and calls the
search tool", ...) spec and the other it(...) spec referenced in the review) so
each test block has exactly one // `@lat`: [[section-id]] placed next to the
corresponding it(...) line.
In `@apps/wodsmith-start/src/mcp/competition-operations.ts`:
- Line 1: Remove the unnecessary top-level import "server-only" from the file:
delete the import "server-only" statement so the file no longer depends on that
stubbed alias; ensure there are no other references to server-only in functions
or exports (e.g., any usage around competition-operations module) and run
typechecks to confirm nothing else requires the removed import.
In `@apps/wodsmith-start/src/mcp/rpc.ts`:
- Line 1: Remove the top-level import "server-only" from this file — delete the
import line so the module no longer depends on server-only (the file already
only ships via the server entrypoint); verify no other code in mcp/rpc.ts
requires server-only and run typecheck to ensure no missing types or references.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 3dd5048e-2f62-4882-bee6-3e08eb226300
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (37)
apps/crm/package.jsonapps/crm/src/components/entity-document-panel.tsxapps/crm/src/lib/mcp-handler.tsapps/crm/src/mcp.tsapps/crm/src/routeTree.gen.tsapps/crm/src/routes/api/crm/documents.tsapps/crm/src/routes/api/mcp.tsapps/crm/src/routes/mcp.tsapps/crm/src/server-fns/auth.tsapps/crm/src/server-fns/crm.tsapps/crm/src/server.tsapps/wodsmith-code-mode-mcp/.dev.vars.exampleapps/wodsmith-code-mode-mcp/alchemy.run.tsapps/wodsmith-code-mode-mcp/package.jsonapps/wodsmith-code-mode-mcp/src/auth.tsapps/wodsmith-code-mode-mcp/src/env.d.tsapps/wodsmith-code-mode-mcp/src/index.tsapps/wodsmith-code-mode-mcp/src/mcp/executor.tsapps/wodsmith-code-mode-mcp/src/mcp/handler.tsapps/wodsmith-code-mode-mcp/src/mcp/outbound.tsapps/wodsmith-code-mode-mcp/src/mcp/server.tsapps/wodsmith-code-mode-mcp/src/mcp/truncate.tsapps/wodsmith-code-mode-mcp/src/types.tsapps/wodsmith-code-mode-mcp/src/wodsmith-service.tsapps/wodsmith-code-mode-mcp/test/mcp/server-protocol.test.tsapps/wodsmith-code-mode-mcp/tsconfig.jsonapps/wodsmith-code-mode-mcp/vitest.config.tsapps/wodsmith-code-mode-mcp/wrangler.jsoncapps/wodsmith-start/package.jsonapps/wodsmith-start/src/mcp/competition-operations.tsapps/wodsmith-start/src/mcp/rpc.tsapps/wodsmith-start/src/server.tsapps/wodsmith-start/src/utils/auth.tsapps/wodsmith-start/test/mcp/competition-operations.test.tsapps/wodsmith-start/vite.config.tslat.md/lat.mdlat.md/mcp.md
| import { isAuthenticatedRequest } from "@/server-fns/auth" | ||
|
|
||
| export async function handleMcpRequest(request: Request) { | ||
| // `@lat`: [[auth]] |
Contributor
There was a problem hiding this comment.
Fix the @lat comment syntax here too.
// `@lat`: [[auth]] does not match the required // @lat: [[section-id]] format.
As per coding guidelines, "Use // @lat: [[section-id]] (for JS/TS/Rust/Go/C) or # @lat: [[section-id]] (for Python) comments to tie source code to concepts documented in lat.md/".
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@apps/crm/src/lib/mcp-handler.ts` at line 6, The inline lat marker comment
uses incorrect backtick/quote syntax; replace the existing ``// `@lat`:
[[auth]]`` with the proper single-line marker format `// `@lat`: [[auth]]` so it
follows the required convention (update the comment in mcp-handler.ts where the
current `@lat` marker appears).
Comment on lines
+166
to
+175
| for (const interaction of interactions) { | ||
| if (interaction.action === "delete") { | ||
| const result = await deleteInteraction({ | ||
| id: requireValue(interaction.id, "interaction id"), | ||
| }) | ||
| results.push({ | ||
| action: "delete", | ||
| id: result.id, | ||
| deleted: result.deleted, | ||
| }) |
Contributor
There was a problem hiding this comment.
Scope campaign interaction deletes to the current campaign.
The delete path only forwards interaction.id. A malformed manage_campaign payload can therefore delete an interaction from a different campaign, even though this helper is supposed to manage the current campaign's nested interactions only.
Suggested direction
if (interaction.action === "delete") {
+ const interactionId = requireValue(interaction.id, "interaction id")
+ await assertInteractionBelongsToCampaign({
+ campaignId,
+ interactionId,
+ })
const result = await deleteInteraction({
- id: requireValue(interaction.id, "interaction id"),
+ id: interactionId,
})🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@apps/crm/src/mcp.ts` around lines 166 - 175, The delete branch in the
interactions loop forwards only interaction.id to deleteInteraction, allowing
deletion outside the current campaign; change the logic in the loop that calls
deleteInteraction (inside the interactions processing for-loop) to require
and/or validate the current campaign id (e.g., use the current campaignId from
the surrounding manage_campaign context) and pass it to deleteInteraction (or
call a campaign-scoped method) so the delete is scoped to the current campaign;
also validate that requireValue(interaction.id, "interaction id") belongs to the
current campaign before pushing the result to results to prevent cross-campaign
deletes.
| version: "1.0.0", | ||
| }) | ||
|
|
||
| // `@lat`: [[architecture]] |
Contributor
There was a problem hiding this comment.
Use the canonical @lat annotation format.
These comments are written as // `@lat`: ... instead of // @lat: [[section-id]], so the docs tooling won't match them reliably.
As per coding guidelines, "Use // @lat: [[section-id]] (for JS/TS/Rust/Go/C) or # @lat: [[section-id]] (for Python) comments to tie source code to concepts documented in lat.md/".
Also applies to: 241-241, 270-270, 299-299, 331-331, 393-393
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@apps/crm/src/mcp.ts` at line 209, Replace the incorrect backticked annotation
comments like // `@lat`: [[architecture]] with the canonical format // `@lat`:
[[architecture]] (and likewise for the other occurrences noted) so the docs
tooling can match them; search for the literal string "`@lat`" in this file
(e.g., the lines currently containing // `@lat`: [[...]]), remove the backticks
and ensure each comment uses // `@lat`: [[section-id]] consistently.
Comment on lines
+241
to
+267
| // `@lat`: [[architecture]] | ||
| server.tool( | ||
| "manage_gym", | ||
| "Create, update, or delete a gym/company based on the user's intent.", | ||
| gymSchema.shape, | ||
| async (input) => { | ||
| if (input.action === "delete") { | ||
| return jsonResponse( | ||
| await deleteGym({ id: requireValue(input.id, "id") }), | ||
| ) | ||
| } | ||
| if (input.action === "create") { | ||
| return jsonResponse( | ||
| await createGym({ | ||
| ...input, | ||
| name: requireValue(input.name, "name"), | ||
| }), | ||
| ) | ||
| } | ||
| return jsonResponse( | ||
| await updateGym({ | ||
| ...input, | ||
| id: requireValue(input.id, "id"), | ||
| name: requireValue(input.name, "name"), | ||
| }), | ||
| ) | ||
| }, |
Contributor
There was a problem hiding this comment.
The update tools currently validate partial input but execute full-replacement semantics.
gymSchema, contactSchema, interactionSchema, and campaignSchema mark these fields optional, but the update branches still throw unless callers resend name, fullName, source, title, and name again for campaigns. Requests like “change status only” will pass schema validation and then fail at runtime.
Either make those fields required in the update schemas/tool descriptions, or let the update helpers handle true partial updates.
Also applies to: 271-328, 362-369
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@apps/crm/src/mcp.ts` around lines 241 - 267, The update branch for the
"manage_gym" tool enforces full-replacement by calling requireValue for fields
like name before calling updateGym, which contradicts the optional fields in
gymSchema; change the update path to only require the id (use
requireValue(input.id, "id")) and pass the rest of input through as a partial so
updateGym performs a true partial update (do not call requireValue for name on
updates), or alternatively make a distinct update schema that marks required
fields explicitly; apply the same fix pattern to the other tools mentioned (the
analogous update branches for contacts, interactions, and campaigns referenced
in the comment).
Comment on lines
+343
to
+377
| const shouldUpdateAudience = | ||
| input.action === "update_audience" || | ||
| input.audienceGymIds !== undefined || | ||
| input.audienceContactIds !== undefined | ||
|
|
||
| if (input.action === "create") { | ||
| const campaign = await createCampaign({ | ||
| ...input, | ||
| name: requireValue(input.name, "name"), | ||
| audienceGymIds: input.audienceGymIds ?? [], | ||
| audienceContactIds: input.audienceContactIds ?? [], | ||
| }) | ||
| const interactions = await applyCampaignInteractionActions({ | ||
| campaignId: campaign.id, | ||
| interactions: input.interactions ?? [], | ||
| }) | ||
| return jsonResponse({ campaign, interactions }) | ||
| } | ||
|
|
||
| const campaignId = requireValue(input.id, "id") | ||
| const campaign = | ||
| input.action === "update" || input.name !== undefined | ||
| ? await updateCampaign({ | ||
| ...input, | ||
| id: campaignId, | ||
| name: requireValue(input.name, "name"), | ||
| }) | ||
| : { id: campaignId } | ||
|
|
||
| const audience = shouldUpdateAudience | ||
| ? await updateCampaignAudience({ | ||
| campaignId, | ||
| audienceGymIds: input.audienceGymIds ?? [], | ||
| audienceContactIds: input.audienceContactIds ?? [], | ||
| }) |
Contributor
There was a problem hiding this comment.
Avoid wiping the untouched audience collection on partial audience updates.
When only audienceGymIds or only audienceContactIds is supplied, the other side is coerced to []. That turns a partial update_audience request into destructive data loss for the omitted audience set.
Preserve undefined through to the lower layer, or reject incomplete audience-update payloads explicitly.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@apps/crm/src/mcp.ts` around lines 343 - 377, The audience update logic
currently coerces missing audience arrays to [] which causes accidental
deletion; change the handling around shouldUpdateAudience and the call to
updateCampaignAudience so that input.audienceGymIds and input.audienceContactIds
are passed through as-is (preserve undefined) instead of defaulting to []—or
alternatively add an explicit validation in the handler to reject partial
audience updates; update references: shouldUpdateAudience,
updateCampaignAudience, and the create/update branches that currently use
audienceGymIds ?? [] / audienceContactIds ?? [] so the lower layer receives
undefined when the caller omitted a field.
Comment on lines
+73
to
+79
| type CrossJsonNode = | ||
| | { t: 0; s: number } | ||
| | { t: 1; s: string } | ||
| | { t: 2; s: 0 | 1 | 2 | 3 } | ||
| | { t: 9; a: CrossJsonNode[] } | ||
| | { t: 10; p: { k: string[]; v: CrossJsonNode[] } } | ||
|
|
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
In TanStack Start v1.132.x, what API is intended to deserialize x-tss-serialized server function response bodies, and which serialized value types/tags does it support?
💡 Result:
In TanStack Start v1.132.x, server function response bodies marked with the x-tss-serialized header are intended to be deserialized by the client-side server function fetcher using seroval’s fromCrossJSON/cross-JSON decoding wired with Start’s default seroval plugins. Specifically: - The request/response mechanism uses the X_TSS_SERIALIZED header (value 'true') to indicate that the body is serialized for client deserialization [1]. - On the client, serverFnFetcher checks the response and, for serialized (non-raw) responses, builds a plugin list and uses seroval’s cross-JSON deserialization pipeline (the code imports fromCrossJSON/toJSONAsync, and wires in a raw-stream deserialize plugin plus getDefaultSerovalPlugins() when needed) [2]. Supported serialized value types/tags: The supported value “tags” come from seroval itself (and any plugins). From Start’s code in v1.132.x, the default deserializer uses getDefaultSerovalPlugins() plus an additional createRawStreamDeserializePlugin plugin; it also supports framed protocol responses via TSS_CONTENT_TYPE_FRAMED (decoded with createFrameDecoder) [2]. However, the exact set of serialized value tags (the individual t/c markers used inside the seroval wire format) is not enumerated in the Start v1.132.x sources we located; Start delegates that definition to seroval and its plugins. The seroval package’s serializer implementation shows that plugins are selected via a per-value “tag” (it checks plugin.tag === node.c) and that missing-plugin situations throw SerovalMissingPluginError [3]. Therefore, Start supports whatever value tags seroval + getDefaultSerovalPlugins() enable, including any raw-stream-related plugin tags provided by createRawStreamDeserializePlugin [2]. If you want the concrete list of tag strings/enum members for v1.132.x, I can do a targeted follow-up search specifically for getDefaultSerovalPlugins() and the seroval wire-format tag constants used by those plugins (those are not fully captured in the results above).
Citations:
- 1: https://github.com/TanStack/router/blob/24cc08f8/packages/start-server-core/src/server-functions-handler.ts
- 2: https://github.com/TanStack/router/blob/87a51603/packages/start-client-core/src/client-rpc/serverFnFetcher.ts
- 3: https://cdn.jsdelivr.net/npm/seroval@1.0.7/src/core/context/serializer.ts
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Show the exact decoder/type around the originally cited lines
sed -n '1,120p' apps/wodsmith-start/src/mcp/competition-operations.ts | nl -ba | sed -n '60,110p'
# Show all referenced ranges if they exist
for r in "703,730" "732,760" "70,110" "650,820"; do
echo "==== $r ===="
sed -n "$r p" apps/wodsmith-start/src/mcp/competition-operations.ts | nl -ba
done
# Find where the error string is thrown
rg -n "Unsupported serialized server function result" apps/wodsmith-start/src/mcp/competition-operations.ts || true
# Find decoder function(s) that parse cross JSON / server function serialized responses
rg -n "(fromCrossJSON|cross.*JSON|CrossJsonNode|tss|x-tss-serialized|decoder|deserialize|toCrossJSON|createRawStreamDeserializePlugin|getDefaultSerovalPlugins)" apps/wodsmith-start/src/mcp/competition-operations.ts || true
# Sanity check: confirm whether the type definition is used by the decoder
rg -n "CrossJsonNode" apps/wodsmith-start/src/mcp/competition-operations.ts || trueRepository: wodsmith/thewodapp
Length of output: 105
🏁 Script executed:
#!/bin/bash
set -euo pipefail
file="apps/wodsmith-start/src/mcp/competition-operations.ts"
echo "== File head (first 160 lines, with line numbers via awk) =="
awk '{printf "%5d:%s\n", NR, $0}' "$file" | sed -n '1,160p'
echo
echo "== Original cited region (lines 60-110) =="
awk '{printf "%5d:%s\n", NR, $0}' "$file" | sed -n '60,110p'
echo
echo "== Search for error string =="
rg -n "Unsupported serialized server function result" "$file" || true
echo
echo "== Search for CrossJsonNode / related decoder symbols =="
rg -n "(CrossJsonNode|x-tss-serialized|fromCrossJSON|toCrossJSON|crossJSON|decoder|deserialize|deserialize|createRawStreamDeserializePlugin|getDefaultSerovalPlugins|seroval)" "$file" || true
echo
echo "== Check the other cited ranges if file is that long =="
wc -l "$file"
for r in "703-706" "732-753"; do
start=${r%-*}; end=${r#*-}
if [ "$(wc -l < "$file")" -ge "$end" ]; then
echo "---- $r ----"
awk '{printf "%5d:%s\n", NR, $0}' "$file" | sed -n "${start},${end}p"
else
echo "---- $r (out of range; file only $(wc -l < "$file") lines) ----"
fi
doneRepository: wodsmith/thewodapp
Length of output: 11702
🏁 Script executed:
#!/bin/bash
set -euo pipefail
file="apps/wodsmith-start/src/mcp/competition-operations.ts"
awk '{printf "%5d:%s\n", NR, $0}' "$file" | sed -n '660,770p'Repository: wodsmith/thewodapp
Length of output: 3639
🏁 Script executed:
#!/bin/bash
set -euo pipefail
file="apps/wodsmith-start/src/mcp/competition-operations.ts"
echo "== Imports/usage signals for seroval/full deserializer =="
rg -n "(fromCrossJSON|getDefaultSerovalPlugins|seroval|createRawStreamDeserializePlugin|x-tss-serialized|fromSimpleCrossJson|toCrossJSON)" "$file" || trueRepository: wodsmith/thewodapp
Length of output: 447
Use TanStack Start/seroval’s full cross-JSON deserializer for x-tss-serialized responses (remove the partial t tag switch).
This code hard-codes support for only t: 0/1/2/9/10 and throws Unsupported serialized server function result for any other serialized value tag, so non-matching seroval/TanStack serialized payloads will fail. TanStack Start’s intended x-tss-serialized path deserializes via seroval’s cross-JSON pipeline (using fromCrossJSON + getDefaultSerovalPlugins() / related plugins), which is not limited to this subset.
type CrossJsonNode =
| { t: 0; s: number }
| { t: 1; s: string }
| { t: 2; s: 0 | 1 | 2 | 3 }
| { t: 9; a: CrossJsonNode[] }
| { t: 10; p: { k: string[]; v: CrossJsonNode[] } }File: apps/wodsmith-start/src/mcp/competition-operations.ts
- Lines 73-79 (
CrossJsonNode) - Also applies to 703-706 and 732-753 (
fromSimpleCrossJson/x-tss-serializedhandling)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@apps/wodsmith-start/src/mcp/competition-operations.ts` around lines 73 - 79,
The current CrossJsonNode union and the fromSimpleCrossJson / x-tss-serialized
handling are a partial ad-hoc deserializer that only supports tags 0/1/2/9/10
and will throw on other seroval/TanStack Start variants; replace this custom
logic by using seroval's full cross-JSON deserialization pipeline: import and
call fromCrossJSON (or the equivalent seroval entry) with
getDefaultSerovalPlugins() (and any Start/TanStack plugins) to deserialize
x-tss-serialized payloads instead of switching on CrossJsonNode.t, updating
references to CrossJsonNode and fromSimpleCrossJson to use the seroval result so
all seroval tags are supported and no manual tag-switching remains.
Comment on lines
+93
to
+104
| export type CompetitionOperationMode = "read" | "write" | ||
|
|
||
| export interface CompetitionOperationSpec { | ||
| id: string | ||
| exportName: string | ||
| category: string | ||
| categoryTitle: string | ||
| mode: CompetitionOperationMode | ||
| source: string | ||
| description: string | ||
| input: string | ||
| } |
Contributor
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major | ⚡ Quick win
Add // @lat: refs for the new MCP contract.
The exported operation catalog and entrypoints are missing the required source-to-doc linkage comments.
As per coding guidelines, **/*.{ts,tsx,js,jsx,py,rs,go,h,c}: Use // @lat: [[section-id]] comments to tie source code to concepts documented in lat.md/`.
Also applies to: 756-774
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@apps/wodsmith-start/src/mcp/competition-operations.ts` around lines 93 - 104,
The new MCP contract types (CompetitionOperationMode and
CompetitionOperationSpec) and the exported operation catalog/entrypoints are
missing the required source-to-doc linkage comments; add the appropriate `//
`@lat`: [[section-id]]` comment lines immediately above the exported symbols
(e.g., above `export type CompetitionOperationMode`, `export interface
CompetitionOperationSpec`, and above the catalog/entrypoint export declarations
referenced later in the file) so each exported type and public entrypoint has a
matching `// `@lat`:` tag linking it to the corresponding section in lat.md.
Comment on lines
+24
to
+35
| export interface McpOperationCall { | ||
| credential: SessionCredential | ||
| operation: string | ||
| input?: unknown | ||
| } | ||
|
|
||
| export interface WodsmithMcpOperationsRpc extends Rpc.WorkerEntrypointBranded { | ||
| listCompetitionOperationSpecs(): Promise< | ||
| ReturnType<typeof listCompetitionOperationSpecs> | ||
| > | ||
| callCompetitionOperation(request: McpOperationCall): Promise<unknown> | ||
| } |
Contributor
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major | ⚡ Quick win
Add // @lat: refs for the RPC surface.
The new RPC request shape and worker entrypoint need the required traceability comments back to lat.md.
As per coding guidelines, **/*.{ts,tsx,js,jsx,py,rs,go,h,c}: Use // @lat: [[section-id]] comments to tie source code to concepts documented in lat.md/`.
Also applies to: 61-81
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@apps/wodsmith-start/src/mcp/rpc.ts` around lines 24 - 35, Add the required
traceability comments for the RPC surface by inserting `// `@lat`: [[section-id]]`
refs above the RPC types/entrypoints; specifically add a `// `@lat`:` comment
above the McpOperationCall interface and above the WodsmithMcpOperationsRpc
declaration (which references listCompetitionOperationSpecs and
callCompetitionOperation) so the new request shape and worker entrypoint are
tied back to lat.md; ensure the same pattern is applied for the related block
covering lines 61-81 (the other RPC types/functions) using the appropriate
section-id(s).
Comment on lines
+540
to
+545
| export function withSessionOverride<T>( | ||
| session: NonNullable<SessionValidationResult>, | ||
| fn: () => T, | ||
| ): T { | ||
| return sessionCacheStorage.run({ session: Promise.resolve(session) }, fn) | ||
| } |
Contributor
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major | ⚡ Quick win
Add a // @lat: ref for withSessionOverride.
This new auth/session behavior should be linked to its design section in lat.md like the rest of the documented concepts.
As per coding guidelines, **/*.{ts,tsx,js,jsx,py,rs,go,h,c}: Use // @lat: [[section-id]] comments to tie source code to concepts documented in lat.md/`.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@apps/wodsmith-start/src/utils/auth.ts` around lines 540 - 545, Add a `//
`@lat`:` reference comment above the withSessionOverride function to link it to
its design doc; place a single-line comment like `// `@lat`: [[session-override]]`
immediately before the exported function declaration `withSessionOverride`
(replace `[[session-override]]` with the exact section id from lat.md if
different) so the source is tied to the documented concept.
Comment on lines
+97
to
+167
| it("generates stable unique ids for organizer and cohost competition functions", () => { | ||
| const ids = competitionOperationSpecs.map((operation) => operation.id) | ||
| expect(new Set(ids).size).toBe(ids.length) | ||
|
|
||
| expect(ids).toEqual( | ||
| expect.arrayContaining([ | ||
| "competitions.createCompetition", | ||
| "competitionDetails.getCompetitionById", | ||
| "divisions.addCompetitionDivision", | ||
| "events.saveCompetitionEvent", | ||
| "eventDivisionMappings.saveEventDivisionMappings", | ||
| "workoutLibrary.getWorkouts", | ||
| "movements.getAllMovements", | ||
| "schedule.createHeat", | ||
| "addresses.createAddress", | ||
| "scores.saveCompetitionScore", | ||
| "leaderboards.getCompetitionLeaderboard", | ||
| "results.publishDivisionResults", | ||
| "registrations.createManualRegistration", | ||
| "purchaseTransfers.initiatePurchaseTransfer", | ||
| "invites.issueInvites", | ||
| "waivers.createWaiver", | ||
| "volunteers.inviteVolunteer", | ||
| "judgeScheduling.assignJudgeToHeat", | ||
| "broadcasts.sendBroadcast", | ||
| "pricingRevenue.updateCompetitionFeeConfig", | ||
| "stripeConnect.getStripeConnectionStatus", | ||
| "coupons.createCoupon", | ||
| "sponsors.createSponsor", | ||
| "cohosts.inviteCohost", | ||
| "submissionVerification.verifySubmissionScore", | ||
| "videoSubmissions.getOrganizerSubmissions", | ||
| "reviewNotes.createReviewNote", | ||
| "seriesDivisions.saveSeriesDivisionMappings", | ||
| "seriesEvents.addEventToSeriesTemplate", | ||
| "seriesCohosts.inviteSeriesCohost", | ||
| "cohostSchedule.cohostCreateHeat", | ||
| ]), | ||
| ) | ||
| }) | ||
|
|
||
| it("executes compiled TanStack server functions through the server entrypoint", async () => { | ||
| const input = { probe: true } | ||
|
|
||
| await expect( | ||
| callCompetitionOperation("movements.getAllMovements", input), | ||
| ).resolves.toEqual({ | ||
| ok: true, | ||
| data: input, | ||
| }) | ||
|
|
||
| expect(movementFnState.directCalls).toBe(0) | ||
| expect(movementFnState.serverCalls).toBe(1) | ||
| expect(movementFnState.serverData).toEqual([input]) | ||
| }) | ||
|
|
||
| it("unwraps serialized Response values from compiled TanStack server functions", async () => { | ||
| const input = { serialized: true } | ||
| movementFnState.responseMode = "serialized-response" | ||
|
|
||
| await expect( | ||
| callCompetitionOperation("movements.getAllMovements", input), | ||
| ).resolves.toEqual({ | ||
| ok: true, | ||
| data: input, | ||
| }) | ||
|
|
||
| expect(movementFnState.directCalls).toBe(0) | ||
| expect(movementFnState.serverCalls).toBe(1) | ||
| expect(movementFnState.serverData).toEqual([input]) | ||
| }) |
Contributor
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major | ⚡ Quick win
Add per-spec // @lat: annotations.
Each new it(...) block needs exactly one adjacent // @lat: [[section-id]] comment; none of the new specs are linked right now.
As per coding guidelines, **/*.{test.ts,test.js,test.py,spec.ts,spec.js,spec.py}: Reference test specifications with exactly one // @lat: or # @lat: comment placed next to the relevant test, not at the top of the file, using format // @lat: [[section-id]] or # @lat`: [[section-id]]``.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@apps/wodsmith-start/test/mcp/competition-operations.test.ts` around lines 97
- 167, Each new test case (the it blocks named "generates stable unique ids for
organizer and cohost competition functions", "executes compiled TanStack server
functions through the server entrypoint", and "unwraps serialized Response
values from compiled TanStack server functions") needs a single adjacent
annotation comment of the form // `@lat`: [[section-id]] placed next to each
respective it(...) declaration; add one unique // `@lat`: [[section-id]] line per
test (adjacent to the it call) so each spec is linked per the test-guideline
format.
Contributor
Author
|
Added the MCP fix in commit What changed:
Verification:
|
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
apps/wodsmith-start/src/mcp/competition-operations.ts (1)
648-665: ⚡ Quick winHandle
Errorvalues explicitly indescribeMcpValue.
Errorfields are mostly non-enumerable, so the current object-path logging often drops the actual failure reason. Add aninstanceof Errorbranch before generic object handling.Suggested patch
function describeMcpValue(value: unknown): Record<string, unknown> { if (value === null) return { type: "null" } if (value === undefined) return { type: "undefined" } if (Array.isArray(value)) return { type: "array", length: value.length } + if (value instanceof Error) { + return { + type: "error", + name: value.name, + message: value.message, + } + } if (value instanceof Response) { return { type: "response", status: value.status, ok: value.ok,🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/wodsmith-start/src/mcp/competition-operations.ts` around lines 648 - 665, describeMcpValue currently treats Error instances as generic objects which hides non-enumerable error details; update the function (describeMcpValue) to add an early branch checking "value instanceof Error" before the generic object branch and return a record containing at least { type: "error", name: value.name, message: value.message, stack: value.stack } (optionally truncating stack/message) so error name/message/stack are preserved in logs.
🤖 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/wodsmith-start/src/mcp/competition-operations.ts`:
- Around line 706-730: The catch currently always falls back to the compiled
server entrypoint (executeServer) which can re-run handlers and duplicate side
effects; instead implement a targeted compatibility check (e.g.
isDirectInvocationCompatibilityError) and only call executeServer when the error
matches known direct-invocation/setup failure patterns (otherwise rethrow);
update the catch in the block surrounding
runWithStartContext/createMcpStartContext/handler so it uses that predicate to
decide between falling back to executeServer or rethrowing the original error.
---
Nitpick comments:
In `@apps/wodsmith-start/src/mcp/competition-operations.ts`:
- Around line 648-665: describeMcpValue currently treats Error instances as
generic objects which hides non-enumerable error details; update the function
(describeMcpValue) to add an early branch checking "value instanceof Error"
before the generic object branch and return a record containing at least { type:
"error", name: value.name, message: value.message, stack: value.stack }
(optionally truncating stack/message) so error name/message/stack are preserved
in logs.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 624cb6fb-a73a-41b0-9747-44aaa94b5723
📒 Files selected for processing (6)
apps/wodsmith-code-mode-mcp/src/mcp/outbound.tsapps/wodsmith-code-mode-mcp/src/wodsmith-service.tsapps/wodsmith-start/src/mcp/competition-operations.tsapps/wodsmith-start/src/mcp/rpc.tsapps/wodsmith-start/test/mcp/competition-operations.test.tslat.md/mcp.md
✅ Files skipped from review due to trivial changes (1)
- lat.md/mcp.md
🚧 Files skipped from review as they are similar to previous changes (3)
- apps/wodsmith-code-mode-mcp/src/mcp/outbound.ts
- apps/wodsmith-start/src/mcp/rpc.ts
- apps/wodsmith-start/test/mcp/competition-operations.test.ts
Comment on lines
+706
to
+730
| try { | ||
| console.info("[MCP operations] Executing direct server function handler", { | ||
| handler: handler.name || "<anonymous>", | ||
| input: describeMcpValue(input ?? {}), | ||
| hasRequest: Boolean(request), | ||
| }) | ||
| const result = await runWithStartContext( | ||
| createMcpStartContext(request), | ||
| () => handler({ data: input ?? {} }), | ||
| ) | ||
| console.info("[MCP operations] Direct server function handler returned", { | ||
| handler: handler.name || "<anonymous>", | ||
| result: describeMcpValue(result), | ||
| }) | ||
| return result | ||
| } catch (error) { | ||
| if (typeof executeServer !== "function") { | ||
| throw error | ||
| } | ||
|
|
||
| console.info("[MCP operations] Direct handler failed; falling back to compiled server entrypoint", { | ||
| handler: handler.name || "<anonymous>", | ||
| error: describeMcpValue(error), | ||
| }) | ||
| } |
Contributor
There was a problem hiding this comment.
Avoid catch-all fallback that can execute the same operation twice.
When direct execution throws, the unconditional fallback to __executeServer can re-run write handlers and duplicate side effects. Fallback should be limited to known compatibility/setup failures; otherwise rethrow.
Suggested direction
} catch (error) {
if (typeof executeServer !== "function") {
throw error
}
+ if (!isDirectInvocationCompatibilityError(error)) {
+ throw error
+ }
console.info("[MCP operations] Direct handler failed; falling back to compiled server entrypoint", {
handler: handler.name || "<anonymous>",
error: describeMcpValue(error),
})
}function isDirectInvocationCompatibilityError(error: unknown): boolean {
return (
error instanceof Error &&
/start context|compiled|executeServer|direct handler/i.test(error.message)
)
}🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@apps/wodsmith-start/src/mcp/competition-operations.ts` around lines 706 -
730, The catch currently always falls back to the compiled server entrypoint
(executeServer) which can re-run handlers and duplicate side effects; instead
implement a targeted compatibility check (e.g.
isDirectInvocationCompatibilityError) and only call executeServer when the error
matches known direct-invocation/setup failure patterns (otherwise rethrow);
update the catch in the block surrounding
runWithStartContext/createMcpStartContext/handler so it uses that predicate to
decide between falling back to executeServer or rethrowing the original error.
Contributor
There was a problem hiding this comment.
1 issue found across 6 files (changes from recent commits).
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="apps/wodsmith-start/src/mcp/competition-operations.ts">
<violation number="1" location="apps/wodsmith-start/src/mcp/competition-operations.ts:721">
P1: The catch block falls back to `executeServer` for all direct-call errors, which can re-run write operations and duplicate side effects. Restrict fallback to known context/bootstrap failures and rethrow other errors.</violation>
</file>
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
| result: describeMcpValue(result), | ||
| }) | ||
| return result | ||
| } catch (error) { |
Contributor
There was a problem hiding this comment.
P1: The catch block falls back to executeServer for all direct-call errors, which can re-run write operations and duplicate side effects. Restrict fallback to known context/bootstrap failures and rethrow other errors.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At apps/wodsmith-start/src/mcp/competition-operations.ts, line 721:
<comment>The catch block falls back to `executeServer` for all direct-call errors, which can re-run write operations and duplicate side effects. Restrict fallback to known context/bootstrap failures and rethrow other errors.</comment>
<file context>
@@ -670,7 +703,38 @@ async function callServerFunction(
+ result: describeMcpValue(result),
+ })
+ return result
+ } catch (error) {
+ if (typeof executeServer !== "function") {
+ throw error
</file context>
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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/wodsmith-start/src/mcp/oauth.ts`:
- Around line 111-158: authorize() issues auth codes to any redirect_uri from
the query string; fix this by persisting registered clients (including an
exact-match array of redirect_uris) at registerClient time (e.g., store under a
KV like env.KV_CLIENTS keyed by clientId) and then in authorize() look up the
client by clientId and verify redirect_uri exactly matches one of the registered
redirect_uris before issuing the code (use the same check in the token exchange
path that consumes OAuthCodeRecord/OAUTH_CODE_PREFIX to ensure client_id +
redirect_uri are validated against the persisted client entry and reject if not
an exact match).
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: abd2fd27-eac7-4e4a-b2d0-e3e26db12b6c
📒 Files selected for processing (9)
apps/wodsmith-code-mode-mcp/alchemy.run.tsapps/wodsmith-code-mode-mcp/src/auth.tsapps/wodsmith-code-mode-mcp/src/env.d.tsapps/wodsmith-code-mode-mcp/src/index.tsapps/wodsmith-code-mode-mcp/src/oauth-resource.tsapps/wodsmith-code-mode-mcp/test/mcp/server-protocol.test.tsapps/wodsmith-start/src/mcp/oauth.tsapps/wodsmith-start/src/server.tslat.md/mcp.md
🚧 Files skipped from review as they are similar to previous changes (3)
- apps/wodsmith-code-mode-mcp/test/mcp/server-protocol.test.ts
- apps/wodsmith-code-mode-mcp/alchemy.run.ts
- apps/wodsmith-code-mode-mcp/src/auth.ts
Comment on lines
+111
to
+158
| async function authorize(request: Request): Promise<Response> { | ||
| const url = new URL(request.url) | ||
| const redirectUri = url.searchParams.get("redirect_uri") | ||
| const clientId = url.searchParams.get("client_id") | ||
| const responseType = url.searchParams.get("response_type") | ||
| const codeChallenge = url.searchParams.get("code_challenge") | ||
| const codeChallengeMethod = url.searchParams.get("code_challenge_method") | ||
| const state = url.searchParams.get("state") | ||
|
|
||
| if ( | ||
| !redirectUri || | ||
| !clientId || | ||
| responseType !== "code" || | ||
| !codeChallenge || | ||
| codeChallengeMethod !== "S256" | ||
| ) { | ||
| return new Response("Invalid OAuth authorization request", { status: 400 }) | ||
| } | ||
|
|
||
| const session = await getSessionFromRequestCookie(request) | ||
| if (!session?.userId) { | ||
| const signInUrl = new URL("/_auth/sign-in", baseUrl(request)) | ||
| signInUrl.searchParams.set("redirect", `${url.pathname}${url.search}`) | ||
| return Response.redirect(signInUrl.toString(), 302) | ||
| } | ||
|
|
||
| const code = randomToken() | ||
| const record: OAuthCodeRecord = { | ||
| clientId, | ||
| redirectUri, | ||
| codeChallenge, | ||
| codeChallengeMethod, | ||
| userId: session.userId, | ||
| createdAt: Date.now(), | ||
| } | ||
| await env.KV_SESSION.put( | ||
| `${OAUTH_CODE_PREFIX}${code}`, | ||
| JSON.stringify(record), | ||
| { | ||
| expirationTtl: CODE_TTL_SECONDS, | ||
| }, | ||
| ) | ||
|
|
||
| return redirectWithParams(redirectUri, { | ||
| code, | ||
| ...(state ? { state } : {}), | ||
| }) | ||
| } |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Look for any persisted OAuth client / redirect_uri allowlist validation
rg -nP --type=ts -C3 'redirect_uri|redirectUri|registered_?client|allow(ed)?[_ ]?redirect' apps/wodsmith-start/src/mcp
rg -nP --type=ts -C3 'mcp_oauth_client|oauth_client' appsRepository: wodsmith/thewodapp
Length of output: 4754
Fix auth-code interception by validating client_id + exact redirect_uri against a persisted client/redirect_uris registry
In apps/wodsmith-start/src/mcp/oauth.ts, authorize() accepts redirect_uri (and client_id) directly from the query string and redirects the issued authorization code there after only checking required OAuth/PKCE parameters—there’s no validation against any registered allowlist/registry. With an attacker-controlled redirect_uri, an authenticated user can be tricked into minting a code that the attacker then redeems at /oauth/token (token checks only that client_id, redirect_uri, and PKCE match the stored code record, which the attacker controls). Repo search in apps/wodsmith-start/src/mcp didn’t surface any other redirect_uri/registered-client allowlist enforcement.
Persist registered clients (and exact redirect_uris) at registerClient time, then enforce exact-match validation of client_id + redirect_uri in authorize() (and ensure the same checks are applied during token redemption).
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@apps/wodsmith-start/src/mcp/oauth.ts` around lines 111 - 158, authorize()
issues auth codes to any redirect_uri from the query string; fix this by
persisting registered clients (including an exact-match array of redirect_uris)
at registerClient time (e.g., store under a KV like env.KV_CLIENTS keyed by
clientId) and then in authorize() look up the client by clientId and verify
redirect_uri exactly matches one of the registered redirect_uris before issuing
the code (use the same check in the token exchange path that consumes
OAuthCodeRecord/OAUTH_CODE_PREFIX to ensure client_id + redirect_uri are
validated against the persisted client entry and reject if not an exact match).
Contributor
There was a problem hiding this comment.
3 issues found across 9 files (changes from recent commits).
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="apps/wodsmith-code-mode-mcp/alchemy.run.ts">
<violation number="1" location="apps/wodsmith-code-mode-mcp/alchemy.run.ts:57">
P2: Always setting `WODSMITH_AUTHORIZATION_SERVER_URL` overrides the localhost fallback and points dev/local MCP OAuth metadata to production.</violation>
</file>
<file name="apps/wodsmith-start/src/mcp/oauth.ts">
<violation number="1" location="apps/wodsmith-start/src/mcp/oauth.ts:121">
P0: `/oauth/authorize` does not validate `client_id`/`redirect_uri` against a registered client, enabling token issuance to attacker-controlled redirect URIs.</violation>
<violation number="2" location="apps/wodsmith-start/src/mcp/oauth.ts:173">
P2: Token endpoint returns `unsupported_grant_type` for malformed authorization_code requests instead of `invalid_request`.</violation>
</file>
Tip: Review your code locally with the cubic CLI to iterate faster.
Re-trigger cubic
| const state = url.searchParams.get("state") | ||
|
|
||
| if ( | ||
| !redirectUri || |
Contributor
There was a problem hiding this comment.
P0: /oauth/authorize does not validate client_id/redirect_uri against a registered client, enabling token issuance to attacker-controlled redirect URIs.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At apps/wodsmith-start/src/mcp/oauth.ts, line 121:
<comment>`/oauth/authorize` does not validate `client_id`/`redirect_uri` against a registered client, enabling token issuance to attacker-controlled redirect URIs.</comment>
<file context>
@@ -0,0 +1,241 @@
+ const state = url.searchParams.get("state")
+
+ if (
+ !redirectUri ||
+ !clientId ||
+ responseType !== "code" ||
</file context>
| WorkerRef({ service: getWodsmithServiceName(stage) }), | ||
| "WodsmithMcpOperations", | ||
| ), | ||
| WODSMITH_AUTHORIZATION_SERVER_URL: getAuthorizationServerUrl(stage), |
Contributor
There was a problem hiding this comment.
P2: Always setting WODSMITH_AUTHORIZATION_SERVER_URL overrides the localhost fallback and points dev/local MCP OAuth metadata to production.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At apps/wodsmith-code-mode-mcp/alchemy.run.ts, line 57:
<comment>Always setting `WODSMITH_AUTHORIZATION_SERVER_URL` overrides the localhost fallback and points dev/local MCP OAuth metadata to production.</comment>
<file context>
@@ -46,6 +54,7 @@ const worker = await Worker("app", {
WorkerRef({ service: getWodsmithServiceName(stage) }),
"WodsmithMcpOperations",
),
+ WODSMITH_AUTHORIZATION_SERVER_URL: getAuthorizationServerUrl(stage),
},
domains: getDomains(stage),
</file context>
Suggested change
| WODSMITH_AUTHORIZATION_SERVER_URL: getAuthorizationServerUrl(stage), | |
| WODSMITH_AUTHORIZATION_SERVER_URL: | |
| stage === "prod" || stage === "demo" || | |
| Boolean(process.env.WODSMITH_AUTHORIZATION_SERVER_URL) | |
| ? getAuthorizationServerUrl(stage) | |
| : undefined, |
| const codeVerifier = String(body.get("code_verifier") ?? "") | ||
|
|
||
| if (grantType !== "authorization_code" || !code || !codeVerifier) { | ||
| return json({ error: "unsupported_grant_type" }, { status: 400 }) |
Contributor
There was a problem hiding this comment.
P2: Token endpoint returns unsupported_grant_type for malformed authorization_code requests instead of invalid_request.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At apps/wodsmith-start/src/mcp/oauth.ts, line 173:
<comment>Token endpoint returns `unsupported_grant_type` for malformed authorization_code requests instead of `invalid_request`.</comment>
<file context>
@@ -0,0 +1,241 @@
+ const codeVerifier = String(body.get("code_verifier") ?? "")
+
+ if (grantType !== "authorization_code" || !code || !codeVerifier) {
+ return json({ error: "unsupported_grant_type" }, { status: 400 })
+ }
+
</file context>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Verification
Live MCP check
Notes
Summary by cubic
Fixes undefined Code Mode MCP results by unwrapping Start-serialized responses, and adds an OAuth-backed MCP stack for searching and executing WODsmith competition and CRM operations.
New Features
apps/wodsmith-code-mode-mcpWorker exposing/mcpand/api/mcp; registerssearchandexecutetools; proxies to Start via a service binding./mcp,/api/mcp) behind existing auth using@modelcontextprotocol/sdk.wodsmith-startwith@tanstack/start-storage-context.Bug Fixes
Response, fixing undefined results in Code Mode; added regression tests for the response shape and MCP server protocol.Written for commit 8daf756. Summary will update on new commits.
Summary by CodeRabbit
New Features
Documentation
Chores
Tests