refactor(environments): split mod.rs into config/devx_core/catalog submodules - #213
Merged
Conversation
…bmodules rust/src/environments/mod.rs was approaching the 1000-line CI limit, blocking contributors adding to it in an open PR and another branch. Splits it by responsibility: GddyEnvConfig + URL resolution helpers (config.rs), DevX Core gateway URL resolution (devx_core.rs), and API-catalog domain base-URL resolution (catalog.rs), with each submodule's tests moving alongside it. mod.rs keeps only the compiled-in registry, the shared Environments instance, and the public resolve/listable/is_known API. Public API is unchanged via re-exports. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Pull request overview
Refactors the rust/src/environments module by splitting the previously large mod.rs into focused submodules while keeping the public crate::environments::* API unchanged. This supports continued growth of environment/URL-resolution logic without hitting the CI 1000-line file limit.
Changes:
- Split environment responsibilities into
config,devx_core, andcatalogsubmodules, withmod.rsretaining the sharedEnvironmentssingleton and public entrypoints. - Centralized test-only env-var helpers into
test_support.rsfor reuse across submodules. - Moved existing tests alongside the code they validate (e.g., DevX Core and config URL resolution tests).
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| rust/src/environments/mod.rs | Introduces submodules and re-exports; keeps the main public environment registry/singleton and top-level APIs. |
| rust/src/environments/config.rs | Holds GddyEnvConfig and shared URL validation/derivation helpers, plus associated tests. |
| rust/src/environments/devx_core.rs | Extracts DevX Core gateway URL resolution and its tests. |
| rust/src/environments/catalog.rs | Extracts API-catalog domain base-URL resolution and its tests. |
| rust/src/environments/test_support.rs | Adds shared test-only env-var locking + RAII env-var restore helper for env-var-touching tests. |
Suppressed comments (1)
rust/src/environments/catalog.rs:93
- This test calls
resolve_catalog_base_url, which reads process env vars viastd::env::var. Because other tests in this module mutate env vars usingEnvGuard/set_var, this should takeENV_LOCKto avoid concurrent env access during those unsafe writes (and to prevent cross-test flakiness).
fn resolve_catalog_base_url_applies_convention_for_non_prod() {
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Make the ENV_LOCK comment a real rustdoc doc comment so the module doc's cross-reference to "ENV_LOCK's own doc" actually resolves, and hold ENV_LOCK in the two catalog.rs tests that indirectly read real process env vars through resolve_catalog_base_url, matching this module's existing test-serialization convention. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 5 out of 5 changed files in this pull request and generated no new comments.
Suppressed comments (1)
rust/src/environments/test_support.rs:25
EnvGuard::setrelies on a caller-heldENV_LOCKfor safety, but the API doesn’t enforce that invariant. It’s easy for a future test to callEnvGuard::set(...)without holding the lock, reintroducing cross-test flakiness and potential UB (sincestd::env::set_var/remove_varareunsafe). Consider encoding the lock requirement in the API (e.g., haveEnvGuard::settake a&MutexGuard<()>, or haveEnvGuard::setacquire and hold the lock internally) so misuse is impossible.
impl EnvGuard {
pub(super) fn set(key: &'static str, value: &str) -> Self {
let prior = std::env::var(key).ok();
// SAFETY: caller holds ENV_LOCK.
#[allow(unsafe_code)]
unsafe {
mguerrero3-godaddy
approved these changes
Aug 18, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
environments/mod.rswas approaching the 1000-line CI limit, blocking contributors adding to it in an open PR and another in-progress branch.config.rs(GddyEnvConfig+ URL resolution/derivation helpers),devx_core.rs(DevX Core gateway URL resolution),catalog.rs(API-catalog domain base-URL resolution), andtest_support.rs(shared test-only env-var guard,#[cfg(test)]-only).mod.rsnow holds only the compiled-inote/prodregistry, the sharedEnvironmentssingleton, and the publicresolve/listable/is_knownAPI.mod.rs), so every existingcrate::environments::*call site is untouched.Test plan
cargo checkpassescargo clippy -- -D warningspasses with zero warningscargo testpasses (615 passed, same 36 environments-module tests as before, moved verbatim)cargo fmt --checkis clean./rust/scripts/check-module-size.shpasses (largest new file is 503 lines)🤖 Generated with Claude Code