-
Notifications
You must be signed in to change notification settings - Fork 7.1k
[app-server] fix config loading for conversations #8765
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
owenlin0
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
approving to get --config flags working for app-server, but +1 to unifying config code / making it easier to follow
owenlin0
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
btw the fix ultimately was that we never read &self.cli_overrides from codex_message_processor? nice find!
yup yup! I agree it's confusing because cli_overrides is an overloaded term and easy to miss when we are not passing in all the overrides. will rename first to make it clearer |
bolinfest
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think some terminology needs to be tightened up here. The things that are currently called harness_overrides are not defined by the harness but a NewConversationParams, so I think some other name is in order.
726dc32 to
fa76165
Compare
fa76165 to
020784d
Compare
|
Lots of changes on this recently so I rely on @.bolinfest for this |
020784d to
15774aa
Compare
15774aa to
5027fd7
Compare
599d690 to
1d69d0d
Compare
1d69d0d to
3a4a023
Compare
bolinfest
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ok, let's go with the typesafe_overrides thing and this is good by me: thanks for fixing!
Co-authored-by: Michael Bolin <[email protected]>
a286130 to
ad4ba98
Compare
Currently we don't load config properly for app server conversations. see: https://linear.app/openai/issue/CODEX-3956/config-flags-not-respected-in-codex-app-server. This PR fixes that by respecting the config passed in.
Tested by running
cargo build -p codex-cli && RUST_LOG=codex_app_server=debug CODEX_BIN=target/debug/codex cargo run -p codex-app-server-test-client -- \ --config model_providers.mock_provider.base_url=\"http://localhost:4010/v2\" \ --config model_provider=\"mock_provider\" \ --config model_providers.mock_provider.name="hello" \ send-message-v2 "hello"and verified that the mock_provider is called instead of default provider.
#closes https://linear.app/openai/issue/CODEX-3956/config-flags-not-respected-in-codex-app-server