Environment variable overrides

[The64thRealm]

Added environment variable overrides:

• WAVEDASH_GAME_ID
• WAVEDASH_UPLOAD_DIR
• WAVEDASH_ENTRYPOINT
• WAVEDASH_GODOT_VERSION
• WAVEDASH_UNITY_VERSION

tool will announce when it's using an override when a command is run

The two builds are uploaded to different games:

This pr is required for the planned godot plugin that integrates cli features.

[cursor]

Bugbot needs on-demand usage enabled

Bugbot uses usage-based billing for this team and requires on-demand usage to be enabled.

A team admin can enable on-demand usage in the Cursor dashboard.

[claude]

Claude finished @The64thRealm's task in 3m 57s —— View job

---

Code Review — Environment variable overrides

• Gather context (read diff + changed files)
• Review src/config.rs override logic
• Review call sites (builds.rs, dev/mod.rs, main.rs, publish.rs, file_staging.rs)
• Static verification of call sites & output paths
• doppler run -- cargo check / clippy / test — blocked, see note at bottom
• Post review

---

The design here is genuinely nice: privatizing the WavedashConfig fields so every read funnels through an accessor is what makes the "announce on first read" behavior self-maintaining, and reject_engine_conflict catching the silent engine-switch is exactly the right instinct. Injecting lookup into apply_env_overrides keeps the tests hermetic. Three things I'd want addressed before merge.

1. Blank WAVEDASH_GAME_ID hard-fails stat/achievement — the opposite of the documented contract

src/config.rs:150-153 states the contract plainly:

A set-but-blank value counts as unset — an unpopulated CI variable shouldn't wipe out a value the config file supplies.

The config path honors that via non_blank (src/config.rs:161). The clap path does not. src/main.rs:205-206 (and the four other identical blocks at 226, 249, 274, 314, 353) pairs env = config::ENV_GAME_ID with value_parser = parse_non_empty_arg, and parse_non_empty_arg (src/main.rs:41-48) errors on blank rather than yielding None. For an Option<String> arg, clap 4 feeds an empty env var straight into the value parser — it only special-cases blank for SetTrue flags.

Failure scenario — the exact CI shape this PR exists for:

env:
  WAVEDASH_GAME_ID: ${{ vars.WAVEDASH_GAME_ID }}   # var not defined → ""

• wavedash build push → blank ignored, falls back to wavedash.toml. ✅
• wavedash stat create --identifier KILLS --name Kills → error: invalid value '' for '--game-id <GAME_ID>': value cannot be empty, exit 2. ❌

Same env, same repo, two opposite behaviors.

There's a second reason to drop clap's env here. Because clap fills the flag before any config is read, the notice can't come from the accessors, so resolve_game_id has to infer it by string-comparing the resolved value against the env var (src/config.rs:302). That heuristic can't distinguish --game-id foo from WAVEDASH_GAME_ID=foo — the code comment acknowledges the false positive, but it also means an explicit --game-id that beats a differing env var is reported as nothing at all, when "CLI flag won over the env var" is the case a CI user most wants to see.

Removing env/value_parser from those six #[arg] blocks and resolving inside resolve_game_id fixes both at once, and puts trim/blank semantics in exactly one place:

pub fn resolve_game_id(cli_game_id: Option<&str>, config_path: &PathBuf) -> Result<String> {
    if let Some(id) = cli_game_id {
        return Ok(id.to_string());          // explicit flag always wins, silently
    }
    if let Some(id) = env_override(ENV_GAME_ID) {
        print_override_notice(&game_id_notice(&id));
        return Ok(id);
    }
    // ...existing config fallback
}

Fix this →

2. WAVEDASH_ENTRYPOINT is accepted, recorded, and then silently discarded on engine projects

entrypoint() (src/config.rs:503-511) returns None whenever any engine section is present, and the announce(Field::Entrypoint) call sits inside the Ok(None) arm. So with a [godot] config:

WAVEDASH_ENTRYPOINT=start.html wavedash build push

the override applies to the struct, pushes a Field::Entrypoint notice that can never fire, and changes nothing about the build. No error, no warning, no output. The user has no way to tell it was ignored.

The compound case is worse. Given an engine-less config with entrypoint = "game.html", setting WAVEDASH_GODOT_VERSION adds a [godot] section — which flips engine_type() to Some(Godot) and makes entrypoint() start returning None. The notice does say "config declared no engine, so [godot] is now in play", but it doesn't mention that the config's entrypoint just stopped being used.

Yes, the toml entrypoint field is ignored under an engine section too, so the semantics are consistent. The difference is visibility: with the toml both lines sit in one file the user is looking at. With an env var set in CI, silence is indistinguishable from success — and this PR's whole premise is that overrides announce themselves. Either reject the combination in apply_env_overrides (consistent with how you already reject engine conflicts) or warn at load that the entrypoint override is inert.

Fix this →

3. file_values_survive_when_nothing_is_set tests the mock, not the code

src/config.rs:654-668 passes (ENV_GAME_ID, " ") and asserts no override applied — but the env() helper at src/config.rs:546-553 already runs every pair through non_blank before building its map. The lookup returns None before apply_env_overrides sees anything, so the test would pass even if blank-handling were removed entirely.

The real blank filtering lives in env_override (src/config.rs:166), which reads the process environment and is never exercised. blank_overrides_are_treated_as_unset covers non_blank in isolation, so the primitive is tested — but nothing tests that the wiring uses it. Simplest fix: have the env() helper stop filtering and pass raw values through, so blank-handling is actually under test at the layer that matters.

Fix this →

Smaller notes

• executable_entrypoint_params() doesn't announce. It reads the jsdos/ruffle/renpy sections without announce(Field::Engine). Harmless today since no env var targets those engines, but it's a gap in the "every read goes through an accessor that announces" invariant the Field doc comment establishes — worth a comment saying why it's exempt, so the next person adding WAVEDASH_RENPY_VERSION doesn't miss it.
• Notice timing on build push. First read of upload_dir is at src/builds.rs:150, and game_id at :173 — both before the uploader starts, so notices land cleanly ahead of progress output. Confirmed no --json command shares a code path with resolve_game_id (the only json flags are on team list / project list), so machine-readable output can't be corrupted by a notice. Good as-is; flagging that I checked because println! into a JSON path would be the easy regression here.
• Ordering::Relaxed on the announce bitmask is correct — fetch_or is atomic and you only need the once-only guarantee, not ordering against other data.
• Uneven engine coverage (godot/unity only) matches the PR description, and reject_engine_conflict handles jsdos/ruffle/renpy configs sensibly, so no concern — just noting it's a deliberate subset.

Verification note

I could not run doppler run -- cargo check, clippy, or test — the Bash commands were denied by this job's tool permissions. Everything above is from static reading. Two consequences worth stating plainly:

• The clap-empty-env mechanism in finding Chunk uploads to <500mb chunks so we can cache on r2-asset-proxy #1 is reasoned from clap 4.5's known handling of env on value-taking args, not observed. Please confirm before acting on it: WAVEDASH_GAME_ID= cargo run -- stat create --identifier X --name Y should reproduce the error.
• Findings iframe'd #2 and Add dev sandbox server and certificate management #3 are verifiable by reading alone and don't depend on a build.

To let me run the suite on future invocations, add Bash(doppler run -- cargo test), Bash(doppler run -- cargo clippy --all-targets) to --allowedTools.
• environment-var-overrides

[franzwarning]

looks great!

[claude]

Claude finished @The64thRealm's task in 5m 26s —— View job

---

Code Review — Environment variable overrides

