P1: Validate config at startup + fix boolean env parsing (#174, #74)

[dkijania]

⚠️ Upgrade note — behaviour changes on deploy

This PR fixes a bug, and fixing it changes running behaviour for anyone who worked around it. Two things to check before rolling out:

1. ENABLE_*=false now actually means false.

Because of #74, setting ENABLE_LOGGING=false, ENABLE_JAEGER=false, ENABLE_INTROSPECTION=false, or ENABLE_BLOCK_TRANSACTION_DETAILS=false left the feature ON — the string "false" is truthy. After this PR they turn OFF on the next deploy.

That is the fix, but it means a deployment that has (unknowingly) depended on the old state will see the feature disappear. If you set one of these to false and want the feature, switch it to true.

Worth noting for ENABLE_BLOCK_TRANSACTION_DETAILS in particular: the mina-explorer needs it on for block-detail views, so any archive that set it to false has actually been serving those and will stop.

2. Invalid config now aborts boot instead of silently defaulting.

Values that used to fall back quietly — BLOCK_RANGE_SIZE=abc → 10000, a non-numeric PORT, a mistyped boolean like ENABLE_JAEGER=sometimes — now fail fast with a clear message. That is the point, but a deployment carrying a typo it never noticed will refuse to start rather than come up misconfigured. The error names the offending variable.

There is no CHANGELOG in the repo yet, so this section is the release note; it should be carried into the 1.0.0 notes (#198).

What & why

Part of the production-readiness epic (#163). Closes #174, closes #74.

Two problems:

1. Boolean env bug (Fix Boolean env vars parsing #74): booleans were read with ad-hoc truthiness — if (process.env.ENABLE_LOGGING), if (!process.env.ENABLE_INTROSPECTION), if (!process.env.ENABLE_JAEGER ...). The string "false" is truthy, so ENABLE_LOGGING=false enabled logging, ENABLE_JAEGER=false enabled tracing, etc.
2. No startup validation: typos (missing PG_CONN, non-numeric PORT) surfaced as confusing runtime behaviour instead of failing fast.

Changes

• New src/config.ts:
  • parseBoolean — understands true/false, 1/0, yes/no, on/off (case-insensitive); unrecognised/empty → fallback.
  • validateConfig / assertValidConfig — aggregate problems (missing PG_CONN, non-positive-integer PORT/BLOCK_RANGE_SIZE, mistyped booleans) and throw one clear message.
• Every boolean env read now goes through parseBoolean (plugins.ts, server.ts, jaeger-tracing.ts) — fixes Fix Boolean env vars parsing #74 and makes the previously-strict === 'true' checks accept the same spellings.
• assertValidConfig() runs first thing at startup, so misconfig fails fast.

Testing

• npm run build — clean
• npm run test:unit — all pass; 11 new assertions covering boolean spellings (incl. the "false" case), each validation rule, error aggregation, and the throw
• npm run lint / npx prettier --debug-check . — clean

🤖 Generated with Claude Code

[SanabriaRusso]

Nice fix — the "false"-is-truthy footgun on ENABLE_LOGGING / ENABLE_JAEGER / ENABLE_INTROSPECTION has bitten operators before, and failing fast on a missing PG_CONN or a typo'd PORT is exactly right. Also nice that it needs no new dependency. Two small, non-blocking asks:

1. Call out the silent behavior flip in the PR description / an upgrade note. Because of the old bug, anyone who set ENABLE_LOGGING=false, ENABLE_JAEGER=false, or ENABLE_INTROSPECTION=false was actually running with those features ON. After this PR they correctly turn OFF on the next deploy. That's the fix — but it's a real, operator-visible change for anyone who's been (unknowingly) relying on the old state, so an explicit "if you set these to false and want the feature, switch to true" line would save a surprise. Same for the new fail-fast: a value that used to silently default (e.g. BLOCK_RANGE_SIZE=abc → 10000) will now abort boot. There's no CHANGELOG in the repo, so the PR body / release notes is the natural place.

2. Lock in the multi-host PG_CONN behavior with a test. I verified the validation is a plain non-empty check, so the documented HA form still validates — which is exactly right, since a stricter URL parser here would break every HA deployment (and the archives the mina-explorer talks to are HA). A regression test would keep a future "hardening" from silently rejecting it:

test('accepts multi-host HA connection strings', () => {
  assert.deepStrictEqual(
    validateConfig({ PG_CONN: 'postgres://host1:5432,host2:5432/archive' }),
    []
  );
});

(The HA form is documented in docs/getting-started.md under the PG_CONN notes.)

[dkijania]

Thanks @SanabriaRusso — both done.

1. Upgrade note. Added to the PR description (c2c2a01 carries the test; the note is in the body, since there's no CHANGELOG). It calls out both flips explicitly: ENABLE_*=false was leaving features ON and will now correctly turn them OFF on next deploy, and previously-silent fallbacks (BLOCK_RANGE_SIZE=abc → 10000) now abort boot.

One case worth singling out that I added while writing it: ENABLE_BLOCK_TRANSACTION_DETAILS=false has the same bug, and the mina-explorer needs that flag on for its block-detail views. So any archive that set it to false has actually been serving them, and will stop after this PR — the fix, but a visible one for exactly the client we've been protecting everywhere else in this train. Flagged it in the note so it's carried into the 1.0.0 release notes (#198).

2. Multi-host PG_CONN test. Added, plus a second covering credentials and query params:

test('accepts multi-host HA connection strings', () => {
  assert.deepStrictEqual(
    validateConfig({ PG_CONN: 'postgres://host1:5432,host2:5432/archive' }),
    []
  );
});

Your point about why is the part worth preserving, so it's in the test as a comment: the non-empty check is deliberate, and a stricter URL parser here would break every HA deployment. The test exists so a future "hardening" fails loudly instead of quietly rejecting the documented form.

[SanabriaRusso]

Verdict: MERGEABLE ✅

Second pass at c2c2a01. Both of @SanabriaRusso's asks are addressed (upgrade note in the body; tests/unit/config.test.ts:42-63 carries the multi-host PG_CONN test and the "why" comment). One factual error in the upgrade note is worth fixing before it lands in the 1.0.0 notes — details under nits.

What I checked

• No newly-mandatory env var. PG_CONN is the only required one, and it was already hard-required: src/db/archive-node-adapter/archive-node-adapter.ts:38-41 throws Missing Postgres Connection String when it is unset. The validator just moves that failure earlier and makes it clearer. Both shipped env templates set it (.env.example.compose:20, .env.example.lightnet:6); there are no k8s/helm manifests in-repo. No existing deployment becomes unbootable for lack of a variable.
• HARD CONSTRAINT Add Actions resolver support #1 (error text) untouched. No resolver/schema/maskedErrors change. The only new error surface is NoSchemaIntrospectionCustomRule, a normal validation error, so Cannot query field / Unknown argument / inBestChain markers keep their graphql-js wording.
• HARD CONSTRAINT Add a Dockerfile to build and run the server #2 (CORS) untouched. src/server/server.ts:24 still origin: process.env.CORS_ORIGIN ?? '*'; CORS_ORIGIN isn't in the validator's var list, so an unset value still yields *.
• Introspection flip is safe for both consumers. useDisableIntrospection (5.0.3, lockfile:3174-3178) only does addValidationRule(NoSchemaIntrospectionCustomRule), and that rule rejects a field only when isIntrospectionType(getNamedType(context.getType())). __typename returns String!, so mina-explorer-api's READINESS_QUERY = "{ __typename }" (app/observability.py:307) is unaffected. Neither consumer queries __schema/__type anywhere (grepped both repos; mina-explorer has no codegen/introspection tooling at all).
• Fail-fast is correct and secret-free. assertValidConfig() is the first statement in main() (src/index.ts), the existing catch does console.error(...) + process.exit(1), and nothing binds a port before it — k8s never sees a briefly-ready pod. PG_CONN is checked for emptiness only and its value is never echoed (message is just PG_CONN is required (Postgres connection string).); only PORT/BLOCK_RANGE_SIZE/ENABLE_* echo their values, none of which are secrets. No leak.
• Live check on prod, so the table below isn't hypothetical: POST {"query":"{ __schema { queryType { name } } }"} to both archive-node-api.gcp.o1test.net and devnet-archive-node-api.gcp.o1test.net returns {"data":{"__schema":{"queryType":{"name":"Query"}}}} today — introspection is currently ON in the o1-labs deployments. And mainnet returns populated transactions.userCommands, which under the old strict === 'true' check means ENABLE_BLOCK_TRANSACTION_DETAILS is already literally true there.

Every var the validator touches

Var	Status	Default	Before	After	Delta on upgrade
PG_CONN	required (unchanged)	—	adapter threw at boot	validator throws earlier, better message	none
PORT	optional	8080	raw string handed to listen()	must be positive integer	non-numeric / 0 / negative now aborts
BLOCK_RANGE_SIZE	optional	10000	Number(x) || 10000	must be positive integer	garbage silently became 10000, now aborts
ENABLE_GRAPHIQL	optional	false	=== 'true'	parseBoolean	1/yes/on/TRUE: off → ON; unrecognised: abort
ENABLE_INTROSPECTION	optional	false	any non-empty string → ON	parseBoolean	false/0/no/off: on → OFF; unrecognised (was ON): abort
ENABLE_LOGGING	optional	false	any non-empty string → ON	parseBoolean	same as above (gates OTel, see nit 5)
ENABLE_JAEGER	optional	false	any non-empty string → ON	parseBoolean	same as above
ENABLE_BLOCK_TRANSACTION_DETAILS	optional	false	=== 'true'	parseBoolean	1/yes/on/TRUE: off → ON; unrecognised: abort
ENABLED_QUERIES, LOG_LEVEL, CORS_ORIGIN, JAEGER_SERVICE_NAME, JAEGER_ENDPOINT	optional	unchanged	direct process.env read	still direct, unvalidated	none (see nits 3 and 4)

Defaults for unset vars are identical before and after, which is why this is mergeable.

Non-blocking nits

1. The upgrade note is wrong about ENABLE_BLOCK_TRANSACTION_DETAILS — please fix before it becomes the 1.0.0 release note.
Old code was process.env.ENABLE_BLOCK_TRANSACTION_DETAILS === 'true' (src/server/server.ts:11-12 on main) — a strict check, not the #74 truthiness bug. So =false already meant off; nobody has been "serving those and will stop". The real delta for this var runs the other way, and it's the more interesting one because it adds payload:

ENABLE_BLOCK_TRANSACTION_DETAILS and ENABLE_GRAPHIQL used a strict === 'true' check, so they are unaffected by the "false" bug. What changes for them is the opposite direction: 1, yes, on, or TRUE were previously read as off and now turn on — for ENABLE_BLOCK_TRANSACTION_DETAILS that means userCommands / zkappCommands / feeTransfers start appearing in blocks responses; for ENABLE_GRAPHIQL it means the playground starts being served at /. The o1-labs archives are unaffected: they already set ENABLE_BLOCK_TRANSACTION_DETAILS=true literally.

The three vars that genuinely flip on → off are exactly the three that used bare truthiness: ENABLE_LOGGING, ENABLE_INTROSPECTION, ENABLE_JAEGER.

2. The one real crash-loop path deserves a line in the note: a junk boolean value.
ENABLE_INTROSPECTION=enabled (or any unrecognised non-empty string) works today — bare truthiness made it ON — and after this PR it aborts boot. That's the intended fail-fast, but it's the only way a currently-running deployment stops booting, so operators should pre-flight it. Suggested addition:

Any ENABLE_* value that isn't one of true/false, 1/0, yes/no, on/off now aborts boot. Under the old code an unrecognised value silently meant on for ENABLE_LOGGING/ENABLE_INTROSPECTION/ENABLE_JAEGER, so check before rolling out:

for v in ENABLE_GRAPHIQL ENABLE_INTROSPECTION ENABLE_LOGGING ENABLE_JAEGER ENABLE_BLOCK_TRANSACTION_DETAILS; do
  printf '%s=%s\n' "$v" "$(printenv "$v")"
done

Introspection is currently on in the o1-labs archives; if their ENABLE_INTROSPECTION is not literally a recognised true spelling, this deploy either turns introspection off or refuses to start. (Turning it off is harmless for mina-explorer and mina-explorer-api — neither uses __schema/__type, and { __typename } readiness probes keep working.)

3. ENABLED_QUERIES is the one env var that can break both consumers, and it's the one the new module doesn't cover. src/resolvers.ts:61-64 splits it and filters both the resolver map and the schema AST — a typo like ENABLED_QUERIES=blocks,event silently deletes events from the schema, and both consumers then get Cannot query field "events" and degrade to empty views rather than failing loudly. It's also undocumented. Exactly the class of bug this PR exists to kill:

// src/config.ts
/** Root query fields in schema.graphql — keep in sync. */
const KNOWN_QUERIES = ['events', 'actions', 'networkState', 'blocks'] as const;

// ...inside validateConfig(), after the POSITIVE_INT_VARS loop:
  const enabledQueries = env.ENABLED_QUERIES;
  if (enabledQueries !== undefined) {
    const names = enabledQueries
      .split(',')
      .map((q) => q.trim())
      .filter((q) => q !== '');
    if (names.length === 0) {
      errors.push(
        'ENABLED_QUERIES is set but lists no queries; unset it to expose all of ' +
          `${KNOWN_QUERIES.join(', ')}.`
      );
    }
    const unknown = names.filter(
      (n) => !(KNOWN_QUERIES as readonly string[]).includes(n)
    );
    if (unknown.length > 0) {
      errors.push(
        `ENABLED_QUERIES contains unknown queries: ${unknown.join(', ')}. ` +
          `Known queries: ${KNOWN_QUERIES.join(', ')}.`
      );
    }
  }

// tests/unit/config.test.ts
test('accepts a valid ENABLED_QUERIES subset', () => {
  assert.deepStrictEqual(
    validateConfig({ ...valid, ENABLED_QUERIES: 'blocks, networkState' }),
    []
  );
});

test('rejects a typo in ENABLED_QUERIES that would delete a root field', () => {
  const errors = validateConfig({ ...valid, ENABLED_QUERIES: 'blocks,event' });
  assert.ok(errors.some((e) => /unknown queries: event/.test(e)));
});

test('rejects an empty ENABLED_QUERIES list', () => {
  assert.ok(
    validateConfig({ ...valid, ENABLED_QUERIES: '' }).some((e) =>
      /ENABLED_QUERIES/.test(e)
    )
  );
});

<!-- docs/getting-started.md, config table -->
| `ENABLED_QUERIES` | *(all)* | Comma-separated subset of `events,actions,networkState,blocks` to expose; omitted fields are removed from the schema |

4. The config module isn't the single source of truth yet. Still reading process.env directly at head: src/index.ts:7 (PORT), src/server/server.ts:9,10,24 (LOG_LEVEL, BLOCK_RANGE_SIZE, CORS_ORIGIN), src/tracing/jaeger-tracing.ts:22,38 and src/tracing/jaeger-setup.ts:12 (JAEGER_*), src/resolvers.ts:61. So BLOCK_RANGE_SIZE is parsed twice by two different expressions (Number(v) in the validator vs Number(v) || 10000 in server.ts). They agree today; exporting the parsed values from config.ts would keep them agreeing.

5. Ordering caveat, harmless today. assertValidConfig() is the first statement in main(), but ESM evaluates ./context.js, ./server/server.js and ./server/plugins.js (and transitively resolvers.ts's ENABLED_QUERIES schema filtering and server.ts's module-level constants) before main() runs. Nothing listens on a port there, so the k8s concern doesn't bite — but if you want the assert to genuinely run first, move it into a side-effect module and make it the first import (import './config-assert.js'; above the others), since imports evaluate in source order.

6. Two trivia: PORT=0 (ephemeral port, used by some test harnesses) is now rejected — probably fine, just deliberate. And the docs row ENABLE_LOGGING | Enable request logging is misleading: it gates useOpenTelemetry (src/server/plugins.ts:16-27), while useLogger is unconditional — so the upgrade note's "logging turns off" is really "tracing turns off".

Automated second-pass review — focus: downstream compatibility with mina-explorer / mina-explorer-api.

[SanabriaRusso]

Approving on the basis of the second-pass review comment above: no mid-to-high severity security, compatibility, or degradation issue found, and the downstream contract with mina-explorer / mina-explorer-api holds — GraphQL validation error text reaches errors[].message verbatim, the browser SPA's cross-origin access is preserved, and the real consumer query shapes (including the 2000-block analytics query and the 500-row page crawl) still pass.

Two things this approval does not mean:

• It does not close the non-blocking items in the review comment. Several are worth fixing before or shortly after merge; they are written up there with patches.
• It does not by itself mean the branch is ready to merge. main requires branches to be up to date, so this needs an update-branch (or a rebase, if the branch is conflicting) first, and a few PRs in this series have cross-PR ordering constraints called out in their review comments.

Automated second-pass review — focus: downstream compatibility with mina-explorer / mina-explorer-api.

[SanabriaRusso]

Approving. I went after the two things that would have made this dangerous — a partial boolean migration, and a startup validator stricter than what actually works in production — and neither is present.

The boolean migration is complete. I grepped every process.env. read in src/ on main: there are exactly five boolean vars and all five are routed through parseBoolean — ENABLE_LOGGING and ENABLE_INTROSPECTION (plugins.ts:16,30), ENABLE_BLOCK_TRANSACTION_DETAILS and ENABLE_GRAPHIQL (server.ts:12,21), ENABLE_JAEGER (jaeger-tracing.ts:23). The remaining raw reads (PORT, PG_CONN, LOG_LEVEL, BLOCK_RANGE_SIZE, CORS_ORIGIN, JAEGER_ENDPOINT, JAEGER_SERVICE_NAME) are strings, numbers, or presence checks and are correctly left alone. A partial migration was the likely defect here and it isn't one. ENABLE_INTROSPECTION=false finally means off.

The PG_CONN validation is correctly loose, and 2eded9b9 is exactly the right test to have written. src/config.ts:53-56 checks non-emptiness only — there is no URI parsing at all — so no postgres.js-valid form can be rejected, multi-host HA included. The new tests assert that postgres://host1:5432,host2:5432/archive and the credentials+?sslmode=require form both validate clean, with a comment stating the looseness is deliberate so a future "hardening" fails loudly. That's the right instinct, and the opposite of the failure mode I was checking for.

Also worth noting the required-PG_CONN check isn't a new fatal condition at all: ArchiveNodeAdapter's constructor (archive-node-adapter.ts:38-42) already throws on a falsy connection string on main. This just moves the failure earlier with a better message.

For the record, the complete set of newly-fatal conditions (src/config.ts:51-101) is: blank PG_CONN (not new, per above); a boolean var with an unrecognised non-empty spelling; PORT/BLOCK_RANGE_SIZE not a positive integer (today these silently fall back via Number(x) || default, so this only rejects garbage); and an unknown name in ENABLED_QUERIES. Nothing there rejects a config that works today except the boolean-spelling case in nit 1.

14b93138 catches something genuinely bad. KNOWN_QUERIES (src/config.ts:21) is ['events','actions','networkState','blocks'] — I diffed that against schema.graphql's type Query and it is an exact match, nothing more or less. And I reproduced what happens today without the validator: ENABLED_QUERIES="" on main builds a schema whose Query type has zero fields — makeExecutableSchema accepts it without complaint — so the server boots healthy, passes every health check, and returns Cannot query field for every request. A silent total outage. Refusing to boot is strictly better.

ENABLED_QUERIES is a server-side boot-time var (it filters the schema in src/resolvers.ts:61-93), not a client-supplied filter, so nothing mina-explorer-api sends can trip it.

Docs check out. Most of both diffs is prettier table re-alignment. The new prose is accurate, and it corrects a pre-existing error: ENABLE_LOGGING gates useOpenTelemetry, so "Enable OpenTelemetry request tracing" is right and the old "Enable request logging" was wrong. The new ENABLED_QUERIES row correctly documents default (all) and the four valid names.

Non-blocking nits

1. y / n / t / f aren't recognised spellings. ENABLE_GRAPHIQL=y works today (yields off, since main compares === 'true') and after this PR aborts startup. The failure is loud, names the variable and value, and is documented — which is why this isn't blocking — but a one-line hedge:

   -const TRUE_VALUES = new Set(['true', '1', 'yes', 'on']);
   -const FALSE_VALUES = new Set(['false', '0', 'no', 'off']);
   +const TRUE_VALUES = new Set(['true', 't', '1', 'yes', 'y', 'on']);
   +const FALSE_VALUES = new Set(['false', 'f', '0', 'no', 'n', 'off']);

2. assertValidConfig() runs after src/resolvers.ts has already evaluated. ESM imports are hoisted, so the ENABLED_QUERIES schema filtering at resolvers.ts:61-93 happens at module-load time, before index.ts reaches the validator. The outcome is still correct (bad schema built, never served, validator throws, process exits 1) so the validation isn't dead code — but the schema is constructed pointlessly first. Only worth restructuring if you touch this again.

3. Release-note item: ENABLE_GRAPHIQL=1 / =yes flips from off to on (previously only the literal 'true' counted). All checked-in examples use "true"/"false" so this is unlikely to bite, but it is an exposure change and belongs in the notes alongside the ENABLE_INTROSPECTION fix.

4. BLOCK_RANGE_SIZE=0 flips from "silently means 10000" to fatal. Theoretical.

Downstream: none for either consumer. The one behavioural flip with downstream reach is ENABLE_INTROSPECTION="false" finally meaning off — on main any non-empty value turns introspection on, so a deployment that set false has been serving introspection this whole time. I grepped both consumer repos: every __schema/introspection hit is developer tooling against the Mina daemon or documentation prose, never a request path against this service. explorer-api's { __typename } probe is unaffected — NoSchemaIntrospectionCustomRule rejects only fields whose type is an introspection type, and __typename returns String. No schema or error-text change.

Merge ordering: src/server/plugins.ts is touched by this PR, #188 and #194 in the same ~20 lines, and docs/getting-started.md's config table by this PR and #188. All semantically compatible — they just overlap textually.

[SanabriaRusso]

Re-approving after the rebase (the previous approval was dismissed by the force-push).

Re-verified the delta against the commit I approved (14b9313). The only content change is the rebase absorbing the env vars that landed on main after my review — ENABLE_METRICS (#191) and READINESS_PING_TIMEOUT_MS (#187) — into this PR's reformatted config tables. I diffed the documented env-var sets both ways: nothing was dropped from README.md or docs/getting-started.md; the only addition beyond main is ENABLED_QUERIES, which is this PR's own. All checks green.

Carrying forward the round-2 verification unchanged:

• The validator is not stricter than reality: PG_CONN is checked for non-emptiness only, with no URI parsing, so no postgres.js-valid form can be rejected. The new tests pin multi-host and credentials+sslmode forms as valid, with a comment saying the looseness is deliberate. Correct call.
• ENABLED_QUERIES validation catches something real: today ENABLED_QUERIES="" builds a zero-field Query schema that boots healthy and fails every request — a silent total outage.

---

Please take this before merge (two lines, CI will not catch it)

The boolean migration was complete when I reviewed it. It is no longer complete, through no fault of this branch — ENABLE_METRICS arrived on main with #191 after my review, and the rebase carried it in untouched:

• src/server/plugins.ts:17 — if (process.env.ENABLE_METRICS === 'true'), not parseBoolean(...).
• src/config.ts — ENABLE_METRICS is absent from BOOLEAN_VARS, so it is not validated.

That makes two statements this PR's own docs add at docs/getting-started.md:175 false for exactly one variable:

Boolean variables (ENABLE_*) accept true/false, 1/0, yes/no, or on/off (case-insensitive) … Any other non-empty spelling now aborts startup.

With this branch as it stands, ENABLE_METRICS=1 (or yes, on, TRUE) silently yields no /metrics endpoint, and a typo silently yields the same instead of aborting. The blast radius is small in practice — the reference manifests in #196 and the runbook all use the literal true — but this is precisely the class of bug (#74) the PR exists to eliminate, and the failure is silent.

Fix: add 'ENABLE_METRICS' to BOOLEAN_VARS and change plugins.ts:17 to parseBoolean(process.env.ENABLE_METRICS). Approving rather than blocking because it regresses nothing relative to main and the shipped reference config is unaffected — but the docs sentence is wrong as written until one of the two is changed.

Release note: two behaviour changes here are breaking and belong in the v1.0.0 notes — ENABLE_INTROSPECTION="false" finally meaning off (deployments that set "false" have been serving introspection this whole time), and ENABLE_GRAPHIQL=1/yes now enabling GraphiQL.