relay(stats): fix TOML stats config silently clobbered by clap update_from

[kixelated]

Summary

Config::load did: parse CLI → deserialize TOML → update_from(cli_args). That re-runs the clap parser onto the TOML-derived config. For a bare bool field with no --flag on the CLI, clap writes Default::default() (false) over the TOML value, silently disabling any setting that was configured only via TOML. Option<T> fields survive because update_from leaves them untouched when the arg is absent.

This was hitting StatsConfig.enabled in particular: a relay launched with localhost.toml (which has [stats] enabled = true) booted with stats publishing disabled, no warning, no log line. The dashboard's stats subscriber would just sit there forever waiting for announces that never came.

What's in this PR

1. The fix (rs/moq-relay/src/stats.rs): pub enabled: bool → pub enabled: Option<bool>, unwrap_or(false) at the call site. Mirrors how IrohEndpointConfig.enabled already handles the same merge.

2. Testable Config::load (rs/moq-relay/src/config.rs): split into the public entry point and a pure parse_and_merge(args) so tests can drive it with synthetic args (no std::env::args, no log.init()).

3. Two regression tests in rs/moq-relay/src/config.rs::tests:

   • cli_does_not_clobber_toml_stats_enabled — TOML enables stats, no overriding CLI flag, expects Some(true). This is the test that would have caught the original bug. As a bonus, reverting the field to bare bool makes the assert_eq!(..., Some(true)) line stop compiling, so type-level regressions get caught at build time.
   • cli_flag_overrides_toml_stats_enabled — belt-and-suspenders that explicit --stats-enabled=false still wins.

4. CLAUDE.md note — a new "Config flags + TOML merge (clap pitfall)" bullet under Rust Conventions so future config knobs reach for Option<T> by default and add a parallel test.

5. Demo configs — enable [stats] on leaf0.toml / leaf1.toml, switch localhost.toml from --stats-node "localhost" to "local/host", so the demo exercises the multi-segment node path that real cluster deployments use. Also picks up the configVersion = 0 lockfile bump from newer bun.

Verification

• cargo test -p moq-relay --lib → 77 passed, 0 failed (2 new tests included).
• End-to-end: cargo run --bin moq-relay -- localhost.toml (no flags, no env) now logs stats publishing enabled prefix=".stats" levels=3 node=Some(Path("local/host")) on startup. Before the fix, this log line never appeared.

Test plan

• CI green (cargo test, cargo fmt, cargo clippy).
• Manual: just demo and confirm stats announces appear on the dashboard's broadcasts page when scoped to anon.

🤖 Generated with Claude Code

[coderabbitai]

Warning

Review limit reached

@kixelated, we couldn't start this review because you've used your available PR reviews for now.

Your plan includes 4 reviews of capacity. Refill in 7 minutes and 13 seconds.

Your organization has run out of usage credits. Purchase more in the billing tab.

⌛ How to resolve this issue?

After more review capacity refills, a review can be triggered using the @coderabbitai review command as a PR comment. Alternatively, push new commits to this PR.

We recommend that you space out your commits to avoid hitting the rate limit.

🚦 How do rate limits work?

CodeRabbit enforces hourly rate limits for each developer per organization.

Our paid plans have higher rate limits than trial, open-source, and free plans. In all cases, review capacity refills continuously over time.

Please see our FAQ for further information.

ℹ️ Review info

⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: f23732f1-25d9-4c45-9791-e54c74a97012

📥 Commits

Reviewing files that changed from the base of the PR and between 33399d6 and 411c36c.

⛔ Files ignored due to path filters (1)

• bun.lock is excluded by !**/*.lock

📒 Files selected for processing (6)

• CLAUDE.md
• demo/relay/leaf0.toml
• demo/relay/leaf1.toml
• demo/relay/localhost.toml
• rs/moq-relay/src/config.rs
• rs/moq-relay/src/stats.rs

Walkthrough

This PR fixes a clap/TOML config interaction bug where CLI argument re-parsing clobbered TOML boolean values. The solution extracts config loading into a parse_and_merge helper that applies CLI args twice—once initially, then again after TOML merge—ensuring explicit CLI flags override TOML defaults. StatsConfig.enabled is changed from bare bool to Option<bool> to prevent clobbering. Regression tests validate that TOML values persist when CLI flags are absent and that explicit CLI flags override TOML. Demo configurations enable stats output, and developer guidance documents the pattern for future boolean config fields.

🚥 Pre-merge checks | ✅ 5

✅ Passed checks (5 passed)

Check name	Status	Explanation
Title check	✅ Passed	The title 'relay(stats): fix TOML stats config silently clobbered by clap update_from' accurately describes the main issue and fix in the changeset.
Description check	✅ Passed	The pull request description provides a comprehensive explanation of the root cause, the fix, testing approach, and verification, all directly related to the changeset.
Docstring Coverage	✅ Passed	Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%.
Linked Issues check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check	✅ Passed	Check skipped because no linked issues were found for this pull request.

_{✏️ Tip: You can configure your own custom pre-merge checks in the settings.}

✨ Finishing Touches

✨ Simplify code

• Create PR with simplified code
• Commit simplified code in branch fix-clap-toml-stats-enabled

---

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

[coderabbitai]

Actionable comments posted: 2

🤖 Prompt for all review comments with AI agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@rs/moq-relay/src/config.rs`:
- Around line 113-119: The tests cli_does_not_clobber_toml_stats_enabled and
cli_flag_overrides_toml_stats_enabled mutate the process-global
MOQ_STATS_ENABLED via unsafe { std::env::remove_var("MOQ_STATS_ENABLED") } while
clap reads that same env, which can race when tests run concurrently; serialize
the mutation by adding a shared test-level lock (e.g., a static Mutex/OnceGuard)
or use a test-serialization helper (serial_test attribute or temp-env crate) so
only one test touches MOQ_STATS_ENABLED at a time, update the tests to acquire
that lock before calling std::env::remove_var("MOQ_STATS_ENABLED") and release
after, and remove the incorrect comment about single-threaded guarantees.
- Around line 69-74: The docstring for the pure loader incorrectly mentions a
`--file` flag; update the merge-order wording to reference the positional TOML
path (Config::file / the `file` positional argument) instead of `--file` so it
reads: "CLI args (`config.file` and any flags) → TOML file (if `file` is set) →
CLI args re-applied..." — edit the comment near the pure `Self::load` (the
function that explains merge order) to replace `--file` with
`file`/`Config::file`.

🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

• Push a commit to this branch (recommended)
• Create a new PR with the fixes

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: 3cba33f5-fdd2-4819-856e-3b4fa4e5c199

📥 Commits

Reviewing files that changed from the base of the PR and between 2915730 and 33399d6.

⛔ Files ignored due to path filters (1)

• bun.lock is excluded by !**/*.lock

📒 Files selected for processing (6)

• CLAUDE.md
• demo/relay/leaf0.toml
• demo/relay/leaf1.toml
• demo/relay/localhost.toml
• rs/moq-relay/src/config.rs
• rs/moq-relay/src/stats.rs