Add session config loader interface#18208
Conversation
| ThreadConfigSource::User(_) => {} | ||
| } | ||
| } | ||
| Ok(()) |
There was a problem hiding this comment.
not sure if this is the best way to actually inject the values, feels a bit dissatisfying that we're merging into two different locations
| .outgoing | ||
| .send_error(request_id, error) | ||
| .await; | ||
| return; |
There was a problem hiding this comment.
unclear to me what the correct error semantics are here
wiltzius-openai
left a comment
wiltzius-openai
left a comment
There was a problem hiding this comment.
seems reasonable to me!
| } | ||
| } | ||
|
|
||
| /// Derive the effective [`Config`] by layering three override sources. |
There was a problem hiding this comment.
think this "three" is up to "five" by now
| /// Config values owned by the service that starts or manages the session. | ||
| #[derive(Clone, Debug, Default, PartialEq)] | ||
| pub struct SessionThreadConfig { | ||
| pub model_provider: Option<String>, |
There was a problem hiding this comment.
should this also be a ModelProviderInfo?
There was a problem hiding this comment.
this is the pointer to a default model provider id
| @@ -406,8 +405,8 @@ impl InProcessClientStartArgs { | |||
| cli_overrides: self.cli_overrides, | |||
| loader_overrides: self.loader_overrides, | |||
| cloud_requirements: self.cloud_requirements, | |||
There was a problem hiding this comment.
this "cloud requirements" value being passed through the config stack is confusingly named -- I think this is like enterprise config requirements sent down from the cloud? But it seems likely we'll trip on it.
rasmusrygaard
force-pushed
the
dev/rasmus/session_config_loader_interface
branch
from
eaef62b to
7033cc3
Compare
rasmusrygaard
force-pushed
the
dev/rasmus/session_config_loader_interface
branch
from
7033cc3 to
88a3e85
Compare
|
|
||
| let thread_config_context = ThreadConfigContext { | ||
| thread_id: None, | ||
| cwd: cwd.as_ref().map(AbsolutePathBuf::to_path_buf), |
There was a problem hiding this comment.
use absolute path everywhere
rasmusrygaard
force-pushed
the
dev/rasmus/session_config_loader_interface
branch
from
2da432c to
8c0d098
Compare
# Conflicts: # codex-rs/config/src/lib.rs # Conflicts: # codex-rs/app-server/src/codex_message_processor.rs
rasmusrygaard
force-pushed
the
dev/rasmus/session_config_loader_interface
branch
from
8c0d098 to
97f28ff
Compare
3 participants
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
ThreadConfigLoaderand related typed payloads incodex-config.SessionThreadConfigcurrently supportsmodel_provider,model_providers, and feature flags.UserThreadConfigis present as an ownership boundary, but does not yet add TOML-backed fields.NoopThreadConfigLoaderpreserves existing behavior when no external loader is configured.StaticThreadConfigLoadersupports tests and simple callers.Taught thread config sources to produce ordinary
ConfigLayerEntryvalues so the existingConfigLayerStackremains 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:
Follow-Ups
This intentionally stops short of adding the remote/service transport. The next pieces are expected to be:
Verification
codex-configfor the loader and layer conversion.codex-coreconfig loader coverage for thread config layer precedence.