• Gather context (read diff + changed files)
• Review src/config.rs override + precedence logic
• Review src/auth.rs unified token env var
• Review call sites (builds.rs, dev/mod.rs, main.rs, publish.rs, file_staging.rs)
• Check whether CI verifies the build
• doppler run -- cargo check / clippy / test — blocked, see note at bottom
• Post review

---

The three findings from the last round are all properly fixed, and two of the fixes went further than what was asked. Removing clap's env and resolving in resolve_game_id (src/config.rs:405-427) puts trim/blank semantics in one place and makes build push and stat agree; the "why not clap env" rationale now lives at both src/main.rs:41-45 and src/config.rs:398-404, which is the right place for it. The entrypoint fix covers the compound case I only described in prose (entrypoint_override_is_rejected_when_an_override_adds_the_engine), and shadowed_entrypoint_notice handles the file-entrypoint variant that can't be rejected — that wasn't asked for and it closes the last silent hole. The env() helper now passes raw values through (src/config.rs:794-800), so file_values_survive_when_overrides_are_blank exercises the real filtering.

Four things on the new commit.

1. auth login --token "" stores a key the CLI then reports as unauthenticated

--token (src/main.rs:144-145) is the only string arg in the file without value_parser = parse_non_empty_arg — build_id has it (:98), all six --game-id args have it. And store_credentials (src/auth.rs:85) doesn't trim.

$ wavedash auth login --token "  "
✓ Successfully stored API key
$ wavedash auth status
Not authenticated. Run 'wavedash auth login' or set WAVEDASH_TOKEN environment variable.

The success line is printed at src/main.rs:456-457; resolve_auth then discards the key at src/auth.rs:151 under the new "a blank key on disk is a corrupt or half-written credentials file" rule. Before this PR that key came back as AuthSource::File and failed at the server with a 401 — noisy, but not self-contradictory. The new rule is the right rule; it just needs the write path to agree with it. --token-stdin already does (src/main.rs:390-393 rejects blank), so today the two ways of supplying the same value disagree.

value_parser = parse_non_empty_arg on the --token arg fixes it and also trims, matching what read_token_from_stdin and resolve_auth already do.

Fix this →

2. Build-only validation runs at load, so it aborts commands that never build

apply_env_overrides bails in two places: both engine version vars set (src/config.rs:575-579) and WAVEDASH_ENTRYPOINT under an engine (src/config.rs:613-621). Both are raised from load, and load is reached by commands that read nothing but game_id — publish (src/publish.rs:110) and stat/achievement through resolve_game_id's config fallback (src/config.rs:416).

Config has [godot]; WAVEDASH_ENTRYPOINT is exported in the shell from an earlier custom-HTML project:

$ wavedash stat create --identifier KILLS --name Kills
Error: No --game-id or WAVEDASH_GAME_ID provided, so game_id has to come from the config:
WAVEDASH_ENTRYPOINT is set to start.html, but this build targets GODOT — engine builds
boot through wavedash's own entrypoint, so the value would be ignored.

There is no build. And because resolve_game_id returns before load when the flag is set, the same environment gives opposite results depending on an unrelated flag: stat create --game-id X succeeds, stat create fails. Rejecting rather than warning is a defensible call — my previous note offered both — but the rejection currently has a wider blast radius than the concern it protects, and the message doesn't fit the command it aborts.

Gating it on the read rather than the load would keep the guarantee where it matters (build push, dev) without leaking into publish/stat/achievement. entrypoint_with_source is already the single place both build paths funnel through.

Fix this →

3. resolve_game_id is the logic that changed, and it's the one thing untested

apply_env_overrides takes an injected lookup (src/config.rs:549-553) specifically so its precedence can be tested hermetically, and there are 20 tests behind that seam. resolve_game_id reads std::env directly through env_override (src/config.rs:412), so the flag > env > file ordering — new in this commit, and the fix for the previous round's finding #1 — has no coverage at all. Same for load's file-optional rule: any_env_override (src/config.rs:231) reads the process environment, so a_config_file_is_unnecessary_when_overrides_supply_what_is_read tests the accessors given a default config, not the decision to build one.

Threading the same impl Fn(&str) -> Option<String> through (a resolve_game_id_with(lookup, ..) inner fn, with the public wrapper passing raw_env) reuses the seam that's already there and gets both under test.

Fix this →

4. Nothing in CI compiles or runs these tests

Worth flagging because the green ✅ on this PR is misleading. release.yml is cargo-dist, and on a pull request line 80 resolves to dist plan only — it computes the release manifest and never invokes the compiler. The other three workflows are auto-tag (greps Cargo.toml), claude.yml, and claude-code-review.yml. There is no cargo build, cargo test, or cargo clippy anywhere in .github/workflows/.

