Make the locale test stub strict about unexpected variable reads

[leynos]

Context

Netsuke has three shapes for the same idea — read-only environment access:

Site	Shape
src/locale_resolution.rs:19	bespoke trait EnvProvider { fn var(&self, key: &str) -> Option<String>; } with a SystemEnv impl
src/output_mode.rs:53	closure |key| env::var(key).ok() passed to resolve_with
src/output_prefs.rs:192,237	the same closure passed to resolve_from_theme_with and resolve_with
src/theme.rs (via resolve_theme)	receives the same closure

The AGENTS.md testing mandate accepts a narrow closure seam where a trait object would be disproportionate, so these seams are not defective and stay as they are. An earlier version of this issue proposed converging all four sites onto mockable::Env; that proposal was withdrawn (see the amendment note below and the discussion in the comments).

What remains is a genuine defect in the test double: test_support::locale_stubs::StubEnv is permissive. A stub that returns None for any undeclared key hides exactly the change a test double should catch — were the code under test altered to read a differently-named variable, through a rename, a typo, or a new precedence rung, the permissive stub would quietly answer None and the test would still pass while asserting nothing about the new read.

Required work

• Retain locale_resolution::EnvProvider and SystemEnv.
• Retain the read_env closure seams in output_mode, output_prefs, and theme.
• Retain test_support::locale_stubs::StubEnv.
• Make StubEnv strict:
  • it returns values only for keys the test explicitly declared;
  • a declared key may be explicitly unset, so "declared but unset" is distinguishable from "never expected to be read" and reports None without panicking;
  • any read of an undeclared key panics, and the panic message names the offending key.

Acceptance criteria

• StubEnv cannot be constructed in a permissive state: every constructor requires the test to declare what may be read.
• A read of a declared, set key returns the declared value; a read of a declared, unset key returns None.
• A read of an undeclared key panics with a message naming the key.
• The strictness is validated by tests covering all three behaviours, including redeclaration semantics (the most recent declaration for a key wins).
• make check-fmt, make lint, and make test pass.

---

Amendment history: this issue originally required replacing EnvProvider and StubEnv with a shared mockable::Env/MockEnv injection across locale, output mode, output preferences, and theme resolution. That convergence proposal was withdrawn — the repository's established seams are proportionate for these single-variable sites — and the body above has been rewritten so every section states the remaining requirement: strictness of StubEnv, not replacement.

[coderabbitai]

🔗 Related PRs

#330 - Inject config environment access (#293) [merged]
#497 - Stop the mock environment mutating the real process environment (#490) [open]
#501 - Give the manifest env() helper an injectable env seam (#484) [open]
#502 - Make the locale test stub strict about unexpected variable reads (#489) [open]
#515 - Harden workspace lint configuration [open]

---

📝 Issue Planner

_{Check the box below or use the @coderabbitai plan command to generate an implementation plan and prompts that you can use with your favorite coding assistant.}

• Create Plan

---

🧪 Issue enrichment is currently in open beta.

You can configure auto-planning by selecting labels in the issue_enrichment configuration.

To disable automatic issue enrichment, add the following to your .coderabbit.yaml:

issue_enrichment:
  auto_enrich:
    enabled: false

💬 Have feedback or questions? Drop into our discord!

[leynos]

Tracked under #496.

[leynos]

Narrowed after checking ortho-config RFC 0002

This issue proposed converging both bespoke environment traits — locale_resolution::EnvProvider and cli::discovery::EnvProvider — onto mockable::Env.

ortho-config RFC 0002 makes the second half a bad idea. The RFC uses Netsuke's src/cli/discovery.rs as its explicit proof case, and proposes absorbing exactly that module's behaviour — ordered selector chains, fail-closed explicit selection, multi-scope stacking — into a ConfigFilePolicy resolver upstream. Its own summary calls the module "exactly the kind of bespoke 'config goblin' the proposed API is meant to absorb".

Converging discovery::EnvProvider onto mockable::Env now would be churn on code slated for deletion, and would then have to be unpicked again when the policy API arrives with its own EnvSource.

Revised scope: locale_resolution::EnvProvider only.

• Replace it and its SystemEnv impl with mockable::Env/DefaultEnv.
• Delete the hand-rolled StubEnv in test_support/src/locale_stubs.rs, which duplicates MockEnv without expectation-setting.
• Update tests/localization_tests.rs, which currently pairs a OnceLock with that stub.
• Leave cli::discovery::EnvProvider in place, and record why.

The read_env closure seams in output_mode, output_prefs, and theme also stay: they are permitted by the mandate, and the convergence argument for them was always weaker than for the duplicated traits.

Tracked under #496.

[leynos]

Abandoning the convergence — mockable is a dev-dependency

I proposed replacing locale_resolution::EnvProvider with mockable::Env. On attempting it, that is the wrong trade.

mockable sits in Netsuke's [dev-dependencies] and reaches the crate only through test_support. locale_resolution is production code with a public API, so re-exporting mockable::Env from it requires moving mockable into [dependencies] — and cargo tree shows it pulling mockall with it:

mockable v3.0.0
├── mockall v0.11.4
└── tracing v0.1.44

Shipping a mocking framework in release builds of a build tool, to delete a two-method trait, is not a good exchange. The bespoke EnvProvider exists precisely so that does not happen. That is a sound reason, not the duplication I characterised it as.

The AGENTS.md mandate also already permits this shape: EnvProvider is a narrow seam of exactly the form it describes, and locale_resolution was never in violation. This issue was tidiness, not compliance, and the tidiness is not worth the dependency.

What is worth doing

One real defect surfaced while investigating. test_support::locale_stubs::StubEnv answers None for every key but one:

impl EnvProvider for StubEnv {
    fn var(&self, key: &str) -> Option<String> {
        if key == locale_resolution::NETSUKE_LOCALE_ENV {
            return self.locale.clone();
        }
        None
    }
}

If the code under test were changed to read a different variable — a rename, a typo, or a new precedence rung — the stub would quietly answer None and the test would still pass. That is the failure mode a test double is supposed to prevent, and it is the one thing MockEnv's expectations would genuinely have bought.

Retitling this issue to fix that directly, without the dependency cost.