Skip to content

Fix #737: [Memory] namespaceFromRequest echoes undefined keys, erasing the token-bound use - #738

Open
Memtensor-AI wants to merge 1 commit into
MemTensor:mainfrom
Memtensor-AI:bugfix/autodev-737-20261025031654937
Open

Memtensor-AI wants to merge 1 commit into
MemTensor:mainfrom
Memtensor-AI:bugfix/autodev-737-20261025031654937

Conversation

@Memtensor-AI

Copy link
Copy Markdown
Collaborator

Description

Fixed #737: header-absent requests no longer erase a token-bound namespace, so multi-tenant session writes stop landing in "local-user".

Root cause was two-layered: namespaceFromRequest() returned own properties with undefined values for every absent x-memmy-* header, and mergeNamespaces() spread that over the token-bound principal namespace, letting undefined win before normalizeNamespace() fell back to "local-user".

Changes (Memory/src/server/http.ts):

  • namespaceFromRequest() now only emits keys for headers that were actually sent (userId/tenantId/projectId/workspaceId/workspacePath/profileLabel/sessionKey), while source/profileId keep their existing defaults.
  • mergeNamespaces() now spreads only defined, non-null fields via a new definedNamespaceFields() helper, hardening every caller that passes a partially filled namespace (e.g. request bodies and source-derived namespaces).

Tests (Memory/tests/contract/memory-rest-service.test.ts):

  • "keeps the token-bound namespace when a request sends only a subset of headers": two cloud tokens sharing a tenant, header-less session open stores userId=tenant-acme/source=source-b; an explicit x-memmy-user-id still overrides.
  • "does not let partial headers erase tenantId while merging request namespaces": profile-only headers preserve userId, tenantId and projectId from the bound namespace.
    Both tests fail on the pre-fix code with "local-user" and pass with the fix.

Verification: npm run typecheck -w @memmy/memory passes; full vitest run --dir tests gives 1438 passed / 1 skipped with 2 failures in tests/cli.test.ts that are pre-existing on the baseline (they need a built dist, reproduced with the fix stashed). Contract suite 54/54 and session suite 132/132 pass.

Related Issue (Required): Fixes #737

Type of change

Please delete options that are not relevant.

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)
  • Refactor (does not change functionality, e.g. code style improvements, linting)
  • Documentation update

How Has This Been Tested?

Automated tests are pending.

  • Unit Test
  • Test Script Or Test Steps (please provide)
  • Pipeline Automated API Test (please provide)

Checklist

  • I have performed a self-review of my own code
  • I have commented my code in hard-to-understand areas
  • I have added tests that prove my fix is effective or that my feature works
  • I have created related documentation issue/PR in MemOS-Docs (if applicable)
  • I have linked the issue to this PR (if applicable)
  • I have mentioned the person who will review this PR

Reviewer Checklist

namespaceFromRequest echoed every x-memmy-* header as an own property,
including absent ones with an undefined value. mergeNamespaces then
spread that object over the token-bound principal namespace, so
undefined overwrote userId/tenantId and normalizeNamespace fell back to
"local-user". Multi-tenant writes were persisted under the wrong user
even though reads still used the principal's userId.

- namespaceFromRequest now only includes headers that were actually sent
- mergeNamespaces drops undefined/null fields before spreading, hardening
  every caller that passes a partially filled namespace
- contract tests cover header-less session open against a cloud token and
  partial headers alongside an explicit namespace

Refs MemTensor#737
@Memtensor-AI

Copy link
Copy Markdown
Collaborator Author

πŸ€– Open Code Review

Target: PR #738
Task: 09fef2c2a778be90
Base: main
Head: bugfix/autodev-737-20261025031654937
Head SHA: 8c8e39108ab9a371d7346bcef65064a5e0a7123a

πŸ” OpenCodeReview found 3 issue(s) in this PR.


1. Memory/src/server/http.ts (L1621-L1622)

The undefined-erasure is only fixed for absent headers. authenticate() calls mergeNamespaces(cloudNamespace, namespaceFromRequest(request, url)) (and the same for scopedApiKeys), i.e. the request namespace is passed as the second argument, and the second argument wins on every spread field. So a client that sends x-memmy-user-id: bob together with a token bound to alice still overrides the token-bound userId/tenantId/projectId. Only source (via the DEFAULT_NAMESPACE_SOURCE guard) and profileId (resolved explicitly) are protected.

Suggest making precedence explicit inside mergeNamespaces β€” the token-bound (principalNamespace) side always wins for every field β€” or swapping the argument order at the authenticate() call sites, so the fix does not rely on callers remembering which position is authoritative. A regression test covering token(userId=alice) + x-memmy-user-id: bob would pin this down.


2. Memory/src/server/http.ts (L1439-L1440)

The "omit absent fields" invariant is now implemented twice with two different mechanisms: the conditional-spread chain here and definedNamespaceFields(). Note the invariant is also only partially applied here β€” source and profileId are still materialized unconditionally (with DEFAULT_NAMESPACE_SOURCE / "default"), which contradicts the comment above and leaves their correct behaviour dependent on the special cases inside mergeNamespaces.

Consider extracting a single helper (e.g. build the raw object, then pass it through the same compaction used by mergeNamespaces) so a future namespace field cannot be compacted in one place but not the other.


3. Memory/src/server/http.ts (L1639-L1645)

definedNamespaceFields copies through an untyped Record<string, unknown> and then casts to Partial<RuntimeNamespace>, so the compiler can no longer tell which namespace fields the loop covers. If a field is later added to (or made required in) RuntimeNamespace β€” authenticatedNamespace/tenantId style additions β€” nothing fails to compile and the field is silently dropped from every merged namespace, which is the exact class of bug this helper is meant to prevent. Iterating a typed key list avoids the cast entirely; strictEnvelopeWithPrincipal already declares such a list (Array<keyof RuntimeNamespace>), so it is worth extracting one shared constant. Note also that the !== null branch is unreachable given RuntimeNamespace only declares string | undefined values.

πŸ’‘ Suggested Change

Before:

  const fields: Record<string, unknown> = {};
  for (const [key, value] of Object.entries(namespace)) {
    if (value !== undefined && value !== null) {
      fields[key] = value;
    }
  }
  return fields as Partial<RuntimeNamespace>;

After:

  const fields: Partial<RuntimeNamespace> = {};
  for (const key of NAMESPACE_FIELD_KEYS) {
    const value = namespace[key];
    if (value !== undefined) {
      fields[key] = value;
    }
  }
  return fields;

Generated by cloud-assistant via Open Code Review.

@Memtensor-AI

Copy link
Copy Markdown
Collaborator Author

πŸ”§ Open Code Review requested Agent fix

Open Code Review found 3 issue(s). I have resumed the development Agent to fix them.

  • Task: 09fef2c2a778be90
  • Fix attempt: 1/2
  • Finding delta: 0 repeated / 3 new / 0 likely resolved

The Agent will push a new commit to this PR branch. OCR will recheck after the commit is pushed.

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[Memory] namespaceFromRequest echoes undefined keys, erasing the token-bound userId β€” multi-tenant writes land in local-user

2 participants