So this PR's ~20 new unit tests — which are the best part of it — are run by nobody, and neither was the compile. A minimal cargo test + cargo clippy --all-targets job would pay for itself immediately here. I can't add it myself (GitHub App permissions don't allow workflow changes), but it's a small file.

Smaller notes

• Engine sections skip the blank-is-unset rule. treat_blank_file_values_as_unset (src/config.rs:479-490) covers game_id, upload_dir, and entrypoint, but not [godot] version = "" or [jsdos] executable = "". engine_version() then hands Some("") to the API, and WAVEDASH_GODOT_VERSION="" — correctly read as unset — falls back to that blank rather than erroring. Given the PR's premise is one uniform rule for blanks, the engine sections are the remaining exception.
• Non-UTF-8 override values are silently dropped. raw_env uses std::env::var (src/config.rs:211), which returns Err for non-UTF-8, so a WAVEDASH_UPLOAD_DIR holding odd bytes reads as unset and quietly falls back to the file. var_os would be needed to distinguish. Unix-only and vanishingly rare — noting it because "unset" and "unrepresentable" collapsing together is exactly the ambiguity the rest of this module works to avoid.
• executable_section()'s exemption is now documented (src/config.rs:700-705), including the instruction for whoever adds WAVEDASH_RENPY_VERSION. That's what I was asking for.
• Notice ordering re-verified on the new commit. build push reads upload_dir (src/builds.rs:150) → entrypoint via FileStaging (:161) → game_id (:173) → engine_version (:175), all before upload_directory_from_scan (:195); dev likewise finishes its reads before the listener binds. And only init.rs prints JSON (:491, :515), which never loads a WavedashConfig — so print_override_notice's println! still can't corrupt machine-readable output.
• dev dropping its own config_path.exists() check (src/dev/mod.rs:70-74) is right: load's message names both --config and wavedash init, so the removed check was strictly worse.

Verification note

doppler run -- cargo check, clippy, and test were all denied by this job's tool permissions again, so this is static reading. The compile is genuinely unverified — see finding #4; the passing "Release" check does not build the crate. Findings #1–#4 are all verifiable by reading, and #4 by reading .github/workflows/release.yml:80.

To let me actually run the suite next time, add Bash(doppler run -- cargo test) and Bash(doppler run -- cargo clippy --all-targets) to --allowedTools in claude-code-review.yml.
• environment-var-overrides

[claude]

Claude finished @The64thRealm's task in 5m 3s —— View job

---

Code Review — Environment variable overrides

• Gather context (diff since last review round)
• Review src/config.rs read-time resolution + validation
• Review src/auth.rs unified token env var
• Review call sites (builds.rs, dev/mod.rs, publish.rs, file_staging.rs, main.rs)
• Review new .github/workflows/ci.yml
• Verify via CI (couldn't run cargo locally — see note)
• Post review

---

The 4406050 refactor is the right call and it's cleanly done. Moving from "merge at load" to "resolve on the read" fixes the previous round's finding #2 at its root rather than special-casing it: EnvOverrides is captured once, every accessor falls through env → file → default, and the two build-only refusals now live in active_engine/entrypoint_with_source, which only build push and dev reach. a_build_only_refusal_does_not_stop_a_game_id_from_resolving pins exactly the bug that was reported, through the real entry point, in both the flag and no-flag spellings. Folding reject_engine_conflict into active_engine means there is now one place engine rules live, and EntrypointSource falling out of which fallback answered instead of being recorded at store time removed a whole class of skew. Adding ci.yml closes last round's #4 — and it works: I confirmed the run below.

Three things, one of them carried over unfixed.

1. auth login --token "" still stores a key the CLI then reports as unauthenticated

Carried over from the last round unaddressed. --token (src/main.rs:144-145) remains the only string arg in the file without value_parser = parse_non_empty_arg — build_id has it (:98), and this commit added it to all six --game-id args. store_credentials (src/auth.rs:85) doesn't trim either.

$ wavedash auth login --token "  "
✓ Successfully stored API key
$ wavedash auth status
Not authenticated. Run 'wavedash auth login' or set WAVEDASH_TOKEN environment variable.

The success line is src/main.rs:456-457; resolve_auth then discards the key at src/auth.rs:151 under this PR's own new rule ("a blank key on disk is a corrupt or half-written credentials file"). That rule is correct — a_blank_stored_key_is_unauthenticated tests it. The write path just doesn't agree with it, so the two ways of supplying the same value now disagree: --token-stdin rejects blank at src/main.rs:391-393, --token accepts it. Before this PR the blank key came back as AuthSource::File and died at the server with a 401 — noisy, but not self-contradictory.

One line, and it also trims, matching read_token_from_stdin and resolve_auth.

Fix this →

2. Engine sections are the one place left where blank isn't unset

treat_blank_file_values_as_unset (src/config.rs:556-567) covers game_id, entrypoint, and upload_dir, with a good comment on why each blank is worse than merely empty. It doesn't touch [godot].version, [unity].version, or [jsdos]/[ruffle]/[renpy]'s version/executable.

So with [godot]\nversion = "":

• engine_version() returns Some("") and builds.rs:175 hands that to the API as the build's engine version.
• WAVEDASH_GODOT_VERSION="" — read as unset, correctly, by EnvOverrides::capture — falls through to that blank rather than to an error. The unpopulated-CI-variable case this whole rule exists for lands on a blank version instead of a message.
• In dev, resolve_engine_entry (src/dev/mod.rs:214) resolves an engine entry for version "" and fails against play with whatever that URL 404s as.

I raised this as a smaller note last round; re-raising it as a finding because the refactor now gives it an obvious home. treat_blank_file_values_as_unset is called from exactly one place and already owns this decision for every other field — dropping a section whose version (or executable) is blank there would make declared_engine see no engine, and the existing "no engine declared" and missing_field paths take over from there. Right now the module's stated invariant has one exception, and it's the one an accessor can't catch.

Fix this →

3. ci.yml is ubuntu-only and unlocked, so it can't catch what release actually builds

The workflow itself is well-judged — the placeholder-env comment explains precisely why it's hermetic and fork-safe, and persist-credentials: false is the right default. Two gaps worth closing while the file is new:

• No --locked. Cargo.lock is committed and dist builds from it, but cargo test will happily update it in-memory. A future PR that bumps a dep in Cargo.toml without committing the lock change passes CI and then releases a different dependency set. cargo test --locked / clippy --all-targets --locked costs nothing here.
• One target. cargo-dist releases macOS and Windows binaries, and this crate has real #[cfg(unix)] branches (src/auth.rs:91-95, :106-110) plus TcpListener/canonicalize paths that behave differently there. A Windows-only compile break still can't surface until a tag is cut. Even just cargo check --target x86_64-pc-windows-msvc would close that.

Also missing: a permissions: block (contents: read is all this job needs) and cargo fmt --check. Both one-liners.

I can't add these myself — GitHub App permissions don't allow workflow modifications. You can find more on that in the FAQ.

Smaller notes

• engine_type() doesn't announce; engine_version() does. Every current caller pairs them (builds.rs:170+:175, dev/mod.rs:92+:129), so no notice is lost today. But the Field doc (src/config.rs:259-262) promises an override is announced on the first read of the field it applies to, and engine_type() is a read of that field that stays silent. executable_section() got an explicit "why this is exempt" comment (:770-775) for exactly this reason — engine_type() deserves the same, or should just call announce_engine.
• dev/mod.rs:214 is unreachable, and its message is now wrong. engine_version() returns Some whenever active_engine() is Some, and the caller only gets there with engine_kind = Some, so the ok_or_else can't fire. If finding iframe'd #2 makes it live again, "{} requires a version in wavedash.toml" names only the file when WAVEDASH_GODOT_VERSION/_UNITY_VERSION could have supplied it.
• Non-UTF-8 override values are still silently dropped. raw_env (src/config.rs:211-213) uses std::env::var, so odd bytes in WAVEDASH_UPLOAD_DIR read as unset and fall back to the file. Unix-only and vanishingly rare; noting it once more only because "unset" and "unrepresentable" collapsing is the one ambiguity this module otherwise works hard to avoid.
• Notice ordering re-verified. build push reads upload_dir (builds.rs:150) → entrypoint via FileStaging::prepare (:161) → engine_type (:170) → game_id/engine_version (:173/:175), all before upload_directory_from_scan (:196). dev finishes its reads before TcpListener::bind (:136). Only init.rs prints JSON and it never builds a WavedashConfig, so print_override_notice's println! still can't corrupt machine-readable output.
• dev running file-less holds up. Dropping the config_path.exists() check is safe: config_parent_dir (dev/mod.rs:298-306) never touches the filesystem for the config path, so a missing ./wavedash.toml with WAVEDASH_UPLOAD_DIR set resolves against the cwd as intended.
• resolve_game_id's error nesting reads a little doubled when the file is absent and nothing is overridden: "…game_id has to come from the config: No config file at …, or set the WAVEDASH_* overrides to run without one." Both halves are individually right; the outer wrapper just re-says what the inner already covered. Cosmetic.

Verification

doppler run -- cargo test / clippy were denied by this job's tool permissions again — but this round that matters much less, because this PR's own CI run passed: run 30583630084 executed cargo clippy --all-targets -- -D warnings and cargo test against 5ffddcd and came back green. So the compile and the ~40 unit tests are genuinely verified, by the workflow this PR adds. Findings #1–#3 are all from static reading and none depend on a local build; #1 is confirmed by src/main.rs:144-145 having no value_parser while :98 and the six --game-id args do.
• environment-var-overrides

[claude]

Claude finished @The64thRealm's task in 6m 3s —— View job

---

Code Review — Environment variable overrides

• Gather context (diff since last review round: efa48a9, e511284)
• Review efa48a9 — blank API key rejection
• Review e511284 — blank engine version / executable
• Re-check carried-over findings
• Verify build + tests (via this PR's own CI — see note)
• Post review

---

Both carried findings are closed, and one of them was fixed better than proposed.

efa48a9 didn't just add value_parser to --token; it put the refusal in store_credentials (src/auth.rs:86-90) so the write path enforces what resolve_auth enforces on the read, whichever way a key arrives. parse_non_empty_arg and read_token_from_stdin now both delegate to config::non_blank, so "blank counts as unset" has one implementation across clap args, stdin, WAVEDASH_*, and the credentials file. That's the version of the fix that can't drift.

e511284 took the harder of the two options I sketched for blank engine versions. Rather than dropping a blank section, it made the fields Option and unset them field-by-field — which preserves the two things dropping would have broken, and the tests say so out loud: a_version_override_supplies_what_a_blank_section_left_out (the override fills the gap without claiming it "brought [godot] into play") and a_blank_engine_section_still_refuses_an_engine_switch (a blank [godot] still blocks WAVEDASH_UNITY_VERSION). Extending the same treatment to executable and loader_url wasn't asked for; a_blank_loader_url_is_no_loader_url covers the case where upload_dir.join("") validates as the directory itself. And missing_engine_field naming the env var only where one exists (version_override_var() returning None for the executable engines) is the detail that keeps the message honest.

Three things left, none of them in the new commits.

1. auth logout reports success while WAVEDASH_TOKEN keeps the CLI authenticated

Same shape as the --token "" finding this commit just fixed — a success line the next command contradicts — one layer up.

resolve_auth gives the environment precedence over the file (src/auth.rs:147-153). Logout (src/main.rs:477-479) only clears the file:

$ export WAVEDASH_TOKEN=wd_live_...
$ wavedash auth logout
✓ Successfully logged out
$ wavedash auth status
✓ Authenticated (via WAVEDASH_TOKEN environment variable)

The user asked to be logged out, was told they were, and is not. The same precedence makes wavedash auth login --token X print ✓ Successfully stored API key (src/main.rs:456) for a key that WAVEDASH_TOKEN then shadows on every request — the stored key is real but inert. That one is at least discoverable, since auth status names the winning source; logout's message has no such out.

This is pre-existing — origin/main's get_auth_info had the same precedence. I'm raising it now because this PR is the one that made token handling rigorous: it promoted the variable to config::ENV_TOKEN alongside the overrides, extracted resolve_auth specifically so precedence would be assertable, and just spent a commit closing the write/read disagreement for blanks. Logout is the last place a message claims something the resolver disagrees with, and the fix is the same kind: name the variable and say the file credentials were removed but the environment still authenticates.

Fix this →

2. ci.yml still can't catch what dist actually builds

Carried from last round; the workflow is new enough that these are cheap to add now.

• No --locked. Cargo.lock is currently in sync — I checked, tempfile 3.27.0 is committed at Cargo.lock:2527. This is a regression guard, not a present bug: without --locked, a future PR that bumps Cargo.toml and forgets the lock passes CI and releases a different dependency set than it tested.
• One target. cargo-dist releases macOS and Windows binaries, and this crate has #[cfg(unix)] branches (src/auth.rs:96-100, :112-116) plus TcpListener/canonicalize paths. A Windows-only break still can't surface until a tag is cut; cargo check --target x86_64-pc-windows-msvc would close it.
• No cargo fmt --check. Concretely: src/config.rs:963-964 has a double blank line before #[cfg(test)] that rustfmt would collapse, so the tree isn't currently formatted and nothing notices.
• No permissions: block. contents: read is all this job needs.

I can't add these myself — GitHub App permissions don't allow workflow modifications (FAQ).

3. dev/mod.rs:214 is now provably dead, and its message is wrong

let version = wavedash_config.engine_version()?.ok_or_else(|| {
    anyhow::anyhow!("{} requires a version in wavedash.toml", kind.as_label())
})?;

engine_version() returns Ok(None) only when active_engine() is None (src/config.rs:862-868), and resolve_engine_entry is reached only from the match arm at src/dev/mod.rs:101 where engine_kind is Some. After e511284 the missing-version case is an Err from missing_engine_field, not a None — so this closure cannot run, and if it somehow did it would name only the file when WAVEDASH_GODOT_VERSION/_UNITY_VERSION could have supplied the version. Worth replacing with something that states the invariant instead of pretending to handle it, so the next reader doesn't take it as a live path.

Fix this →

Smaller notes

• The engine sections' fields are still pub, and now nothing outside config.rs reads them — I grepped; the only .version/.executable hits elsewhere are updater.rs on an unrelated type. Making them private would turn "someone reads .version directly and skips missing_engine_field" from a review catch into a compile error, which is the same reasoning that privatized WavedashConfig's own fields.
• engine_type() still doesn't announce. Carried; still harmless, because every caller pairs it with engine_version() (builds.rs:170+:175, dev/mod.rs:92+:129). But executable_section() got an explicit exemption comment (src/config.rs:846-853) and engine_type() is the remaining silent read of an announced field.
• --token and --token-stdin together: stdin wins silently (src/main.rs:446-450). Pre-existing, and now that both reject blank identically it matters less.
• main.rs has no test module, so parse_non_empty_arg is uncovered — but it's now a one-line delegate to non_blank, which is tested, so that's the right amount of nothing.
• Non-UTF-8 override values are still silently dropped (raw_env, src/config.rs:211-213, uses std::env::var). Unix-only, vanishingly rare; last mention.
• executable_entrypoint_params() reading file sections without consulting active_engine() is safe, and I checked rather than assumed: every caller reaches an active_engine()-backed read first — FileStaging::prepare calls entrypoint_with_source()? before executable_files_to_validate()? (file_staging.rs:55, :71), and builds.rs calls engine_type()? at :170 before :175. So an engine conflict or a two-section config errors before the direct read is reached.
• Notice ordering re-verified on e511284. build push: upload_dir (builds.rs:150) → entrypoint (:161) → engine_type (:170) → game_id/engine_version (:173/:175), all before upload_directory_from_scan (:196). resolve_game_id's notice can't double-print with the accessor's, since it returns before building a config when the env var wins. Only init.rs prints JSON and it never builds a WavedashConfig.

Verification

doppler run -- cargo test / clippy / fmt were denied by this job's tool permissions again, so the findings above are static reading. The build itself is verified, though: this PR's own CI run 30585269751 ran cargo clippy --all-targets -- -D warnings and cargo test against e511284 and passed, so the compile and the ~45 unit tests are green. Finding #2's lockfile and formatting claims I confirmed by reading Cargo.lock:2527 and src/config.rs:963-964 directly; #1 and #3 are call-graph reasoning over src/auth.rs, src/main.rs, and src/dev/mod.rs.

To let me run the suite locally next time, add Bash(doppler run -- cargo test) and Bash(doppler run -- cargo clippy --all-targets) to --allowedTools in claude-code-review.yml.
• environment-var-overrides

[claude]

Claude finished @The64thRealm's task in 3m 30s —— View job

---

Code Review — Environment variable overrides

• Gather context (diff since last review round: e511284 → 2a46554)
• Review what the merge from main pulled in / interacts with
• Re-check carried-over findings
• Verify build + tests (this PR's own CI, run 30662543886 — green)
• Post review

---

The only change since the last round is 2a46554, the merge of main. It brings in clear-playtest-data and rebases main.rs, and the merge resolution is mostly right: the new command routes its --game-id through resolve_game_id (src/main.rs:740), so it picks up WAVEDASH_GAME_ID and the file-optional loader for free rather than reading game_id directly — which is exactly what the privatized-fields design was for. require_api_key's auth_info.api_key.unwrap() (src/clear_playtest_data.rs:98) is also still sound after the auth rework: resolve_auth only returns AuthSource::None alongside api_key: None, so the unwrap is guarded by the match.

One new finding from the merge, and three carried over.

1. clear-playtest-data --game-id is the one --game-id the merge left without the blank guard

src/main.rs:142-146 is the odd one out in this file now:

#[arg(
    long = "game-id",
    help = "Game ID (defaults to game_id in wavedash.toml)"
)]
game_id: Option<String>,

The other six (:287, :307, :329, :353, :392, :427) all carry value_parser = parse_non_empty_arg and all say "defaults to WAVEDASH_GAME_ID, then game_id in wavedash.toml". Two consequences:

• A blank flag beats a set override, silently. resolve_game_id_with returns on any Some (src/config.rs:506-508) — a typed flag wins outright by design. So --game-id "" yields Ok(""), the WAVEDASH_GAME_ID branch at :509 is never reached, and clear_playtest_data.rs:155-158 POSTs to {api_host}/api/games//clear-playtest-data. The user gets whatever the server says about an empty path segment instead of the value cannot be empty that every other subcommand gives them. The shape is the CI one this PR exists for: --game-id "$GAME_ID" with GAME_ID unset while WAVEDASH_GAME_ID is exported.
• --help disclaims a feature the command has. clear-playtest-data --help says the fallback is wavedash.toml; the override works anyway. For the godot plugin this PR is groundwork for, the help text is the discovery surface.

--username (:154-160) has the same gap and it matters more here than elsewhere: omitting it means everyone, so --username "" displays player "" (clear_playtest_data.rs:124) and sends "username": "" (:163-165) — a blank whose meaning is the server's to decide, on the one command in this CLI that can't be undone.

Fix this →

2. auth logout still reports success while WAVEDASH_TOKEN keeps the CLI authenticated

Carried, unaddressed. resolve_auth gives the environment precedence over the file (src/auth.rs:147-154); Logout (src/main.rs:547-550) only removes the file:

$ export WAVEDASH_TOKEN=wd_live_...
$ wavedash auth logout
✓ Successfully logged out
$ wavedash auth status
✓ Authenticated (via WAVEDASH_TOKEN environment variable)   # main.rs:555

Same shape as the blank---token disagreement efa48a9 closed, one layer up: a success line the next command contradicts. auth login --token X has the milder version — ✓ Successfully stored API key for a key WAVEDASH_TOKEN then shadows on every request — but auth status at least names the winning source there, and logout's message has no such out. Pre-existing precedence, raised because this PR is the one that made token handling rigorous enough for it to stand out.

Fix this →

3. ci.yml still can't catch what dist builds — and it's now measurably behind the tree

Carried. Three of these I can point at concretely rather than hypothetically:

• No cargo fmt --check. The tree is currently unformatted: src/config.rs:963-964 still has the double blank line before #[cfg(test)] that rustfmt collapses. The merge even pulled in rustfmt fixes from main for two #[arg] blocks in achievements — so formatting is drifting in both directions with nothing watching.
• No --locked. Cargo.lock is in sync today. Without it, a future PR that bumps Cargo.toml and forgets the lock passes CI and then releases a dependency set it never tested.
• One target. cargo-dist ships macOS and Windows; this crate has #[cfg(unix)] branches (src/auth.rs:99-103, :114-118) and TcpListener/canonicalize paths. A Windows-only break still can't surface until a tag is cut. cargo check --target x86_64-pc-windows-msvc closes it.
• No permissions: block. contents: read is all this job needs.

I can't add these myself — GitHub App permissions don't allow workflow modifications (FAQ).

4. dev/mod.rs:214 is still dead, with a message that's now wrong

let version = wavedash_config.engine_version()?.ok_or_else(|| {
    anyhow::anyhow!("{} requires a version in wavedash.toml", kind.as_label())
})?;

Carried. engine_version() returns Ok(None) only when active_engine() is None, and resolve_engine_entry is reached only where engine_kind is Some — so after e511284 a missing version arrives as an Err from missing_engine_field, never as None. And the message names only the file, when WAVEDASH_GODOT_VERSION/_UNITY_VERSION could have supplied it. Better as an expect stating the invariant than a closure pretending to handle a live path.

Fix this →

Smaller notes

• The override notice does reach clear-playtest-data, and I checked rather than assumed. resolve_game_id prints env override: WAVEDASH_GAME_ID → game_id = … (config.rs:284-290) before handle_clear_playtest_data runs, and both the confirmation (clear_playtest_data.rs:137-143) and the success line (:179-182) echo the resolved id. So an exported WAVEDASH_GAME_ID redirecting a wipe away from the local wavedash.toml is announced. Worth being aware that under --force that one println! is the entire guard on an irreversible operation targeting a game the user never typed — not a defect, but it's the sharpest edge the overrides now touch.
• is_non_interactive() (clear_playtest_data.rs:104-112) duplicates is_browser_login_unavailable in main.rs and says so in a comment. Came in with the merge, not this PR; noting it because a shared helper in config alongside non_blank is where the rest of this PR put exactly this kind of one-rule-one-implementation logic.
• Engine section fields are still pub and nothing outside config.rs reads them — making them private would turn "someone skips missing_engine_field" into a compile error, the same reasoning that privatized WavedashConfig's fields.
• engine_type() still doesn't announce. Harmless — every caller pairs it with engine_version() (builds.rs:170+:175, dev/mod.rs:92+:129) — but executable_section() got an explicit exemption comment and this is the remaining silent read of an announced field.
• Notice ordering re-verified on the merge. main.rs's new ClearPlaytestData arm resolves game_id before any output of its own; nothing in clear_playtest_data.rs prints JSON, and init.rs is still the only JSON path and still never builds a WavedashConfig. So print_override_notice's println! can't corrupt machine-readable output.
• Non-UTF-8 override values are still silently dropped (raw_env, config.rs:211-213). Unix-only, vanishingly rare; not raising it again.

Verification

doppler run -- cargo clippy / test / fmt were all denied by this job's tool permissions again, so the findings are static reading. The build is verified independently, though: this PR's CI run 30662543886 ran cargo clippy --all-targets -- -D warnings and cargo test against the merge commit 2a46554 and passed — so the merge compiles clean and the ~45 unit tests are green. Finding #3's formatting claim I confirmed by reading src/config.rs:963-965 directly (cargo fmt --check itself was denied); #1, #2 and #4 are call-graph reasoning over src/main.rs, src/config.rs, src/clear_playtest_data.rs, src/auth.rs and src/dev/mod.rs.

To let me run the suite locally next time, add Bash(doppler run -- cargo test), Bash(doppler run -- cargo clippy --all-targets) and Bash(cargo fmt --check) to --allowedTools in claude-code-review.yml.
• environment-var-overrides

[claude]

Claude finished @The64thRealm's task in 3m 4s —— View job

---

Code Review — Environment variable overrides

• Gather context (diff since last review round: 2a46554 → bdfe208)
• Review bdfe208 — require_api_key / is_non_interactive dedup + the new parser test
• Re-check carried-over findings
• Verify build + tests (this PR's own CI, run 30666678277 — green)
• Post review

---

bdfe208 fixed last round's finding #1 by fixing the class, not the instance, and the test is the best part of the commit. every_game_id_arg_rejects_a_blank_value (src/main.rs:781-841) walks the clap command tree instead of listing the seven subcommands, so the next --game-id added the way clear-playtest-data was — on main, in lines the parser edit never touched, merging clean — fails a test instead of surviving review. The checked.len() >= 7 floor closes the obvious hole in that approach (a rename leaving the walk asserting over nothing), and the doc comment explains why it parses real argv rather than inspecting the arg's ValueParser. That's the version that can't rot.

The two dedups are also the right ones. Four verbatim require_api_key copies collapsing into src/auth.rs:198-207 removes four chances for the refusal wording and the AuthSource match to drift apart independently, and is_non_interactive (src/main.rs:463-465) absorbs the copy whose own comment admitted it was mirroring — env_flag_enabled("CI") is byte-identical to the inline logic it replaced, so nothing changed behaviorally on the way. I checked that require_api_key's unwrap() is still sound: resolve_auth (src/auth.rs:147-172) only returns AuthSource::None alongside api_key: None, so the match guards it.

Two things from this commit, three carried.

1. --game-id got the blank guard; --username on the same command didn't

The fix landed on exactly one of the two args I raised. src/main.rs:154-161:

#[arg(
    short = 'u',
    long = "username",
    visible_alias = "user",
    help = "Only clear this player's data; omit to clear everyone's"
)]
username: Option<String>,

And it's the one where a blank means more. Omitting --username clears everyone's data, so Some("") is not a no-op that falls back — it's a third state neither the CLI nor the help text has a meaning for. main.rs:748 passes it through as Some(""), clear_playtest_data.rs:99-102 renders the confirmation as player "", and :141-142 sends "username": "". What that matches is the server's to decide, on the one irreversible command in this CLI.

The --force path is where it bites: no prompt, no chance to notice the empty quotes, and the CI shape this PR exists for produces it directly — --username "$PLAYER" --force with PLAYER unset. Every other typed string arg in main.rs now rejects blank (--build-id, --token, all seven --game-ids), so this is the last one out, and the new tree-walking test only looks at game-id.

Extending the walk to any arg in a small allowlist of blank-hostile long names would keep the test's "catches the next one too" property rather than adding a second hardcoded case.

Fix this →

2. The now-centralized auth refusal is the one place that doesn't mention WAVEDASH_TOKEN

src/auth.rs:198-207:

AuthSource::None => bail!("Not authenticated. Run `wavedash auth login` first.")

The wording is carried over verbatim from the four copies, so this isn't a regression — but centralizing it is the moment to fix it, and this PR is what makes it wrong. resolve_auth gives WAVEDASH_TOKEN precedence over the file (src/auth.rs:148-154), auth status names it (src/main.rs:565), and auth login's own non-interactive bail names it (:537-539) precisely because a browser isn't reachable there. require_api_key's callers — stat, achievement, team create, clear-playtest-data — are the scripted commands most likely to be running where auth login cannot, and theirs is the only refusal that offers just the interactive path.

Same one-sentence shape as the others: "Not authenticated. Set WAVEDASH_TOKEN, or run wavedash auth login."

While in there, the unwrap() can stop being a thing a reader has to verify. api_key.is_some() iff source != None, so matching on the key directly is exactly equivalent and needs no comment defending it:

auth_manager.get_auth_info().api_key.ok_or_else(|| anyhow!("Not authenticated. …"))

Fix this →

3. auth logout still reports success while WAVEDASH_TOKEN keeps the CLI authenticated

Carried, unaddressed. src/main.rs:556-559 removes only the file; resolve_auth prefers the environment:

$ export WAVEDASH_TOKEN=wd_live_...
$ wavedash auth logout
✓ Successfully logged out
$ wavedash auth status
✓ Authenticated (via WAVEDASH_TOKEN environment variable)   # main.rs:565

The same shape as the blank---token disagreement efa48a9 closed and the --game-id "" one bdfe208 closed — a success line the next command contradicts. It's the last one left. auth login --token X has the milder variant (✓ Successfully stored API key for a key WAVEDASH_TOKEN then shadows on every request), but there auth status at least names the winner; logout's message has no such out. Pre-existing precedence, raised because this PR is what made token handling rigorous enough for it to stand out.

Fix this →

4. dev/mod.rs:214 is still dead, with a message that's now wrong

let version = wavedash_config.engine_version()?.ok_or_else(|| {
    anyhow::anyhow!("{} requires a version in wavedash.toml", kind.as_label())
})?;

Carried. engine_version() returns Ok(None) only when active_engine() is None (src/config.rs:865-868), and resolve_engine_entry is reached only where engine_kind is Some — so after e511284 a missing version arrives as an Err from missing_engine_field, never as None. The message also names only the file when WAVEDASH_GODOT_VERSION/_UNITY_VERSION could have supplied it. An expect stating the invariant beats a closure pretending to handle a live path.

Fix this →

5. ci.yml still can't catch what dist builds

Carried. Two of these are measurable against the tree right now, not hypothetical:

• No cargo fmt --check. src/config.rs:963-964 still has the double blank line before #[cfg(test)] that rustfmt collapses — the tree is unformatted today and nothing notices.
• One target. cargo-dist ships macOS and Windows; this crate has #[cfg(unix)] branches (src/auth.rs:99, :114) plus TcpListener/canonicalize paths. A Windows-only break still can't surface until a tag is cut. cargo check --target x86_64-pc-windows-msvc closes it.
• No --locked. In sync today; without it a future PR that bumps Cargo.toml and forgets the lock passes CI and releases a dependency set it never tested.
• No permissions: block. contents: read is all this job needs.

I can't add these myself — GitHub App permissions don't allow workflow modifications (FAQ).

Smaller notes

• is_non_interactive landed in the binary root, not config. It works, and the doc comment naming the condition rather than a consequence is right. But clear_playtest_data now reaches up through crate:: into main.rs for a rule, while the sibling one-rule-one-implementation helper (non_blank) lives in config where every module can reach it without that inversion. Cosmetic; noting only because the placement is the thing that decides whether the next module reuses it or writes a third copy.
• The new test's blank set is good — "", " ", "\t", "\n" covers the $(cat key)-with-trailing-newline shape as well as the unset-CI-variable one, and it's asserting ValueValidation specifically rather than "some error", so a subcommand that fails for a missing required arg instead can't pass by accident.
• Engine section fields are still pub and nothing outside config.rs reads them — private would turn "someone skips missing_engine_field" into a compile error, the same reasoning that privatized WavedashConfig's own fields.
• engine_type() still doesn't announce. Harmless — every caller pairs it with engine_version() (builds.rs:170+:175, dev/mod.rs:92+:129) — but executable_section() got an explicit exemption comment and this is the remaining silent read of an announced field.
• Notice ordering re-verified on bdfe208. No output paths moved: resolve_game_id's notice (config.rs:284) still precedes handle_clear_playtest_data, and the confirmation (clear_playtest_data.rs:113-119) and success line (:151-154) both echo the resolved id. init.rs is still the only JSON path and still never builds a WavedashConfig, so print_override_notice's println! can't corrupt machine-readable output.
• --force + an exported WAVEDASH_GAME_ID remains the sharpest edge these overrides touch: one println! is the entire guard on an irreversible wipe of a game the user never typed. Not a defect — the notice is there and it fires — just the thing to keep in mind if the notice ever moves behind a --quiet.

Verification

cargo test / clippy / fmt were denied by this job's tool permissions again (both bare and doppler run -- forms), so the findings are static reading. The build is verified independently: this PR's CI run 30666678277 ran cargo clippy --all-targets -- -D warnings and cargo test against bdfe208 and passed — so the compile is clean, the new tree-walk test passes, and the ~46 unit tests are green. Finding #5's formatting claim I confirmed by reading src/config.rs:962-965 directly; #1–#4 are call-graph reasoning over src/main.rs, src/auth.rs, src/clear_playtest_data.rs, src/config.rs and src/dev/mod.rs.

To let me run the suite locally next time, add Bash(doppler run -- cargo test), Bash(doppler run -- cargo clippy --all-targets) and Bash(cargo fmt --check) to --allowedTools in claude-code-review.yml.

--- • environment-var-overrides

[claude]

Claude finished @The64thRealm's task in 2m 51s —— View job

---

Code Review — Environment variable overrides

• Gather context (diff since last review round: bdfe208 → b591143)
• Review b591143 — logout message, require_api_key, the dev expect, comment trims
• Re-check carried-over findings
• Verify build + tests (this PR's own CI, run 30670397587 — green)
• Post review

---

b591143 closes three of the four carried findings, and the dev one is closed properly rather than papered over. Replacing the dead ok_or_else with an expect (src/dev/mod.rs:213-215) is only as good as the invariant behind it, and the commit went and pinned the invariant: an_engine_kind_always_arrives_with_a_version (src/config.rs:1618-1663) asserts, per section, that a declared kind carries a version and that a bare section fails engine_type() too — which is the half that actually makes the expect unreachable, since dev only reaches resolve_engine_entry through a Some from engine_type(). I walked it independently: active_engine() is a pure read over immutable fields, engine_version() returns Ok(None) only when it returns Ok(None), so the two can't disagree within a call.

require_api_key (src/auth.rs:188-195) now names WAVEDASH_TOKEN for the scripted commands that most need it, and dropping the AuthSource match for api_key.ok_or_else removes the unwrap whose safety previously had to be argued. Logout is fixed too. Two things left on it, plus the carried CI note.

1. --username is still the one blank that gets through, on the one irreversible command

Third round for this. src/main.rs:155-161:

#[arg(
    short = 'u',
    long = "username",
    visible_alias = "user",
    help = "Only clear this player's data; omit to clear everyone's"
)]
username: Option<String>,

