docs(env): document LOG_LEVEL, and guard against the next undocumented variable - #583
Conversation
…d variable CLAUDE.md says every environment variable is documented in .env.example with an example. Nothing enforced that, and LOG_LEVEL drifted: src/lib/logger.ts reads it, the Helm chart writes it from config.logLevel, docs/HELM_CHART.md lists it, and the file operators are told to copy never mentioned it. The new block names the four accepted values, says an unrecognised value is ignored rather than rejected, and gives both NODE_ENV-dependent defaults, since 'the default depends on NODE_ENV' is the part an operator cannot guess. tests/unit/env-documentation.test.ts extracts every literal process.env.NAME read under src/ and requires each to be documented or allowlisted. The five allowlisted names are platform or build-time, each with its reason, and two further tests keep the allowlist from becoming a dumping ground: every entry must still be read somewhere, and no entry may also be documented. A third asserts the extractor found something, so the guard cannot pass vacuously. Documentation is matched per line rather than by substring, so a variable mentioned in prose does not count as documented. Verified the guard fails before the .env.example change (2 failures) and passes after (5 pass). Dynamic process.env[name] reads are out of scope, per the issue. Closes libredb#566
|
Thanks, this is a strong first PR. The failing test first with both directions shown, the non-vacuity assertion on the extractor, and allowlist entries carrying reasons as data rather than comments are all things I usually have to ask for. One blocker and one thing worth knowing. Blocker: Biome. Lint, Typecheck and Build fails on formatting only, in the new test file. bun run format:fix fixes it. The full local gate set is in CONTRIBUTING.md; running it before pushing catches this class of failure without a CI round trip. Worth knowing: the guard covers less than its name suggests. The test reads "every variable read under src/", but the extractor only sees literal process.env.NAME. In this repo that is the minority pattern. Measured on your branch: 56 names documented in .env.example, 40 matched by the extractor, 22 documented names invisible to it. They are invisible because the name usually lives in a constant or a table field: src/lib/agent/config.ts:54 export const AGENT_ENABLED_ENV = "LIBREDB_AGENT_ENABLED"; Both are string literals, so both are statically resolvable. The consequence is worth sitting with: had logger.ts read LOG_LEVEL through a constant, this guard would not have caught the drift it was written for. I am not asking you to extend the extractor in this PR. I am asking the doc comment to say what the guard does not cover, so the next person looking at a green check knows what that green means. Your dynamic-reads note is correct, it just reads like an edge case when it is most of the repo. Extending it later (resolve same-file const X = "NAME", then follow process.env[X]) would make a good follow-up. |
`bun run format` (biome format) is a required check and it wanted both multi-line call expressions collapsed onto one line. Formatting only; the five assertions are unchanged and still pass.
|
Separately, nothing has actually run on this PR yet. All four workflow runs are sitting at Please take current main before the next round. From a fork: Why now rather than at merge time: the chart version sync guard is the first step of |
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
|
Merging. The formatting fix cleared the blocker, and every required check ran for real this time rather than sitting at The scope note I asked for did not make it in, and I am not holding the PR for it — the guard is a clear improvement as it stands, and you turned the blocker around quickly. But the gap is bigger than "an edge case", and I measured it again on today's So the follow-up is worth more than it looked when I first raised it. Filed as #609 with the measurements and the two shapes to add, and it is yours first if you want it. |
Closes #566.
The failing test first, as the repo rules ask. Verified in both directions on this branch:
.env.example. A─── Logging ───block in the file's existing style, above Seed Connections. It names the four accepted values, says an unrecognised value is ignored rather than rejected (which is whatlogger.tsdoes, and not what a reader would assume), and gives bothNODE_ENV-dependent defaults —debugoutside production,infoin production. That last part is the bit an operator cannot guess, and it is why the example line is left commented: unset is the right setting for most deployments, and the reason to set it is getting debug output out of a production container without rebuilding. Cross-referenced todocs/HELM_CHART.mdsince the chart writes it fromconfig.logLevel.The guard.
tests/unit/env-documentation.test.ts, modelled onagent-documentation.test.ts. It extracts every literalprocess.env.NAMEundersrc/— including the optional-chainedprocess.env?.NAMEformlogger.tsactually uses, which a naive pattern would miss — and requires each to be documented or allowlisted.The five allowlisted names are the ones the issue identified as platform or build-time (
NODE_ENV,NEXT_RUNTIME,PORT,NEXT_PUBLIC_APP_VERSION,NEXT_PUBLIC_MANAGED_POLL_MS), each carrying its reason as a string rather than a comment.Three assertions exist to stop this guard rotting the way the documentation did:
LOG_LEVELandJWT_SECRET. Without it, a broken pattern would make the main check pass on an empty list, which is the failure mode a guard like this actually has;Documentation is matched per line (
NAME=, with or without a leading#) rather than by substring, so a variable merely mentioned in prose does not count.Dynamic
process.env[name]reads are out of scope, per the issue — they cannot be resolved statically, and reporting the expression as a name would be worse than not reporting it.Docs plus one test file; no product code.