refactor: extract config.rs into focused submodules#306
Merged
Conversation
Mechanical module extraction of src/config.rs (8,324 lines) into 8 focused submodules with no behavior changes. All 301 tests pass, all public type names remain stable via re-exports from the module root. New layout: src/config.rs — module root, re-exports, tests src/config/types.rs — domain types (Config, AgentConfig, Binding, etc.) src/config/toml_schema.rs — Toml* serde structs and defaults src/config/load.rs — Config::load, from_toml, env/secret resolution src/config/providers.rs — provider constants and bootstrap helpers src/config/runtime.rs — RuntimeConfig + reload logic src/config/watcher.rs — spawn_file_watcher src/config/permissions.rs — Discord/Slack/Telegram/Twitch permission builders src/config/onboarding.rs — run_onboarding
Contributor
|
Caution Review failedThe pull request is closed. ℹ️ Recent review infoConfiguration used: Organization UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (2)
WalkthroughAdds a full, file-scoped configuration subsystem: TOML schema and domain types, multi-source loading and validation (file/env/secrets), provider/routing inference, interactive onboarding, runtime hot-reload state, permission extraction, and a filesystem watcher that triggers selective live reloads. Changes
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
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. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (2)
src/config/load.rs (1)
287-494: Consolidate duplicated provider bootstrap logic inload_from_env.This section repeats nearly identical provider insertion blocks twice. It works because of
or_insert_with, but it’s brittle and easy to desynchronize.♻️ Refactor direction
- // Populate providers from env vars (same as from_toml does) - if let Some(anthropic_key) = llm.anthropic_key.clone() { ... } - ... - if let Some(minimax_cn_key) = llm.minimax_cn_key.clone() { ... } + populate_llm_providers_from_keys(&mut llm, anthropic_from_auth_token);Then keep a single helper (shared by
load_from_envandfrom_toml) for provider map population.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/config/load.rs` around lines 287 - 494, The code duplicates provider insertion logic in load_from_env; remove the repeated blocks and consolidate into a single provider-population helper (e.g., populate_providers) that fills llm.providers using the existing add_shorthand_provider and the entry/or_insert_with patterns; update load_from_env to call this helper (and also make from_toml call it) so all provider keys (anthropic, openrouter, kilo, zhipu, zai-coding-plan, opencode-zen, opencode-go, minimax, minimax-cn, openai) are added exactly once to llm.providers; keep existing symbols like add_shorthand_provider, load_from_env, from_toml and the ProviderConfig construction logic but move them into the single helper to avoid duplication.src/config/toml_schema.rs (1)
546-611: Consider extracting shared email fields for future refactoring.
TomlEmailConfigandTomlEmailInstanceConfigshare nearly identical fields. This duplication pattern (also present in Discord, Slack, Telegram, Twitch configs) is valid for multi-instance inheritance but could be consolidated in a future phase.Since the PR describes this as Phase 1 with no behavior changes, this is fine to defer.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/config/toml_schema.rs` around lines 546 - 611, TomlEmailConfig and TomlEmailInstanceConfig duplicate nearly all fields; extract the shared fields into a new struct (e.g., TomlEmailCommon or TomlEmailFields) and embed it into both configs (use #[serde(flatten)] to preserve the TOML shape) so TomlEmailConfig and TomlEmailInstanceConfig keep only unique fields like instances and name; update references to the moved fields accordingly (types/field names: enabled, imap_host, imap_port, imap_username, imap_password, imap_use_tls, smtp_host, smtp_port, smtp_username, smtp_password, smtp_use_starttls, from_address, from_name, poll_interval_secs, folders, allowed_senders, max_body_bytes, max_attachment_bytes).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/config/onboarding.rs`:
- Around line 245-296: The code appends raw user strings into TOML via
config_content.push_str(&format!(...)) (see uses of toml_key, provider_value,
routing.channel/branch/worker/compactor/cortex, agent_id, discord.token,
guild_id and the channel/dm id lists), which can produce invalid TOML if inputs
contain quotes/newlines; change those direct interpolations to produce
TOML-escaped string literals instead — for each value (toml_key/provider_value,
routing.* fields, agent_id, discord.token, guild_id and every
channel_ids/dm_user_ids element) create a toml::Value::String (or otherwise use
toml::to_string on the string) and append the serialized/escaped result so the
pushed lines use properly quoted and escaped TOML strings rather than raw
format! interpolation.
In `@src/config/runtime.rs`:
- Around line 177-203: The reload_config path currently updates many fields but
omits the RuntimeConfig.opencode field; modify reload_config to store the new
opencode state (e.g., call self.opencode.store(Arc::new(resolved.opencode))) so
changes to [defaults.opencode] are applied at runtime, and if
opencode_server_pool parameters (path, permissions, max_servers) are intended to
be hot-reloadable, add logic in the same reload sequence to rebuild or swap the
opencode_server_pool (identify and reconcile via the opencode_server_pool
manager or field name) when resolved.opencode or its pool subfields change.
In `@src/config/types.rs`:
- Around line 119-128: ApiConfig and WebhookConfig currently derive Debug while
exposing the auth_token field; implement a custom Debug for both types that
omits or redacts the auth_token (e.g., show "<redacted>" or
"Some(***REDACTED***)" instead of the real token) so that when Config (which
derives Debug) is printed it cannot leak credentials; update the impl Debug for
ApiConfig and WebhookConfig to format all other fields normally but replace
auth_token with the redacted placeholder.
In `@src/config/watcher.rs`:
- Around line 41-57: The callback currently ignores the Err variant of the
Result<Event, notify::Error>, which swallows backend watcher failures; update
the closure that matches on result (the move |result: std::result::Result<Event,
notify::Error>| { ... }) to handle the Err(e) case instead of dropping it—log
the error (using the project's logging/tracing facility) with context like
"watcher callback error" and the error value, or otherwise propagate it if
appropriate; keep the existing tx.send(event) behavior for Ok(event) and only
use let _ = on tx.send where receiver may be dropped.
- Around line 198-230: When a platform block is removed the existing ArcSwap
held by discord_permissions / slack_permissions / telegram_permissions /
twitch_permissions remains stale; add an else branch for each platform check
(matching the pattern used for Discord) that stores a cleared/default
permissions snapshot into perms (e.g., replace the Arc with an empty/default
platform permissions instance) so the ArcSwap is updated even when
config.messaging.<platform> is None; update the SlackPermissions::from_config,
TelegramPermissions::from_config and TwitchPermissions::from_config blocks to
mirror the Discord pattern and store a cleared/default Arc when the
corresponding config is absent.
---
Nitpick comments:
In `@src/config/load.rs`:
- Around line 287-494: The code duplicates provider insertion logic in
load_from_env; remove the repeated blocks and consolidate into a single
provider-population helper (e.g., populate_providers) that fills llm.providers
using the existing add_shorthand_provider and the entry/or_insert_with patterns;
update load_from_env to call this helper (and also make from_toml call it) so
all provider keys (anthropic, openrouter, kilo, zhipu, zai-coding-plan,
opencode-zen, opencode-go, minimax, minimax-cn, openai) are added exactly once
to llm.providers; keep existing symbols like add_shorthand_provider,
load_from_env, from_toml and the ProviderConfig construction logic but move them
into the single helper to avoid duplication.
In `@src/config/toml_schema.rs`:
- Around line 546-611: TomlEmailConfig and TomlEmailInstanceConfig duplicate
nearly all fields; extract the shared fields into a new struct (e.g.,
TomlEmailCommon or TomlEmailFields) and embed it into both configs (use
#[serde(flatten)] to preserve the TOML shape) so TomlEmailConfig and
TomlEmailInstanceConfig keep only unique fields like instances and name; update
references to the moved fields accordingly (types/field names: enabled,
imap_host, imap_port, imap_username, imap_password, imap_use_tls, smtp_host,
smtp_port, smtp_username, smtp_password, smtp_use_starttls, from_address,
from_name, poll_interval_secs, folders, allowed_senders, max_body_bytes,
max_attachment_bytes).
ℹ️ Review info
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
src/config.rssrc/config/load.rssrc/config/onboarding.rssrc/config/permissions.rssrc/config/providers.rssrc/config/runtime.rssrc/config/toml_schema.rssrc/config/types.rssrc/config/watcher.rs
![tembo[bot]](https://avatars.githubusercontent.com/in/1223475?s=60&v=4){kind=small}
Incorporates changes from main into config submodules: - providers.rs: add X-Title legacy header for OpenRouter attribution - types.rs: derive PartialEq/Eq on OpenCodeConfig - load.rs: add warn_unknown_config_keys for unrecognised TOML keys - runtime.rs: ArcSwap opencode_server_pool + hot-reload logic - config.rs (tests): parking_lot mutex, ANTHROPIC_BASE_URL env guard, test isolation fixes, new warn_unknown_config_keys tests
This was referenced Mar 4, 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
1 participant
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
src/config.rsinto 8 focused submodules undersrc/config/, with no behavior changespub usere-exports from the module root — 127+ downstream importers are unaffectedModule layout
config.rstypes.rsConfig,AgentConfig,Binding, messaging configs, etc.)load.rsConfig::load,from_toml, validation, env/secret resolutiontoml_schema.rsToml*serde structs anddefault_*helperswatcher.rsspawn_file_watcherproviders.rsdefault_provider_config, routing bootstrappermissions.rsonboarding.rsrun_onboardingruntime.rsRuntimeConfigstruct + implVerification
just gate-prpasses (fmt, clippy, cargo check --all-targets, 301 lib tests, integration test compile)mod.rsfiles — usessrc/config.rsas module root per repo conventions