--game-id on this same struct got value_parser = parse_non_empty_arg in bdfe208. --username didn't, and it's the arg where a blank has no defined meaning at all: omitting it clears everyone's data, so Some("") is a third state the CLI never defines. clear_playtest_data.rs:100 renders the confirmation as player "", :139-140 sends "username": "", and what that matches is the server's call.

Under --force there's no prompt to notice the empty quotes, and the CI shape this PR exists to serve produces it directly: --username "$PLAYER" --force with PLAYER unset. Every other typed string arg in main.rs now refuses blank — --build-id, --token, all seven --game-ids. This is the last one, on the only command that can't be undone.

The tree-walking test (src/main.rs:783) is the right place to put the guard, since widening its long-name set keeps the "catches the next one too" property instead of adding a second hardcoded case.

Fix this →

2. Logout got the shadowing warning; login --token didn't

src/main.rs:552-563 is the right fix and the wording is good — it says what was removed, what still authenticates, and what to do about it. But the same precedence produces the same contradiction one line up at src/main.rs:526:

$ export WAVEDASH_TOKEN=wd_live_aaa
$ wavedash auth login --token wd_live_bbb
✓ Successfully stored API key
$ wavedash build push        # authenticates as …aaa, forever

The key is stored, and it is inert on every request until the variable is unset. auth status does name the winning source, so it's discoverable — but that was equally true of logout before this commit, and the argument for fixing logout ("nothing announces the precedence as it happens") applies verbatim here. Right now the two arms of the same match action disagree about whether that's worth saying.

