Skip to content

fix(deep-scan): preserve caller tool settings across scan turns - #2012

Draft
mldangelo-oai wants to merge 14 commits into
mainfrom
mdangelo/codex/deep-scan-parent-tool-settings
Draft

mldangelo-oai wants to merge 14 commits into
mainfrom
mdangelo/codex/deep-scan-parent-tool-settings

Conversation

@mldangelo-oai

@mldangelo-oai mldangelo-oai commented Oct 25, 2026 •

Copy link
Copy Markdown
Collaborator

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

  • If native app-server supports an explicit configuration directory at startup, read its effective routing there while retaining the caller’s process working directory. On the pinned native binary, use settings supplied by the SDK. Keep native validation errors intact.
  • Use the installed plugin declaration, including inline and alternate-file forms. Preserve direct-call opt-outs. When another server normalizes to the same namespace, retain a caller's existing direct or excluded sibling name. A fully configured native registration can select the coordinator by its server ID without renaming the peer.
  • Retain native tool policy when a caller supplies a custom server transport but the SDK cannot see a machine-level direct opt-out. If the caller explicitly supplies the omission list, update that same server setting without changing any unrelated tool namespace.
  • Run a requested post-scan prompt with the caller’s original client and original thread when available. Prepare the selected target list and check the saved spending limit before this follow-up can start, including if a configuration read fails after registration.

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 on a379f433.

  • 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 preceding 45f53570, 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 a379f433 CLI 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 at ab47a485 preserved 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 ab47a485 finished 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

  • No customer, partner, prospect, or user identities, data, or identifying details are included.
  • No credentials, personal data, private source, scan findings, or nonpublic links or tickets are included.
  • I reviewed the branch name, title, description, commits, changes, comments, logs, screenshots, attachments, and links for public disclosure.

@alandelong-oai alandelong-oai left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 alandelong-oai left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 alandelong-oai left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Comment on lines +64 to +65
cwd: process.cwd(),
configCwd: options.scanDir,

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[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.

Comment on lines +137 to +141
overrides.push(
`features.non_prefixed_mcp_tool_names=${inlineToml({
enabled: true,
server_names: [...(Array.isArray(selected) ? selected : []), owner],
})}`,

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[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 alandelong-oai left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Comment on lines +159 to +163
server !== owner &&
isRecord(settings) &&
settings["enabled"] !== false &&
(!Array.isArray(settings["enabled_tools"]) ||
settings["enabled_tools"].length > 0),

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[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.

Comment on lines +55 to +59
try {
return parse([...routing.values()].join("\n")) as JsonObject;
} catch {
// Leave overlapping or malformed TOML for exec's own parser.
return undefined;

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[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 alandelong-oai left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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;

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[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.

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug Something isn't working

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants