Fix MCP env aliasing, explicit missing-env errors, and add memory smoke diagnostics - #121
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
📝 WalkthroughWalkthroughThis PR adds a new environment-variable alias resolution module ( ChangesPandora MCP env alias helpers and consumers
Estimated code review effort: 3 (Moderate) | ~25 minutes Sequence Diagram(s)sequenceDiagram
participant Client
participant McpRoute as MCP Route
participant McpAuth as mcp-auth
participant EnvHelpers as pandora-mcp-env
Client->>McpRoute: request with Authorization Bearer token
McpRoute->>McpAuth: resolvePandoraMcpPrincipal(env)
McpAuth->>EnvHelpers: getPandoraMcpBearerSecret(env)
EnvHelpers-->>McpAuth: status ok or missing
McpAuth->>EnvHelpers: getPandoraMcpDbKey(env)
EnvHelpers-->>McpAuth: status ok or missing
McpAuth-->>McpRoute: principal or error code
McpRoute->>EnvHelpers: getPandoraSupabaseUrl(env)
EnvHelpers-->>McpRoute: status ok or missing
McpRoute-->>Client: Supabase client created or error thrown
Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 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 |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 9115389c13
ℹ️ 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".
| await step("analyze_memory_candidates", async () => analyzeMemoryCandidatesTool(client, principal, { namespace: ns, text: seed, source: "memory_smoke", mode: "candidate_only" }, env)); | ||
| await step("capture_adaptive_memory candidate_only", async () => captureAdaptiveMemoryTool(client, principal, { namespace: ns, text: seed, source: "memory_smoke", mode: "candidate_only" }, env)); | ||
| await step("capture_adaptive_memory auto_capture_allowed", async () => captureAdaptiveMemoryTool(client, principal, { namespace: ns, text: seed, source: "memory_smoke", mode: "auto_capture_allowed" }, env)); | ||
| await step("capture_memory_event", async () => captureMemoryEventTool(client, principal, { namespace: ns, raw_text: seed, source: "memory_smoke", sensitivity: "low" }, env), env.PANDORA_ENABLE_MCP_CAPTURE !== "true"); |
There was a problem hiding this comment.
When npm run memory:smoke is run against a configured environment with PANDORA_ENABLE_MCP_CAPTURE=true, these diagnostics use the service-role client and default to the real_life namespace, but the called tools are not read-only: analyzeMemoryCandidatesTool/captureAdaptiveMemoryTool insert candidate rows and captureMemoryEventTool inserts a captured memory_events row. A smoke/diagnostics command can therefore contaminate production user memory with the seed text instead of only reporting readiness; keep this path dry-run/read-only or require an explicit non-dry-run opt-in.
Useful? React with 👍 / 👎.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
app/api/mcp/route.ts (1)
30-42: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winUncaught throw from
createMcpClient()yields an opaque 500 instead of the intended explicit error.
createMcpClient()throws a plainErrorwhen Supabase URL/DB key resolution fails, but the call at Line 42 (insidehandle) isn't wrapped in try/catch. In Next.js App Router, an uncaught throw inside a route handler results in a generic, unhelpful 500 response — not the structured{ ok: false, code, message }shape produced byjsonErrorfor other failure paths in this same file. This defeats the PR's goal of surfacing clear "Missing server env" errors for this route.Wrap the client construction in try/catch and return a
jsonError-style response.🔧 Proposed fix
async function handle(request: Request) { if (request.method === "OPTIONS") return new Response(null, { status: 204, headers: corsHeaders(request) }); const principal = resolvePandoraMcpPrincipal(request); if (!principal.ok) return jsonError(principal, request); - const server = createPandoraMcpServer({ client: createMcpClient(), principal }); + let client: MemoryBridgeDbClient; + try { + client = createMcpClient(); + } catch (e) { + const headers = new Headers(corsHeaders(request)); + return NextResponse.json({ ok: false, code: "mcp_env_missing", message: e instanceof Error ? e.message : String(e) }, { status: 500, headers }); + } + const server = createPandoraMcpServer({ client, principal });🤖 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 `@app/api/mcp/route.ts` around lines 30 - 42, The `handle` flow is letting `createMcpClient()` throw uncaught, which turns missing Supabase env resolution into a generic 500 instead of the structured `jsonError` response used elsewhere. Wrap the `createMcpClient()` call in `handle` with try/catch, and on failure return a `jsonError`-style response that preserves the explicit env-missing message from `getPandoraSupabaseUrl` and `getPandoraMcpDbKey` so the route returns a clear client error instead of an opaque server error.
🧹 Nitpick comments (3)
tests/unit/pandora-mcp-env.test.ts (1)
19-23: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueRedundant dynamic import.
requireMcpCaptureEnabled/requireMcpDistillationEnabledare re-imported dynamically here, butmcp-authis already statically imported at Line 3. Add these to the existing static import instead.♻️ Proposed fix
-import { resolvePandoraMcpPrincipal } from "`@/lib/services/mcp-auth`"; +import { resolvePandoraMcpPrincipal, requireMcpCaptureEnabled, requireMcpDistillationEnabled } from "`@/lib/services/mcp-auth`";- it("reports disabled capture and distillation with explicit env names", async () => { - const { requireMcpCaptureEnabled, requireMcpDistillationEnabled } = await import("`@/lib/services/mcp-auth`"); + it("reports disabled capture and distillation with explicit env names", () => { expect(requireMcpCaptureEnabled({ PANDORA_ENABLE_MCP_CAPTURE: "false" }).message).toBe("capture_disabled: PANDORA_ENABLE_MCP_CAPTURE is not true"); expect(requireMcpDistillationEnabled({ PANDORA_ENABLE_MCP_DISTILLATION: "false" }).message).toBe("distillation_disabled: PANDORA_ENABLE_MCP_DISTILLATION is not true"); });🤖 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 `@tests/unit/pandora-mcp-env.test.ts` around lines 19 - 23, The test re-imports requireMcpCaptureEnabled and requireMcpDistillationEnabled dynamically even though mcp-auth is already statically imported; move these symbols into the existing top-level import in pandora-mcp-env.test.ts and remove the await import() inside the test. Keep the test body using the same function names so it still verifies the explicit env-name error messages.scripts/memory-smoke.ts (2)
11-11: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAlias lists duplicated from
pandora-mcp-env.ts.
aliaseshardcodes the same env-var lists already defined ingetPandoraMcpBearerSecret/getPandoraMcpDbKey/getPandoraSupabaseUrl. If those source lists change, this diagnostic output silently drifts and misreports which env var is active.Export the alias arrays from
pandora-mcp-env.tsand import them here instead of re-declaring.🤖 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 `@scripts/memory-smoke.ts` at line 11, The alias lists in the memory smoke script are duplicated from the env helpers, so the diagnostic output can drift from the real source of truth. Export the alias arrays from the functions in pandora-mcp-env.ts (the ones used by getPandoraMcpBearerSecret, getPandoraMcpDbKey, and getPandoraSupabaseUrl) and import them into memory-smoke.ts instead of hardcoding aliases there, so the active env-var reporting always stays in sync.
14-14: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winFragile failure detection via string matching on serialized JSON.
step()determines failure by checking whether the stringified result contains'"ok":false'or'Invalid API key'rather than checking the actualokproperty on the parsed result. If a tool's failure shape doesn't literally serialize to those substrings, a real failure could be reported as PASS, undermining the diagnostic's purpose.🤖 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 `@scripts/memory-smoke.ts` at line 14, The failure check in step() is relying on string matching over JSON.stringify(r), which can miss real tool failures. Update step() to inspect the returned value directly from the parsed result, using the ok field (and any other explicit failure indicators already present in the step output flow) instead of searching for substrings like "ok":false or "Invalid API key". Keep the existing out() reporting path in step() so the name, optional flag, and summary text still flow through unchanged.
🤖 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.
Inline comments:
In `@lib/services/mcp-auth.ts`:
- Around line 25-40: `resolvePandoraMcpPrincipal` currently returns
`mcp_token_env_missing` before it can try OAuth, which blocks OAuth-only
deployments. Update the auth flow so `getPandoraMcpBearerSecret` is only
required for the static bearer-token path in `resolvePandoraMcpPrincipal`, then
always fall through to `verifyPandoraMcpOAuthAccessToken` when the configured
token is missing or doesn’t match. Keep the existing `authType` and `userId`
checks, and make `mcp_token_env_missing` the final fallback only after both
bearer-token and OAuth validation fail.
In `@lib/services/pandora-mcp-env.ts`:
- Around line 13-15: The new fallback in getPandoraMcpDbKey silently promotes
the MCP route to use SUPABASE_SERVICE_ROLE_KEY, which broadens access instead of
failing closed. Update getPandoraMcpDbKey so it only returns a dedicated
PANDORA_MCP_DB_KEY or PANDORA_MEMORY_BRIDGE_DB_KEY by default, and make any use
of SUPABASE_SERVICE_ROLE_KEY explicit and opt-in via a separate flag or
configuration path. Ensure the app/api/mcp/route.ts flow surfaces a clear
missing-key error when no dedicated key is configured rather than silently using
the service-role secret.
In `@scripts/memory-smoke.ts`:
- Line 7: The namespace selection in memory-smoke.ts defaults to the
production-like real_life namespace, which can leak or pollute real data when
the smoke/diagnostics flow runs. Update the ns initialization logic so the safe
test namespace (au) is the default, and only switch to real_life when there is
an explicit opt-in via PANDORA_MEMORY_SMOKE_NAMESPACE; make sure the same guard
applies to the tool sequence in step() and the write/read calls like
captureAdaptiveMemoryTool, captureMemoryEventTool, createSessionDigestTool,
getMemoryContextTool, getAdaptiveContextTool, and distillContextPackTool.
---
Outside diff comments:
In `@app/api/mcp/route.ts`:
- Around line 30-42: The `handle` flow is letting `createMcpClient()` throw
uncaught, which turns missing Supabase env resolution into a generic 500 instead
of the structured `jsonError` response used elsewhere. Wrap the
`createMcpClient()` call in `handle` with try/catch, and on failure return a
`jsonError`-style response that preserves the explicit env-missing message from
`getPandoraSupabaseUrl` and `getPandoraMcpDbKey` so the route returns a clear
client error instead of an opaque server error.
---
Nitpick comments:
In `@scripts/memory-smoke.ts`:
- Line 11: The alias lists in the memory smoke script are duplicated from the
env helpers, so the diagnostic output can drift from the real source of truth.
Export the alias arrays from the functions in pandora-mcp-env.ts (the ones used
by getPandoraMcpBearerSecret, getPandoraMcpDbKey, and getPandoraSupabaseUrl) and
import them into memory-smoke.ts instead of hardcoding aliases there, so the
active env-var reporting always stays in sync.
- Line 14: The failure check in step() is relying on string matching over
JSON.stringify(r), which can miss real tool failures. Update step() to inspect
the returned value directly from the parsed result, using the ok field (and any
other explicit failure indicators already present in the step output flow)
instead of searching for substrings like "ok":false or "Invalid API key". Keep
the existing out() reporting path in step() so the name, optional flag, and
summary text still flow through unchanged.
In `@tests/unit/pandora-mcp-env.test.ts`:
- Around line 19-23: The test re-imports requireMcpCaptureEnabled and
requireMcpDistillationEnabled dynamically even though mcp-auth is already
statically imported; move these symbols into the existing top-level import in
pandora-mcp-env.test.ts and remove the await import() inside the test. Keep the
test body using the same function names so it still verifies the explicit
env-name error messages.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 6af5922d-4a29-43a3-a41a-a1d8f8d93f8b
📒 Files selected for processing (6)
app/api/mcp/route.tslib/services/mcp-auth.tslib/services/pandora-mcp-env.tspackage.jsonscripts/memory-smoke.tstests/unit/pandora-mcp-env.test.ts
| const configuredToken = getPandoraMcpBearerSecret(env); | ||
| const suppliedToken = bearerToken(request); | ||
| if (!configuredToken || !suppliedToken) return { ok: false, status: 401, code: "mcp_token_missing", message: "MCP bearer token is required." }; | ||
| if (safeEqual(suppliedToken, configuredToken)) { | ||
| if (!env.PANDORA_MCP_USER_ID) return { ok: false, status: 403, code: "mcp_user_id_missing", message: "Pandora MCP user id is not configured." }; | ||
| if (!env.PANDORA_MCP_DB_KEY) return { ok: false, status: 403, code: "mcp_db_key_missing", message: "Pandora MCP database key is not configured." }; | ||
| if (!configuredToken.ok) return { ok: false, status: 403, code: "mcp_token_env_missing", message: configuredToken.message }; | ||
| if (!suppliedToken) return { ok: false, status: 401, code: "mcp_token_missing", message: "MCP bearer token is required." }; | ||
| if (safeEqual(suppliedToken, configuredToken.value)) { | ||
| if (!env.PANDORA_MCP_USER_ID) return { ok: false, status: 403, code: "mcp_user_id_missing", message: "Missing server env: PANDORA_MCP_USER_ID" }; | ||
| const dbKey = getPandoraMcpDbKey(env); | ||
| if (!dbKey.ok) return { ok: false, status: 403, code: "mcp_db_key_missing", message: dbKey.message }; | ||
| return { ok: true, authType: "mcp_bearer_token", userId: env.PANDORA_MCP_USER_ID }; | ||
| } | ||
| const oauth = verifyPandoraMcpOAuthAccessToken(suppliedToken, env); | ||
| if (!oauth.ok) return { ok: false, status: 401, code: "mcp_token_invalid", message: "MCP bearer token is invalid." }; | ||
| if (!env.PANDORA_MCP_USER_ID) return { ok: false, status: 403, code: "mcp_user_id_missing", message: "Pandora MCP user id is not configured." }; | ||
| if (!env.PANDORA_MCP_DB_KEY) return { ok: false, status: 403, code: "mcp_db_key_missing", message: "Pandora MCP database key is not configured." }; | ||
| if (!env.PANDORA_MCP_USER_ID) return { ok: false, status: 403, code: "mcp_user_id_missing", message: "Missing server env: PANDORA_MCP_USER_ID" }; | ||
| const dbKey = getPandoraMcpDbKey(env); | ||
| if (!dbKey.ok) return { ok: false, status: 403, code: "mcp_db_key_missing", message: dbKey.message }; | ||
| return { ok: true, authType: "mcp_oauth_access_token", userId: oauth.payload.user_id }; |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Missing bearer-secret env blocks OAuth authentication entirely.
resolvePandoraMcpPrincipal returns mcp_token_env_missing at Line 27 as soon as getPandoraMcpBearerSecret fails, before ever checking suppliedToken or reaching the OAuth branch (Line 35). Since OAuth (verifyPandoraMcpOAuthAccessToken) verifies against a completely independent secret, a deployment that intentionally relies solely on OAuth (no PANDORA_MCP_TOKEN/PANDORA_MCP_API_KEY/PANDORA_API_KEY/MEMORY_API_KEY set) will have every request — including ones bearing a perfectly valid OAuth access token — rejected with a "missing server env" error. OAuth-only auth becomes impossible.
Reorder so OAuth is attempted whenever the static secret doesn't match/isn't configured, and only report mcp_token_env_missing as the final fallback when both mechanisms fail.
🔧 Proposed fix
export function resolvePandoraMcpPrincipal(request: Request, env: Partial<NodeJS.ProcessEnv> = process.env): PandoraMcpPrincipal {
if (env.PANDORA_ENABLE_MCP !== "true") return { ok: false, status: 403, code: "mcp_disabled", message: "Pandora MCP is disabled." };
const configuredToken = getPandoraMcpBearerSecret(env);
const suppliedToken = bearerToken(request);
- if (!configuredToken.ok) return { ok: false, status: 403, code: "mcp_token_env_missing", message: configuredToken.message };
if (!suppliedToken) return { ok: false, status: 401, code: "mcp_token_missing", message: "MCP bearer token is required." };
- if (safeEqual(suppliedToken, configuredToken.value)) {
+ if (configuredToken.ok && safeEqual(suppliedToken, configuredToken.value)) {
if (!env.PANDORA_MCP_USER_ID) return { ok: false, status: 403, code: "mcp_user_id_missing", message: "Missing server env: PANDORA_MCP_USER_ID" };
const dbKey = getPandoraMcpDbKey(env);
if (!dbKey.ok) return { ok: false, status: 403, code: "mcp_db_key_missing", message: dbKey.message };
return { ok: true, authType: "mcp_bearer_token", userId: env.PANDORA_MCP_USER_ID };
}
const oauth = verifyPandoraMcpOAuthAccessToken(suppliedToken, env);
- if (!oauth.ok) return { ok: false, status: 401, code: "mcp_token_invalid", message: "MCP bearer token is invalid." };
- if (!env.PANDORA_MCP_USER_ID) return { ok: false, status: 403, code: "mcp_user_id_missing", message: "Missing server env: PANDORA_MCP_USER_ID" };
- const dbKey = getPandoraMcpDbKey(env);
- if (!dbKey.ok) return { ok: false, status: 403, code: "mcp_db_key_missing", message: dbKey.message };
- return { ok: true, authType: "mcp_oauth_access_token", userId: oauth.payload.user_id };
+ if (oauth.ok) {
+ if (!env.PANDORA_MCP_USER_ID) return { ok: false, status: 403, code: "mcp_user_id_missing", message: "Missing server env: PANDORA_MCP_USER_ID" };
+ const dbKey = getPandoraMcpDbKey(env);
+ if (!dbKey.ok) return { ok: false, status: 403, code: "mcp_db_key_missing", message: dbKey.message };
+ return { ok: true, authType: "mcp_oauth_access_token", userId: oauth.payload.user_id };
+ }
+ if (!configuredToken.ok) return { ok: false, status: 403, code: "mcp_token_env_missing", message: configuredToken.message };
+ return { ok: false, status: 401, code: "mcp_token_invalid", message: "MCP bearer token is invalid." };
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const configuredToken = getPandoraMcpBearerSecret(env); | |
| const suppliedToken = bearerToken(request); | |
| if (!configuredToken || !suppliedToken) return { ok: false, status: 401, code: "mcp_token_missing", message: "MCP bearer token is required." }; | |
| if (safeEqual(suppliedToken, configuredToken)) { | |
| if (!env.PANDORA_MCP_USER_ID) return { ok: false, status: 403, code: "mcp_user_id_missing", message: "Pandora MCP user id is not configured." }; | |
| if (!env.PANDORA_MCP_DB_KEY) return { ok: false, status: 403, code: "mcp_db_key_missing", message: "Pandora MCP database key is not configured." }; | |
| if (!configuredToken.ok) return { ok: false, status: 403, code: "mcp_token_env_missing", message: configuredToken.message }; | |
| if (!suppliedToken) return { ok: false, status: 401, code: "mcp_token_missing", message: "MCP bearer token is required." }; | |
| if (safeEqual(suppliedToken, configuredToken.value)) { | |
| if (!env.PANDORA_MCP_USER_ID) return { ok: false, status: 403, code: "mcp_user_id_missing", message: "Missing server env: PANDORA_MCP_USER_ID" }; | |
| const dbKey = getPandoraMcpDbKey(env); | |
| if (!dbKey.ok) return { ok: false, status: 403, code: "mcp_db_key_missing", message: dbKey.message }; | |
| return { ok: true, authType: "mcp_bearer_token", userId: env.PANDORA_MCP_USER_ID }; | |
| } | |
| const oauth = verifyPandoraMcpOAuthAccessToken(suppliedToken, env); | |
| if (!oauth.ok) return { ok: false, status: 401, code: "mcp_token_invalid", message: "MCP bearer token is invalid." }; | |
| if (!env.PANDORA_MCP_USER_ID) return { ok: false, status: 403, code: "mcp_user_id_missing", message: "Pandora MCP user id is not configured." }; | |
| if (!env.PANDORA_MCP_DB_KEY) return { ok: false, status: 403, code: "mcp_db_key_missing", message: "Pandora MCP database key is not configured." }; | |
| if (!env.PANDORA_MCP_USER_ID) return { ok: false, status: 403, code: "mcp_user_id_missing", message: "Missing server env: PANDORA_MCP_USER_ID" }; | |
| const dbKey = getPandoraMcpDbKey(env); | |
| if (!dbKey.ok) return { ok: false, status: 403, code: "mcp_db_key_missing", message: dbKey.message }; | |
| return { ok: true, authType: "mcp_oauth_access_token", userId: oauth.payload.user_id }; | |
| const configuredToken = getPandoraMcpBearerSecret(env); | |
| const suppliedToken = bearerToken(request); | |
| if (!suppliedToken) return { ok: false, status: 401, code: "mcp_token_missing", message: "MCP bearer token is required." }; | |
| if (configuredToken.ok && safeEqual(suppliedToken, configuredToken.value)) { | |
| if (!env.PANDORA_MCP_USER_ID) return { ok: false, status: 403, code: "mcp_user_id_missing", message: "Missing server env: PANDORA_MCP_USER_ID" }; | |
| const dbKey = getPandoraMcpDbKey(env); | |
| if (!dbKey.ok) return { ok: false, status: 403, code: "mcp_db_key_missing", message: dbKey.message }; | |
| return { ok: true, authType: "mcp_bearer_token", userId: env.PANDORA_MCP_USER_ID }; | |
| } | |
| const oauth = verifyPandoraMcpOAuthAccessToken(suppliedToken, env); | |
| if (oauth.ok) { | |
| if (!env.PANDORA_MCP_USER_ID) return { ok: false, status: 403, code: "mcp_user_id_missing", message: "Missing server env: PANDORA_MCP_USER_ID" }; | |
| const dbKey = getPandoraMcpDbKey(env); | |
| if (!dbKey.ok) return { ok: false, status: 403, code: "mcp_db_key_missing", message: dbKey.message }; | |
| return { ok: true, authType: "mcp_oauth_access_token", userId: oauth.payload.user_id }; | |
| } | |
| if (!configuredToken.ok) return { ok: false, status: 403, code: "mcp_token_env_missing", message: configuredToken.message }; | |
| return { ok: false, status: 401, code: "mcp_token_invalid", message: "MCP bearer token is invalid." }; |
🤖 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 `@lib/services/mcp-auth.ts` around lines 25 - 40, `resolvePandoraMcpPrincipal`
currently returns `mcp_token_env_missing` before it can try OAuth, which blocks
OAuth-only deployments. Update the auth flow so `getPandoraMcpBearerSecret` is
only required for the static bearer-token path in `resolvePandoraMcpPrincipal`,
then always fall through to `verifyPandoraMcpOAuthAccessToken` when the
configured token is missing or doesn’t match. Keep the existing `authType` and
`userId` checks, and make `mcp_token_env_missing` the final fallback only after
both bearer-token and OAuth validation fail.
| export function getPandoraMcpDbKey(env: Partial<NodeJS.ProcessEnv> = process.env) { | ||
| return firstPresent(env, ["PANDORA_MCP_DB_KEY", "PANDORA_MEMORY_BRIDGE_DB_KEY", "SUPABASE_SERVICE_ROLE_KEY"]); | ||
| } |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | 🏗️ Heavy lift
DB key fallback widens blast radius to full service-role privileges.
getPandoraMcpDbKey now silently falls back to SUPABASE_SERVICE_ROLE_KEY when no dedicated PANDORA_MCP_DB_KEY/PANDORA_MEMORY_BRIDGE_DB_KEY is set. Per the line-range change details, the prior code read env.PANDORA_MCP_DB_KEY directly with no such fallback — this is new exposure. The Supabase service-role key always bypasses Row Level Security, so any deployment that hasn't set a dedicated scoped key will have the public-facing MCP route (app/api/mcp/route.ts) operate with full, RLS-bypassing database privileges instead of failing closed with a clear "missing dedicated key" error.
Consider making the service-role fallback opt-in (e.g., a separate explicit flag) rather than a silent alias, so misconfigured deployments fail loudly instead of quietly running with elevated privileges.
🤖 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 `@lib/services/pandora-mcp-env.ts` around lines 13 - 15, The new fallback in
getPandoraMcpDbKey silently promotes the MCP route to use
SUPABASE_SERVICE_ROLE_KEY, which broadens access instead of failing closed.
Update getPandoraMcpDbKey so it only returns a dedicated PANDORA_MCP_DB_KEY or
PANDORA_MEMORY_BRIDGE_DB_KEY by default, and make any use of
SUPABASE_SERVICE_ROLE_KEY explicit and opt-in via a separate flag or
configuration path. Ensure the app/api/mcp/route.ts flow surfaces a clear
missing-key error when no dedicated key is configured rather than silently using
the service-role secret.
| import type { MemoryBridgeDbClient } from "../lib/services/memory-bridge-service"; | ||
|
|
||
| const seed = "Creative workflow preference: preserve continuity and user feedback for future writing."; | ||
| const ns = (process.env.PANDORA_MEMORY_SMOKE_NAMESPACE === "au" ? "au" : "real_life") as "real_life" | "au"; |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
Default namespace risks polluting/leaking real data.
ns defaults to "real_life" unless PANDORA_MEMORY_SMOKE_NAMESPACE is explicitly set to "au" (Line 7). Since this script performs real writes (captureAdaptiveMemoryTool with auto_capture_allowed, captureMemoryEventTool, createSessionDigestTool) and reads (getMemoryContextTool, getAdaptiveContextTool, distillContextPackTool) against whatever Supabase instance the env points to, running npm run memory:smoke/memory:diagnostics without remembering to set the namespace env var will inject synthetic seed memories into the real_life namespace, and step() logs up to 500 chars of each tool's JSON response (Line 14) — which could include genuine pre-existing personal memory content pulled from real_life — straight to stdout/CI logs.
Default to the safe test namespace and require explicit opt-in to touch real_life.
🔧 Proposed fix
-const ns = (process.env.PANDORA_MEMORY_SMOKE_NAMESPACE === "au" ? "au" : "real_life") as "real_life" | "au";
+const ns = (process.env.PANDORA_MEMORY_SMOKE_NAMESPACE === "real_life" ? "real_life" : "au") as "real_life" | "au";Also applies to: 21-35
🤖 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 `@scripts/memory-smoke.ts` at line 7, The namespace selection in
memory-smoke.ts defaults to the production-like real_life namespace, which can
leak or pollute real data when the smoke/diagnostics flow runs. Update the ns
initialization logic so the safe test namespace (au) is the default, and only
switch to real_life when there is an explicit opt-in via
PANDORA_MEMORY_SMOKE_NAMESPACE; make sure the same guard applies to the tool
sequence in step() and the write/read calls like captureAdaptiveMemoryTool,
captureMemoryEventTool, createSessionDigestTool, getMemoryContextTool,
getAdaptiveContextTool, and distillContextPackTool.
1 participant
Motivation
Invalid API keyfailures because the MCP route and auth expected specific env names and would construct a Supabase client with empty strings when keys were missing.Description
lib/services/pandora-mcp-env.tsto centralize env alias resolution for MCP bearer token, Supabase URL, and DB/service-role key and to return explicitMissing server env: <KEY_NAME>style messages.lib/services/mcp-auth.tsto use the new env resolver and to return explicit errors for missing token/user/db-key and to replace generic gate messages withcapture_disabled: PANDORA_ENABLE_MCP_CAPTURE is not trueanddistillation_disabled: PANDORA_ENABLE_MCP_DISTILLATION is not true.app/api/mcp/route.tsto validate Supabase URL and DB key aliases before creating the Supabase client so a missing config no longer surfaces as a SupabaseInvalid API key.scripts/memory-smoke.tsandnpmscriptsmemory:smoke/memory:diagnosticsto check env presence, gates, Supabase connectivity, candidate analysis/capture, context retrieval, distillation, and open-loops using a neutral seed text.tests/unit/pandora-mcp-env.test.tsto cover alias resolution and the new explicit error/gate messages.package.jsonscript entries added for convenience.Testing
npm run typecheckandnpm run lint(lint had pre-existing warnings only). — passed.tests/unit/pandora-mcp-auth.test.tsandtests/unit/pandora-mcp-tools.test.ts— passed.tests/unit/pandora-mcp-env.test.tscovering alias acceptance and explicit missing-env/gate messages — passed.npm run test— all tests passed (85 files, 550 tests in CI run here).npm run buildand rannpm run env:policy— both succeeded (build emitted Next.js warnings only).npm run memory:smokein this environment and it failed safely due to intentionally-missing deployment secrets, producing explicit missing-env messages (Missing server env: NEXT_PUBLIC_SUPABASE_URL,Missing server env: PANDORA_MCP_DB_KEY,Missing server env: PANDORA_MCP_USER_ID or PANDORA_MEMORY_BRIDGE_USER_ID) rather thanInvalid API key; this is expected in a non-secret local environment.Files changed or added (key):
lib/services/pandora-mcp-env.ts,lib/services/mcp-auth.ts,app/api/mcp/route.ts,scripts/memory-smoke.ts,tests/unit/pandora-mcp-env.test.ts,package.json.Codex Task
Summary by CodeRabbit
New Features
Bug Fixes
Tests