Skip to content

Document missing env vars and add a CI check for the invariant - #173

Open
Otfrugger wants to merge 1 commit into
determined-001:mainfrom
Otfrugger:document-env-vars-and-ci-check
Open

Document missing env vars and add a CI check for the invariant#173
Otfrugger wants to merge 1 commit into
determined-001:mainfrom
Otfrugger:document-env-vars-and-ci-check

Conversation

@Otfrugger

Copy link
Copy Markdown
Contributor

closes #164

Summary

The issue's premise was that apps/api/src/env.ts is the single reader of process configuration. That's no longer true — grepping apps/api/src for process.env. (excluding env.ts itself) turned up six more files reading env vars directly: routes/telemetry.ts, routes/webhooks.ts, services/webhook-sender.ts, services/container.ts, services/secret-crypto.ts, services/ssrf-guard.ts. Most of those reads (TELEMETRY_TOKEN, WEBHOOK_SECRET_ENCRYPTION_KEY, NODE_ENV) were already fine or already documented, but three were not: WEBHOOK_HOST_ALLOWLIST, ANCHOR_PROBE_FAILURE_THRESHOLD, ANCHOR_PROBE_COOLDOWN_MS. I've scoped the fix (and the new check) to all of apps/api/src, not just env.ts, so this doesn't just recreate the same gap for those files.

.env.example additions (commented, with default + one-line effect)

  • RATE_LIMIT_STRICT_WINDOW_MS / RATE_LIMIT_STRICT_MAX
  • TRUST_PROXY_HOPS — carries the exact blank-value footgun warning from docs/MAINNET.md (TRUST_PROXY_HOPS= parses to 0, silently collapsing every client into one rate-limit bucket)
  • REDIS_URL, DEFAULT_SELLER_WALLET (previously prose-only, now a real line), DEFAULT_SELLER_NAME
  • WEBHOOK_HOST_ALLOWLIST, ANCHOR_PROBE_FAILURE_THRESHOLD, ANCHOR_PROBE_COOLDOWN_MS
  • WATCHER_CONCURRENCY, WATCHER_MAX_ACCOUNTS_PER_TICK, WATCHER_CIRCUIT_BREAKER_THRESHOLD, WATCHER_CIRCUIT_BREAKER_COOLDOWN_MS, WATCHER_IDLE_BACKOFF_TICKS, WATCHER_AGGRESSIVE_POLL_TICKS (noted as currently unused by the watcher loop — isNewAccount bypasses backoff entirely instead of consulting this var)
  • SHUTDOWN_TIMEOUT_MS

None of these need a .env.public.example entry: per that file's own header ("lists only what differs from .env.example, plus everything the guardrails make mandatory"), TRUST_PROXY_HOPS, DEFAULT_SELLER_WALLET and REDIS_URL are already there, and the rest don't have mainnet-specific defaults.

CI check

Added scripts/check-env-docs.mjs, mirroring the existing scripts/check-domain-boundary.mjs pattern (dependency-free .mjs, regex scan, process.exit(1) with actionable output). It scans apps/api/src for process.env.X reads and fails if X has no matching X= line in .env.example or .env.public.example, with an explicit exemption list (currently NODE_ENV, RENDER_EXTERNAL_HOSTNAME — both platform-injected, not operator-set). Wired into package.json as docs:check-env-docs and into .github/workflows/ci.yml alongside the other docs:check-* steps.

Verification

  • Ran node scripts/check-env-docs.mjs directly (Node is available in this environment; pnpm/turbo/vitest are not) — passes clean against the updated .env.example.
  • Byte-scanned every changed file for stray control bytes before committing — none found.
  • Could not run: pnpm typecheck, pnpm test, or the full pnpm docs:check-env-docs via the turbo/pnpm pipeline, since no pnpm/turbo install is available here. Please run those before merging — the standalone node scripts/check-env-docs.mjs invocation above is the only local verification I could do.

Caveats

  • Scope is larger than the issue's literal variable list (added the 3 self-discovered vars above) — flagging in case that's more than intended.
  • WATCHER_AGGRESSIVE_POLL_TICKS is documented honestly as unused dead config rather than inventing behavior for it; may be worth a separate follow-up to either wire it up or remove it.

…iant

Add commented KEY=default entries to .env.example for every process.env
read under apps/api/src that had no matching line in an example file:
RATE_LIMIT_STRICT_WINDOW_MS/MAX, TRUST_PROXY_HOPS, REDIS_URL,
DEFAULT_SELLER_WALLET, DEFAULT_SELLER_NAME, WEBHOOK_HOST_ALLOWLIST,
ANCHOR_PROBE_FAILURE_THRESHOLD/COOLDOWN_MS, WATCHER_CONCURRENCY,
WATCHER_MAX_ACCOUNTS_PER_TICK, WATCHER_CIRCUIT_BREAKER_THRESHOLD/COOLDOWN_MS,
WATCHER_IDLE_BACKOFF_TICKS, WATCHER_AGGRESSIVE_POLL_TICKS, SHUTDOWN_TIMEOUT_MS.
TRUST_PROXY_HOPS carries the blank-value footgun warning from docs/MAINNET.md.

Add scripts/check-env-docs.mjs, mirroring check-domain-boundary.mjs: it
scans apps/api/src for process.env.X reads and fails if X (outside an
explicit NODE_ENV/RENDER_EXTERNAL_HOSTNAME exemption for platform-injected
vars) isn't documented in .env.example or .env.public.example. Wired into
CI via a new docs:check-env-docs script.
@drips-wave

drips-wave Bot commented Aug 28, 2026

Copy link
Copy Markdown

@Otfrugger 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

@vercel

vercel Bot commented Aug 28, 2026

Copy link
Copy Markdown

@Otfrugger is attempting to deploy a commit to the determined's projects Team on Vercel.

A member of the Team first needs to authorize it.

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

8.10 - Ten environment variables the code reads are in no example file

1 participant