feat(plugin): suggest scan models and reasoning settings - #805
feat(plugin): suggest scan models and reasoning settings#805daneschneider-oai wants to merge 8 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, handleMessage() does not actually ignore every server-initiated request as the comment says. Codex RequestId allows integer IDs, so a server request with a numeric id is treated as the pending client response and can trigger the mismatched-ID/protocol failure. Could this distinguish requests by method (or responses by result/error) before matching the ID?
sylvesterkaczmarek
left a comment
sylvesterkaczmarek
left a comment
There was a problem hiding this comment.
One protocol edge case in readModelCatalog(): the Codex ModelListResponse schema makes nextCursor optional, but this loop only treats explicit null as end-of-pagination. A valid final page that omits the field is rejected as an invalid cursor. Could the termination check use page.nextCursor == null?
mldangelo-oai
left a comment
mldangelo-oai
left a comment
There was a problem hiding this comment.
Reviewed exact draft head 3615bb06584944ba691550ff1b907bb99e3b9894. Three focused comments below; the existing JSON-RPC request-classification and pagination feedback still applies.
| upgrade && !isCyberModel(upgrade.model) | ||
| ? upgrade | ||
| : catalog | ||
| ? recommendedNonCyberModel(catalog) |
There was a problem hiding this comment.
[P2] Keep the selected chain's non-cyber recommendation
latestAvailableUpgrade() returns the final eligible target, including Astra. For legacy-cyber -> scan-specialist -> astra-cyber, upgrade is Astra, so this branch discards scan-specialist and falls back to an unrelated catalog default. The resulting guidance can recommend unrelated-default for vulnerability scanning while separately offering astra-cyber, even though the selected model explicitly points to scan-specialist. Please retain the latest visible non-cyber candidate while walking the selected model's declared chain, then fall back to a visible non-cyber default and its chain; cover both paths.
| this.child = spawn(executablePathForSpawn(options.codexPath), args, { | ||
| cwd: options.cwd, | ||
| ...(options.env === undefined ? {} : { env: options.env }), | ||
| stdio: ["pipe", "pipe", "pipe"], |
There was a problem hiding this comment.
[P2] Hide the pre-scan subprocess on Windows
Please set windowsHide: true on this spawn. This shared client now backs routine model guidance, so an ordinary Windows desktop scan can start the catalog app-server with Node's default windowsHide: false and flash a console window before the scan. The repository's other production Codex spawns hide their windows, and #806's equivalent model-catalog spawn does too. This one option preserves the stdio behavior and avoids the Windows UI regression.
| "Prefer the declared non-cyber upgrade to an unrelated default.", | ||
| ); | ||
| assert.match(advice, /use scan-upgrade with xhigh reasoning\?/); | ||
| for (const cyber of [false, true]) { |
There was a problem hiding this comment.
[P3] Collapse the duplicated guidance matrix
The MCP runtime matrix below already covers cyber-only warning, low-effort actionable guidance, their combination, and no guidance through both source and bundled servers; assertDeliveryInstruction owns the delivery contract, and the advice assertions above cover the maximal combined suggestion. Keep the one distinct catalog-upgrade-only case plus a single-question assertion on advice, but collapse this 2x2x2 table. That preserves the meaningful branches while removing roughly 18-20 lines of repeated expectations.
3 participants
Summary
Add one pre-scan nudge for non-Astra cyber models, available model upgrades, and reasoning below
xhigh.Changes
Use effective host settings and catalog upgrade metadata. Extract a shared app-server client; keep permission checks in Deep Scan. Ask before a new chat-started desktop scan begins.
Interaction and copy
Show one combined, applicable question through the app's blocking input form:
Offer Change settings (Recommended) and Continue current settings. Continue only after an explicit Continue; Change stops before scan creation so the user can adjust the selector and resubmit. If input is unavailable, ask in chat and stop. Unknown
xhighsupport addsif your model supports it. Cyber-only advice has no question and continues after a visible warning. Native Security tab scans skip repeated advice when their launch instructions explicitly say the New scan form handled model guidance before launch; older native continuations retain a declarative advisory. Headless scans receive one warning.Testing
All 25 MCP scripts, 17 custom-validation tests, typecheck, formatting, and source checks passed; verified source and bundled runtimes.
Risk and rollout
Named upgrades require catalog evidence; missing metadata leaves generic advice. Catalog data may not reflect app-specific filtering or runtime-only profile overrides.
Companion: CLI #806.
Public disclosure review