The check is also worth asking the resolver for rather than restating. AuthSource derives PartialEq (src/auth.rs:30), so after clear_credentials() the question is just get_auth_info().source == AuthSource::Environment — which is precedence-by-construction, where std::env::var(ENV_TOKEN).ok().and_then(config::non_blank) at :554-557 is a third hand-copy of the rule that has to stay in step with resolve_auth. Same call before store_credentials answers it for login.

And as written the logic sits inline in run()'s match arm, so nothing can test it — the previous round's suggestion to keep it a pure function over (env_token, file_existed) was aimed at that. resolve_auth has five tests precisely because it was extracted; this decision has none.

Fix this →

3. ci.yml still can't catch what dist builds

Carried, and the first item is measurable against the tree right now rather than hypothetical.

• No cargo fmt --check. src/config.rs:962-963 still has the double blank line before #[cfg(test)] that rustfmt collapses. The tree is unformatted today and nothing notices.
• One target. cargo-dist ships macOS and Windows; this crate has #[cfg(unix)] branches (src/auth.rs:99, :114) plus TcpListener/canonicalize paths. A Windows-only break can't surface until a tag is cut — cargo check --target x86_64-pc-windows-msvc closes it.
• No --locked. In sync today; without it a future PR that bumps Cargo.toml and forgets the lock passes CI and releases a dependency set it never tested.
• No permissions: block. contents: read is all this job needs.

