feat(memory-platform): per-key MCP scoping, key rotation, plan entitlement, and the web surface

[undivisible]

Memories as a platform: per-key scoping, key rotation, plan entitlement, and the web surface

Combines the backend and web slices of the memory-platform product into one reviewable change.

Stacks on PR #11033. This branches from pr-10783-memory-api-mcp-zkr, not main — the /v1/memory/platform endpoints do not exist on main. Review the delta against that base.

---

Review round 2 — what the bots found, and what was actually wrong

Codex and cubic filed 30 inline comments. Several were correct and the product did not work. Corrections in this round, most severe first.

1. The embeddable widget could not be framed at all

Not a sandbox or storage problem, as an earlier revision of this description guessed. next.config.mjs sent X-Frame-Options: DENY on every route, so /memory-platform/widget — a widget whose entire purpose is to be embedded in a host page — rendered a broken-document placeholder inside any iframe, in dev and in production alike.

Only the widget route is opened up, with Content-Security-Policy: frame-ancestors *; every other route keeps DENY. X-Frame-Options cannot be relaxed per-route by a later rule, so the catch-all excludes the widget path with a negative lookahead. Framing it from any origin is safe because the widget carries no ambient authority: the published embed sandboxes it without allow-same-origin, so it has an opaque origin, no cookies, and no durable storage, and holds no session a framing page could exercise. It is inert until the host explicitly hands it a token.

2. The widget had no way to get a session when framed

The sandboxed frame's opaque origin denies it every durable storage API, so Firebase cannot establish a session inside it and live search always fell through to "Sign in". Relaxing the sandbox is not the fix — a document granted both allow-scripts and allow-same-origin can rewrite its own sandbox attribute and escape entirely, which test_memory_platform_docs_guards.py correctly fails on.

The host page owns the credential instead. The frame asks with omi.memory.embed.session-request; the host mints a short-lived token server-side for the visitor it has already authenticated and replies with omi.memory.embed.session. The token lives only in hook memory for the frame's lifetime. Top-level, the Firebase session remains the source of truth.

Messages are validated by event.source, never by origin. An opaque-origin sender always reports origin === "null", so the previous event.origin !== 'https://h.omi.me' check dropped every event.

3. Rotation: the previous "strictly uncached before the swap" claim was wrong

The earlier description claimed the retired hash was uncached strictly before the swap and that this closed the window. That claim was wrong as written. Deleting the cache first loses the race it was meant to win: an authentication that already read the pre-swap Firestore document writes the retired hash back into the cache after the delete, keeping a rotated-away secret valid for the remainder of the one-hour TTL.

Rotation now tombstones the retired hash durably before touching the cache, and authentication refuses to honor or write a cache entry whose fence is not provably absent. The tombstone outlives every auth-cache TTL, so the racing write is harmless — no later request can authorize from it. An unreadable fence is treated as retired, which costs a Firestore lookup and never authorizes a retired secret. MCP and Developer keys share one helper, retired_hash_fences_cache, so the two rotations cannot drift.

4. Scope semantics were not rollout-safe

Per-key scopes became authoritative without bumping MCP_API_KEY_AUTH_CONTEXT_VERSION. During a rolling deploy an old revision — whose normalize_mcp_scopes always widened to full access — could cache full scopes for a key a new revision recorded as read-only, and the new revision would honor that entry for up to an hour. Version bumped to 4.

5. Search rendered empty

The client declared memories; ProductMemorySearchResponse returns items. The field never existed on the wire, so every successful search decoded to nothing.

6. Browser requests never reached the backend

The client modules resolved their base URL from envConfig.API_URL, which reads process.env.API_URL — not a NEXT_PUBLIC_* variable, so Next.js never inlines it into the client bundle. In the browser it is undefined, the base collapses to '', and every key, billing, and platform request went to the web origin as a relative path. All three browser clients now share one browserApiBase() helper reading NEXT_PUBLIC_API_BASE_URL. Audited every module in src/lib/api/: apps.ts is server-only and correctly keeps API_URL.

Also fixed

• getToken identity was recreated every render; the keys and billing pages use it as a useCallback dependency and those callbacks as useEffect dependencies, so every successful load immediately triggered another. Those pages issued requests continuously. Memoized.
• Router bound the legacy global db proxy; now resolves get_firestore_client() at the call boundary.
• The documented 400 for invalid search bounds was unreachable — Query constraints made FastAPI answer 422 before the handler ran. The handler owns validation now.
• X-App-Platform: web on /v1/payments/available-plans. Without it should_show_new_plans treats the storefront as a legacy client: the catalog drops Operator and exposes deprecated Neo.
• Billing panel used Promise.all, so one failure discarded the successful sibling and rendered the fallback Free plan next to the error. Settled independently.
• Snippets: MCP config pointed at https://api.omi.me/mcp, which is registered nowhere (the transport is /v1/mcp/sse); the proxy example implied a durable OMI_SERVER_KEY works against a route that accepts only Firebase ID tokens.
• Docs: the quota section pointed at /v1/payments/overage-info, which meters chat questions, not platform requests.
• Guard scope: the quota seam tripwire scanned for /v1/payments/ rather than the quota path it claims to protect.

Reviewed and rejected

• Masthead offset (cubic, P1, confidence 10): claimed the masthead starts beneath the fixed AppHeader and asked for pt-[6.5rem]/md:pt-28. Verified visually against a production build at 1280px and 390px — the masthead clears the header exactly. AppHeader is 4rem, which is also what main's own mobile-menu top-16 assumes. Applying the suggestion would open a 2.5rem gap.
• Duplication findings (rotate_dev_key vs rotate_mcp_key, memory_platform_usage.py vs phone_call_usage.py, DB-layer orchestration): fair observations, but extracting shared credential-rotation and monthly-counter primitives is a refactor with blast radius beyond this PR. The rotation fence is the shared surface that matters for correctness and it is factored. Not doing a speculative extraction inside a security fix.

