refactor: make kv redis optional#326
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
![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: 71068a7bcd
ℹ️ 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".
| SUPABASE_SERVICE_ROLE_KEY: z.string().min(1), | ||
| KV_REST_API_TOKEN: z.string().min(1), | ||
| KV_REST_API_URL: z.url(), | ||
| KV_REST_API_TOKEN: z.string().min(1).optional(), |
There was a problem hiding this comment.
When a deployment or local .env.local still contains the old KV_REST_API_URL= / KV_REST_API_TOKEN= entries with blank values, these fields are present as empty strings, so serverSchema rejects them before the new optional KV logic can run. That makes bun run dev/prebuild fail even though the runtime helper treats empty KV values as not_configured, so existing environments copied from the previous .env.example cannot actually opt out without deleting the variables entirely.
Useful? React with 👍 / 👎.
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
| context: { | ||
| email: validationResult.address, | ||
| reason: warnedAlternateEmail.reason, | ||
| }, |
There was a problem hiding this comment.
🔒 Agentic Security Review
Severity: MEDIUM
The new KV-unavailable warning path logs raw user emails via context.email (validationResult.address). This introduces direct PII logging for signup attempts whenever KV is not configured or temporarily failing.
Impact: Email addresses can be exposed through centralized log pipelines and wider operator access beyond the primary auth data path.
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
| .refine( | ||
| (data) => Boolean(data.KV_REST_API_URL) === Boolean(data.KV_REST_API_TOKEN), | ||
| { | ||
| message: 'KV_REST_API_URL and KV_REST_API_TOKEN must be set together', | ||
| path: ['KV_REST_API_URL'], | ||
| } | ||
| ) |
There was a problem hiding this comment.
🟡 The new KV .refine block sets path: ['KV_REST_API_URL'] only, but validates a two-variable invariant. When an operator sets KV_REST_API_URL but forgets KV_REST_API_TOKEN (or vice versa), Zod's prettifyError will attribute the failure to KV_REST_API_URL — potentially the var that was actually set correctly. The two other refines in this same file (billing, captcha) list both fields in the path; fix is path: ['KV_REST_API_URL', 'KV_REST_API_TOKEN'].
Extended reasoning...
What the bug is
In scripts/check-app-env.ts:9-15, the new refine validates the together-or-neither invariant for KV configuration:
.refine(
(data) => Boolean(data.KV_REST_API_URL) === Boolean(data.KV_REST_API_TOKEN),
{
message: 'KV_REST_API_URL and KV_REST_API_TOKEN must be set together',
path: ['KV_REST_API_URL'],
}
)The check inspects two environment variables, but the issue path references only one. When the predicate fails, the path Zod surfaces points at KV_REST_API_URL regardless of which side of the pair was actually omitted.
Why this is inconsistent
The two other paired-variable refines added to this same file follow the opposite convention — they list both fields in path:
- Billing refine (lines 26-30):
path: ['BILLING_API_URL', 'NEXT_PUBLIC_STRIPE_PUBLISHABLE_KEY'] - Captcha refine (lines 42-46):
path: ['NEXT_PUBLIC_TURNSTILE_SITE_KEY', 'TURNSTILE_SECRET_KEY']
So the new KV refine deviates from a convention already established in the same file.
Concrete proof / step-by-step
- Operator sets
KV_REST_API_URL=https://kv.example.comin.env.localbut forgetsKV_REST_API_TOKEN. - CI runs
bun run check:env(or whatever invokes this script). Boolean(data.KV_REST_API_URL) === Boolean(data.KV_REST_API_TOKEN)→true === false→false. Refine emits an issue withpath: ['KV_REST_API_URL'].validateEnvcallsz.prettifyError(parsed.error)(seesrc/lib/env.ts:73), which formats issues using theirpathas the field label.- The output points the operator at
KV_REST_API_URL— the variable they did set correctly — even though the missing one isKV_REST_API_TOKEN.
The full message text still names both vars, so a careful reader can recover. But the path label is misleading in CI/build logs, and exactly inverts what's useful when half the pair is set.
Addressing the refutation
One verifier objected that listing two strings in path is itself an imprecise use of Zod's API (since path semantically represents a nested object path, not a list of offending fields), so the new code isn't strictly worse — just different. That's a fair critique of the broader pattern, but the practical prettifyError output for a top-level schema renders path joined as a label, so listing both fields does produce a more useful pointer to the operator than listing just one. More importantly, the value here is consistency within the file: the rest of the file uses the two-element form, so a future reader scanning the refines will expect the same shape. The fix is one line and aligns with the established pattern.
Severity & fix
Nit. The script still fails the build with a message naming both vars, so no operator is truly stuck. Fix:
path: ['KV_REST_API_URL', 'KV_REST_API_TOKEN'],1 participant
@vercel/kv redis is now optional for the dashboard. The app boots and runs core auth + dashboard flows without
KV_REST_API_URL/KV_REST_API_TOKENconfigured.