I can't add these myself — GitHub App permissions don't allow workflow modifications (FAQ).

Smaller notes

• WAVEDASH_TOKEN is now half constant, half string literal. This commit routed require_api_key (auth.rs:192) and logout (main.rs:560) through config::ENV_TOKEN, but three user-facing messages still spell it out: main.rs:531 (login's non-interactive bail), :568 (auth status Environment), :584 (auth status None). Not a bug — the constant's value can't change without someone noticing — but the reason to have introduced ENV_TOKEN was to stop the name living in N places, and it currently lives in five.
• The comment trims cut one thing worth keeping. Most of what b591143 removed was justified — require_api_key and is_non_interactive read fine on their own now. The exception is the every_game_id_arg_rejects_a_blank_value doc: the paragraph about clear-playtest-data arriving without a parser and merging clean because the two edits never touched the same lines is the argument for walking the tree instead of listing subcommands, and the trimmed version no longer explains why the test is shaped that way. The checked.len() >= 7 floor lost its "guards against a rename leaving zero assertions" note for the same reason — that one's a bare magic number now.
• clear_playtest_data's trimmed comment (:103) dropped the note that its non-interactive refusal is the same check that stops auth login reaching for a browser. Fine, since is_non_interactive is now shared and its name carries it.
• Engine section fields are still pub and nothing outside config.rs reads them — private would turn "someone skips missing_engine_field" into a compile error, the same reasoning that privatized WavedashConfig's own fields.
• engine_type() still doesn't announce. Harmless, since every caller pairs it with engine_version() (builds.rs:170+:175, dev/mod.rs:92+:129) — and the new test now makes that pairing an invariant rather than a coincidence. Still the one silent read of an announced field, where executable_section() got an explicit exemption comment.
• Notice ordering re-verified on b591143. No output path moved; init.rs is still the only JSON path and still never builds a WavedashConfig, so print_override_notice's println! can't corrupt machine-readable output.

Verification

cargo test / clippy / fmt were denied by this job's tool permissions again (both bare and doppler run -- forms), so the findings are static reading. The build is verified independently: this PR's CI run 30670397587 ran cargo clippy --all-targets -- -D warnings and cargo test against b591143 and passed — so the compile is clean and the ~47 unit tests, including the new an_engine_kind_always_arrives_with_a_version, are green. Finding #3's formatting claim I confirmed by reading src/config.rs:960-965 directly; #1 and #2 are call-graph reasoning over src/main.rs, src/auth.rs, and src/clear_playtest_data.rs.

To let me run the suite locally next time, add Bash(doppler run -- cargo test), Bash(doppler run -- cargo clippy --all-targets) and Bash(cargo fmt --check) to --allowedTools in claude-code-review.yml.

---

• environment-var-overrides

[franzwarning]

might want to say "game_id in wavedash.toml. override w/ x". rather than "defaults" since most people will not use this

[claude]

Claude finished @The64thRealm's task in 2m 51s —— View job

---

Code Review — Environment variable overrides

• Gather context (diff since last review round: b591143 → f34672f)
• Review f34672f — --game-id help wording
• Re-check carried-over findings
• Verify build + tests (this PR's own CI, run 30841438505 — green)
• Post review

---

f34672f is a pure wording change: all seven --game-id help strings go from "defaults to WAVEDASH_GAME_ID, then game_id in wavedash.toml" to "defaults to game_id in wavedash.toml. override with WAVEDASH_GAME_ID". It answers @franzwarning's point correctly — leading with the file is right, because the file is what almost every reader has and the variable is the exception. And it applied the change to all seven rather than the one in the screenshot, which is what keeps clear-playtest-data from drifting back out of line the way it did in the merge.

Two things about the new string, then the three carried findings.

1. On a flag's own help, "override with WAVEDASH_GAME_ID" says the opposite of the precedence

src/main.rs:145 (and :290, :310, :332, :356, :395, :430):

--game-id <GAME_ID>  Game ID (defaults to game_id in wavedash.toml. override with WAVEDASH_GAME_ID)

The intended reading is "override the file with the variable", and that's accurate. But this text is attached to --game-id, so the nearest antecedent for "override" is the flag the user is reading about — and the variable does not override the flag. resolve_game_id_with returns on any Some from the flag before it ever looks at the environment (src/config.rs:503-508), deliberately and with a comment saying so.

Concretely, that's the CI shape this PR exists for:

env:
  WAVEDASH_GAME_ID: 12345
run: wavedash stat create --game-id "$LEGACY_ID" --identifier KILLS --name Kills

A reader who took the help at face value expects 12345; they get $LEGACY_ID. The old wording ("defaults to WAVEDASH_GAME_ID, then game_id in wavedash.toml") stated the whole chain in fallback order and couldn't be read backwards — the reorder is an improvement in emphasis but a regression in precision.

Something that does both — file first, and no claim about beating the flag:

help = "Game ID (default: game_id in wavedash.toml; WAVEDASH_GAME_ID overrides the file)"

Two smaller things in the same string while it's being touched:

• wavedash.toml. override — a period followed by a lowercase word. Every other multi-clause help in this file uses a semicolon ("Only clear this player's data; omit to clear everyone's", main.rs:159), and clap renders these inline, so the sentence break reads as a typo in --help output.
• @franzwarning asked to drop "defaults", and it's still there. Not worth a round-trip on its own, but if the string is getting edited anyway, "default: …" is both shorter and the thing that was asked for.

Fix this →

2. The string is hand-copied seven times, and it has already drifted once

This commit had to edit seven identical literals to change one sentence, and the reason all seven needed editing is that the previous round found clear-playtest-data carrying a different one — it arrived from main with "defaults to game_id in wavedash.toml" while the other six said "defaults to WAVEDASH_GAME_ID, then …", and neither edit touched the other's lines so the merge was clean and the disagreement invisible. That's the same failure mode every_game_id_arg_rejects_a_blank_value (src/main.rs:777) was written to make impossible for the parser, one attribute over.

A const GAME_ID_HELP: &str referenced by all seven #[arg(help = GAME_ID_HELP)] makes it structural, and the existing walk already has every --game-id arg in hand — one assert_eq!(arg.get_help(), GAME_ID_HELP) inside it would cover the text the same way the blank check covers the parser.

Fix this →

3. --username is still the one blank that gets through, on the one irreversible command

Fourth round. src/main.rs:155-161:

#[arg(
    short = 'u',
    long = "username",
    visible_alias = "user",
    help = "Only clear this player's data; omit to clear everyone's"
)]
username: Option<String>,

