fix(ai-sdk): only warn about requestContext conflict when route context has entries#17654
Conversation
|
|
@Aayush-engineer is attempting to deploy a commit to the Mastra Team on Vercel. A member of the Team first needs to authorize it. |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
💤 Files with no reviewable changes (1)
WalkthroughThe workflow route handler's warning for ignored requestContext now requires ChangesRequest Context Warning Refinement
🎯 1 (Trivial) | ⏱️ ~3 minutes 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
PR triageLinked issue check passed (#17653). Mastra uses CodeRabbit for automated code reviews. Please address all feedback from CodeRabbit by either making changes to your PR or leaving a comment explaining why you disagree with the feedback. Since CodeRabbit is an AI, it may occasionally provide incorrect feedback. PR complexity score
Applied label: Changed test gateChanged Test Gate is pending. The |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
client-sdks/ai-sdk/src/workflow-route.ts (1)
242-254:⚠️ Potential issue | 🔴 Critical | ⚡ Quick winCritical: Incomplete fix - merge logic must also check
size()to prevent silent data loss.The warning condition at line 242 correctly checks
contextRequestContext.size() > 0, but the merge logic at line 254 still usescontextRequestContext || params.requestContext. Since an emptyRequestContextinstance is truthy, line 254 will use the emptycontextRequestContextinstead ofparams.requestContext, silently ignoring the user-provided context without any warning.This defeats the purpose of the fix. Per the PR objective, when
contextRequestContextis empty (defaulted by middleware), it should be treated as "not set" andparams.requestContextshould be used.🔧 Proposed fix to align merge logic with warning condition
- if (contextRequestContext && params.requestContext && contextRequestContext.size() > 0) { + const hasRouteContext = contextRequestContext && contextRequestContext.size() > 0; + if (hasRouteContext && params.requestContext) { mastra .getLogger() ?.warn( `"requestContext" from the request body will be ignored because "requestContext" is already set in the route options.`, ); } const handlerOptions = { mastra, workflowId: workflowToUse, params: { ...params, - requestContext: contextRequestContext || params.requestContext, + requestContext: hasRouteContext ? contextRequestContext : params.requestContext, }, includeTextStreamParts, sendReasoning, sendSources, };🤖 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 `@client-sdks/ai-sdk/src/workflow-route.ts` around lines 242 - 254, The merge logic currently uses contextRequestContext || params.requestContext which will pick an empty RequestContext instance (truthy) and discard params.requestContext; change the assignment in handlerOptions.params.requestContext to only use contextRequestContext when it has entries by replacing the fallback with a size() check (e.g., use contextRequestContext when contextRequestContext && contextRequestContext.size() > 0, otherwise use params.requestContext) so it matches the earlier warning condition in workflow-route.ts (mastra.getLogger().warn).
🤖 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.
Outside diff comments:
In `@client-sdks/ai-sdk/src/workflow-route.ts`:
- Around line 242-254: The merge logic currently uses contextRequestContext ||
params.requestContext which will pick an empty RequestContext instance (truthy)
and discard params.requestContext; change the assignment in
handlerOptions.params.requestContext to only use contextRequestContext when it
has entries by replacing the fallback with a size() check (e.g., use
contextRequestContext when contextRequestContext && contextRequestContext.size()
> 0, otherwise use params.requestContext) so it matches the earlier warning
condition in workflow-route.ts (mastra.getLogger().warn).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 9e469823-fb11-4ebb-ae16-07ad22e5cf95
📒 Files selected for processing (1)
client-sdks/ai-sdk/src/workflow-route.ts
2 participants
Description
The
requestContextwarning inworkflowRoutewas firing on every request that included arequestContextin the body, even when there was no real conflict.Root cause: the server middleware at
server-adapter/index.tsalways setsc.set('requestContext', c.env?.requestContext ?? new RequestContext())— socontextRequestContextis neverundefined, it's at least an emptyRequestContextinstance. This made thecontextRequestContext && params.requestContextcheck always truthy when a bodyrequestContextwas present.Fix: only warn when
contextRequestContextactually has entries (size() > 0), meaning it was explicitly populated — not just defaulted to an empty instance by the middleware.Related issue(s)
Fixes #17653
Type of change
Checklist
ELI5
The system was warning that request info from the request body would be ignored even when there was no real conflict. The fix: only show that warning when the route's requestContext actually contains entries, not when it's just an empty default.
Summary
This PR fixes a spurious warning in workflowRoute that was emitted whenever a request body included requestContext, even if the route's requestContext was only the middleware-provided empty default.
Related Issue: fixes
#17653Type: Bug fix