---

Backend

Per-key scoping. McpApiKeyCreate accepts scopes; the route 400s on an unknown or empty scope list rather than silently widening. Absent or unreadable scope state predates the per-key contract and still resolves to full access, so already-issued keys keep working — proven by test_unmigrated_key_without_recorded_scopes_still_authorizes_with_full_access.

Rotation issues a new secret for an existing key id, preserving name, scopes, app identity, creation time, and grants, returning the raw key exactly once. Hard cutover by design, now actually enforced by the retirement fence above. An unconfirmed fence or delete returns 503 rather than issuing a new secret — the caller is told rotation is unavailable instead of being handed a second live credential.

Quota: free tier 1000/month, every paid plan uncapped. platform_api_requests_per_month on PlanLimits, metered with a Redis monthly counter. None means uncapped, so shipping this gate cannot regress a paying subscriber, and Free keeps a usable allowance on an API that was previously ungated. Over-quota is a bounded 429 naming plan, limit, usage, and reset instant. Counter outages fail open through record_fallback.

Metering paid usage is out of scope and needs human sign-off. Charging for platform-API usage is a pricing decision, deliberately not made here.

Web

/memory-platform plus /docs, /embed, /keys, /billing, and /widget, on Tailwind + shadcn in a dark theme.

Guards added

Cross-boundary guards compare the declared TypeScript interfaces against real introspection of the Pydantic models, so renaming a model field fails until the client follows: test_web_quota_client_type_matches_the_response_model (MemoryPlatformQuota) and test_web_search_client_type_matches_the_response_model (ProductMemorySearchResponse). Siblings forbid browser clients from reading the server-only API_URL, and pin the widget-route frame policy. Labelled as static checkers — they read source rather than executing it.

---

Verification

Mutation-verified (broke it, confirmed the failure, reverted):

Guard	Mutation	Result
Rotation fence	force retired_hash_fences_cache to False	both interleaving tests fail — retired secret still authorizes
Cache version	restore MCP_API_KEY_AUTH_CONTEXT_VERSION = 3	read-only key widened to all nine scopes
Search decode	rename items back to memories	assert not {'memories'}
Browser origin	restore envConfig.API_URL in mcp-keys.ts	origin guard fails
400 bounds	restore the Query constraints	assert 422 == 400 on all five cases
Frame policy	restore the '/(.*)?' catch-all	frame guard fails

Backend — 55 tests pass across test_api_key_rotation.py, test_mcp_api_key_scoping.py, test_mcp_api_key_full_access.py, test_memory_platform_quota.py, test_memory_platform_router.py, test_memory_platform_docs_guards.py, test_api_key_route_contract.py, test_memory_platform_api_service.py, run through backend/test.sh (not bare pytest).

Web (web/frontend) — npm test 105/105 pass; npm run lint clean; npx tsc --noEmit reports 33 errors, all pre-existing and unchanged (same 33 with the branch stashed), in src/components/trends/, tendencies/, apps/, chat/, tasks/, and memories/memory-list/ — files this PR does not touch. npm run build succeeds.

Browser — the embed now actually works

Against a production build, with a host page framing the widget at sandbox="allow-scripts" (no allow-same-origin) and a stub backend returning a real items payload:

ANY msg type=omi.memory.embed.session-request origin="null" sourceMatches=true
sent session token
ANY msg type=omi.memory.embed.ready         origin="null" sourceMatches=true
ANY msg type=omi.memory.embed.resize        origin="null" sourceMatches=true

GET /v1/memory/platform/search?query=launch&limit=20&offset=0 auth= Bearer host-minted-token

The results rendered in the frame. That single run exercises four of the fixes end-to-end: the frame loads (frame policy), the handshake completes with origin="null" proving the opaque origin and sourceMatches=true proving source validation is the right control, the request reaches the configured backend origin rather than a relative path, and the response decodes from items.

All six routes return 200 on the production server. The masthead offset was inspected at 1280px and 390px.

Clicked through, signed in, against a local stack

An earlier revision of this PR said the signed-in flows were "verified by contract, not
clicked, because there is no Firebase project available locally". That gap is now closed.
web/frontend can point at the Firebase Auth emulator (NEXT_PUBLIC_FIREBASE_AUTH_EMULATOR_HOST,
gated to non-production builds and loopback hosts only), and the repo's own harness
(PROVIDER_MODE=offline make dev-up) supplies the backend on 127.0.0.1:8000 with the
Firestore/Auth emulators. Setup is documented in web/frontend/README.md.

Signed in through the real header sign-in button as an emulator Google account
(uid LHKnvoooJ9Xbi7NmpFCghHJ16ioN), then exercised, in a browser:

Flow	What I saw
Create key with selected scopes	POST /v1/mcp/keys 200. Dialog showed omi_mcp_802481c4... once with "Copy it now. You will not see this key again."
After dismissing the reveal	Table renders name / omi_mcp_8024...ecf9… / conversations.read goals.read memories.read / created / last-used only. No raw key anywhere.
400 on a bad scope list	Submitting with every scope unchecked returned 400 and rendered Invalid scopes. Available: [...]. This surfaced a real defect — the message was painted behind the still-open dialog's overlay. Fixed in this PR; the message now renders inside the dialog.
Rotation	POST /v1/mcp/keys/{id}/rotate 200, new secret shown once. Old secret: GET /v1/mcp/memories 200 before, 401 after. New secret: 200. No grace window.
Revocation	DELETE /v1/mcp/keys/{id} 204, row disappears, the secret then returns 401.
Billing panel	Real plan and quota. A sibling request genuinely failed (GET /v1/payments/available-plans 500 — no Stripe credentials locally) and the panel still rendered the plan from overage-info 200 and 0 / 1000 requests from GET /v1/memory/platform/quota 200, with the error shown alongside. This is exactly the Promise.allSettled behaviour the code comments claim.
Framed widget	Against a production build, from a separate origin (http://127.0.0.1:4321), sandbox="allow-scripts" with no allow-same-origin: session-request origin="null" srcMatch=true → host-minted token → ready → resize.

The dev-harness CORS gap this exposed (backend default-deny allowlist was empty, so the
first browser POST died on preflight with 400) is fixed in a separate commit.

Still not exercised, and why

• Search returning actual result rows. The wire field is confirmed to be items — an
  empty-result search returns {"uid":...,"query":...,"items":[],...}, so this PR's
  items decode is right. But any non-empty result 500s on a pre-existing backend bug
  that is not in this PR's diff: fetch_default_product_memory_search returns
  List[Dict[str, Any]] projections (memory_id, memory_layer, lifecycle_status, ...)
  while ProductMemorySearchResponse.items is declared List[MemoryItem], so FastAPI's
  response validation rejects every row. Reproduced with a real canonical user:
  GET /v1/memory/platform/search?query=zzzznomatch → 200, ?query=software → 500. Filed as GET /v1/memory/platform/search 500s on any non-empty result page #11438; no router test covers a non-empty page, which is why it has gone unnoticed.
• A framed widget calling the API directly. The handshake works, but the frame's own
  fetch is preflighted with Origin: null (that is what the opaque sandbox origin means)
  and the backend's explicit allowlist rejects it — observed as
  OPTIONS /v1/memory/platform/search ... 400 from the framed instance. Allowing null
  would be unsafe, so in practice the framed widget must go through the server-side proxy
  the docs already recommend; the direct-call branch is unreachable when framed as published.
• Stripe checkout and the customer portal, which need real Stripe credentials.

Needs human sign-off

• Opening /memory-platform/widget to frame-ancestors * is a deliberate relaxation of a repository-wide X-Frame-Options: DENY. The reasoning is above and I believe it is sound, but it is a security-policy decision on a shared config file and should be confirmed rather than assumed.
• Charging for platform-API usage remains an unmade pricing decision.

Product invariants affected

• INV-MEM-1

Failure-Class: none

[cursor]

Bugbot couldn't run - usage limit reached

Bugbot is counted against Cursor usage for this user or team, and this run hit a usage or spend limit.

A user or team admin can review and increase usage limits in the Cursor dashboard.

(requestId: serverGenReqId_ac56f57e-1c58-410a-8fbd-f54e36f1815c)

[chatgpt-codex-connector]

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 32f2068be9

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

• Open a pull request for review
• Mark a draft as ready
• Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

[chatgpt-codex-connector]

Fence authentication while rotating cached credentials

When an old-key authentication overlaps rotation, it can miss the just-deleted cache, read the still-old Firestore hash before key_ref.update(), and repopulate the retired hash after the rotation completes; subsequent requests then authorize from that cache for up to its one-hour TTL. The same delete-before-swap ordering exists in rotate_dev_key, so both supposedly hard-cutover rotations need serialization or generation/version fencing that prevents an in-flight lookup from recaching the retired credential.

Useful? React with 👍 / 👎.

[undivisible]

Correct, and the PR body's claim that the retired hash was "uncached strictly before the swap" was wrong as written. Fixed in cd2be95.

Rotation now tombstones the retired hash durably before deleting the cache, and authentication refuses to honor or write a cache entry whose fence is not provably absent. The tombstone outlives the auth-cache TTL, so the racing write is harmless. Unreadable fence is treated as retired. MCP and dev keys share one helper (retired_hash_fences_cache) so they can't drift.

Regression tests reproduce the interleaving by committing the rotation inside the authentication window. Mutation-verified — forcing the fence to False:

test_rotation_fences_an_authentication_that_races_the_secret_swap FAILED
E  assert {'app_id': 'mcp-api', 'key_id': 'key-1', ...} is None
test_dev_rotation_fences_an_authentication_that_races_the_secret_swap FAILED

[chatgpt-codex-connector]

Stabilize the token getter before using it in effects

For a signed-in user, this creates a new getToken function on every render. KeysManager and BillingPanel use it as a useCallback dependency and then use those callbacks as useEffect dependencies, so every successful load updates state, recreates the callback, and immediately starts another load; these pages continuously issue key, payment, and quota requests. Memoize getToken or otherwise remove the unstable function identity from the effect chain.

Useful? React with 👍 / 👎.

[chatgpt-codex-connector]

Read search results from the response's items field

Every successful platform search is decoded with the wrong property name: the backend's ProductMemorySearchResponse returns items, while this interface declares memories and MemoryWidget reads response.memories. Consequently, a signed-in search discards all returned memories and always renders an empty result list; align the client type and consumer with the actual items payload.

Useful? React with 👍 / 👎.

[undivisible]

Fixed in 56dfd0a — decoded from items, widget updated. Added test_web_search_client_type_matches_the_response_model, which diffs the TS interface against real ProductMemorySearchResponse.model_fields introspection. Mutation-verified: renaming back to memories fails with assert not {'memories'}. Confirmed end-to-end in a framed production build — the results render.

[chatgpt-codex-connector]

Authenticate the proxy with a supported credential

The published server-proxy example implies that a durable OMI_SERVER_KEY can call /v1/memory/platform/search, but that route depends on get_current_user_uid, which explicitly accepts only Firebase ID tokens and rejects MCP/developer key families; a static Firebase ID token would also expire. Users following this example therefore receive 401s, so the proxy must forward or mint a refreshable user session, or the route must explicitly support an appropriate scoped server key.

Useful? React with 👍 / 👎.

[undivisible]

Fixed in c34b135 — the proxy example forwards the visitor's own refreshable Omi ID token instead of a durable key, and the credentials section now states that this route accepts only Firebase ID tokens and rejects the MCP/dev key families.

[chatgpt-codex-connector]

Validate the sandboxed frame by source instead of its URL origin

With the documented sandbox="allow-scripts" and no allow-same-origin, messages from the frame have the opaque origin "null", not https://h.omi.me, so this listener drops every ready/resize event. The iframe example also points at your-app.example, making the hard-coded Omi origin inconsistent even without sandboxing; validate event.source against the expected iframe window and handle the deliberately opaque origin safely.

Useful? React with 👍 / 👎.

[undivisible]

Fixed in c34b135 — snippet and doc both validate event.source === frame.contentWindow and explain why origin can't work here. Confirmed live: every message from the sandboxed frame arrived as origin="null" with sourceMatches=true.

[chatgpt-codex-connector]

Point MCP clients at the registered SSE endpoint

The published MCP client configuration uses https://api.omi.me/mcp, but a repo-wide route search finds the hosted transport only at /v1/mcp/sse, which is also the endpoint reported by the MCP info and OAuth resource configuration; there is no /mcp route or rewrite. Anyone copying this configuration receives a 404 and cannot connect, so use https://api.omi.me/v1/mcp/sse throughout the snippets and docs.

Useful? React with 👍 / 👎.

[undivisible]

Fixed in c34b135 — snippets and docs/memory/api-service.md now use https://api.omi.me/v1/mcp/sse.

[chatgpt-codex-connector]

Use the injectable Firestore client in the new router

This new router binds the legacy global db compatibility proxy and passes it through every authorization, search, and ingest path, coupling the handlers to process-global state and preventing tests or alternate runtimes from injecting the owned client boundary. The backend guide explicitly reserves db for legacy code and requires new paths to resolve get_firestore_client() at the call boundary with injectable helpers.

AGENTS.md reference: backend/AGENTS.md:L191-L193

Useful? React with 👍 / 👎.

[undivisible]

Fixed in 29e1246 — every authorization, search, and ingest path resolves get_firestore_client() at the call boundary. Tests inject the client through an autouse fixture instead of asserting on the global.

[chatgpt-codex-connector]

Preserve successful billing data when a sibling request fails

If either /v1/payments/overage-info or /v1/payments/available-plans fails, Promise.all discards the successful sibling response and neither state value is updated. The page then renders the fallback Free, zero questions, and zero overage alongside the error even when the backend successfully returned the user's paid plan and usage; settle these requests independently and retain whichever result succeeded.

Useful? React with 👍 / 👎.

[undivisible]

Fixed in be74f02 — both requests settle independently with Promise.allSettled, so a successful sibling is retained and the error is reported alongside real data.

[chatgpt-codex-connector]

Return the documented 400 for invalid search bounds

For real HTTP requests with a query over 500 characters or an out-of-range limit/offset, FastAPI enforces these Query constraints before entering search_memory_platform, so _validate_search_bounds never produces its intended 400 and clients receive the framework's 422 response instead. The direct-call unit test and published API documentation promise 400; either let the handler own validation or update the public contract and clients to expect 422.

Useful? React with 👍 / 👎.

[undivisible]

Confirmed and fixed in 29e1246 — the handler owns validation now; the bounds stay documented as OpenAPI descriptions. Mutation-verified: restoring the Query constraints fails all five cases with assert 422 == 400.

[chatgpt-codex-connector]

Point quota consumers at the platform quota endpoint

This quota section directs API consumers to /v1/payments/overage-info, but that endpoint reports chat-question usage and chat overage rather than Memory Platform requests. The newly added platform allowance is exposed by /v1/memory/platform/quota; following the current documentation yields unrelated counts and no platform limit or remaining, so document the actual quota endpoint here.

Useful? React with 👍 / 👎.

[undivisible]

Fixed in be74f02 — the quota section now documents GET /v1/memory/platform/quota and its fields, and states that /v1/payments/overage-info is a different meter (chat questions).

[cubic-dev-ai]

15 issues found across 67 files

Confidence score: 2/5

• In web/frontend/src/lib/api/memory-platform.ts, the search response is modeled as memories while the endpoint returns items, so successful queries can look empty to users. Align the client type and widget consumption to items so results actually render.
• In backend/database/mcp_api_key.py, concurrent rotate requests can each return success with different secrets even though only the last secret remains valid, which creates immediate auth failures for one caller. Make rotation a serialized/conditional transition (or fail/retry the second request) so responses stay truthful.
• In backend/utils/mcp_scopes.py (and the optional scopes flow in backend/models/mcp_api_key.py), empty/invalid scope input can normalize to full access, creating an unintended privilege escalation path. Treat empty or invalid scopes as an error path and require explicit scopes at creation.
• In web/frontend/src/lib/api/billing.ts, scheduled-cancel reactivation can succeed server-side but appear to do nothing because the UI only handles redirect URLs and not status/message responses. Model the non-redirect success shape and surface a clear success state in the panel.

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="web/frontend/src/app/memory-platform/components/copy-button.tsx">

<violation number="1" location="web/frontend/src/app/memory-platform/components/copy-button.tsx:31">
P3: When clipboard write fails (e.g. navigator.clipboard unavailable in non-secure contexts or permission denied), the catch block silently does nothing, so the user gets no feedback that the copy failed and the button stays looking idle. Consider a transient error state or at least a console/diagnostic so failure isn't silent.</violation>
</file>

<file name="web/frontend/src/app/memory-platform/components/platform-shell.tsx">

<violation number="1" location="web/frontend/src/app/memory-platform/components/platform-shell.tsx:25">
P1: Memory-platform masthead starts beneath the fixed AppHeader because this shell reserves only 4rem; reserve 6.5rem on narrow screens and 7rem at `md` so page content clears the header.</violation>
</file>

<file name="web/frontend/src/lib/api/memory-platform.ts">

<violation number="1" location="web/frontend/src/lib/api/memory-platform.ts:27">
P1: Successful platform searches render no results because the client models `memories` while the endpoint returns `items`. Model the response as `items` and have the widget consume that field.</violation>
</file>

<file name="web/frontend/src/app/memory-platform/components/embed-preview.tsx">

<violation number="1" location="web/frontend/src/app/memory-platform/components/embed-preview.tsx:32">
P3: The `ready` event is emitted before this parent effect subscribes, so the live preview cannot observe or demonstrate readiness. Register the listener before mounting `MemoryWidget`, or route readiness through a component callback.</violation>
</file>

<file name="backend/database/mcp_api_key.py">

<violation number="1" location="backend/database/mcp_api_key.py:260">
P1: Two overlapping rotate requests can each return 200 with a different secret, but only the last writer's secret authenticates. Make rotation a serialized/conditional state transition so a second request retries or fails rather than returning a credential immediately superseded by the other request.</violation>
</file>

<file name="web/frontend/src/lib/api/billing.ts">

<violation number="1" location="web/frontend/src/lib/api/billing.ts:98">
P2: Reactivating a scheduled-to-cancel subscription succeeds without a redirect but appears to do nothing because this response type omits the backend’s `status`/`message` path and the panel only handles `url`. Model `session_id`, `status`, and `message`, then show the reactivation result or reload billing state.</violation>
</file>

<file name="backend/tests/unit/test_api_key_rotation.py">

<violation number="1" location="backend/tests/unit/test_api_key_rotation.py:127">
P3: The dev-key rotation test is weaker than its MCP counterpart: it never asserts `rotated_at` was set, never verifies the retired secret was evicted from the auth cache, and the ownership test omits the missing-key case (the MCP tests cover all three). A regression in dev rotation's cache-clear ordering or `rotated_at` write would pass unnoticed.</violation>
</file>

<file name="backend/models/mcp_api_key.py">

<violation number="1" location="backend/models/mcp_api_key.py:24">
P2: McpApiKeyCreate.scopes is optional, so a new key can be created with scopes omitted entirely. The router's validation is guarded by `if key_data.scopes is not None`, and a None value flows into create_mcp_key → normalize_mcp_scopes(None), which resolves to MCP_FULL_ACCESS_SCOPES. This contradicts the PR's per-key scoping intent (and its claim that create-path scope problems no longer widen to full access): a client that omits scopes silently gets a full-access key. Consider making scopes required for new key creation (or treating None on create as an invalid-scope 400) so every new key carries explicit scopes.</violation>
</file>

<file name="backend/tests/unit/test_memory_platform_router.py">

<violation number="1" location="backend/tests/unit/test_memory_platform_router.py:24">
P3: The module-scoped fixture mutates `sys.modules`/`utils.other.endpoints` with a hand-built stub instead of using the sanctioned `backend/testing/import_isolation.py` helper or a conftest-based seam, which the repo guidance (AGENTS.md 'Never mutate sys.modules at module scope in tests' and prior feedback to avoid module-scope sys.modules stubs for routers) steers away from. The partial stub (only `get_current_user_uid`, no `with_rate_limit`) plus module `__all__`-driven imports can silently mask behavior of `_rate_limited_uid`/`within_rate_limit`; prefer the sanctioned isolation helper so the pattern stays consistent and auditable.</violation>
</file>

<file name="backend/database/dev_api_key.py">

<violation number="1" location="backend/database/dev_api_key.py:118">
P2: This new function places an entire rotation orchestration in the database layer: it orders the cache invalidation strictly before the Firestore secret swap, generates the new credential, updates the document, and then builds the response model through the read boundary. Per the repo convention, DB modules should stay focused on persistence, with this decision/orchestration logic living in a util/service layer and the DB write carrying only the reference object id. As it stands, correctness-critical ordering ("delete retired secret from cache before persisting the new one") is buried inside the low-level persistence module, which makes the safety invariant harder to audit, and it duplicates the same orchestration already wedged into `mcp_api_key.rotate_mcp_key`. Consider extracting the rotation flow (read → ownership check → cache retire → swap → projection) into a shared service/util used by both key kinds, keeping the DB layer as thin persistence.</violation>

<violation number="2" location="backend/database/dev_api_key.py:147">
P3: New `rotate_dev_key` is a near-verbatim duplicate of the existing `rotate_mcp_key` (~60 lines). The two differ only in the cache-delete function, key generator, Pydantic model, app-id handling, and scope normalizer; the structure (read doc → ownership check → hash validity → retire cache strictly → generate → update → project → strict-parse) is identical. Since these two credential types share the exact same hard-cutover rotation contract, the shared flow can be parameterized by a small kind descriptor (cache fn, generator, model, normalize helpers) and reused, so rotation semantics can't drift between Dev and MCP keys. If a shared helper is intentionally avoided, a comment explaining why the duplication is worthwhile would help.</violation>
</file>

<file name="backend/models/memory_platform.py">

<violation number="1" location="backend/models/memory_platform.py:71">
P3: MemoryPlatformContract is a dead alias that simply equals MemoryPlatformCapability and is not referenced anywhere except its own definition and the __all__ export (the router and utils import MemoryPlatformCapability directly). Since nothing consumes the alias, this adds a redundant public name that can drift from the real model. Consider dropping it (and its __all__ entry) or, if a stable contract name is intended, documenting what it guarantees beyond the concrete model.</violation>
</file>

<file name="backend/database/memory_platform_usage.py">

<violation number="1" location="backend/database/memory_platform_usage.py:64">
P3: This new module substantially duplicates `database/phone_call_usage.py`: `_period_id`, `_period_reset_epoch`, `_key`, the 40-day `_TTL_SECONDS`, and the entire `reserve_current_month_slot` INCR/EXPIRE/over-limit-DECR logic are near-verbatim copies (the only addition is `record_fallback` on the failure path). That is a maintenance hazard — a bug or policy change in the shared monthly-counter semantics (e.g., rollover handling, TTL, over-limit rollback) now has to be fixed in two places that can drift apart. Consider extracting the common monthly Redis-counter helper and having both modules reuse it, keeping only the component-specific bits (key prefix, fallback reporting) local.</violation>
</file>

<file name="backend/utils/mcp_scopes.py">

<violation number="1" location="backend/utils/mcp_scopes.py:32">
P2: The per-key scope normalizer opens back up to full access any time the resolved scope set is empty. normalize_mcp_scopes([]) and normalize_mcp_scopes(["bogus"]) both return the full 9-scope set, and this is the exact helper the auth path, rotate path, and DB-layer create path all funnel through. That directly contradicts the PR's stated invariant that a key "authorizes exactly the scopes recorded on its document and nothing else" and that unknown/empty scopes fail closed. The HTTP create router shields the create flow, but the DB-layer create_mcp_key and the authentication path trust this helper, so a key whose stored scopes field is empty/corrupt (or a future direct caller of create_mcp_key that forgets router validation) silently escalates to full access instead of failing closed. I'd suggest distinguishing the legacy fallback (missing/unreadable state → full access, which is intentional) from an explicitly present empty/unknown scope set (→ no access), and adding a test for the all-unknown/empty case.</violation>
</file>

<file name="backend/utils/mcp_memory_platform.py">

<violation number="1" location="backend/utils/mcp_memory_platform.py:10">
P3: build_memory_platform_payload() is a duplicate adapter that only delegates to build_mcp_memory_platform_payload() and is referenced exclusively by the test file — no production code path calls it (the REST router returns MemoryPlatformCapability directly, and mcp_sse.py calls build_mcp_memory_platform_payload()). This is an alias that only exists to back tests, consistent with the repo's 'no duplicate adapters' guidance. Consider removing it and updating the test to assert against build_mcp_memory_platform_payload(), or just inline its only test use.</violation>
</file>

<sub>Reply with feedback, questions, or to request a fix.

Re-trigger cubic</sub>

[cubic-dev-ai]

P1: Memory-platform masthead starts beneath the fixed AppHeader because this shell reserves only 4rem; reserve 6.5rem on narrow screens and 7rem at md so page content clears the header.

Prompt for AI agents

Check if this issue is valid — if so, understand the root cause and fix it. At web/frontend/src/app/memory-platform/components/platform-shell.tsx, line 25:

<comment>Memory-platform masthead starts beneath the fixed AppHeader because this shell reserves only 4rem; reserve 6.5rem on narrow screens and 7rem at `md` so page content clears the header.</comment>

<file context>
@@ -0,0 +1,70 @@
+ */
+export default function PlatformShell({ active, children }: PlatformShellProps) {
+  return (
+    <div className="dark min-h-screen bg-[#0a0a0a] pt-16 text-neutral-100 antialiased">
+      <div className="sticky top-16 z-30 border-b border-white/10 bg-[#0a0a0a]/80 backdrop-blur">
+        <div className="mx-auto flex max-w-6xl items-center gap-6 px-5 py-4 md:px-8">
</file context>

Suggested change

	<div className="dark min-h-screen bg-[#0a0a0a] pt-16 text-neutral-100 antialiased">
	<div className="dark min-h-screen bg-[#0a0a0a] pt-[6.5rem] text-neutral-100 antialiased md:pt-28">

[undivisible]

Not reproducible — declining this one. Verified visually against a production build at 1280px and 390px: the masthead clears the fixed AppHeader exactly, no overlap at either width. AppHeader is 4rem, which is what main's own mobile-menu top-16 also assumes. The suggested pt-[6.5rem]/md:pt-28 would open a ~2.5rem gap.

[cubic-dev-ai]

P1: Successful platform searches render no results because the client models memories while the endpoint returns items. Model the response as items and have the widget consume that field.

Prompt for AI agents

Check if this issue is valid — if so, understand the root cause and fix it. At web/frontend/src/lib/api/memory-platform.ts, line 27:

<comment>Successful platform searches render no results because the client models `memories` while the endpoint returns `items`. Model the response as `items` and have the widget consume that field.</comment>

<file context>
@@ -0,0 +1,100 @@
+}
+
+export interface PlatformSearchResponse {
+  memories?: PlatformMemoryItem[];
+  [key: string]: unknown;
+}
</file context>

[cubic-dev-ai]

P3: The module-scoped fixture mutates sys.modules/utils.other.endpoints with a hand-built stub instead of using the sanctioned backend/testing/import_isolation.py helper or a conftest-based seam, which the repo guidance (AGENTS.md 'Never mutate sys.modules at module scope in tests' and prior feedback to avoid module-scope sys.modules stubs for routers) steers away from. The partial stub (only get_current_user_uid, no with_rate_limit) plus module __all__-driven imports can silently mask behavior of _rate_limited_uid/within_rate_limit; prefer the sanctioned isolation helper so the pattern stays consistent and auditable.

Prompt for AI agents

Check if this issue is valid — if so, understand the root cause and fix it. At backend/tests/unit/test_memory_platform_router.py, line 24:

<comment>The module-scoped fixture mutates `sys.modules`/`utils.other.endpoints` with a hand-built stub instead of using the sanctioned `backend/testing/import_isolation.py` helper or a conftest-based seam, which the repo guidance (AGENTS.md 'Never mutate sys.modules at module scope in tests' and prior feedback to avoid module-scope sys.modules stubs for routers) steers away from. The partial stub (only `get_current_user_uid`, no `with_rate_limit`) plus module `__all__`-driven imports can silently mask behavior of `_rate_limited_uid`/`within_rate_limit`; prefer the sanctioned isolation helper so the pattern stays consistent and auditable.</comment>

<file context>
@@ -0,0 +1,133 @@
+    saved = snapshot_sys_modules(_MODULE_NAMES)
+    auth_module = _auth_stub()
+    sys.modules.pop('routers.memory_platform', None)
+    sys.modules['utils.other.endpoints'] = auth_module
+    other_package = sys.modules.get('utils.other')
+    if isinstance(other_package, types.ModuleType):
</file context>

[undivisible]

The fixture uses the repo's memory_import_isolation helper (snapshot_sys_modules/restore_sys_modules) and restores in a finally, so it is the sanctioned seam rather than an ad-hoc mutation. Kept as-is; backend-module-isolation passes.

[cubic-dev-ai]

P3: New rotate_dev_key is a near-verbatim duplicate of the existing rotate_mcp_key (~60 lines). The two differ only in the cache-delete function, key generator, Pydantic model, app-id handling, and scope normalizer; the structure (read doc → ownership check → hash validity → retire cache strictly → generate → update → project → strict-parse) is identical. Since these two credential types share the exact same hard-cutover rotation contract, the shared flow can be parameterized by a small kind descriptor (cache fn, generator, model, normalize helpers) and reused, so rotation semantics can't drift between Dev and MCP keys. If a shared helper is intentionally avoided, a comment explaining why the duplication is worthwhile would help.

Prompt for AI agents

Check if this issue is valid — if so, understand the root cause and fix it. At backend/database/dev_api_key.py, line 147:

<comment>New `rotate_dev_key` is a near-verbatim duplicate of the existing `rotate_mcp_key` (~60 lines). The two differ only in the cache-delete function, key generator, Pydantic model, app-id handling, and scope normalizer; the structure (read doc → ownership check → hash validity → retire cache strictly → generate → update → project → strict-parse) is identical. Since these two credential types share the exact same hard-cutover rotation contract, the shared flow can be parameterized by a small kind descriptor (cache fn, generator, model, normalize helpers) and reused, so rotation semantics can't drift between Dev and MCP keys. If a shared helper is intentionally avoided, a comment explaining why the duplication is worthwhile would help.</comment>

<file context>
@@ -113,6 +115,64 @@ def create_dev_key(user_id: str, name: str, scopes: Optional[List[str]] = None)
+    if cache_deleted is not True:
+        raise ApiKeyRevocationUnavailableError("Developer API key cache invalidation was not confirmed")
+
+    raw_key, hashed_key, key_prefix = generate_dev_api_key()
+    now = datetime.now(timezone.utc)
+    scopes = _normalize_dev_scopes(key_data.get("scopes"))
</file context>

[undivisible]

Acknowledged, not doing it here. The correctness-critical part of the rotation contract — the retirement fence — is now shared (retired_hash_fences_cache), so the safety invariant can't drift between the two key kinds. Extracting the full read/own/retire/swap/project flow is a wider refactor than belongs in a security fix; leaving the remaining structural duplication for a follow-up.

[cubic-dev-ai]

P3: MemoryPlatformContract is a dead alias that simply equals MemoryPlatformCapability and is not referenced anywhere except its own definition and the all export (the router and utils import MemoryPlatformCapability directly). Since nothing consumes the alias, this adds a redundant public name that can drift from the real model. Consider dropping it (and its all entry) or, if a stable contract name is intended, documenting what it guarantees beyond the concrete model.

Prompt for AI agents

Check if this issue is valid — if so, understand the root cause and fix it. At backend/models/memory_platform.py, line 71:

<comment>MemoryPlatformContract is a dead alias that simply equals MemoryPlatformCapability and is not referenced anywhere except its own definition and the __all__ export (the router and utils import MemoryPlatformCapability directly). Since nothing consumes the alias, this adds a redundant public name that can drift from the real model. Consider dropping it (and its __all__ entry) or, if a stable contract name is intended, documenting what it guarantees beyond the concrete model.</comment>

<file context>
@@ -0,0 +1,91 @@
+    status: Literal["created"] = "created"
+
+
+MemoryPlatformContract = MemoryPlatformCapability
+
+
</file context>

[undivisible]

Agreed — dead alias. Leaving the removal for the follow-up that also handles the duplicate build_memory_platform_payload adapter, to keep this round scoped to the correctness fixes.

[cubic-dev-ai]

P3: build_memory_platform_payload() is a duplicate adapter that only delegates to build_mcp_memory_platform_payload() and is referenced exclusively by the test file — no production code path calls it (the REST router returns MemoryPlatformCapability directly, and mcp_sse.py calls build_mcp_memory_platform_payload()). This is an alias that only exists to back tests, consistent with the repo's 'no duplicate adapters' guidance. Consider removing it and updating the test to assert against build_mcp_memory_platform_payload(), or just inline its only test use.

Prompt for AI agents

Check if this issue is valid — if so, understand the root cause and fix it. At backend/utils/mcp_memory_platform.py, line 10:

<comment>build_memory_platform_payload() is a duplicate adapter that only delegates to build_mcp_memory_platform_payload() and is referenced exclusively by the test file — no production code path calls it (the REST router returns MemoryPlatformCapability directly, and mcp_sse.py calls build_mcp_memory_platform_payload()). This is an alias that only exists to back tests, consistent with the repo's 'no duplicate adapters' guidance. Consider removing it and updating the test to assert against build_mcp_memory_platform_payload(), or just inline its only test use.</comment>

<file context>
@@ -0,0 +1,11 @@
+    return build_memory_platform_capability().model_dump(mode="json")
+
+
+def build_memory_platform_payload() -> dict[str, Any]:
+    return build_mcp_memory_platform_payload()
</file context>

[undivisible]

Agreed, same follow-up as the MemoryPlatformContract alias — a test-only adapter that should be inlined.

[cubic-dev-ai]

P3: This new module substantially duplicates database/phone_call_usage.py: _period_id, _period_reset_epoch, _key, the 40-day _TTL_SECONDS, and the entire reserve_current_month_slot INCR/EXPIRE/over-limit-DECR logic are near-verbatim copies (the only addition is record_fallback on the failure path). That is a maintenance hazard — a bug or policy change in the shared monthly-counter semantics (e.g., rollover handling, TTL, over-limit rollback) now has to be fixed in two places that can drift apart. Consider extracting the common monthly Redis-counter helper and having both modules reuse it, keeping only the component-specific bits (key prefix, fallback reporting) local.

Prompt for AI agents

Check if this issue is valid — if so, understand the root cause and fix it. At backend/database/memory_platform_usage.py, line 64:

<comment>This new module substantially duplicates `database/phone_call_usage.py`: `_period_id`, `_period_reset_epoch`, `_key`, the 40-day `_TTL_SECONDS`, and the entire `reserve_current_month_slot` INCR/EXPIRE/over-limit-DECR logic are near-verbatim copies (the only addition is `record_fallback` on the failure path). That is a maintenance hazard — a bug or policy change in the shared monthly-counter semantics (e.g., rollover handling, TTL, over-limit rollback) now has to be fixed in two places that can drift apart. Consider extracting the common monthly Redis-counter helper and having both modules reuse it, keeping only the component-specific bits (key prefix, fallback reporting) local.</comment>

<file context>
@@ -0,0 +1,93 @@
+    return used, _period_reset_epoch(now)
+
+
+def reserve_current_month_request(uid: str, monthly_limit: int) -> Tuple[bool, int, int]:
+    """Atomically reserve one request against the month's allowance.
+
</file context>

[undivisible]

Agreed the monthly-counter logic is duplicated from phone_call_usage.py. Extracting a shared Redis monthly-counter primitive touches an existing metered path, so it's out of scope for this PR — tracking as follow-up rather than doing it alongside the auth changes.

[Git-on-my-level]

Thanks for pushing this forward. The shape is cohesive around a backend-authoritative memory platform, but I don't think this head is ready to merge yet.

Blocking items I see:

• docs/api-reference/app-client-openapi.json updates the app-client schema, but the public developer OpenAPI artifact is still stale. The failing Public Developer API contract reports that docs/api-reference/openapi.json needs to be regenerated after adding the new /v1/memory/platform/* and key-rotation routes. Because this PR publishes developer-facing API docs, both public contract artifacts need to match the backend routes before merge.
• app/lib/backend/schema/gen/payments_wire.g.dart and app/lib/backend/schema/gen/subscription_usage_wire.g.dart add platformApiRequestsPerMonth, but the Dart analyzer ratchet is failing with one new avoid_print occurrence. Please either remove the new print or update the analyzer baseline if the new occurrence is intentional.
• The backend unit suite is failing in tests/unit/test_api_key_listability_contract.py and tests/unit/test_mcp_api_key_auth_context.py: several existing auth-context fake Redis adapters now lack the new api_key_hash_is_retired method used by backend/database/dev_api_key.py and backend/database/mcp_api_key.py. This looks like a test-fixture drift from the new retirement-fence seam rather than evidence that the production Redis path is broken, but the contract tests still need to be updated so the key-rotation/auth-cache behavior is covered consistently.

Specific code notes:

• backend/database/api_key_metadata.py, backend/database/redis_db.py, backend/database/dev_api_key.py, and backend/database/mcp_api_key.py add a useful rotation tombstone and avoid trusting caches for retired hashes. The production seam is directionally sound, but the new redis_db.api_key_hash_is_retired() dependency needs the test doubles updated everywhere auth lookup is exercised.
• backend/utils/mcp_scopes.py changes MCP scopes from always-full-access to per-key scoping. Please add/keep explicit coverage for malformed recorded scope lists: the docstring says a recorded list is authoritative, so make sure an empty or unknown-only stored list cannot accidentally widen to full access unless that is an intentional legacy-compatibility rule.
• backend/routers/memory_platform.py correctly routes discovery, search, quota, and ingest through authenticated user sessions and the canonical MemoryService; the metering-after-validation placement is good. Because this is a new memory read/write surface, maintainer sign-off is still needed for the product/API boundary.
• backend/route_policy_manifest.yaml records the new memory platform and rotation routes. Please keep that in sync with the regenerated OpenAPI artifacts so route policy, backend code, and public developer docs agree.
• web/frontend/next.config.mjs, docs/memory/embedding.md, and web/frontend/src/app/memory-platform/hooks/use-embed-session.ts document and implement the sandboxed embed approach. The no-allow-same-origin guidance is security-positive, but the widget framing/session-token model is a sensitive web/API boundary and should get human maintainer sign-off before it ships.
• docs/memory/backend-authority.md affects AI coding/review agents and MCP users by telling them Omi backend memory is authoritative and local/zkr stores are only replicas or pending buffers. The guidance appears safe and aligned with the new backend contract, but because it changes agent-facing instructions for how integrations should write memory, it should be reviewed deliberately by a maintainer.

Leaving this as changes requested because required CI/contract checks are failing. Separately, the new backend-authoritative memory platform, scoped MCP keys, rotation, and embeddable web surface are product/security-sensitive enough to need maintainer sign-off before merge.

---

by AI on behalf of David — if you need David’s attention urgently, please @Git-on-my-level and escalate with need human response.