--game-id on this same struct has value_parser = parse_non_empty_arg (:144). --username doesn't, and it's the arg where a blank has no defined meaning at all: omitting it clears everyone's data, so Some("") is a third state the CLI never defines. clear_playtest_data.rs:99 renders the confirmation scope from it, and :139-140 sends "username": "" — what that matches is the server's call.

Under --force there's no prompt to notice the empty quotes, and the CI shape this PR exists to serve produces it directly: --username "$PLAYER" --force with PLAYER unset. Every other typed string arg in main.rs refuses blank — --build-id (:98), --token, all seven --game-ids. This is the last one, on the only command that can't be undone.

Fix this →

4. Logout warns about WAVEDASH_TOKEN shadowing; login --token still doesn't

Carried. src/main.rs:548-563 is the right fix and the wording is good. The same precedence produces the same contradiction two lines up at src/main.rs:523-526:

$ export WAVEDASH_TOKEN=wd_live_aaa
$ wavedash auth login --token wd_live_bbb
✓ Successfully stored API key
$ wavedash build push        # authenticates as …aaa, forever

The key is stored and inert on every request until the variable is unset. Two arms of the same match action currently disagree about whether that's worth saying.

The check is also worth asking the resolver for rather than restating: AuthSource derives PartialEq, so get_auth_info().source == AuthSource::Environment is precedence-by-construction, where std::env::var(config::ENV_TOKEN).ok().and_then(config::non_blank) at :554-557 is a third hand-copy of a rule that has to stay in step with resolve_auth (src/auth.rs:148-154). And as written the logic sits inline in run()'s match arm, so nothing tests it — resolve_auth has five tests precisely because it was extracted.

