fix(mcp): preserve cached workspace arguments across schema upgrades - #375
JunkaiWang-TheoPhy wants to merge 2 commits into
Conversation
ChatGPT can retain the former workspaceId argument after schemas migrate to workspace_id. Normalize only that alias at the authenticated HTTP edge and reject contradictory handles before tool validation, while keeping the advertised schema canonical. Constraint: Preserve OAuth and workspace containment boundaries Confidence: high Scope-risk: narrow Tested: HTTP regression fails with original server and passes with compatibility adapter; all 26 server tests; TypeScript typecheck Not-tested: Live ChatGPT cached session and packaged installation Related: Waishnav#373
Alias conflicts must remain recognizable to MCP clients: retain string, numeric and null request IDs and accept notifications without sending JSON-RPC responses. Existing generic error responses retain their default null ID. Confidence: high Scope-risk: narrow Tested: Original conflict handler fails string and numeric ID plus notification regressions; all 31 server tests pass after repair; TypeScript typecheck Not-tested: Live ChatGPT error presentation
|
Navigate logical layers of code changes, visualize relationships, and explore their blast radius. No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Advanced Run ID: 📒 Files selected for processing (2)
Included review availability: This review used your included allowance. Your plan provides up to 8 included reviews per hour; 6 remain after this review. 📝 WalkthroughWalkthroughThe ChangesCached workspace argument compatibility
Priority: ➖ Normal Estimated code review effort: 2 (Simple) | ~10 minutes Change: Bug fix · Severity of issue fixed: Medium Suggested reviewers: Merge Risk: ⚪ Minimal · up to The compatibility change appears ready to merge after normal checks; no concrete blocking risk remains. Security Architecture ReviewSecurity architecture risk: 🔵 Low · up to The endpoint accepts one legacy workspace argument after authentication and keeps the existing workspace and file-access checks. No bypass was found, but the available coverage does not establish that every client behavior has been exercised. Retained concerns Security review detailsSecurity Blast Radius
Trust Boundaries and Controls
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ 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. A rabbit taps the fields in line Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
 |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| for (const workspaceId of [null, 42, {}, []]) { | ||
| const invalid = await postModernMcp(localBaseUrl, accessToken, "tools/call", { | ||
| name: "read", arguments: { workspaceId, path: "note.txt" }, | ||
| }); | ||
| const body = await invalid.json(); | ||
| assert.ok(body.error || body.result?.isError, JSON.stringify(body)); | ||
| assert.notEqual(invalid.status, 500); |
There was a problem hiding this comment.
The assertions accept any tool error for a non-string workspaceId. They still pass when schema validation is bypassed and the value fails later during workspace handling, so a validation regression could go unnoticed. Assert that the response specifically reports invalid workspace_id input.
Artifacts
HTTP probe source for non-string workspace IDs
- The executed probe authenticates, sends four non-string IDs to the read endpoint, and applies the test's exact two assertions.
HTTP responses with string schema validation enabled
- The command and captured responses show HTTP 200 OK validation errors for all four inputs, with both original assertions passing.
HTTP responses with string schema validation bypassed
- The command and captured responses show HTTP 200 OK non-validation errors for the same inputs, yet both original assertions still pass.
Comments Outside DiffThese findings sit on lines the diff does not cover, so they could not be posted inline. Each one leaves this list once its file changes.
|
1 participant
A ChatGPT connector created against v1.0.8 can retain
workspaceIdin cached tool calls after the server starts advertisingworkspace_id. The authenticated workspace and canonical read still work, but the old call fails input validation. #339 documents this upgrade boundary, and #373 narrows the proposed transition behavior.This adds one compatibility step at the authenticated HTTP edge: normalize an own
tools/call.arguments.workspaceIdproperty before existing tool validation. Public schemas continue to advertise snake_case. Identical old/new values are accepted; contradictory values return Invalid params with the original JSON-RPC request ID. Notifications receive an empty HTTP 202 response. Invalid workspace values still reach the existing schema checks, and OAuth and filesystem authorization are unchanged.The HTTP regression fails on the original implementation with
workspace_id: Invalid input: expected string, received undefined, then passes after the change. Retaining the new tests while reverting the ID fix also reproducedexpected: 'cached-request'/actual: nulland the notification-status mismatch. All 31 server tests and TypeScript typecheck passed locally after restoration. Tests cover canonical schemas, both aliases, conflicts, invalid values, authentication, outside-root reads, response IDs, and notifications. Live ChatGPT error presentation and packaged installation were not tested.#303 intentionally added no compatibility aliases. This proposal is limited to the observed cached workspace argument and its refresh path; other legacy field names still require the host to refresh descriptors. It does not extend aliases through the domain model. The two touched files contain the HTTP edge change and its regression coverage. External research references do not apply to this maintenance change.
Closes #373. Related to #339 and #303.
Summary by CodeRabbit
workspaceIdare now handled consistently withworkspace_id. Requests that provide conflicting values receive an HTTP 400 error.