refactor(deep-scan): compose ordinary scans and merge their results - #939
mldangelo-oai wants to merge 135 commits into
Conversation
…/codex/simplify-deep-scan
![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: 54eb9b6399
ℹ️ 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".
| 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.
Fresh evidence beyond the earlier blank-value reports: when the native MCP host inherits a valid shorthand such as CODEX_HOME=~/.codex (or a valid path padded with whitespace), this passes the literal value to realpath, which does not perform shell expansion and fails with ENOENT before either fresh or resumed Deep Scan workers start. The established configuredCodexHome resolver in sdk/typescript/src/auth.ts trims and expands these values, so canonicalize that resolved path instead and cover the inherited worker environment.
AGENTS.md reference: AGENTS.md:L30-L34
Useful? React with 👍 / 👎.
|
@codex review Please review the current head, including the saved-result completion and runtime lifecycle fixes. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 03f32c951a
ℹ️ 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".
| function isCodexCybersecurityPolicyRefusal(error: unknown): boolean { | ||
| const message = error instanceof Error ? error.message : String(error); | ||
| // SDK diagnostics can include repository text; match complete runtime refusals. | ||
| return [ |
There was a problem hiding this comment.
When a child receives the existing Codex policy message Request flagged for possible cybersecurity risk.—the form already exercised by tests-ts/fixtures/codex-review.mjs—this exact allowlist returns false because it includes only the This content... and This request has been... variants. runPass then retries the blocked request up to three times and may continue sibling or later discovery instead of applying the intended fatal policy stop; include this runtime form and keep the strict vocabulary aligned with the shared refusal helper.
AGENTS.md reference: sdk/typescript/AGENTS.md:L20-L20
Useful? React with 👍 / 👎.
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: a066b6a1e5
ℹ️ 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 codex = await resolveTrustedCodex( | ||
| inheritedEnvironment, | ||
| input.scan.targetPath, | ||
| ); |
There was a problem hiding this comment.
🛡️ Codex Security Review · Automatically triggered
Fresh evidence beyond the prior target-local executable report: the shipped Deep Scan skill explicitly tells scoped scans to pass the scoped directory itself as targetPath. Here that narrower path is the only protected root. If host PATH contains another attacker-controlled directory in the same repository (for example /repo/node_modules/.bin) and the scan targets /repo/src, the resolver accepts that directory's codex; native preparation then uses it as CODEX_CLI_PATH before any worker sandbox. A repository author can therefore gain current-user execution and read forwarded API keys or stored credentials when an operator starts a scoped Deep Scan. Protect the enclosing Git worktree, not only targetPath.
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 👍 / 👎.
Consolidate saved-result accounting, report rendering, publication fixtures, and build setup while removing retired compatibility paths and redundant code. Completed historical reports and credential, path, artifact-integrity, and per-scan isolation protections remain supported. Active v1 recovery and unreleased path-inferred database snapshots are intentionally no longer supported, as documented in the pull request. Validation: 45 CI jobs passed with two expected skips at f5737c4. Three native code reviews and independent verification found no actionable regressions. Codex security review passed; the GitHub code review was still running at merge preparation, with its prerelease migration concern independently triaged.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: e367e17997
ℹ️ 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".
| yield* platform === "win32" | ||
| ? resolveWindowsDirectFromSearchPath( | ||
| searchPath, | ||
| executableName, | ||
| originalCwd, | ||
| ) | ||
| : resolveFromSearchPath(searchPath, executableName, originalCwd); |
There was a problem hiding this comment.
Fresh evidence after the earlier fix: this final implementation again searches Windows PATH only for a directly present codex.exe. When Codex is installed through the supported npm/managed-package layout, the PATH directory contains shims while the executable is under node_modules/@openai/codex; cached installations likewise require their dedicated lookup. With no explicit CODEX_CLI_PATH, both fresh and resumed native Deep Scans therefore report that no external executable exists. Restore the package, managed-root, and cached-install candidates before rejecting startup.
AGENTS.md reference: AGENTS.md:L30-L34
Useful? React with 👍 / 👎.
Summary
Deep Scan runs ordinary Standard scans through the shared SDK lifecycle and merges their completed reports. The parent preserves settings, findings, coverage, stopping decisions, and accounting across interruption and recovery.
This includes #1072, #1073, #1074, and #1075.
Changes
Testing
Validation completed locally. The final CI-only follow-up removes the redundant Windows test-step timeout; SDK, plugin, runtime, and test sources are unchanged from the validated commit (
907ac45a):CI-only follow-up: all 302 workflow contract tests passed; YAML comparison, formatting, and source-tree equivalence checks passed.
Full Python suite: 1,201 passed, eight platform skips, and 109 subtests passed. The final worker, refusal-message, and legacy-accounting corrections change only TypeScript source and tests. Plugin-source equivalence was checked against the tested commit.
Full SDK suite: 3,542 passed, 46 platform/integration skips, and zero failures in each of the seeded (
12345) and randomized (4281723884) runs; each made 76,177 assertions.All five portable plugin checks passed: Ruff lint and format, SDK
build:ci, source compatibility, and compatibility tests. Package build, installed-package smoke, TypeScript/generated checks, and formatting passed.Red/green regressions cover unknown final usage, incomplete custom-validation drafts, orderly process exit and late failures, retryable repository diagnostics, and supported runtime refusals. The final composition suite passed all 120 cases, including child cancellation, merge stopping, and completion-order recovery. Thirty focused recovery cases passed, covering legacy conversion, unknown historical cost, restored logs, saved receipts, and sealed/terminal recovery. Fresh/resumed CLI and SDK child-launch checks, native workers, and concurrent scan isolation passed. An actual pinned Codex runtime verified the worker configuration, whose preparation source is unchanged in the final commit: ordinary Standard retained the workbench, Deep discovery and merge disabled it, and an unrelated synthetic MCP remained available in all three roles. The original configuration stayed unchanged.
Authenticated QA of the validated package completed one Deep discovery pass and a distinct merge session against a synthetic control. Both expected findings were retained exactly once with complete coverage. CLI, database, and history accounting matched the two actual session receipts; both sealed contracts, package and target hashes, and process cleanup passed. Earlier authenticated QA covered a two-pass vulnerable synthetic control, a fixed control, and custom validation; artifact digests, target integrity, and session accounting passed. These prior runs retain their exact package provenance.
Projection equivalence passed for 24 synthetic cases and seven saved real Deep Scan responses, including pagination and selected findings. Selected real-result views used 29 database queries instead of 52. Earlier stack QA covered parallel Deep scans, fixed controls, interruption/resume, cost limits, cancellation, native MCP retries, and ordinary-prompt equivalence.
Three fresh native reviews and a separate verification pass completed with no remaining findings on the final local commit (
a066b6a1). GitHub CI runs the cross-platform matrix.Risk and rollout
Ship the plugin and bundled SDK together. Migrations 42 and 43 are append-only. Public commands, flags, output schemas, and saved checkpoint extensions remain supported. An empty capped result can have a null thread ID and retains partial coverage.
The change affects discovery, recovery, accounting, worker settings, and publication. Model merge input and guidance change; this PR does not claim improved scan precision, recall, or end-to-end latency. Live QA uses small synthetic repositories on Linux.
Public disclosure review