Fix this →

5. ci.yml still can't catch what dist builds

Carried. The first item is measurable against the tree right now, not hypothetical.

• No cargo fmt --check. src/config.rs:963-964 still has the double blank line before #[cfg(test)] that rustfmt collapses. The tree is unformatted today and nothing notices — I read the lines directly, since cargo fmt --check itself was denied.
• One target. cargo-dist ships macOS and Windows; this crate has #[cfg(unix)] branches in src/auth.rs plus TcpListener/canonicalize paths. A Windows-only break can't surface until a tag is cut — cargo check --target x86_64-pc-windows-msvc closes it.
• No --locked on either step (ci.yml:39, :42). In sync today; without it a future PR that bumps Cargo.toml and forgets the lock passes CI and releases a dependency set it never tested.
• No permissions: block. contents: read is all this job needs.

I'm unable to add these myself — GitHub App permissions don't allow workflow modifications. More in the FAQ.

Smaller notes

• WAVEDASH_GAME_ID is spelled literally in all seven help strings, where config::ENV_GAME_ID exists. clap's help needs a const-evaluable string, so concat! or a const built from it is the only way to route through the constant — worth it if finding iframe'd #2's const lands anyway, ignorable otherwise. Same situation as ENV_TOKEN still being spelled out in three user-facing messages (main.rs:530, :569, :584).
• The seven help strings are the only place the precedence is documented for users. config.rs:481-482's doc comment states it correctly for readers of the source, and resolve_game_id's error names both --game-id and WAVEDASH_GAME_ID — but there's no README or --help epilogue listing the five WAVEDASH_* variables anywhere. For the godot plugin this PR is groundwork for, that list existing somewhere discoverable is worth more than any of the wording above.
• Engine section fields are still pub and nothing outside config.rs reads them — private would turn "someone skips missing_engine_field" into a compile error, the same reasoning that privatized WavedashConfig's own fields.
• engine_type() still doesn't announce. Harmless, since every caller pairs it with engine_version(), and an_engine_kind_always_arrives_with_a_version now makes that pairing an invariant. Still the one silent read of an announced field, where executable_section() got an explicit exemption comment.
• Notice ordering unchanged by this commit — no output path moved; init.rs is still the only JSON path and still never builds a WavedashConfig, so print_override_notice's println! can't corrupt machine-readable output.

Verification

cargo fmt --check, cargo test and cargo clippy were all denied by this job's tool permissions again (bare and doppler run -- forms), so the findings are static reading. The build is verified independently: this PR's CI run 30841438505 ran cargo clippy --all-targets -- -D warnings and cargo test against f34672f and passed, so the compile is clean and the unit tests are green. Finding #5's formatting claim I confirmed by reading src/config.rs:960-965; #1's precedence claim by reading src/config.rs:503-509; #2's drift history from git log -p on the seven #[arg] blocks; #3 and #4 are call-graph reasoning over src/main.rs, src/auth.rs and src/clear_playtest_data.rs.

To let me run the suite locally next time, add Bash(doppler run -- cargo test), Bash(doppler run -- cargo clippy --all-targets) and Bash(cargo fmt --check) to --allowedTools in claude-code-review.yml.

---

• environment-var-overrides