refactor(config): config consolidation for Configra#58
Conversation
Absorb the OKF (Ontology Knowledge Framework) manifest + wiki from KooshaPari/phenotype-config (originally at commit f86f8e9 on main, 2026-06-17). The OKF documents the project's governance artifacts (charter, intent, review) and intent provenance (cursor logs). Per ADR-031, all content migrates to Configra. What's added: - docs/phenotype-config-absorbed/okf/README.md: index with absorption header - docs/phenotype-config-absorbed/okf/manifest.okf.yaml: OKF manifest (paths updated to point at governance/ + intent/ subdirs) - docs/phenotype-config-absorbed/okf/wiki/README.md: LLM wiki placeholder Source: https://github.com/KooshaPari/phenotype-config/tree/f86f8e9/okf ADR: ADR-031 (Configra canonical config substrate) L5-110: T10.1 (Configra absorption) PR-#57
Replace hardcoded port (8080), log level ("info"), and idempotency
TTL (86400s) in pheno-config and settly with canonical defaults from
the configra-config meta-configuration crate.
Changes:
- Add configra-config dependency to pheno-config (Cargo.toml)
- Add configra-config dependency to settly (Cargo.toml)
- pheno-config/src/lib.rs: use ConfigraConfig::default() for
port and log_level defaults instead of hardcoded literals
- settly/src/adapters/idempotency.rs: use ConfigraConfig::default()
for idempotency TTL instead of hardcoded 86_400
- Add configra-config workspace member, example config files
(TOML + JSON), and CONFIG.md documentation
- Update pheno-config module docs to reference configra-config
as the source of defaults
All defaults remain semantically identical. No behavioral changes.
There was a problem hiding this comment.
Code Review
This pull request introduces the configra-config crate to centralize meta-configuration defaults (such as ports, log levels, and idempotency TTLs) across the workspace, updating pheno-config and settly to utilize these shared defaults. The review feedback highlights opportunities to improve robustness and performance: specifically, making ConfigraConfig::load fallible to prevent silently ignoring malformed configuration files, filtering out empty environment variables in from_env to ensure proper default fallbacks, and optimizing ConfigBuilder::new to avoid redundant default configuration instantiations and string clones.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
| pub fn load(path: Option<&Path>) -> Self { | ||
| match path { | ||
| Some(p) if p.exists() => Self::from_file(p).unwrap_or_default(), | ||
| _ => Self::from_env(), | ||
| } | ||
| } |
There was a problem hiding this comment.
The load function silently falls back to ConfigraConfig::default() if from_file fails (e.g., due to a syntax error in the TOML/JSON file). This can lead to extremely hard-to-debug issues in production where a malformed configuration file is silently ignored, and the application runs with default settings instead of the intended overrides. Consider providing a fallible try_load function that propagates the ConfigError, allowing the application to fail-fast on startup.
| pub fn load(path: Option<&Path>) -> Self { | |
| match path { | |
| Some(p) if p.exists() => Self::from_file(p).unwrap_or_default(), | |
| _ => Self::from_env(), | |
| } | |
| } | |
| pub fn load(path: Option<&Path>) -> Result<Self> { | |
| match path { | |
| Some(p) if p.exists() => Self::from_file(p), | |
| _ => Ok(Self::from_env()), | |
| } | |
| } |
| let default_log_level = | ||
| std::env::var(env_name("DEFAULT_LOG_LEVEL")).unwrap_or_else(|_| "info".to_owned()); | ||
|
|
||
| let db_path_template = std::env::var(env_name("DB_PATH_TEMPLATE")) | ||
| .unwrap_or_else(|_| "/var/lib/{name}.db".to_owned()); |
There was a problem hiding this comment.
If the environment variable CONFIGRA_DEFAULT_LOG_LEVEL or CONFIGRA_DB_PATH_TEMPLATE is set to an empty string, std::env::var will return Ok(""), and from_env() will use the empty string instead of falling back to the default value. In contrast, pheno-config explicitly filters out empty strings (using if !v.is_empty()). To ensure consistency and prevent empty configuration values, consider filtering out empty strings using .filter(|v| !v.is_empty()) before unwrapping.
let default_log_level = std::env::var(env_name("DEFAULT_LOG_LEVEL"))
.ok()
.filter(|v| !v.is_empty())
.unwrap_or_else(|| "info".to_owned());
let db_path_template = std::env::var(env_name("DB_PATH_TEMPLATE"))
.ok()
.filter(|v| !v.is_empty())
.unwrap_or_else(|| "/var/lib/{name}.db".to_owned());| pub fn new() -> Self { | ||
| Self { | ||
| url: None, | ||
| port: 8080, | ||
| log_level: String::from("info"), | ||
| port: ConfigraConfig::default().service.default_port, | ||
| log_level: ConfigraConfig::default().service.default_log_level.clone(), | ||
| db_path: None, | ||
| feature_flags: Vec::new(), | ||
| } |
There was a problem hiding this comment.
In ConfigBuilder::new(), ConfigraConfig::default() is called twice, which unnecessarily constructs the entire configuration structure twice and performs redundant heap allocations for the default strings. Additionally, default_log_level is cloned. By instantiating ConfigraConfig::default() once, you can avoid the duplicate construction and directly move the default_log_level string without cloning it.
| pub fn new() -> Self { | |
| Self { | |
| url: None, | |
| port: 8080, | |
| log_level: String::from("info"), | |
| port: ConfigraConfig::default().service.default_port, | |
| log_level: ConfigraConfig::default().service.default_log_level.clone(), | |
| db_path: None, | |
| feature_flags: Vec::new(), | |
| } | |
| pub fn new() -> Self { | |
| let defaults = ConfigraConfig::default(); | |
| Self { | |
| url: None, | |
| port: defaults.service.default_port, | |
| log_level: defaults.service.default_log_level, | |
| db_path: None, | |
| feature_flags: Vec::new(), | |
| } | |
| } |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 3c9780b263
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| - uses: actions/checkout@v4 | ||
|
|
||
| - name: Install MSRV toolchain | ||
| uses: dtolnay/rust-toolchain@1.75 |
There was a problem hiding this comment.
Use a toolchain that satisfies pheno-config's MSRV
In the new .github/workflows/ci.yml MSRV job, CI installs Rust 1.75 and then runs cargo check --workspace; I inspected the workspace manifests and crates/pheno-config/Cargo.toml declares rust-version = "1.82", so this job will fail for every PR/push that checks the full workspace before code is built. Either run this workspace check on 1.82 or exclude/split the crate that requires the newer compiler.
Useful? React with 👍 / 👎.
| tracing = { workspace = true, optional = true } | ||
| # Meta-configuration: pull default ports, log levels, and other | ||
| # previously-hardcoded values from configra-config. | ||
| configra-config = { path = "../configra-config" } |
There was a problem hiding this comment.
Add a registry version for configra-config
This new dependency is path-only in a publishable crate, while the added release workflow publishes pheno-config; I verified the packaging semantics with cargo package -p pheno-config --allow-dirty --no-verify, which aborts with all dependencies must have a version requirement specified when packaging because Cargo removes path specs for packaged dependencies. The same path-only dependency was added to settly, so tag releases cannot publish these crates unless configra-config gets a version requirement and is published before its dependents.
Useful? React with 👍 / 👎.
| /// - [`ConfigraConfig::from_file()`] — TOML or JSON file | ||
| /// - [`ConfigraConfigBuilder`] — programmatic construction | ||
| #[derive(Debug, Clone, Serialize, Deserialize)] | ||
| pub struct ConfigraConfig { |
There was a problem hiding this comment.
Default missing top-level config sections
Partial TOML/JSON files are documented and tested as valid, but #[serde(default)] is only on the nested section structs; missing root fields like idempotency and watcher are still required during ConfigraConfig::from_file deserialization. A file containing only [service] default_port = 5000 therefore errors with a missing top-level section instead of filling defaults, so add defaults on the root struct/fields as well.
Useful? React with 👍 / 👎.
feat: continues the ADR-031 migration gates work
Stage 1 config consolidation: extracted hardcoded config to configra-config crate
Summary
Replaces hardcoded configuration defaults (port
8080, log level"info", idempotency TTL86400s) with canonical defaults sourced from the newconfigra-configmeta-configuration crate.Changes
New:
crates/configra-config/ConfigraConfigstruct with three sub-config groups:ServiceConfig,IdempotencyConfig,WatcherConfigCONFIGRA_*prefix), config files (TOML/JSON), or programmatic builderconfigra.example.toml,configra.example.jsonModified:
crates/pheno-config/src/lib.rsload_from_env()now usesConfigraConfig::default().service.default_port/.default_log_levelinstead of hardcoded8080/"info"ConfigBuilder::new()same treatmentModified:
crates/settly/src/adapters/idempotency.rsInMemoryIdempotencyStore::default_ttl()usesConfigraConfig::default().idempotency.default_ttl_secsinstead of hardcoded86_400Documentation
docs/CONFIG.md— full reference of every config key, its type, default, env var, and originOther
configra-configexamples/cascade.rs,quickstart.rs,validation.rs) modernized for the current APIVerification
cargo check --workspacepasses. All defaults remain semantically identical — no behavioral changes.Closes config consolidation phase (ADR-031 follow-up).