[Jossy-ossy]

Summary

• Centralize runtime environment access in the Zod-backed config module and validate numeric, boolean, network, and production-required settings.
• Replace direct environment reads across service modules with typed config accessors.
• Document per-network overrides and add validation regression coverage.

Closes #338

[drips-wave]

@Jossy-ossy Great news! 🎉 Based on an automated assessment of this PR, the linked Wave issue(s) no longer count against your application limits.

You can now already apply to more issues while waiting for a review of this PR. Keep up the great work! 🚀

Learn more about application limits

[Jossy-ossy]

@Miracle656 I resolved the merge conflicts and updated this branch. Please take another look and merge when ready.

[Miracle656]

The direction here is right and I want it — one Zod-backed module instead of process.env reads scattered across eighteen files is a real improvement, and the per-network override documentation is overdue. But the strictness is miscalibrated in a way that would take the service down, so I cannot merge it yet. One small change fixes most of it.

1. This would crash-loop production

src/config.ts:83:

if (env.NODE_ENV === 'production') {
  for (const key of ['DATABASE_URL', 'REDIS_URL', 'ADMIN_API_KEY', 'ADMIN_TOKEN'] as const) {
    if (!env[key]?.trim()) ctx.addIssue({ code: 'custom', path: [key], message: 'is required in production' })
  }
}

and parseEnvironment throws on any issue. config is a module-level export (src/index.ts reads config.api.port), so it is built at import time — which makes a missing key a boot failure, not a request failure.

Three of those four do not belong in that list, because the code already handles their absence safely:

• REDIS_URL — optional everywhere. Your own src/config.ts:377 gives it a default: getEnvironment().REDIS_URL ?? 'redis://localhost:6379'. Redis on this stack is a cache and a job queue, not the thing the process exists for. It is also the key most likely to be unset in the live environment right now.
• ADMIN_API_KEY — used in exactly one place, src/routes/pairs.ts:68, which already fails closed: if (!ADMIN_API_KEY || key !== ADMIN_API_KEY). Unset means that one route rejects everything, which is the correct degradation.
• ADMIN_TOKEN — same, src/api/auth.ts:79: if (!adminToken) return false.

So for all three, absence currently costs you one route or one cache. After this PR it costs you the whole API.

I am not reasoning from theory here. I did this exact thing to wraith yesterday and took it down for about forty minutes: a merged validateEnv() made a missing per-network RPC URL fatal at boot, the service crash-looped, and the fix was to narrow the check to DATABASE_URL and DIRECT_DATABASE_URL only. The rule that came out of it: a boot-time check may only cover what makes the whole process useless. A database URL qualifies. A cache URL and two route guards do not.

Please narrow the list to DATABASE_URL (and DIRECT_DATABASE_URL if you want it). Everything else should keep degrading at the point of use.

2. The list contradicts the defaults in the same file

Worth noticing on its own: src/config.ts:374 and :377 supply fallbacks for DATABASE_URL and REDIS_URL:

url: getEnvironment().DATABASE_URL ?? 'postgresql://lens:lens@localhost:5432/lens',
url: getEnvironment().REDIS_URL ?? 'redis://localhost:6379',

Those ?? branches can never be reached in production, because validation refuses before they run. Either the default is real and the key is optional, or the key is required and the default is a lie. Right now the file says both.

3. A removed test is recording a behaviour change that is not flagged

Our new CI guard caught this, and I think it caught something worth discussing rather than just a deletion:

1 test case(s) on origin/main would be removed by merging:
  - src/__tests__/bazaarRateLimit.test.ts :: falls back to its defaults for a nonsense limit

You replaced it with fails on a nonsense limit instead of silently accepting invalid configuration. I understand the intent and in a vacuum I agree with it. But look at what it means in place: previously a typo in BAZAAR_CATALOG_WRITES_PER_MIN quietly gave you 10/min. Now a typo in an optional tuning knob refuses the environment, and because config is built at import, that is the service not starting.

A rate-limit knob is exactly the kind of setting where falling back to a sane default is the better failure. Log it loudly, do not refuse. If you disagree, that is a legitimate position — but it is a deliberate behaviour change on a running service and it should be stated in the PR description, not left for a deleted test to imply.

Either way, please call it out explicitly and apply the tests-removed-intentionally label so the guard passes for the right reason.

4. A fallback that can no longer fire

src/bazaar/rateLimit.ts:

function envLimit(value: string | undefined, fallback: number): number {
  const raw = Number(value)
  return Number.isFinite(raw) && raw > 0 ? Math.floor(raw) : fallback
}

With validation rejecting a non-numeric value upstream, fallback is now dead. Either drop it, or keep the tolerant parse and drop the strict validation for this key — but not both, or the next reader cannot tell which layer is in charge.

What I would merge

Keep all of the typed accessors and the documentation. Narrow the production-required list to the database URL. Let malformed optional knobs log and fall back. Then the dead fallback in envLimit can stay honestly, and nothing in this PR can stop the service starting.

That is a small diff from where you are, and the rest of the work is good.

[Jossy-ossy]

Note for maintainers: GRAPHQL_MAX_QUERY_DEPTH is in the numeric-validation list, so a malformed value now falls back to 3 with a warning. main previously threw at startup. Please decide whether this should fail closed instead. Production now requires only DATABASE_URL, and malformed values for other keys are logged by name and ignored.