-
Notifications
You must be signed in to change notification settings - Fork 1.9k
fix(app-server): pass -c model="..." to codex app-server so options.model takes effect
#408
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
base: main
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -604,10 +604,10 @@ async function captureTurn(client, threadId, startRequest, options = {}) { | |
| } | ||
| } | ||
|
|
||
| async function withAppServer(cwd, fn) { | ||
| async function withAppServer(cwd, fn, clientOptions = {}) { | ||
| let client = null; | ||
| try { | ||
| client = await CodexAppServerClient.connect(cwd); | ||
| client = await CodexAppServerClient.connect(cwd, clientOptions); | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
When no shared session exists, this call enters the default broker path before any direct app-server is spawned. Useful? React with 👍 / 👎. |
||
| const result = await fn(client); | ||
| await client.close(); | ||
| return result; | ||
|
|
@@ -626,7 +626,7 @@ async function withAppServer(cwd, fn) { | |
| throw error; | ||
| } | ||
|
|
||
| const directClient = await CodexAppServerClient.connect(cwd, { disableBroker: true }); | ||
| const directClient = await CodexAppServerClient.connect(cwd, { ...clientOptions, disableBroker: true }); | ||
| try { | ||
| return await fn(directClient); | ||
| } finally { | ||
|
|
@@ -823,7 +823,7 @@ export async function runAppServerReview(cwd, options = {}) { | |
| error: turnState.error, | ||
| stderr: cleanCodexStderr(client.stderr) | ||
| }; | ||
| }); | ||
| }, { model: options.model }); | ||
| } | ||
|
|
||
| export async function runAppServerTurn(cwd, options = {}) { | ||
|
|
@@ -890,7 +890,7 @@ export async function runAppServerTurn(cwd, options = {}) { | |
| touchedFiles: collectTouchedFiles(turnState.fileChanges), | ||
| commandExecutions: turnState.commandExecutions | ||
| }; | ||
| }); | ||
| }, { model: options.model }); | ||
| } | ||
|
|
||
| export async function findLatestTaskThread(cwd) { | ||
|
|
||
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.
When a ready broker session already exists,
ensureBrokerSessionreturns it before reaching this newmodelspawn argument, so a latertask --model gpt-5.4can still attach to an app-server that was originally started without-c model=.... In the ChatGPT-account scenario described by this fix, that leaves the unsupported default model in effect until the user ends/cleans up the shared session; store/compare the broker's model or bypass/restart the broker when a model override is requested.Useful? React with 👍 / 👎.