Repository navigation
fix(deep-scan): preserve caller tool settings across scan turns - #2012
mldangelo-oai wants to merge 14 commits into
Conversation
alandelong-oai
left a comment
There was a problem hiding this comment.
Thanks for fixing the caller-settings regression. The coordinator-only override preserves native configuration layers and ordered caller overrides, while the requested follow-up returns to the original settings and resumes the same thread when its ID is known. Worker settings and native tool policies remain intact; normalized MCP namespace collisions remain an inherited limitation.
Builds, typechecking, formatting and focused synthetic lifecycle checks passed. Exact-tree CI reports confirm the changed routing/direct tests on Linux and configuration-read tests on Windows pass. Broader local integration was limited by filesystem and fixture restrictions, and the remaining CI jobs were still running when reviewed.
alandelong-oai
left a comment
There was a problem hiding this comment.
Thanks for preserving the caller's tool settings. No material findings in this owner-level merge-safety review. The coordinator-only overrides preserve native configuration and direct namespaces, respect selected-plugin exclusions, and restore the original settings for requested post-scan work, including registered configuration failures. Explicit native policy and alternate declarations retain the limitations described in the PR.
Current-head builds, typechecking, formatting, portable plugin checks and worker preflight passed. Twelve focused routing cases, four synthetic API lifecycle cases and four native configuration-only checks also passed. Broader local integration was limited by filesystem/PATH constraints. Full cross-platform SDK verification remains pending after a Windows runner setup crash; the hosted rerun is in progress.
alandelong-oai
left a comment
There was a problem hiding this comment.
Thanks for the compatibility work. This owner-level merge-safety review found two remaining caller-setting regressions: native configuration can fail during startup before the scan-directory read, and resolving an MCP name collision can remove another server's explicit direct routing. Both are detailed inline and need attention before approval. The selected-path preparation, resumed-budget ordering and original-client follow-up changes look sound.
Exact-head builds, type/format checks, portable plugin checks, worker preflight and focused routing/follow-up checks passed locally; the applicable Bun checks were rerun with CI-pinned 1.3.14, including all four synthetic lifecycle cases. A native configuration-only check confirmed the startup failure; the naming finding is supported by the pinned native implementation and a helper assertion. Local integration had filesystem/PATH limitations. Verified exact-tree CI reports show all Linux SDK tests passing (7,188 passed, 110 skipped), with relevant macOS/Windows cases also passing; the remaining cross-platform jobs were still running.
| cwd: process.cwd(), | ||
| configCwd: options.scanDir, |
There was a problem hiding this comment.
[P2] Match the parent configuration directory during app-server startup
The app-server loads and validates configuration before it can receive config/read, so sending scanDir in that later RPC does not make this read equivalent to the real parent launched with --cd scanDir. With model_instructions_file="instructions.md" and that benign file present only in the scan directory, the pinned native app-server fails at startup here; the same configuration started in the scan directory succeeds. This newly added preflight therefore rejects a valid caller configuration before the coordinator starts. The reader needs the same startup configuration directory as the parent while preserving launcher-relative paths; the RPC cwd alone cannot provide that separation.
| overrides.push( | ||
| `features.non_prefixed_mcp_tool_names=${inlineToml({ | ||
| enabled: true, | ||
| server_names: [...(Array.isArray(selected) ? selected : []), owner], | ||
| })}`, |
There was a problem hiding this comment.
[P2] Preserve explicit direct routing when removing a namespace collision
Moving only codex-security to the bare namespace also changes the other server's published name. In pinned Codex, codex.security colliding with codex-security is exposed as mcp__codex_security_e789902954e0; a caller can explicitly put that name in direct_only_tool_namespaces. This override removes the collision, so the peer becomes mcp__codex_security, but the direct list retains only its old hashed name plus codex_security. The peer therefore loses the caller's requested direct routing during the coordinator turn. Preserve the affected direct selection when changing names, or leave the collision policy unchanged when that selection cannot be carried forward.
alandelong-oai
left a comment
There was a problem hiding this comment.
Thanks for addressing the startup-directory and peer-renaming findings from the previous review. This owner-level merge-safety pass found two remaining cases that drop the coordinator's direct dispatch: a plugin-only registration with a filtered-out peer, and valid ordered feature overrides on the provider-profile path. Both are detailed inline and need attention before approval. The original-client follow-up, selected-path preparation and resumed-budget ordering look sound.
SDK/plugin builds, typechecking, formatting, portable source checks and worker preflight passed. Focused routing, configuration-only and synthetic lifecycle checks support the findings; broader local tests hit filesystem-ownership and PATH restrictions. Current-head CI has no failures so far, and package/native/portable checks passed, but SDK and cross-platform integration jobs are still running.
| server !== owner && | ||
| isRecord(settings) && | ||
| settings["enabled"] !== false && | ||
| (!Array.isArray(settings["enabled_tools"]) || | ||
| settings["enabled_tools"].length > 0), |
There was a problem hiding this comment.
[P2] Preserve coordinator routing when a plugin peer has no admitted tools
A nonempty peer allowlist does not mean that peer contributes a namespace. With a plugin-only codex-security registration, non_prefixed_mcp_tool_names=false, and a codex_security peer whose enabled_tools=["absent"] matches none of its advertised tools, this marks the coordinator namespace as occupied and the later guard returns no override. Pinned Codex filters tools before resolving namespace collisions, so the coordinator actually keeps mcp__codex_security: main makes it direct, but this revision leaves it in Code Mode and permits the original yielding-coordinator failure again. Preserve direct dispatch using the admitted catalog or native server identity while retaining the peer's routing; the configured-transport branch currently covers only native registrations.
| try { | ||
| return parse([...routing.values()].join("\n")) as JsonObject; | ||
| } catch { | ||
| // Leave overlapping or malformed TOML for exec's own parser. | ||
| return undefined; |
There was a problem hiding this comment.
[P2] Preserve valid ordered feature overrides in the fallback
Separate CLI overrides are not necessarily one valid TOML document. For a parent-routed scan with a provider profile, a valid configuration containing nested features.code_mode = { enabled: true, direct_only_tool_namespaces: ["audit"] } and the literal dotted key "features.code_mode.enabled": true produces a dotted flag followed by a whole features={...} table. Stock Codex 0.164 accepts those ordered flags and retains the settings, but this parse rejects them and returns undefined; the public API then skips the coordinator override entirely. Main kept the coordinator direct for the same configuration. Preserve native override ordering instead of discarding valid overlapping flags, and cover this through the provider-profile serializer.
alandelong-oai
left a comment
There was a problem hiding this comment.
Thanks for preserving inherited omission policy and the caller’s follow-up settings. Owner-level merge-safety review of 4d9154c: material routing regressions remain, so this is a COMMENT review.
The new inline covers a configured native transport with no explicit omission list: even without a machine-level restriction, the coordinator loses the direct routing that main supplies. The earlier filtered-peer finding and ordered provider-profile override finding also remain reproducible at this head; those comments are not duplicated here.
Validation: fresh SDK/plugin builds, type checks, formatting, portable source checks, worker preflight, four synthetic caller-lifecycle cases, and ten isolated native configuration controls. Independently verified current-head CI: 57 jobs passed and two were skipped; its tested merge tree equals this head. The routing, direct-scan, API and component suites pass across Linux, macOS and Windows, with platform skips retained.
Local focused suites were partially blocked by filesystem-ownership and PATH fixture constraints; those failures are not counted as passes. Configuration checks made no model turns or MCP calls, and I did not rerun a full scan. Keep the inherited opt-out protection while resolving the unrestricted-transport and existing routing cases.
| const servers = config["mcp_servers"]; | ||
| const owner = isRecord(servers) ? servers[PLUGIN_NAME] : undefined; | ||
| // Even a supplied transport can inherit a machine-level direct opt-out. | ||
| if (isRecord(owner) && !Array.isArray(owner["omit_tools_from"])) return; |
There was a problem hiding this comment.
[P2] Preserve direct dispatch for transports without an omission override
On pinned stock Codex, a caller-supplied mcp_servers.codex-security transport without omit_tools_from always takes this return, including with an empty machine configuration and no direct-call opt-out. The API consequently appends no coordinator override. A native configuration-only check with Code Mode enabled accepts this configuration without any omission; the base implementation retains both coordinator direct namespaces. This valid, noncolliding configuration therefore loses the direct dispatch needed to keep the parent waiting for its coordinator. Preserve inherited omission policy while restoring direct dispatch when no omission applies; replacing an unseen omission list would discard caller policy.
Summary
Follow-up to #2007 for #1967. The initial fix keeps the first CLI command waiting for its Deep Scan coordinator. This draft addresses compatibility with caller-supplied tool settings, selected plugin registrations and post-scan prompts.
Please leave this PR in draft. The current revision is
4d9154c6. On stock Codex two custom routing configurations still differ from main (details below). Independent review and CI must pass before readiness.Changes
Testing
Current revision: four affected SDK suites, 327 passed, four Windows-only cases skipped on Linux, zero failures (39,857 assertions); package type checks and formatting passed.
At
4d9154c6, eight installed-package routing checks exercised an explicit or inherited tool omission with stock Codex. The SDK retained an inherited direct opt-out, honored an explicit empty list, and preserved an unrelated machine tool when the caller supplied the omission list but inherited the transport. Also checked the native error for an invalid transport. These inspect the native tool list. Source CI for this revision. The full scan below was ona379f433.At
a379f433, four affected SDK suites passed locally: 327 passed, four Windows-only cases skipped on Linux, zero failures. The SDK type and format checks passed. Source CI at a379f433 passed (57 jobs passed, two skipped), including all 28 targeted Windows cases. At the preceding45f53570, Source CI also passed, including all 27 targeted Windows cases.On the installed Linux SDK with the new startup guard, stock Codex 0.164 completed a single direct coordinator turn in 53 seconds, including a 40-second reducer. The requested second turn read the same original sealed manifest under the caller's original settings. On a separately built native binary that supports a configuration directory, relative instructions and database paths resolved in the scan; the process stayed in the caller’s directory.
With the guarded-sibling fix on stock Codex, the caller's explicit direct and excluded settings kept the sibling out of the Code Mode cell. The excluded-sibling tool list matched the one on main byte for byte.
The installed
a379f433CLI completed a 12,500-file, 3,700-directory non-Git scan in one native parent turn, with four workers, three allowed subagents per worker and a 120-discovery-run limit. Ten discoveries and four reducers succeeded. The original manifest returned at 67 seconds; the CLI exited 0 at 94 seconds. A separate termination and resume check atab47a485preserved the original three discovery IDs and output hashes. These Linux tests used stock Codex and scripted local model responses. The issue has reproduction and recovery instructions.Source CI at
ab47a485finished successfully after a Windows runner setup failure was retried. Raw reports confirmed the three old fixture failures passed on all three platforms. The independently built npm archives matched the CI packages across all 746 files.Risk and rollout
Stock Codex 0.164 cannot load a different configuration directory at app-server startup. The SDK fallback cannot extend direct-only lists it cannot see, such as a machine-level list; this limitation also exists on main. Two tested custom configurations can leave the coordinator in Code Mode where main exposed it directly: (1) a plugin-only owner, explicitly prefixed names, and a peer whose allowlist matches no tools; (2) a caller-supplied native transport with no explicit omission list and no actual machine restriction. A grant by normalized name can select the peer instead if the owner is unavailable, and replacing an unseen omission list can drop the caller's policy. These cases block readiness. This change cannot reopen a scan already marked failed.
Public disclosure review