Repository navigation
fix(deep-scan): wait for the coordinator before ending CLI scans - #2007
Conversation
Keep an eligible CLI scan and its coordinator running when the parent returns before a manifest exists. Verify the original checkout and session on explicit resume, reuse saved workers, and report only measured usage. Keep cancellations, terminal failures, and workflows with additional actions on their existing paths. Refs #1967.
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
alandelong-oai
left a comment
alandelong-oai
left a comment
There was a problem hiding this comment.
This owner-level merge-safety review found one regression that needs attention: the new internal managedAttempt flag changes saved workflow identity, preventing validation or patching from reusing a cached scan. The existing validation test fails with the matching digest error in hosted CI; details and a narrow fix are inline.
The owner-preservation checks, explicit resume path, and separate handoff subtotal otherwise fit the reported parent-return failure. I also reviewed the cancellation, managed-attempt, publication, and saved-checkout boundaries.
Validation at 98c3581a: SDK/plugin builds, type checks, formatting, plugin source checks, 103 focused API/CLI tests and 138 cost/log tests passed locally using the CI-pinned Bun version. Hosted Python reports show 476 relevant tests passed and one skipped. Broader local lifecycle tests were limited by existing output-directory ownership checks, so I am not claiming a full local suite or end-to-end pass.
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. |
…t turn Replace the draft recovery workaround with direct parent dispatch. A yielded code-mode cell can outlive turn completion; a direct MCP tool call returns the manifest before the model can end the turn. Preserve existing caller namespaces and direct engine selection.
mldangelo-oai
changed the title
alandelong-oai
left a comment
alandelong-oai
left a comment
There was a problem hiding this comment.
Owner-level merge-safety pass on d3868d1: no blocking findings. Thank you for replacing the recovery workaround with the native namespace setting. It keeps the parent call pending while preserving caller namespaces and the existing worker, authentication, and direct-engine routes. The earlier workflow-identity issue is removed.
The SDK/plugin builds, type and format checks, and 43 focused tests passed. A benign local Codex 0.164/MCP delay check confirmed that the tool finishes before parent continuation. Broader validation remains limited by local filesystem/PATH failures (the PATH failures also reproduce on the pinned base), duplicate-Ajv eval CI errors, and a timeout in an unchanged macOS packaging test. This review does not establish recovery after genuine coordinator loss.
Preserve native configuration errors for malformed code mode settings while keeping valid coordinator calls attached. Bring in the main-branch eval build fix for the new CI run.
![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: e6a6585e70
ℹ️ 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".
| direct_only_tool_namespaces: [ | ||
| ...new Set([...direct, "mcp__codex_security", "codex_security"]), |
There was a problem hiding this comment.
When another configured or enterprise-managed MCP server normalizes to codex_security—for example, a dotted codex.security registration—Codex disambiguates the collision by adding a hash suffix to both callable namespaces. Neither hard-coded entry then matches the Codex Security coordinator, so it remains routed through code mode and the early-turn termination this change is meant to prevent can still fail the scan. Derive the collision-resolved namespace from Codex rather than assuming these two base spellings.
AGENTS.md reference: AGENTS.md:L14-L19
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Reproduced on the merged code. With two other servers advertising the same tool name, the original coordinator was available only inside a yielding cell. In #2012, the native server-name selector keeps the genuine coordinator direct when a free name is available. The installed CLI completed the original scan in one turn, and neither other server was called. The follow-up is still in draft; an empty peer allowlist and alternate plugin declarations have separate regression coverage in progress.
| mode === "deep" && !directDeepScan | ||
| ? deepScanParentConfig(session.sessionConfig) |
There was a problem hiding this comment.
For a parent-driven Deep Scan with postScanPrompt, this configuration is attached to the thread for its entire lifetime, so the later follow-up at runPostScan also sees Codex Security tools as direct-only instead of using the caller's original code-mode settings. That needlessly changes routing and exposes the plugin's full direct schemas during caller-supplied post-scan work; run the follow-up with the original configuration or otherwise limit this override to the coordinator turn.
AGENTS.md reference: AGENTS.md:L14-L19
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Confirmed that the coordinator-only setting leaked into the post-scan turn. #2012 resumes the original thread with the caller’s original client for a requested post-scan prompt. In the installed comparison, that prompt could retrieve the sealed scan with the caller’s tools. The serial controls passed; two lookup failures in the first concurrent custom-plugin batch remain unexplained. The follow-up is still under review.
| direct_only_tool_namespaces: [ | ||
| ...new Set([...direct, "mcp__codex_security", "codex_security"]), |
There was a problem hiding this comment.
When /etc/ProgramData, cloud-managed, or another native Codex configuration layer already supplies features.code_mode.direct_only_tool_namespaces, that value is absent from session.sessionConfig, which is built only from the wrapper's defaults and explicit overrides. Supplying this new higher-precedence array therefore replaces rather than extends the native list, silently routing every previously direct-only namespace back through code mode during the scan. Preserve the effective native entries instead of constructing the replacement solely from the wrapper snapshot.
AGENTS.md reference: AGENTS.md:L14-L19
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Confirmed with an installed CLI and a machine-wide native tool setting: the merged code removed that existing direct tool from the parent menu. #2012 reads the effective native settings with the scan’s ordered overrides, then extends the existing list. The machine-wide tool and the coordinator were both direct in the native comparison; explicit cell-only policy was preserved on both revisions. The fix is in the follow-up draft.
Summary
Fixes #1967. A parent-driven CLI Deep Scan can announce that its coordinator is still running and then finish its turn. With the standard code-mode route, the model can receive a still-running cell before the tool returns its manifest. Closing the parent can then terminate the reducer. The SDK subsequently fails the scan, and
scans resumecannot reopen a failed scan.Changes
Send the Codex Security MCP namespace directly to the native Deep Scan parent so its coordinator call stays pending until the tool returns. This replaces the first draft’s recovery workaround. Independent review found that dotted or managed namespace settings and a caller-supplied post-scan prompt need an additional correction before this PR is ready. A separate loss of the coordinator process is being investigated independently.
Testing
gpt-6-sol/xhigh, default code-mode yield, a real MCP host, SQLite workbench, and worker processes. The provider responses and worker analysis were scripted on Linux.main(c5dbc050)d3868d10)Both runs made two parent requests. All 18 cases in the routing and CLI-validation suites passed separately on Windows, Linux and macOS, including the cached-workflow regression from the first draft. The first two-file revision’s CI run also had an eval build error inherited from
mainand a macOS package-test timeout.The current revision,
c6c1d7b1, includes the merged eval build fix and preserves native rejection of an invalidfeatures.code_modevalue. Its installed package again completed the original command in one native turn (51.0 seconds). All six routing tests, TypeScript checking and formatting passed locally. The eval and macOS package jobs passed in hosted CI. The PR remains in draft pending the compatibility corrections above.Risk and rollout
This changes how the parent sees the plugin's tools: native Codex exposes this setting per namespace, so direct schemas add some prompt input. An explicit native policy that requires third-party tools to use code mode still takes precedence. This does not reopen scans already marked failed. No release or schema migration is required to review the change.
Public disclosure review