mirror of
https://github.com/pchuan98/codex.git
synced 2026-07-01 00:31:56 +08:00
Add session config loader interface (#18208)
## Why Cloud-hosted sessions need a way for the service that starts or manages a thread to provide session-owned config without treating all config as if it came from the same user/project/workspace TOML stack. The important boundary is ownership: some values should be controlled by the session/orchestrator, some by the authenticated user, and later some may come from the executor. The earlier broad config-store shape made that boundary too fuzzy and overlapped heavily with the existing filesystem-backed config loader. This PR starts with the smaller piece we need now: a typed session config loader that can feed the existing config layer stack while preserving the normal precedence and merge behavior. ## What Changed - Added `ThreadConfigLoader` and related typed payloads in `codex-config`. - `SessionThreadConfig` currently supports `model_provider`, `model_providers`, and feature flags. - `UserThreadConfig` is present as an ownership boundary, but does not yet add TOML-backed fields. - `NoopThreadConfigLoader` preserves existing behavior when no external loader is configured. - `StaticThreadConfigLoader` supports tests and simple callers. - Taught thread config sources to produce ordinary `ConfigLayerEntry` values so the existing `ConfigLayerStack` remains the place where precedence and merging happen. - Wired the loader through `ConfigBuilder`, the config loader, and app-server startup paths so app-server can provide session-owned config before deriving a thread config. - Added coverage for: - translating typed thread config into config layers, - inserting thread config layers into the stack at the right precedence, - applying session-provided model provider and feature settings when app-server derives config from thread params. ## Follow-Ups This intentionally stops short of adding the remote/service transport. The next pieces are expected to be: 1. Define the proto/API shape for this interface. 2. Add a client implementation that can source session config from the service side. ## Verification - Added unit coverage in `codex-config` for the loader and layer conversion. - Added `codex-core` config loader coverage for thread config layer precedence. - Added app-server coverage that verifies session thread config wins over request-provided config for model provider and feature settings.
This commit is contained in:
committed by
GitHub
Unverified
parent
513dc28717
commit
7b994100b3
@@ -10,7 +10,7 @@ This module is the canonical place to **load and describe Codex configuration la
|
||||
|
||||
Exported from `codex_core::config_loader`:
|
||||
|
||||
- `load_config_layers_state(fs, codex_home, cwd_opt, cli_overrides, overrides, cloud_requirements) -> ConfigLayerStack`
|
||||
- `load_config_layers_state(fs, codex_home, cwd_opt, cli_overrides, overrides, cloud_requirements, thread_config_loader) -> ConfigLayerStack`
|
||||
- `ConfigLayerStack`
|
||||
- `effective_config() -> toml::Value`
|
||||
- `origins() -> HashMap<String, ConfigLayerMetadata>`
|
||||
@@ -29,6 +29,9 @@ Precedence is **top overrides bottom**:
|
||||
3. **Session flags** (CLI overrides, applied as dotted-path TOML writes)
|
||||
4. **User** config (`config.toml`)
|
||||
|
||||
Thread config entries supplied by `thread_config_loader` are inserted according
|
||||
to their translated `ConfigLayerSource` precedence.
|
||||
|
||||
Layers with a `disabled_reason` are still surfaced for UI, but are ignored when
|
||||
computing the effective config and origins metadata. This is what
|
||||
`ConfigLayerStack::effective_config()` implements.
|
||||
@@ -41,6 +44,7 @@ Most callers want the effective config plus metadata:
|
||||
use codex_core::config_loader::{
|
||||
CloudRequirementsLoader, LoaderOverrides, load_config_layers_state,
|
||||
};
|
||||
use codex_config::NoopThreadConfigLoader;
|
||||
use codex_exec_server::LOCAL_FS;
|
||||
use codex_utils_absolute_path::AbsolutePathBuf;
|
||||
use toml::Value as TomlValue;
|
||||
@@ -54,6 +58,7 @@ let layers = load_config_layers_state(
|
||||
&cli_overrides,
|
||||
LoaderOverrides::default(),
|
||||
CloudRequirementsLoader::default(),
|
||||
&NoopThreadConfigLoader,
|
||||
).await?;
|
||||
|
||||
let effective = layers.effective_config();
|
||||
|
||||
@@ -9,6 +9,8 @@ use crate::config_loader::layer_io::LoadedConfigLayers;
|
||||
use codex_app_server_protocol::ConfigLayerSource;
|
||||
use codex_config::CONFIG_TOML_FILE;
|
||||
use codex_config::ConfigRequirementsWithSources;
|
||||
use codex_config::ThreadConfigContext;
|
||||
use codex_config::ThreadConfigLoader;
|
||||
use codex_config::config_toml::ConfigToml;
|
||||
use codex_config::config_toml::ProjectConfig;
|
||||
use codex_exec_server::ExecutorFileSystem;
|
||||
@@ -127,6 +129,7 @@ pub async fn load_config_layers_state(
|
||||
cli_overrides: &[(String, TomlValue)],
|
||||
overrides: LoaderOverrides,
|
||||
cloud_requirements: CloudRequirementsLoader,
|
||||
thread_config_loader: &dyn ThreadConfigLoader,
|
||||
) -> io::Result<ConfigLayerStack> {
|
||||
let ignore_user_config = overrides.ignore_user_config;
|
||||
let ignore_user_and_project_exec_policy_rules =
|
||||
@@ -161,6 +164,15 @@ pub async fn load_config_layers_state(
|
||||
)
|
||||
.await?;
|
||||
|
||||
let thread_config_context = ThreadConfigContext {
|
||||
thread_id: None,
|
||||
cwd: cwd.clone(),
|
||||
};
|
||||
let thread_config_layers = thread_config_loader
|
||||
.load_config_layers(thread_config_context)
|
||||
.await
|
||||
.map_err(io::Error::other)?;
|
||||
|
||||
let mut layers = Vec::<ConfigLayerEntry>::new();
|
||||
|
||||
let cli_overrides_layer = if cli_overrides.is_empty() {
|
||||
@@ -283,6 +295,10 @@ pub async fn load_config_layers_state(
|
||||
));
|
||||
}
|
||||
|
||||
for thread_config_layer in thread_config_layers {
|
||||
insert_layer_by_precedence(&mut layers, thread_config_layer);
|
||||
}
|
||||
|
||||
// Make a best-effort to support the legacy `managed_config.toml` as a
|
||||
// config layer on top of everything else. For fields in
|
||||
// `managed_config.toml` that do not have an equivalent in
|
||||
@@ -332,6 +348,16 @@ pub async fn load_config_layers_state(
|
||||
.with_user_and_project_exec_policy_rules_ignored(ignore_user_and_project_exec_policy_rules))
|
||||
}
|
||||
|
||||
fn insert_layer_by_precedence(layers: &mut Vec<ConfigLayerEntry>, layer: ConfigLayerEntry) {
|
||||
match layers
|
||||
.iter()
|
||||
.position(|existing| existing.name.precedence() > layer.name.precedence())
|
||||
{
|
||||
Some(index) => layers.insert(index, layer),
|
||||
None => layers.push(layer),
|
||||
}
|
||||
}
|
||||
|
||||
/// Attempts to load a config.toml file from `config_toml`.
|
||||
/// - If the file exists and is valid TOML, passes the parsed `toml::Value` to
|
||||
/// `create_entry` and returns the resulting layer entry.
|
||||
|
||||
@@ -15,6 +15,9 @@ use crate::config_loader::RequirementSource;
|
||||
use crate::config_loader::load_requirements_toml;
|
||||
use crate::config_loader::version_for_toml;
|
||||
use codex_config::CONFIG_TOML_FILE;
|
||||
use codex_config::SessionThreadConfig;
|
||||
use codex_config::StaticThreadConfigLoader;
|
||||
use codex_config::ThreadConfigSource;
|
||||
use codex_config::config_toml::ConfigToml;
|
||||
use codex_config::config_toml::ProjectConfig;
|
||||
use codex_exec_server::LOCAL_FS;
|
||||
@@ -100,6 +103,7 @@ async fn returns_config_error_for_invalid_user_config_toml() {
|
||||
&[] as &[(String, TomlValue)],
|
||||
LoaderOverrides::default(),
|
||||
CloudRequirementsLoader::default(),
|
||||
&codex_config::NoopThreadConfigLoader,
|
||||
)
|
||||
.await
|
||||
.expect_err("expected error");
|
||||
@@ -131,6 +135,7 @@ async fn ignore_user_config_keeps_empty_user_layer() -> std::io::Result<()> {
|
||||
..Default::default()
|
||||
},
|
||||
CloudRequirementsLoader::default(),
|
||||
&codex_config::NoopThreadConfigLoader,
|
||||
)
|
||||
.await?;
|
||||
|
||||
@@ -161,6 +166,7 @@ async fn ignore_rules_marks_config_stack_for_exec_policy_rule_skip() -> std::io:
|
||||
..Default::default()
|
||||
},
|
||||
CloudRequirementsLoader::default(),
|
||||
&codex_config::NoopThreadConfigLoader,
|
||||
)
|
||||
.await?;
|
||||
|
||||
@@ -185,6 +191,7 @@ async fn returns_config_error_for_invalid_managed_config_toml() {
|
||||
&[] as &[(String, TomlValue)],
|
||||
overrides,
|
||||
CloudRequirementsLoader::default(),
|
||||
&codex_config::NoopThreadConfigLoader,
|
||||
)
|
||||
.await
|
||||
.expect_err("expected error");
|
||||
@@ -270,6 +277,7 @@ extra = true
|
||||
&[] as &[(String, TomlValue)],
|
||||
overrides,
|
||||
CloudRequirementsLoader::default(),
|
||||
&codex_config::NoopThreadConfigLoader,
|
||||
)
|
||||
.await
|
||||
.expect("load config");
|
||||
@@ -303,6 +311,7 @@ async fn returns_empty_when_all_layers_missing() {
|
||||
&[] as &[(String, TomlValue)],
|
||||
overrides,
|
||||
CloudRequirementsLoader::default(),
|
||||
&codex_config::NoopThreadConfigLoader,
|
||||
)
|
||||
.await
|
||||
.expect("load layers");
|
||||
@@ -354,6 +363,56 @@ async fn returns_empty_when_all_layers_missing() {
|
||||
}
|
||||
}
|
||||
|
||||
#[tokio::test]
|
||||
async fn includes_thread_config_layers_in_stack() -> anyhow::Result<()> {
|
||||
let tmp = tempdir()?;
|
||||
let cwd_dir = tmp.path().join("project");
|
||||
tokio::fs::create_dir_all(&cwd_dir).await?;
|
||||
let cwd = AbsolutePathBuf::from_absolute_path(&cwd_dir)?;
|
||||
let layers = load_config_layers_state(
|
||||
LOCAL_FS.as_ref(),
|
||||
tmp.path(),
|
||||
Some(cwd),
|
||||
&[("features.plugins".to_string(), TomlValue::Boolean(true))],
|
||||
LoaderOverrides::without_managed_config_for_tests(),
|
||||
CloudRequirementsLoader::default(),
|
||||
&StaticThreadConfigLoader::new(vec![ThreadConfigSource::Session(SessionThreadConfig {
|
||||
features: BTreeMap::from([("plugins".to_string(), false)]),
|
||||
..Default::default()
|
||||
})]),
|
||||
)
|
||||
.await?;
|
||||
|
||||
let layer_sources = layers
|
||||
.layers_high_to_low()
|
||||
.into_iter()
|
||||
.map(|layer| layer.name.clone())
|
||||
.collect::<Vec<_>>();
|
||||
assert_eq!(
|
||||
layer_sources,
|
||||
vec![
|
||||
super::ConfigLayerSource::SessionFlags,
|
||||
super::ConfigLayerSource::SessionFlags,
|
||||
super::ConfigLayerSource::User {
|
||||
file: AbsolutePathBuf::resolve_path_against_base(CONFIG_TOML_FILE, tmp.path()),
|
||||
},
|
||||
super::ConfigLayerSource::System {
|
||||
file: super::system_config_toml_file()?,
|
||||
},
|
||||
]
|
||||
);
|
||||
assert_eq!(
|
||||
layers
|
||||
.effective_config()
|
||||
.get("features")
|
||||
.and_then(TomlValue::as_table)
|
||||
.and_then(|features| features.get("plugins")),
|
||||
Some(&TomlValue::Boolean(false))
|
||||
);
|
||||
|
||||
Ok(())
|
||||
}
|
||||
|
||||
#[cfg(target_os = "macos")]
|
||||
#[tokio::test]
|
||||
async fn managed_preferences_take_highest_precedence() {
|
||||
@@ -396,6 +455,7 @@ flag = false
|
||||
&[] as &[(String, TomlValue)],
|
||||
overrides,
|
||||
CloudRequirementsLoader::default(),
|
||||
&codex_config::NoopThreadConfigLoader,
|
||||
)
|
||||
.await
|
||||
.expect("load config");
|
||||
@@ -498,6 +558,7 @@ allowed_sandbox_modes = ["read-only"]
|
||||
&[] as &[(String, TomlValue)],
|
||||
loader_overrides,
|
||||
CloudRequirementsLoader::default(),
|
||||
&codex_config::NoopThreadConfigLoader,
|
||||
)
|
||||
.await?;
|
||||
|
||||
@@ -560,6 +621,7 @@ allowed_approval_policies = ["never"]
|
||||
&[] as &[(String, TomlValue)],
|
||||
loader_overrides,
|
||||
CloudRequirementsLoader::default(),
|
||||
&codex_config::NoopThreadConfigLoader,
|
||||
)
|
||||
.await?;
|
||||
|
||||
@@ -713,6 +775,7 @@ allowed_approval_policies = ["on-request"]
|
||||
guardian_policy_config: None,
|
||||
}))
|
||||
}),
|
||||
&codex_config::NoopThreadConfigLoader,
|
||||
)
|
||||
.await?;
|
||||
|
||||
@@ -931,6 +994,7 @@ async fn load_config_layers_includes_cloud_requirements() -> anyhow::Result<()>
|
||||
&[] as &[(String, TomlValue)],
|
||||
LoaderOverrides::default(),
|
||||
cloud_requirements,
|
||||
&codex_config::NoopThreadConfigLoader,
|
||||
)
|
||||
.await?;
|
||||
|
||||
@@ -974,6 +1038,7 @@ async fn load_config_layers_fails_when_cloud_requirements_loader_fails() -> anyh
|
||||
"cloud requirements failed",
|
||||
))
|
||||
}),
|
||||
&codex_config::NoopThreadConfigLoader,
|
||||
)
|
||||
.await
|
||||
.expect_err("cloud requirements failure should fail closed");
|
||||
@@ -1021,6 +1086,7 @@ async fn project_layers_prefer_closest_cwd() -> std::io::Result<()> {
|
||||
&[] as &[(String, TomlValue)],
|
||||
LoaderOverrides::default(),
|
||||
CloudRequirementsLoader::default(),
|
||||
&codex_config::NoopThreadConfigLoader,
|
||||
)
|
||||
.await?;
|
||||
|
||||
@@ -1166,6 +1232,7 @@ async fn project_layer_is_added_when_dot_codex_exists_without_config_toml() -> s
|
||||
&[] as &[(String, TomlValue)],
|
||||
LoaderOverrides::default(),
|
||||
CloudRequirementsLoader::default(),
|
||||
&codex_config::NoopThreadConfigLoader,
|
||||
)
|
||||
.await?;
|
||||
|
||||
@@ -1206,6 +1273,7 @@ async fn codex_home_is_not_loaded_as_project_layer_from_home_dir() -> std::io::R
|
||||
&[] as &[(String, TomlValue)],
|
||||
LoaderOverrides::default(),
|
||||
CloudRequirementsLoader::default(),
|
||||
&codex_config::NoopThreadConfigLoader,
|
||||
)
|
||||
.await?;
|
||||
|
||||
@@ -1263,6 +1331,7 @@ async fn codex_home_within_project_tree_is_not_double_loaded() -> std::io::Resul
|
||||
&[] as &[(String, TomlValue)],
|
||||
LoaderOverrides::default(),
|
||||
CloudRequirementsLoader::default(),
|
||||
&codex_config::NoopThreadConfigLoader,
|
||||
)
|
||||
.await?;
|
||||
|
||||
@@ -1334,6 +1403,7 @@ async fn project_layers_disabled_when_untrusted_or_unknown() -> std::io::Result<
|
||||
&[] as &[(String, TomlValue)],
|
||||
LoaderOverrides::default(),
|
||||
CloudRequirementsLoader::default(),
|
||||
&codex_config::NoopThreadConfigLoader,
|
||||
)
|
||||
.await?;
|
||||
let project_layers_untrusted: Vec<_> = layers_untrusted
|
||||
@@ -1373,6 +1443,7 @@ async fn project_layers_disabled_when_untrusted_or_unknown() -> std::io::Result<
|
||||
&[] as &[(String, TomlValue)],
|
||||
LoaderOverrides::default(),
|
||||
CloudRequirementsLoader::default(),
|
||||
&codex_config::NoopThreadConfigLoader,
|
||||
)
|
||||
.await?;
|
||||
let project_layers_unknown: Vec<_> = layers_unknown
|
||||
@@ -1439,6 +1510,7 @@ async fn project_trust_does_not_match_configured_alias_for_canonical_cwd() -> st
|
||||
&[] as &[(String, TomlValue)],
|
||||
LoaderOverrides::default(),
|
||||
CloudRequirementsLoader::default(),
|
||||
&codex_config::NoopThreadConfigLoader,
|
||||
)
|
||||
.await?;
|
||||
|
||||
@@ -1592,6 +1664,7 @@ async fn invalid_project_config_ignored_when_untrusted_or_unknown() -> std::io::
|
||||
&[] as &[(String, TomlValue)],
|
||||
LoaderOverrides::default(),
|
||||
CloudRequirementsLoader::default(),
|
||||
&codex_config::NoopThreadConfigLoader,
|
||||
)
|
||||
.await?;
|
||||
let project_layers: Vec<_> = layers
|
||||
@@ -1660,6 +1733,7 @@ async fn project_layer_without_config_toml_is_disabled_when_untrusted_or_unknown
|
||||
&[] as &[(String, TomlValue)],
|
||||
LoaderOverrides::default(),
|
||||
CloudRequirementsLoader::default(),
|
||||
&codex_config::NoopThreadConfigLoader,
|
||||
)
|
||||
.await?;
|
||||
let project_layers: Vec<_> = layers
|
||||
@@ -1720,6 +1794,7 @@ async fn cli_overrides_with_relative_paths_do_not_break_trust_check() -> std::io
|
||||
&cli_overrides,
|
||||
LoaderOverrides::default(),
|
||||
CloudRequirementsLoader::default(),
|
||||
&codex_config::NoopThreadConfigLoader,
|
||||
)
|
||||
.await?;
|
||||
|
||||
@@ -1763,6 +1838,7 @@ async fn project_root_markers_supports_alternate_markers() -> std::io::Result<()
|
||||
&[] as &[(String, TomlValue)],
|
||||
LoaderOverrides::default(),
|
||||
CloudRequirementsLoader::default(),
|
||||
&codex_config::NoopThreadConfigLoader,
|
||||
)
|
||||
.await?;
|
||||
|
||||
|
||||
Reference in New Issue
Block a user