Repository navigation
refactor(deep-scan)!: unify execution and durable result recovery - #1123
Conversation
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
Security findingsBlocking findings (1)
ℹ️ 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. |
|
@codex review |
There was a problem hiding this comment.
Copilot review overview
🟡 Changes recommended
Tilde-based Codex homes fail during environment snapshotting, and most new native runtime tests are not executed by CI.
Review effort: Balanced
Findings: 2
Open (2)
What changed in this PR
Prepares native scans to inherit the invoking Codex runtime, configuration, authentication, executable, and permissions.
Changes:
- Adds ambient runtime and native scan preparation.
- Adds trusted executable and sandbox-permission handling.
- Adds native runtime tests and CI coverage.
| File | Description |
|---|---|
.github/workflows/node-ci.yml |
Adds native test steps. |
sdk/typescript/tests-support/process-environment.mjs |
Adds environment restoration helper. |
sdk/typescript/src/version.ts |
Loads versions via bundled JSON import. |
sdk/typescript/src/execution-preparation.ts |
Adds ambient execution/runtime preparation. |
sdk/typescript/src/auth.ts |
Forwards authentication configuration. |
sdk/typescript/src/api.ts |
Integrates ambient runtime dependencies. |
plugins/codex-security/mcp-app/tsconfig.json |
Adds DOM library types and reformats includes. |
plugins/codex-security/mcp-app/src/native-scan.ts |
Prepares native scan clients and options. |
plugins/codex-security/mcp-app/src/native-permissions.ts |
Resolves inherited sandbox restrictions. |
plugins/codex-security/mcp-app/src/native-executable.ts |
Resolves trusted Codex executables and environment. |
plugins/codex-security/mcp-app/tests/test_native_scan.mjs |
Tests native scan preparation. |
plugins/codex-security/mcp-app/tests/test_native_permissions.mjs |
Tests sandbox metadata handling. |
plugins/codex-security/mcp-app/tests/test_native_executable.mjs |
Tests executable discovery. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| - name: Test native scan preparation | ||
| run: node --test --test-name-pattern="native preparation" plugins/codex-security/mcp-app/tests/test_native_scan.mjs |
| delete environment.CODEX_HOME; | ||
| } else if (codexHome !== undefined && codexHome.length > 0) { | ||
| // Resolve symlink/.. paths before consumers normalize them or change cwd. | ||
| environment.CODEX_HOME = await fs.realpath(codexHome); |
![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: 5bfc9854b6
ℹ️ 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".
| registeredScan?: { | ||
| scanId: string; | ||
| scanDir: string; | ||
| threadId: string; | ||
| handoffClaimToken?: string; |
There was a problem hiding this comment.
When the client returned by prepareNativeScan is run, this field is never consumed: registerScan does not include registeredScan in its ScanOptions pick or forward its scanId, threadId, and claim token in the registration JSON. The workbench consequently takes the fresh-registration path instead of binding the already claimed native scan, which will normally fail because that scan directory is already populated (or could create a second scan record). Wire this metadata through the registration path before exposing it on the prepared options.
Useful? React with 👍 / 👎.
| platform, | ||
| originalCwd, | ||
| )) { | ||
| if (platform === "win32" && isWindowsAppsPath(candidate)) continue; |
There was a problem hiding this comment.
On Windows installations where CODEX_CLI_PATH is a WindowsApps alias—or where Codex is available only through CODEX_MANAGED_PACKAGE_ROOT/the LOCALAPPDATA cache—this skips the alias and then has no usable candidate, so every native scan fails with “Could not resolve a Codex executable.” The existing discovery-worker resolver in deep-scan/executor.ts handles these exact environments by resolving the managed package or cached native binary; reuse that resolution here rather than limiting lookup to direct PATH entries and the cwd.
AGENTS.md reference: AGENTS.md:L30-L34
Useful? React with 👍 / 👎.
| delete environment.CODEX_HOME; | ||
| } else if (codexHome !== undefined && codexHome.length > 0) { | ||
| // Resolve symlink/.. paths before consumers normalize them or change cwd. | ||
| environment.CODEX_HOME = await fs.realpath(codexHome); |
There was a problem hiding this comment.
When CODEX_HOME uses a supported tilde form such as ~/.codex or contains surrounding whitespace, this calls realpath on the literal, untrimmed value. The normal configuredCodexHome path handling trims and expands these forms, so native preparation instead throws ENOENT before it can use the invoking account and credentials. Normalize and expand the value before canonicalizing it for the later cwd change.
AGENTS.md reference: AGENTS.md:L49-L50
Useful? React with 👍 / 👎.
| (await gitMarkerRoot(input.scan.targetPath, signal, "outermost")) ?? | ||
| input.scan.targetPath, |
There was a problem hiding this comment.
Executable trust is checked only against targetPath, even though the existing scan directory and each knowledgeBasePaths source are also repository- or model-controlled inputs. If PATH contains the scan directory (for example through .) or a separate knowledge-base repository, resolveTrustedCodex accepts and executes a codex binary from that location because it lies outside the sole protected root; prepareAmbientExecution can then run it during authentication with the invoking user's environment. Resolve and exclude all scan-controlled roots before selecting the executable.
AGENTS.md reference: AGENTS.md:L6-L8
Useful? React with 👍 / 👎.
mldangelo-oai
changed the title
alandelong-oai
left a comment
alandelong-oai
left a comment
There was a problem hiding this comment.
Reviewed 5bfc9854b68ee044dd8e94bab05dfb9c6f1dc3d8 against mdangelo/codex/pr939-stack-17-scan-recovery at 1a009b9bd2c41c344af4fa454c9f596b9c2a4499, scoped to this PR's diff and touched files.
Reviewed native executable/configuration/authentication selection, inherited filesystem permissions, and preparation coverage. No additional distinct findings beyond the existing review comments. The native-permissions assertions passed against the exact TypeScript source using an adapted Node loader; this was not a packaged-runtime test.
Review included simplification/deletion opportunities. Full repository and platform test suites were not run.
5bfc985 to
e53883b
Compare
1a009b9 to
abd6b3a
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: e53883b183
ℹ️ 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".
| return { | ||
| client, | ||
| options: { | ||
| ...options, |
There was a problem hiding this comment.
When a claimed scan is resumed after the plugin has been upgraded, this options object ignores recipe.pluginVersion, so the SDK's existing expectedPluginVersion check never runs and the new plugin can continue artifacts created under an incompatible schema or workflow. The CLI resume loader already maps this recipe field to expectedPluginVersion; native preparation should do the same so a version mismatch fails before modifying the scan.
AGENTS.md reference: AGENTS.md:L11-L12
Useful? React with 👍 / 👎.
e53883b to
c9c9ae4
Compare
abd6b3a to
10e9da2
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: c9c9ae4de4
ℹ️ 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".
| persistentCredentialHome: true, | ||
| preserveCodexHomeConfig: true, | ||
| bootstrapWorkspace, | ||
| configPath: join(bootstrapWorkspace, "config-preflight.toml"), |
There was a problem hiding this comment.
When this runtime executes a native Deep Scan, leaving deepScanConfigPath unset makes #runLocal fall back to <codexHome>/codex-security/config.toml and write the resolved settings there. Fresh scans therefore mutate the caller's persistent configuration, resumed scans can replace it with saved settings, and concurrent scans share the same file; when CODEX_SECURITY_DEEP_SCAN_CONFIG_PATH selects another file, workers can also read a different configuration from the one the SDK wrote. Set deepScanConfigPath to a scan-local path under bootstrapWorkspace, alongside configPath.
AGENTS.md reference: AGENTS.md:L30-L34
Useful? React with 👍 / 👎.
10e9da2 to
81fa0fb
Compare
c9c9ae4 to
06527bb
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 06527bb891
ℹ️ 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".
| execution.environment["CODEX_HOME"] || | ||
| configuredCodexHome(execution.environment), | ||
| ); | ||
| const bootstrapWorkspace = await createIsolatedHome(); |
There was a problem hiding this comment.
When the scan target contains the OS temporary directory—for example, a repository-local TMPDIR or a scan of /tmp—this creates the bootstrap workspace inside the untrusted target because it omits the location validator used by the ordinary #prepareRuntime path. The scan sandbox grants workspace-root writes, and the generated config and copied plugin are exposed through this directory, so repository-driven commands can modify scan runtime assets or cause the scan to inspect its own artifacts. Pass the protected-root validator through native runtime preparation, or otherwise reject a bootstrap workspace nested under any protected root.
AGENTS.md reference: sdk/typescript/AGENTS.md:L21-L23
Useful? React with 👍 / 👎.
| options: { | ||
| ...options, | ||
| ...deep.settings, | ||
| inheritedPermissions, |
There was a problem hiding this comment.
When the caller aborts after prepareNativeScan has returned, the signal supplied to this helper is absent from the returned ScanOptions, so the subsequent client.run(..., prepared.options) continues launching and running discovery and reducer work despite cancellation, potentially consuming the remaining scan budget. Include signal in the prepared options so the same cancellation lifetime covers both preparation and execution.
Useful? React with 👍 / 👎.
| const marketplaceRoot = | ||
| preparedPlugin?.marketplaceRoot ?? | ||
| (await createMarketplace( | ||
| bootstrapWorkspace, | ||
| execution.pluginRoot, |
There was a problem hiding this comment.
createMarketplace only copies the plugin and writes its marketplace manifest; unlike bootstrapPlugin, it does not register the marketplace or enable the plugin in Codex configuration. Because nativeScanConfiguration also strips any existing managed plugin registration and this runtime keeps the caller's CODEX_HOME, the SDK child has no configuration reference to this isolated marketplace, so the Codex Security MCP server is not loaded and a Deep Scan cannot call start_codex_security_deep_scan. Add the isolated marketplace and enabled-plugin registration to the effective session configuration without mutating the caller's home.
Useful? React with 👍 / 👎.
faizan-oai
left a comment
faizan-oai
left a comment
There was a problem hiding this comment.
Reviewed native settings, authentication, executable selection, and worker permission forwarding as part of the full stack. One reproducible test issue below: a caller's exported credential can make the fixture fail and appear in assertion output. The focused native suites pass when credential variables are isolated.
| const keys = [ | ||
| "PATH", | ||
| "CODEX_HOME", | ||
| "CODEX_CLI_PATH", | ||
| "CODEX_SECURITY_CONFIG_PATH", | ||
| "CODEX_SECURITY_DEEP_SCAN_CONFIG_PATH", | ||
| ]; |
There was a problem hiding this comment.
[P2] Isolate credential variables in this fixture
This saved/restored key list omits OPENAI_API_KEY and CODEX_API_KEY, while prepareNativeScan snapshots the caller's environment. With OPENAI_API_KEY=synthetic-review-openai-key and no CODEX_API_KEY, the synthetic child retains that key and assert.equal(process.openai, undefined) fails, printing the value into the test log. I reproduced this at stack tip 24713fb088ba; clearing both keys makes the case pass. Add both names to this list and delete both variables before preparing the fixture, letting the existing synthetic keys supply the test credentials. I verified that four-line change under the same synthetic environment.
mldangelo-oai
changed the title
alandelong-oai
left a comment
alandelong-oai
left a comment
There was a problem hiding this comment.
Reviewed the native preparation change at 06527bb891794d687f4aa32767bb130ef6bcef1c with three independent passes and root verification. Two existing issues remain reproducible:
- Per-scan Deep Scan settings overwrite the invoking home’s saved defaults. A real SDK
run()probe changed a synthetic saved worker count from 7 to 2 before output allocation. - The child-launch test does not isolate ambient API-key variables. Supplying a synthetic
OPENAI_API_KEYproduces the assertion failure.
The three new native test files pass in the baseline environment. Full native-to-SDK activation is reviewed separately at its stack boundary. No duplicate inline comments added.
06527bb to
fdef28b
Compare
alandelong-oai
left a comment
alandelong-oai
left a comment
There was a problem hiding this comment.
Three independent restack passes and root reconciliation are complete.
The test credential isolation fix is present, but the own-head ambient runtime still has no isolated Deep configuration path. Per-run overrides therefore fall back to the invoking account's codex-security/config.toml, preserving the previously reproduced settings mutation. The later #1162 runtime removes this write; the current PR needs that isolation at its own boundary.
Approval remains withheld. This review distinguishes the PR's own head from fixes verified later in the stack; it does not duplicate existing inline findings.
7df192d to
52c7a9b
Compare
fdef28b to
32f2372
Compare
0e27989 to
bd8e9b8
Compare
ab0e7f3 to
515ae24
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: bd8e9b8847
ℹ️ 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".
| ); | ||
| }, | ||
| ); | ||
| if (!overridden) environmentHeaders[name] = transfer(value); |
There was a problem hiding this comment.
When http_headers and env_http_headers contain the same HTTP header with different casing (for example, Authorization and authorization) and the mapped environment value is missing, blank, or invalid, this adds the literal fallback under the new spelling but leaves the stale mapping intact. The child therefore receives two case-insensitively equivalent headers; an invalid stale value can still make Codex reject the provider configuration, and otherwise precedence is ambiguous instead of reliably preserving the requested literal fallback. Remove every case-insensitive match before inserting the generated mapping.
AGENTS.md reference: sdk/typescript/AGENTS.md:L21-L24
Useful? React with 👍 / 👎.
| codexHome, | ||
| preserveCodexHomeConfig: true, |
There was a problem hiding this comment.
For a path-scoped native scan, returning the caller's real Codex home here makes the existing allocation in api.ts derive targetPathsFile from dirname(runtime.codexHome). With the default home this writes ~/codex-security-target-paths-*.json instead of using the isolated bootstrap workspace, and a valid custom CODEX_HOME whose parent is not writable makes the scan fail at startup; an interrupted process can also leave the authorized scope metadata behind. Place this helper file under bootstrapWorkspace or another validated per-scan location.
AGENTS.md reference: sdk/typescript/AGENTS.md:L21-L24
Useful? React with 👍 / 👎.
alandelong-oai
left a comment
alandelong-oai
left a comment
There was a problem hiding this comment.
Reviewed head bd8e9b88470eb513a6b7b4c9612654eabfc09269 against base 515ae24cb58d6bd7db2eadd63754a8840228a5ef using three fresh independent HIGH full actual-diff reviews, including all changed tests and fixtures, followed by root synthesis. Original reports and their review times are preserved.
Two introduced test fixture defects block source approval, detailed inline: the authentication tests pre-create a nonprivate scan directory and fail before authentication; the Windows provider fixture registers an unnormalized executable while production spawns its extended-path form. The earlier missing-home ENOENT source issue is fixed by creating the native ambient home before realpath.
The five selected SDK files passed 46 tests with 1 skip locally. The native executable-selection test passed, while all eight authentication cases failed at the output-directory check. Hosted MCP ran 316 passes and 9 failures: those eight fixture failures plus a bwrap loopback startup error. The selected Windows shard had 682 passes, 12 skips and the provider fixture failure. A separate parallel run had 3,979 passes, 53 skips, nine 30-second timeouts and four unhandled errors; their cause remains unresolved. These counts belong to separate executions.
The standalone sandbox denial test failed during bwrap loopback configuration, so it does not establish a permission bypass. Case-variant provider-header mappings and native scoped metadata under the real home parent remain conditional compatibility concerns; supported runtime impact is unestablished. No all-configurations-safe or fix-alone-green claim follows.
Hosted CI at 2026-10-04T12:53:09.313276+00:00–2026-10-04T12:53:33.556119+00:00 was pending with failed jobs already present. Source approval, CI and successful integration are separate. This prefix contains main 1fb0e5f, but whole-stack successful integration remains unestablished.
No product model, clean installation or local Windows run was performed. Native binaries were retained after source/dependency/artifact identity checks; no fresh native compilation occurred. Individual full-diff reviews qualify selective unchanged long-file context.
| try { | ||
| await mkdir(home, { mode: 0o700 }); | ||
| await mkdir(target); | ||
| await mkdir(join(root, "scan")); |
There was a problem hiding this comment.
[P2] Create the authentication fixture output directory with0700
The fixture pre-creates scan using default mkdir permissions, but the real SDK requires existing output directories to be private. Under the normal022 umask all eight bearer/env-key cases fail with OutputDirectoryError before onAuthentication runs, both locally and in the hosted MCP job. Create this fixture directory with mode0700 while keeping the product permission check; otherwise these tests never exercise the authentication behavior they claim to cover.
| `, | ||
| ); | ||
| const spawn = spyOn(childProcess, "spawn").mockImplementation( | ||
| fixtureSpawn(executable, script, () => {}), |
There was a problem hiding this comment.
[P2] Match the normalized Windows executable in the spawn fixture
On Windows disabledMcpServers spawns the extended-length form of the synthetic executable, while fixtureSpawn is registered with the unnormalized path and only matches exact strings. It falls through to a real spawn of the nonexistent synthetic-codex.exe and the new provider test fails with ENOENT before credential assertions. Normalize the intercepted path with the same executablePathForSpawn helper used by execution-worker-policy.test.ts, preserving the real production executable behavior.
515ae24 to
c14d94d
Compare
bd8e9b8 to
42c3a60
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 42c3a6004b
ℹ️ 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".
| recipe.knowledgeBasePaths ?? | ||
| (inheritedEnvironment.CODEX_SECURITY_KNOWLEDGE_BASE | ||
| ? [inheritedEnvironment.CODEX_SECURITY_KNOWLEDGE_BASE] | ||
| : undefined), |
There was a problem hiding this comment.
When resuming a scan whose saved recipe omits knowledgeBasePaths because the original scan had no knowledge base, this fallback imports the current CODEX_SECURITY_KNOWLEDGE_BASE value. The resumed discovery and reducer workers then receive a new authoritative input that was not part of the original scan, so changing the ambient environment between runs can change the scan's scope and findings. Distinguish fresh preparation from replay and treat an omitted saved value as an empty selection during resume.
AGENTS.md reference: AGENTS.md:L31-L35
Useful? React with 👍 / 👎.
| if (!isDeny) { | ||
| throw unsupportedParentSandbox( | ||
| "parent filesystem glob grants cannot be preserved", | ||
| ); |
There was a problem hiding this comment.
When a valid managed parent profile contains an absolute glob-shaped read or write grant alongside the required root-read entry, this branch rejects native Deep Scan preparation outright. Only denials are transported into the stricter child profile, and the same function already ignores ordinary path and special-path grants, so there is no grant-preservation requirement here; rejecting the known glob form merely prevents scans under otherwise compatible organization profiles. Ignore non-deny glob entries as well.
AGENTS.md reference: AGENTS.md:L43-L47
Useful? React with 👍 / 👎.
alandelong-oai
left a comment
alandelong-oai
left a comment
There was a problem hiding this comment.
Reviewed exact head 42c3a6004be5cbdb91ae0fbe18c9533aba7d1f14 against base c14d94d15e2d4d73726692f92558bcbbe29e11e5. Three fresh independent HIGH full actual-diff passes, including every changed test/fixture, plus root synthesis.
The two prior fixture findings are fixed in source: auth output directories now use 0700, and the provider fixture normalizes executable paths. The policy fixture now normalizes its path too; the Linux deny-glob test explicitly enables networking for the filesystem-denial assertion. Locally, five selected SDK tests and eight auth cases passed on macOS; nine portable checks, SDK build, types, format and source compatibility passed. The first local host bundle attempt failed because my setup omitted retained generated native JavaScript; after restoring the verified prior payload it passed. No test was rerun before that correction. This is not Windows/Linux success or an explanation of historical timeouts and cancellation. Two new P2 source findings below remain. The old fresh-home ENOENT source fix remains intact.
Hosted CI is recorded separately in the final review evidence; it is not inferred from these local or retained checks. Source approval, hosted CI and whole-stack/current-main integration remain separate. No product model, local Windows run, clean install, fresh native compilation or deployment validation was performed.
Remaining source findings:
- P2: a saved recipe with no knowledge-base selection imports the current ambient knowledge base.
- P2: a compatible managed profile with root read and a redundant absolute glob grant is rejected.
Hosted CI observation 2026-10-04T13:37:39.993325+00:00 to 2026-10-04T13:38:02.237036+00:00: this exact head was pending. No failed jobs were reported for this head in that observation. Later source/metadata readbacks are not CI refreshes.
| auth: recipe.auth, | ||
| cyberAccessProgram: recipe.cyberAccessProgram, | ||
| knowledgeBasePaths: | ||
| recipe.knowledgeBasePaths ?? |
There was a problem hiding this comment.
[P2] Preserve the saved empty knowledge-base selection
A saved recipe for a scan with no knowledge base omits knowledgeBasePaths: the touched API producer passes knowledgeBase?.sources and scanRecipe omits undefined values. Replaying that recipe after CODEX_SECURITY_KNOWLEDGE_BASE changes takes this fallback and prepares a new knowledge input that was absent from the saved scan. Distinguish fresh preparation from saved replay, and treat an omitted saved selection as empty. This finding establishes changed prepared replay input; it does not claim an observed model result or deployment effect.
| "a parent filesystem permission has an invalid glob", | ||
| ); | ||
| } | ||
| if (!isDeny) { |
There was a problem hiding this comment.
[P2] Ignore redundant absolute glob grants
A managed restricted parent with the required root-read entry plus an absolute glob_pattern read grant is rejected here. That grant adds no restriction beyond root read, and this function already ignores ordinary path and special-path grants while returning only denials for the stricter child profile. Validate the recognized glob shape and ignore compatible non-deny grants instead of rejecting native preparation. Keep the root-read requirement and validation of denial semantics. The finding is the local rejection of this supported metadata shape, not a claim about a particular deployed organization.
Consolidate approved Deep Scan changes into the parent topic branch.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 8e9a059fbb
ℹ️ 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".
| preparation = await workbench([ | ||
| "prepare-scan-completion", | ||
| "--scan-id", | ||
| scanId, | ||
| ]); |
There was a problem hiding this comment.
For a scan supplied through registeredScan with a non-null handoff claim, this call and the subsequent complete-scan call omit --claim-token; complete_scan_locked requires the matching claim and rejects either request. Consequently, once the claimed registration path is used, the native scan can finish its model work but cannot prepare or seal its results; carry registeredScan.handoffClaimToken through the publication context and all other mutating workbench calls.
AGENTS.md reference: AGENTS.md:L31-L35
Useful? React with 👍 / 👎.
| JSON.stringify({ | ||
| recipe, | ||
| userContext: scanOptions.scanPrompt, | ||
| ...(scanOptions.workflowId === undefined | ||
| ? {} |
There was a problem hiding this comment.
runPass sets deepScanPass: true, but registerScan neither picks that option nor emits the corresponding parentScanRole: "deep_pass" in this registration object. The workbench therefore stores the child with a null role, and the first refreshPasses call rejects it in savedPassIndex as “not an assigned Deep Scan pass,” so a production Deep Scan aborts after its first child batch. Forward the role during fresh child registration.
Useful? React with 👍 / 👎.
| snapshotDigest, | ||
| targetRevision, | ||
| scopeFileCount, | ||
| } = await registerScan({ |
There was a problem hiding this comment.
After this registration returns, options.onRegisteredScan is never invoked anywhere in the production client, although runPass relies on it to save the child's scan ID. If a child throws after registration, pass.scanId remains undefined and the retry starts a fresh scan in the same populated pass directory instead of setting resumeScanId; the composition tests mask this by explicitly invoking the callback in their fake client. Invoke the callback immediately after successful registration so in-flight and resumed workers retain their identity.
AGENTS.md reference: AGENTS.md:L31-L35
Useful? React with 👍 / 👎.
| let active = this.active.get(input.scan.scanId); | ||
| if (!active) { | ||
| const controller = new AbortController(); | ||
| const promise = Promise.resolve() |
There was a problem hiding this comment.
This active map deduplicates callers only within one NativeScanHost; a second MCP host or process holding the same scan claim can simultaneously prepare and run the same scan directory. The new acquireScanExecution helper is otherwise unused, so nothing prevents both processes from mutating the same checkpoints and canonical artifacts. Acquire that lock for input.scan.scanDir before preparation and retain it until client cleanup completes.
AGENTS.md reference: sdk/typescript/AGENTS.md:L21-L24
Useful? React with 👍 / 👎.
Consolidate Deep Scan changes and their reviewed follow-up fixes into the parent topic branch.
mldangelo-oai
changed the title
Preserve the child source tree while recording its consolidated parent.
885bf9b
into
mdangelo/codex/pr939-stack-02-severity-history
There was a problem hiding this comment.
🛡️ Codex Security Review · Automatically triggered
Here are some automated security review suggestions for this pull request.
Reviewed commit: f3f7540584
ℹ️ About Codex security reviews in GitHub
This is an experimental Codex feature. Security reviews are triggered when:
- You comment "@codex security review"
- A regular code review gets triggered (for example, "@codex review" or when a PR is opened), and you’re opted in so security review runs alongside code review
Once complete, Codex will leave suggestions, or a comment if no findings are found.
| const source = expandHome(path, inheritedEnvironment); | ||
| return (await gitMarkerRoot(source, signal, "outermost")) ?? source; |
There was a problem hiding this comment.
🛡️ Codex Security Review · Automatically triggered
When an operator selects a non-Git knowledge-base file whose attacker-controlled parent is on PATH, protect that parent before resolving Codex. The final root-expansion change still falls back to source itself for a standalone file, so a sibling codex is outside the protected file path and is accepted and executed with the user's authority. Fresh evidence is this non-Git-file fallback; the added regression covers only a knowledge directory beneath .git. Use the file's canonical parent as the fallback root.
SECURITY.md reference: SECURITY.md:L11-L14
Dismiss this finding: Reply with @codex security dismiss <reason> [context]. Codex will resolve this conversation automatically; GitHub may require a page refresh to show the result.
Valid reasons: false-positive, duplicate, out-of-scope, compensating-control, risk-accepted, or other. Example: @codex security dismiss duplicate Already flagged by another review
What each reason means
false-positive— Not a vulnerabilityduplicate— Already tracked elsewhereout-of-scope— Outside this review's scopecompensating-control— Mitigated by another controlrisk-accepted— Risk intentionally acceptedother— Another reason; context required
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
codex-security/sdk/typescript/src/api.ts
Lines 603 to 607 in f3f7540
Restore the model in recovered workflow metadata
When complete-scan succeeds but writing the FindingWorkflow stage receipt fails, the next invocation enters this completed-scan recovery branch and synthesizes new metadata. It restores the thread, usage, and cost but omits the model even when it is available from the saved cost or scan record, so the returned ScanResult.turnResult.model differs from the original result and that degraded metadata is then persisted by workflow.complete. Include the saved model when reconstructing the turn result.
ℹ️ 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".
| environment.CODEX_HOME = await fs.realpath( | ||
| expandHome(codexHome.trim(), environment), | ||
| ); |
There was a problem hiding this comment.
When CODEX_HOME explicitly names a directory that has not been created yet, this realpath throws ENOENT before prepareAmbientRuntime reaches its existing mkdir(requestedHome, { recursive: true }), so a valid fresh API-key setup cannot start a native scan. The default-home case from the earlier review is now handled, but the explicit configured-home path still fails; normalize the value here without requiring existence, or create it before canonicalizing it.
AGENTS.md reference: AGENTS.md:L11-L12
Useful? React with 👍 / 👎.
| if child["seal_manifest_digest"] is not None or ( | ||
| isinstance(manifest_scan, dict) | ||
| and ( | ||
| manifest_scan.get("sealedAt") is not None or manifest_scan.get("artifacts") is not None |
There was a problem hiding this comment.
When an interrupted child has written a valid unsealed manifest with scan.artifacts: [], this predicate classifies it as sealed. _prepare_scan_finalization itself removes an empty list before deciding seal status, and the resume path likewise treats it as unsealed; here, stopped Deep Scan recovery skips merge_saved_results, where pending checkpoints and candidates are retained, and instead attempts sealed finalization, which can fail or omit the child's unfinished evidence. Treat only a nonempty artifact list as sealed here and in the equivalent recovery predicates.
AGENTS.md reference: AGENTS.md:L11-L12
Useful? React with 👍 / 👎.
Summary
Run SDK and plugin Deep Scans through one lifecycle built from ordinary Standard child scans. Save accepted findings, child membership, matching decisions, and accounting so an interrupted scan can continue or finish publication without repeating completed discovery or accepted matching.
Draft publication now commits a complete snapshot before exporting files. Recovery preserves accepted assessments and original evidence, and explicit recovery can incorporate corrected child results without changing unrelated parent or sibling findings.
Changes
Testing
Completed on the integrated changes:
Full Python suite: 1,673 passed, 8 skipped, plus 162 subtests passed.
Full MCP suite: 172 passed, no failures or skips.
Ruff lint and format, MCP TypeScript check, SDK
build:ci, SDK formatting and type checks: passed.Portable plugin source compatibility and its 9 tests: passed.
Plugin and production builds, the 617-entry package check, and installed-package smoke tests: passed.
Focused recovery, worker-launch, publication-retry, and concurrent-draft regression tests: passed.
Three fresh native code reviews and independent verification on the reviewed integration commit: passed, with no findings.
Full SDK suite in both seed-12345 and randomized order, three CI shards each: 3,990 passed, 48 skipped per run across 177 files. One randomized shard hit a 30-second timeout during high host load; rerunning that entire shard alone with the same seed and unchanged timeout passed. The original timeout log was retained.
Risk and rollout
The base PR remains open against
main. New changes merged intomainduring consolidation overlap this work and require a separate conflict-resolution pass before the base PR can merge.Live Deep Scan continuation requires version-3 checkpoints, the original checkout, and the owning merge-session logs once matching has started. Finish unfinished coordinator/version-2 scans with their original version or start a new scan. Historical artifacts remain readable; completing an older sealed Deep Scan with required cost tracking still requires a usable receipt. A saved accepted version-3 merge can retry publication without repeating discovery or matching.
SDK callers must allow
ScanResult.threadIdandScanResultOptions.threadIdto benull. Draft clients must reuse finding identities and coverage/deferred IDs for updates; missing IDs create new rows, except an unambiguous deferred candidate can reuse its existing ID.postScanPromptstarts a separate conversation with explicit saved-result context and a working directory underartifacts/follow-up/<id>. Requested repository changes retain the scan's existing permissions; sealed scan artifacts remain protected.Workbench compatibility changes:
begin-deep-scanreturns the SDK scan record and removes--available-parallelismand--workflow-version.get-deep-scan,claim-deep-scan-coordinator,upsert-deep-scan-worker,claim-deep-scan-dedup,commit-deep-scan-dedup,finish-deep-scan,fail-deep-scan, andrecord-deep-scan-publication-failure, plus coordinator-generation and MCP artifact-writer dispatch.write-scan-draft --scan-id <id>accepts{documents, checkpoint}on stdin when--draft-pathis omitted; staged-file input remains supported.--claim-tokento continuation/accounting operations,--defer-publicationtocancel-scanandfail-scan, and--after-stop/--cost-jsontopreserve-scan-results. Ordinary stop commands continue publishing available results by default.--workersnow controls a batch of independent Standard scans, merged in original order before the next batch starts.--stop-after-no-newcounts successfully merged scans and is checked after each batch, so a batch can pass the threshold; failed scans consume the discovery-run budget without counting toward that threshold. Publicscans resume SCAN_IDremains limited to eligible Deep Scans. Deep Scan option defaults are unchanged. Database migrations apply through the existing migration runner; already-overwritten historical assessments cannot be reconstructed. Source plugin builds require the SDK checkout, while the distributed plugin contains its runtime dependencies.Public disclosure review