chore: Add a CI check keeping environment-variables.md in sync with src
- Dominant language
- Rust
- Stars
- 467
- Forks
- 54
- Avg merge
- 4h 25m
- Merged PRs (30d)
- 310
Description
## Problem / Background
`docs/environment-variables.md` is the reference page for the `MLXCEL_*` surface, and nothing enforces that it stays in sync with the variables the code actually reads. The page is maintained by hand, so every new `std::env::var("MLXCEL_...")` call site is a silent opportunity for drift, and the drift is only ever discovered when someone happens to look.
Measured on `main` at `d5abf1fa`:
- `MLXCEL_*` names appearing as string literals in `src/**/*.rs`: **155**
- `MLXCEL_*` names listed in `docs/environment-variables.md`: **147**
- Read in `src` but absent from the doc page: **25**
Reproduce with:
```bash
grep -rhoE '"MLXCEL_[A-Z0-9_]+"' src --include='*.rs' | tr -d '"' | sort -u > /tmp/src_vars.txt
grep -ohE '`MLXCEL_[A-Z0-9_]+`' docs/environment-variables.md | tr -d '`' | sort -u > /tmp/doc_vars.txt
comm -23 /tmp/src_vars.txt /tmp/doc_vars.txt
```
The 25 undocumented names, with a file that reads each:
| Variable | Read from |
|---|---|
| `MLXCEL_ACTIVATION_MULT` | `src/execution/memory_estimate.rs` |
| `MLXCEL_AUDIO_PROBE_DIR` | `src/audio/encoder.rs` |
| `MLXCEL_DECODE_ALLOWLIST` | `src/distributed/disaggregated/coordinator.rs` |
| `MLXCEL_DIFFUSION_DEBUG_CANVAS` | `src/models/diffusion_gemma/generate.rs` |
| `MLXCEL_DSA_DENSE` | `src/models/deepseek_v32_indexer.rs` |
| `MLXCEL_DUMP_CONFIG` | `src/lib/mlxcel-xla/src/emitter/mod.rs` |
| `MLXCEL_DUMP_DIR` | `src/lib/mlxcel-xla/src/emitter/mod.rs` |
| `MLXCEL_DUMP_OUT` | `src/lib/mlxcel-xla/src/emitter/mod.rs` |
| `MLXCEL_ENABLE_MTP_BATCH_RAGGED` | `src/server/batch/speculative_burst.rs` |
| `MLXCEL_FREEZE_GOLDENS` | `src/lib/mlxcel-xla/src/validation.rs` |
| `MLXCEL_FREEZE_MOE` | `src/lib/mlxcel-xla/src/validation.rs` |
| `MLXCEL_MINIMAX_M3_DENSE` | `src/models/minimax_m3_indexer.rs` |
| `MLXCEL_MLX_COMMIT` | `src/lib/mlxcel-core/src/autotune/store.rs` |
| `MLXCEL_MOE_N` | `src/lib/mlxcel-xla/src/emitter/mod.rs` |
| `MLXCEL_MTP_DISABLE_DIVERGENT_FIX` | `src/models/gemma4.rs` |
| `MLXCEL_PHI4MM_AUDIO_PRECISION` | `src/lib/mlxcel-xla/src/phi4_audio.rs` |
| `MLXCEL_ROUTER_STATS_VERBOSE` | `src/server/router_front.rs` |
| `MLXCEL_SPEC_DIST_SAMPLES` | `src/lib/mlxcel-core/src/speculative/distribution_tests.rs` |
| `MLXCEL_TEST_GUMBEL_SAMPLES` | `src/lib/mlxcel-core/src/sampling_gumbel_tests.rs` |
| `MLXCEL_TEST_REJECTION_SAMPLES` | `src/lib/mlxcel-core/src/sampling_rejection_tests.rs` |
| `MLXCEL_XLA_GEMMA3N_TRACE_LAYER_START` | `src/models/gemma3n.rs` |
| `MLXCEL_XLA_IREE_DIST` | `src/lib/mlxcel-xla/src/numeric_probe.rs` |
| `MLXCEL_XLA_IREE_RUN_MODULE` | `src/lib/mlxcel-xla/src/emitter/gemma3n_qmv.rs` |
| `MLXCEL_XLA_IREE_TEST_TARGET` | `src/lib/mlxcel-xla/src/emitter/gemma3n_emit.rs` |
| `MLXCEL_XLA_REFERENCE_EXPECT_UNEXPANDED_IDS` | `src/server/batch/xla_worker_admission.rs` |
**Not all 25 are documentation gaps.** Several are test-only or fixture-authoring knobs that operators should never set: `MLXCEL_TEST_GUMBEL_SAMPLES`, `MLXCEL_TEST_REJECTION_SAMPLES`, `MLXCEL_SPEC_DIST_SAMPLES`, `MLXCEL_FREEZE_GOLDENS`, `MLXCEL_FREEZE_MOE`. A raw set difference is therefore not a usable gate: it would demand that fixture-freeze switches be published as operator documentation.
Two further caveats bound what any checker can claim:
1. A path-based test exclusion is not sufficient on its own. `MLXCEL_FREEZE_GOLDENS` and `MLXCEL_FREEZE_MOE` are read from `#[cfg(test)]` code inside `src/lib/mlxcel-xla/src/validation.rs`, a file whose name carries no `test` marker. Only 3 of the 25 live in a `*_tests.rs` file.
2. The grep is a floor on the drift, not an exact count. It sees literal names only, so any variable assembled by concatenation or interpolation at the call site would be missed. (No such call site exists today, which is why the naive count happens to be complete right now, but the checker should not assume that stays true.)
Origin: issue #1104 proposed this as a follow-up, in its own words "a CI check that diffs `MLXCEL_*` occurrences in `src/**/*.rs` against the table in `docs/environment-variables.md`, modeled on `scripts/ci/check_kernel_dtype_keys.py`. That is a larger change than this issue." PR #1119 then fixed six specific omissions by hand. This issue is about stopping the drift from recurring rather than fixing one round of it.
## Proposed Solution
Add a checker that fails when a `MLXCEL_*` variable is read from non-test `src` code and is absent from `docs/environment-variables.md`, with an explicit allowlist for the variables that are deliberately undocumented.
Follow the three-part shape already established by `scripts/ci/check_kernel_dtype_keys.py` (131 lines, issues #1053 and #1054):
1. **Script**: `scripts/ci/check_env_var_docs.py`, standard library only, no toolchain required, exits non-zero and names every offending variable.
2. **Makefile target**: a `verify-env-var-docs` target next to `verify-kernel-dtype-keys` (`Makefile:582-585`), added to the aggregate `verify` target.
3. **CI job**: a job in `.github/workflows/ci.yml` modeled on the "kernel dtype keys" job. That job is deliberately **not** behind the `changes` path filter, and this one must not be either: the check has to fire when only `docs/environment-variables.md` changes (a row deletion is as much a break as a new call site), and when only `src` changes. A `rust`-filtered job would miss the docs-only half.
Design notes:
- **Allowlist over silent exclusion.** `check_kernel_dtype_keys.py` documents why it has no allowlist; this check needs the opposite choice, and for a reason worth writing down in the same style. Test-only and fixture-authoring variables are a real category here, and an in-script allowlist with a one-line justification per entry makes the allowlist itself the durable record of the "this is internal, not operator-facing" decision. A blanket path or `#[cfg(test)]` exclusion would hide those same variables without recording that anyone judged them.
- **Failure message must be actionable.** Naming the variable alone is not enough to act on: the reader needs to know where it is read so they can judge whether it is operator-facing. Print the variable and at least one reading file path, plus the two ways to resolve (document it, or allowlist it with a reason).
- **Direction of the check.** The `src` to docs direction is the one that matters for drift. A docs-to-`src` direction (a documented variable no longer read anywhere) is a reasonable second phase and would have caught stale rows, but it needs care around variables read from `build.rs`, benches, tests, or scripts rather than `src`, and around MLX-native names such as `MLX_CUDA_GRAPH_CACHE_SIZE` that the page documents but mlxcel never reads. Treat it as optional here.
## Acceptance Criteria
- [ ] `scripts/ci/check_env_var_docs.py` exists, uses only the Python standard library, and needs no Rust toolchain or build.
- [ ] The check extracts `MLXCEL_*` names read from `src/**/*.rs` and compares them against the names documented in `docs/environment-variables.md`.
- [ ] An explicit in-script allowlist covers deliberately undocumented variables, each entry carrying a short reason (test-only, fixture-authoring, internal diagnostic).
- [ ] The failure message names the specific variable and at least one file that reads it, and states both resolutions (add a row to the page, or add an allowlist entry with a reason).
- [ ] The check passes on `main` after each of the 25 currently missing variables is either documented on the page or allowlisted with a reason. Operator-facing ones get documented; test-only and fixture-authoring ones get allowlisted.
- [ ] The check fails on a synthetic regression: adding a new `std::env::var("MLXCEL_NEW_THING")` read in non-test `src` code without a doc row is rejected, and deleting a documented row for a variable still read is rejected.
- [ ] `make verify-env-var-docs` runs the check, and the aggregate `verify` target includes it.
- [ ] A CI job in `.github/workflows/ci.yml` runs the check unconditionally, not behind the `rust` path filter, so a docs-only change is still gated.
- [ ] The script's module docstring explains the rationale and the allowlist policy, in the style of `check_kernel_dtype_keys.py`.
## Technical Considerations
- Extraction from Rust should be robust enough not to be defeated by ordinary formatting, but it does not need to parse Rust. Matching `MLXCEL_*` string literals is sufficient for the current call sites; anything it cannot see (a name built at runtime) should be treated as out of scope and stated as such in the docstring rather than silently assumed impossible.
- Doc-side extraction should accept a variable documented anywhere on the page, not only inside a table cell. The page mixes tables with prose sections, and a table-only parser would generate false failures as the page evolves.
- Detecting "non-test" code needs to cover both `*_tests.rs` files and `#[cfg(test)]` blocks inside ordinary modules. Given the allowlist exists, the simplest defensible rule is to scan everything and let the allowlist carry the exceptions, which also keeps the record of what was judged internal.
- Reference points: `scripts/ci/check_kernel_dtype_keys.py`, `Makefile:582-585` (`verify-kernel-dtype-keys`), and the "kernel dtype keys" job at `.github/workflows/ci.yml:139-157`, including its comment explaining why it sits outside the `changes` filter.
Contributor guide
Research direction
Start with scripts/ci/check_kernel_dtype_keys.py, then inspect Makefile:582-585 and the kernel dtype keys job in .github/workflows/ci.yml:139-157. Reproduce the documented grep comparison and review docs/environment-variables.md plus the listed Rust read sites before defining the checker’s scope. Done means the standard-library script, Makefile target, aggregate verify entry, and unconditional CI job satisfy the acceptance criteria and catch both synthetic regressions.
Written by the indexing model from the issue text.
Assessment
- Tech stack
- github-actions, python, rust
- Domain
- ci-cd, documentation, tooling
- Issue type
- Feature
- Difficulty
- 4/5
- Estimated time
- 3-5 days
- Activity status
- Quiet
- Clarity
- Clearly specified
- Newbie friendliness
- 68/100