Fix nonexistent container tag handling in MCP tools#1192
Conversation
|
Claude finished @ishaanxgupta's task in 1m 24s —— View job Reviewing PR #1192
Overview: Adds upfront container tag existence validation to Issues found: None — this looks good to ship. The latest commit (276d354) addresses both issues from the previous vorflux review:
Additional observations:
Score: 10/10 |
Deploying with
|
| Status | Name | Latest Commit | Updated (UTC) |
|---|---|---|---|
| ❌ Deployment failed View logs |
supermemory-mcp | 99501d3 | Jul 04 2026, 02:47 PM |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
Overview: Adds upfront container tag existence validation to set-active-tag and search_memory tools, with improved error messaging for 403 responses.
Issues found: None — this looks good to ship.
The implementation is well-structured:
- RBAC checks correctly precede existence checks in both tools, preventing information leakage about valid container tags
- The
containerTagExistshelper uses a sensible cache-first strategy with one-shot refresh to handle freshly-created tags - Error handling is consistent:
search-memory.tsrelies on its outer try-catch whileset-active-tag.tswraps the async check appropriately - The improved 403 message in the client provides helpful context as defense-in-depth
Score: 10/10
Deploying with
|
| Status | Name | Latest Commit | Preview URL | Updated (UTC) |
|---|---|---|---|---|
| ✅ Deployment successful! View logs |
supermemory-app | 99501d3 | Commit Preview URL Branch Preview URL |
Jul 04 2026, 02:49 PM |
…rch_memory set-active-tag accepted any tag string even if it did not exist, and search_memory surfaced a generic "Access forbidden" 403 for unknown tags. Both tools now validate the tag against the user's container tag list and return a clear "does not exist" error pointing at listSpaces. Co-authored-by: Cursor <cursoragent@cursor.com>
ved015
force-pushed
the
fix/mcp-invalid-container-tag
branch
from
500a4b5 to
df671c6
Compare
|
Warning This pull request is not mergeable via GitHub because a downstack PR is open. Once all requirements are satisfied, merge this PR as a stack on Graphite.
How to use the Graphite Merge QueueAdd the label Main to this PR to add it to the merge queue. You must have a Graphite account in order to use the merge queue. Sign up using this link. An organization admin has enabled the Graphite Merge Queue in this repository. Please do not merge from GitHub as this will restart CI on PRs being processed by the merge queue. This stack of pull requests is managed by Graphite. Learn more about stacking. |
![vorflux[bot]](https://avatars.githubusercontent.com/in/2483280?s=60&v=4){kind=small}
There was a problem hiding this comment.
Summary
Reviewed — found 2 issue(s). I reviewed the MCP container-tag validation changes across apps/mcp/src/server/client/index.ts, apps/mcp/src/server/tools/search-memory.ts, apps/mcp/src/server/tools/set-active-tag.ts, and the new apps/mcp/src/server/tools/validate-container-tag.ts for correctness, missing error handling, and regressions.
Findings
apps/mcp/src/server/tools/search-memory.ts
search_memoryvalidates only an explicitargs.containerTag, but the tool searches witheffectiveTagafterresolveContainerTag(), so an invalid stored active tag can still reach the API.
apps/mcp/src/server/tools/validate-container-tag.ts
containerTagExists()converts refresh failures into a nonexistent-tag result becauserefreshContainerTags()can swallow failures and leave the old cache unchanged.
Verdict
| } | ||
| if ( | ||
| args.containerTag && | ||
| !(await containerTagExists(deps, args.containerTag)) |
There was a problem hiding this comment.
This validates only an explicit args.containerTag, but the request below uses effectiveTag after deps.resolveContainerTag(args.containerTag). If activeContainerTag already contains a deleted/nonexistent tag, calling search_memory without containerTag skips this check and still constructs the client with the invalid resolved tag. Please validate the resolved effectiveTag before deps.getClient(effectiveTag), not just the raw argument.
| containerTag: string, | ||
| ): Promise<boolean> { | ||
| if (deps.cachedContainerTags().includes(containerTag)) return true | ||
| await deps.refreshContainerTags() |
There was a problem hiding this comment.
containerTagExists() treats deps.refreshContainerTags() as authoritative, but the current refresh implementation can catch/log failures without throwing and leave the cached list unchanged. On a cache miss, a transient /v3/container-tags/list failure then returns false here and reports a valid tag as nonexistent. Please make validation distinguish could not validate from does not exist or otherwise propagate/use the fetched refresh result instead of converting refresh failures into missing tags.
TestingPR-scoped MCP invalid container-tag behavior passed functional verification through targeted invalid-tag coverage, real local Wrangler MCP transport checks, changed-file Biome CI, and an existing auth smoke subset. The existing OAuth smoke subset had unrelated stale-contract failures, and the temporary targeted tests verified the new invalid-tag behavior without repository code changes. Commands run: cd /code/supermemoryai/supermemory/apps/mcp
export PATH="$HOME/.bun/bin:$PATH"
bunx vitest run --config "/var/tmp/testing_responses/3dd136e3-1bcf-4918-9827-9b8c06734872/vitest.targeted.config.ts" --pool=forks --no-file-parallelism
cd /code/supermemoryai/supermemory/apps/mcp
export PATH="$HOME/.bun/bin:$PATH"
bun run dev:app
node "/var/tmp/testing_responses/3dd136e3-1bcf-4918-9827-9b8c06734872/local-mcp-invalid-tag.mjs"
cd /code/supermemoryai/supermemory
export PATH="$HOME/.bun/bin:$PATH"
bunx biome ci apps/mcp/src/server/client/index.ts apps/mcp/src/server/tools/search-memory.ts apps/mcp/src/server/tools/set-active-tag.ts apps/mcp/src/server/tools/validate-container-tag.ts
cd /code/supermemoryai/supermemory/apps/mcp
export PATH="$HOME/.bun/bin:$PATH"
bunx vitest run e2e/auth.test.ts e2e/oauth.test.ts --pool=forks --no-file-parallelism
NODE_OPTIONS=--max-old-space-size=8192 bunx tsc --noEmit -p apps/mcp/tsconfig.jsonResult: Verdict✅ Passed. PR-scoped invalid container-tag behavior was verified; the observed OAuth and TypeScript failures appear unrelated/existing rather than introduced by this PR. |
Return structured error views from set-active-tag so the widget shows clear user-facing copy instead of a generic unrecognized-response error. Validate the resolved workspace in search_memory, and distinguish tag list refresh failures from genuinely missing tags. Co-authored-by: Cursor <cursoragent@cursor.com>
There was a problem hiding this comment.
Overview: Adds upfront container tag existence validation to set-active-tag and search_memory tools, with improved error messaging for 403 responses.
Issues found: None — this looks good to ship.
The latest commit (276d354) addresses both issues from the previous vorflux review:
-
effectiveTagvalidation fixed:search_memorynow validates the resolvedeffectiveTag(lines 57-65), not just the rawargs.containerTag. If a storedactiveContainerTagreferences a deleted tag, the check will catch it before hitting the API. -
Refresh failure handling fixed:
refreshContainerTags()now returnsboolean, andvalidateContainerTag()returns a three-state enum ("exists" | "missing" | "unavailable"). Transient API failures return"unavailable"with a retry-friendly error message, while confirmed missing tags return"missing"with the specific "does not exist" error.
Additional observations:
- RBAC checks correctly precede existence checks in both tools, preventing information leakage about valid container tags
- The improved 403 message in
client/index.tsprovides helpful context as defense-in-depth - Widget error handling properly surfaces both structured and text-only error results
Score: 10/10
Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
2 participants
Summary
set-active-tagaccepted any container tag string, even one that does not exist (e.g.eng_949_invalid_nonexistent_tag), and happily persisted it as the active workspace. It now validates the tag against the user's actual container tag list (cached list with a one-shot refresh for freshly created tags) and returnsContainer tag '<tag>' does not exist. Use listSpaces to see the available container tags.search_memorywith a nonexistentcontainerTagsurfaced the genericError: Access forbidden.from the API 403. It now performs the same existence check up front and returns the clear "does not exist" error instead.listSpacesinstead of the bare "Access forbidden." message, in case a 403 still reaches it through another path.