feat(cli): offer scan model and reasoning upgrades - #806
feat(cli): offer scan model and reasoning upgrades#806daneschneider-oai wants to merge 7 commits into
Conversation
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
sylvesterkaczmarek
left a comment
sylvesterkaczmarek
left a comment
There was a problem hiding this comment.
Hi, the no-catalogue path treats an unknown model as supporting xhigh (model === undefined), so the CLI can still print “Use xhigh for the best scanning results” even when it could not load the catalogue or verify support. Could the warning keep the same “if your model supports it” qualifier instead of asserting xhigh for an unknown model?
mldangelo-oai
left a comment
mldangelo-oai
left a comment
There was a problem hiding this comment.
Reviewed exact draft head 5281ef76d3021a42b216aeac2e96831fa6fc7847. Two correctness issues and one cross-surface simplification are inline below. I did not repeat the existing unknown-model/xhigh feedback, which remains applicable.
| let choice = choices.get(key); | ||
| if (choice === undefined) { | ||
| choice = choose(configuration, loadModels, signal); | ||
| choices.set(key, choice); |
There was a problem hiding this comment.
[P2] Evict rejected selections from the shared cache
choices retains a rejected promise indefinitely. If the first selection for a configuration rejects during authentication discovery or confirmation, later component workers and bulk-scan retries reuse that rejection without invoking their own loader or prompt. Please preserve the useful in-flight and successful-choice caching, but remove the entry on rejection with an identity guard so an older promise cannot delete a replacement. Add a regression showing that a second call invokes a fresh loader and succeeds after the first rejects.
| : { maxCostUsd: options.maxCost }), | ||
| }, | ||
| createSecurity: dependencies.createSecurity, | ||
| createSecurity: (config) => |
There was a problem hiding this comment.
[P2] Apply the accepted model choice to the whole component command
Passing chooseModel only into each CodexSecurity worker makes the accepted choice worker-local. With --auto, planning has already run with config before selection occurs, and cross-component matching later receives that same unchanged config. Accepting the offered upgrade therefore changes component scans and the dashboard but leaves planning and root-cause matching on the old model and effort, despite the README saying the choice applies to the current command. Please resolve selection once before component orchestration and pass one command-local effective config to planning, workers, matching, and the dashboard; add an integration test covering all three model-call stages.
| ); | ||
| let cyberWarning: string | undefined; | ||
| if (isNonAstraCyberModel(selectedModel?.model ?? model)) { | ||
| const defaultModel = availableModels?.find( |
There was a problem hiding this comment.
[P3] Drive both guidance surfaces from one policy contract
getScanModelAdvice() duplicates the pure upgrade/default/cyber/reasoning policy also added in #805, and the implementations already diverge. For example, #805 can follow a cyber default's declared chain to a visible non-cyber successor, while this implementation filters out that default before traversal. Keep the package-specific app-server, authentication, and prompt adapters separate, but put the pure policy in a shared module if packaging permits, or at least run both implementations against shared conformance vectors. That prevents the two surfaces from producing different advice for the same catalog.
3 participants
Summary
Offer one model/effort nudge before scanning. Keep startup and prompts in the normal terminal until scan activity begins.
Changes
Use catalog-backed upgrades and share choices across matching scans. Check native ChatGPT authentication first; require TTY input and output for interactive UI.
Interaction and copy
Combine applicable sentences into one stderr warning:
Ask one confirmation for eligible changes:
Use {new_model} with xhigh reasoning for this scan?Use {new_model} for this scan?Use xhigh reasoning for this scan?Default No. Yes applies the proposed changes for this command. Headless/JSON/CI runs warn without prompts or changes. Dry runs skip the lookup. Missing catalog metadata still permits the
xhighopt-in; explicitly unsupported efforts are excluded.Detected sign-in failure:
Testing
293 focused tests, real PTY checks, and bundled Codex 0.149.1 with synthetic local authentication responses. Typecheck and formatting passed.
Risk and rollout
Catalog failures remain advisory. Saved settings and cost limits are preserved. Authentication uses native conditional refresh.
Companion: plugin #805.
Public disclosure review