Make local environment optional in EnvironmentManager#23369
Conversation
starr-openai
force-pushed
the
starr/optional-local-env-cand1
branch
from
fef203d to
51e12c7
Compare
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 49c3495b60
ℹ️ About Codex in GitHub
Your team has set up Codex to 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 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
jif-oai
left a comment
jif-oai
left a comment
There was a problem hiding this comment.
Don't we mix a bit everything here? I feel like this PR is collapsion “is local selectable?”, “which env is selected/default?”, and “does this process still have host-local capability at all?”
Is it on purpose? I get that this is a temporary thing (right?)
I'm a bit worried on the blast radius. Are we sure no prod clients + training won't be impacted?
This isn't temporary - if anything the current code is the temporary path due to the 'fallback to local' paths. The purpose is to make 'local' environment truly optional, and if unset remove these fallback paths. Likewise, we want to shift towards no reliance on a 'default' environment, and instead always have a turn chosen or model selected (via tool call params) environment. The startup configuration (environments.toml or |
|
Prod clients should be quite safe, since local env is enabled by default and we don't have any real remote client users yet. |
| )?; | ||
| let environment_manager = if loader_overrides.ignore_user_config { | ||
| EnvironmentManager::from_env(local_runtime_paths).await | ||
| EnvironmentManager::from_env(Some(local_runtime_paths)).await |
There was a problem hiding this comment.
I don't hink local_runtime_paths should be optional. This is just wiring for sandbox when local runtime is available. It shouldn't be the signal on whether the local runtime is available or not.
There was a problem hiding this comment.
^ local_runtime_paths is only ever used to construct the local environment. If there is no local env, we don't need local_runtime_paths.
| } | ||
|
|
||
| impl MessageProcessor { | ||
| fn fs_processor(&self) -> Result<&FsRequestProcessor, JSONRPCErrorError> { |
There was a problem hiding this comment.
As in why do we need to make fs_processor conditional today?
There was a problem hiding this comment.
if no local env, we should disable app-server fs/* apis
| pub fn require_local_environment(&self) -> Arc<Environment> { | ||
| match self.try_local_environment() { | ||
| Some(environment) => environment, | ||
| None => panic!("local environment is not configured"), |
There was a problem hiding this comment.
we should never ever panic.
| HashMap::with_capacity(environments.len() + usize::from(include_local)); | ||
| let local_environment = Arc::new(Environment::local(local_runtime_paths)); | ||
| if include_local { | ||
| let local_environment = if include_local { |
There was a problem hiding this comment.
I don't like how many ways to control the presence of local environment there is.
I'm even more convinced that EnvironmentProvider should be responsible for returning an instance of local Environment when it has it and local_environment method should just be a filter over the list of available environments.
There was a problem hiding this comment.
If EnvironmentProvider creates local, then we need to either use a convention (e.g. 'local' environment id) or another param (e.g. 'is_local') on the return here to indicate which is which.
However, this is still clumsy - since now the creation and meaning of 'local' can vary across providers when 'local' env should always refer to the same thing regardless.
It's possible we wouldn't need to have 'local_environment' on EnvironmentManager at all if we provide a different way to access it for internal use-cases - e.g. the RuntimeCapabilities struct I was proposing yesterday.
There was a problem hiding this comment.
another param (e.g. 'is_local')
We already have is_remote don't we?
| } | ||
| // `thread/shellCommand` is app-server's local-host shell escape hatch, | ||
| // not the normal turn-selected shell tool path. | ||
| if self |
There was a problem hiding this comment.
yes - thread/shellCommand cannot run without a local env
| } | ||
|
|
||
| #[cfg(test)] | ||
| mod tests { |
There was a problem hiding this comment.
is it better for these to be integration tests?
| let config = match server.launch() { | ||
| McpServerLaunch::Configured(config) => config.as_ref().clone(), | ||
| }; | ||
| if let Some(reason) = runtime_environment.startup_unavailable_reason(server_name, &config) { |
There was a problem hiding this comment.
we have 2 places that interpret experimental_environment.
should startup_unavailable_reason return an environment to run on or an error?
we can remove extra defensive checks below.
There was a problem hiding this comment.
we have 2 places that interpret experimental_environment.
we honestly need to kill this and change the semantics here. MCP servers shouldn't be bool on/off remote, but should be made per env
3 participants
Summary
EnvironmentManagerlocal environment/runtime paths optionalrequire_local_environment/try_local_environmentValidation
//codex-rs/exec-server:exec-server-unit-tests//codex-rs/app-server-client:app-server-client-unit-tests//codex-rs/core:core-unit-testscases