feat: add config structs for services with inline env var config#1205
Conversation
geoffjay
left a comment
There was a problem hiding this comment.
Review: feat: add config structs for services with inline env var config
Stack Position
This PR sits at the top of a blocked stack:
This PR cannot merge until the chain below it is resolved. The blocking issues in #1201 (AGENTD_HISTORY_SIZE collision, incomplete env var table, orchestrator rename) and the missing-test issue in #1203 propagate upward.
Blocking Issues
1. Silent config-file errors via unwrap_or_default() — all six new config.rs files
Same issue flagged in #1204. Every service's load() calls:
let shared = agentd_common::config::load().unwrap_or_default();If the TOML config file is present but malformed, this silently returns Default::default() — the operator gets no error message, no log line, and no indication that their config was ignored. This will be extremely painful to debug in production.
Required fix in all six files (ask, communicate, core, notify, orchestrator, wrap):
let shared = agentd_common::config::load().unwrap_or_else(|e| {
tracing::warn!(
"failed to load config file, using compiled defaults: {e:#}"
);
agentd_common::config::AgentdConfig::default()
});2. Diagnostic regression in crates/wrap/src/main.rs
The old main.rs had three explicit warn!() calls for invalid env var values:
warn!("AGENTD_WRAP_HISTORY_BYTES={:?} is not a valid usize; using default {} bytes", raw, DEFAULT_HISTORY_BYTES);
warn!("AGENTD_WRAP_CHANNEL_CAPACITY={:?} is not a valid usize; using default {}", raw, DEFAULT_CHANNEL_CAPACITY);
warn!("AGENTD_WRAP_CHANNEL_CAPACITY=0 is invalid; clamped to 1");These were deleted in this PR but not moved into WrapConfig::load(). The new load() silently clamps or defaults bad values without telling the operator. The test test_channel_capacity_zero_clamped_to_one is a good sign — but a test passing silently doesn't help an operator who set AGENTD_WRAP_CHANNEL_CAPACITY=abc and wonders why their config was ignored.
Required fix — add warn! in WrapConfig::load() for each fallback:
// history_bytes
let history_bytes = std::env::var("AGENTD_WRAP_HISTORY_BYTES")
.ok()
.and_then(|v| {
v.parse::<usize>().map_err(|_| {
tracing::warn!(
"AGENTD_WRAP_HISTORY_BYTES={v:?} is not a valid usize; using default {} bytes",
DEFAULT_HISTORY_BYTES
);
}).ok()
})
.unwrap_or(base.history_bytes);
// channel_capacity (with clamp-to-1 warn)
let channel_capacity = std::env::var("AGENTD_WRAP_CHANNEL_CAPACITY")
.ok()
.and_then(|v| {
v.parse::<usize>().map_err(|_| {
tracing::warn!(
"AGENTD_WRAP_CHANNEL_CAPACITY={v:?} is not a valid usize; using default {}",
DEFAULT_CHANNEL_CAPACITY
);
}).ok()
})
.unwrap_or(base.channel_capacity);
let channel_capacity = if channel_capacity == 0 {
tracing::warn!("AGENTD_WRAP_CHANNEL_CAPACITY=0 is invalid; clamped to 1");
1
} else {
channel_capacity
};Non-Blocking Suggestions
crates/wrap/src/config.rs — TODO comments on hardcoded defaults
WrapConfig::load() falls back to DEFAULT_HISTORY_BYTES and DEFAULT_CHANNEL_CAPACITY constants rather than the shared schema fields (base.history_bytes, base.channel_capacity). This is expected given the schema gaps noted in #1201 (WrapConfig in shared schema is missing these fields). Leave a // TODO: use base.history_bytes once WrapConfig schema is complete comment so the gap is trackable.
crates/orchestrator/src/config.rs — unused port variable
There is a let port = ... binding that appears to be unused — it's computed but not stored in the returned OrchestratorConfig. Either include it in the struct or remove it to silence the dead-code warning.
What's Working Well
- All test modules correctly acquire
ENV_LOCKbefore mutating env vars - Save/restore/remove pattern is applied consistently: env vars are removed before the assert so a panic during setup doesn't leak state into other tests
- The
test_channel_capacity_zero_clamped_to_onetest in wrap is a nice edge-case addition - Service-local
config.rsmodules are a clean separation frommain.rsconfig wiring
Summary
Two blocking issues must be fixed before merge:
- All six
load()functions needunwrap_or_elsewith atracing::warn!instead of silentunwrap_or_default() WrapConfig::load()must restore the diagnosticwarn!()calls that were removed fromwrap/main.rs
Add a ValidateConfig trait to agentd_common with implementations for all 12 shared config sections. Add AgentdConfig::validate() that collects errors from every section. Implement ValidateConfig on all service-level config structs and wire validate() into each service's startup path so misconfigurations fail early with descriptive error messages. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Create dedicated config.rs modules for core, orchestrator, wrap, ask, notify, and communicate. Each struct loads from agentd_common::config::load() first, then overlays legacy AGENTD_* env vars for backward compatibility. All inline env::var() calls in main.rs replaced with config struct access. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
- Add impl ValidateConfig for EmbeddingConfig and LanceConfig in crates/memory/src/config.rs (blocking gap: memory was the only service with no validation at startup) - Wire validation in memory/main.rs: call validate() on embedding and lance configs, load shared config for port/host so port=0 is caught before any I/O with a clear error message - Remove validate_inner() indirection in crates/index/src/config.rs; move logic directly into impl ValidateConfig for IndexConfig and import the trait in index/main.rs and the test module - Make validate_url() pub in agentd-common so service crates can reuse it without duplicating the http/https check Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…ilures
Replace silent unwrap_or_default() with unwrap_or_else(|e| { tracing::warn\!(...) })
in all six new service config loaders (ask, communicate, core, notify, orchestrator,
wrap). Restore diagnostic warn\!() calls for invalid AGENTD_WRAP_HISTORY_BYTES and
AGENTD_WRAP_CHANNEL_CAPACITY env vars with TODO comments referencing #1201.
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
- Create docs/configuration.md with quick start, file location, precedence rules, complete key reference table, per-service sections, migration guide, and four example configs (minimal, development, production, Docker) - Fix the env var table in agentd-common/config.rs to include all overrides (AGENTD_INDEX_LANGUAGES, AGENTD_RECONCILE_INTERVAL_SECS, all MCP URLs, AGENTD_NOTIFY_SERVICE_URL, AGENTD_COLLECTION_INTERVAL_SECS) - Add link to configuration reference from README.md Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
|
This change is part of the following stack:
Change managed by git-spice. |
docs: add configuration system documentation
feat: add ValidateConfig trait and per-service validation
Creates dedicated
config.rsmodules for the six services that previously had all configuration read inline inmain.rs: core, orchestrator, wrap, ask, notify, and communicate.Each config struct follows the same pattern as the services migrated in #1204:
agentd_common::config::load()AGENTD_*environment variables for backward compatibilityenv::var()calls inmain.rswith config struct field accessThe
wrapandorchestratorbackends continue to useBackendType::from_env_strict()for backend selection — that function already encapsulatesAGENTD_BACKENDparsing with proper error messages.Closes #1197