refactor(deep-scan): compose ordinary scans and merge their results - #939
mldangelo-oai wants to merge 36 commits into
Conversation
|
@codex review |
|
Codex Review: Didn't find any major issues. Another round soon, please! Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
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". |
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. |
|
@codex review |
![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: 53b4f2a23f
ℹ️ 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".
| reportCompletedCost(pass.directory, result.cost); | ||
| executionSignal.throwIfAborted(); |
There was a problem hiding this comment.
When the first completed child pushes the combined cost above maxCostUsd, reportCompletedCost synchronously triggers the parent's cost abort, so this check throws after adding the child to accepted but before mergePending can consolidate or publish it. Because no merge turn has started yet, #run also has no budgetRecovery.threadId, skips complete-budget-exhausted-scan, and fails the parent instead of returning the completed child's findings with partial coverage. Defer propagation until the accepted pass is merged and published.
AGENTS.md reference: sdk/typescript/AGENTS.md:L25-L26
Useful? React with 👍 / 👎.
| await Promise.all( | ||
| staged.map((path) => artifactWriter!.remove(path)), | ||
| ); |
There was a problem hiding this comment.
If either staged-file removal fails after write-scan-draft has successfully published the canonical parent draft, this finally block rejects the publish operation. runDeepScans then marks the composition failed and ultimately throws even if its catch-path publication retry succeeds, so a transient cleanup error turns an otherwise valid Deep Scan into a failed scan. Treat removal of these now-unreferenced staging files as best-effort cleanup and report the error without replacing the scan result.
Useful? React with 👍 / 👎.
| if (record.progress.status === "complete") | ||
| reportCompletedCost(pass.directory, record.cost ?? null); | ||
| else if (record.cost) input.onCost(pass.directory, record.cost); |
There was a problem hiding this comment.
If the process stops after fail-scan commits a child's failed status but before pass.failed, consecutiveErrors, and the checkpoint are saved, a resume observes the child as failed here but never copies that status back into the checkpoint. The pass is then excluded from unfinished without contributing to the consecutive-error state; at the run cap this can make an all-failed Deep Scan proceed to an empty successful aggregate, and before the cap it incorrectly resets the configured failure streak. Reconcile persisted failed child records into the pass/error state during refresh.
Useful? React with 👍 / 👎.
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 690cd7b6fc
ℹ️ 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".
| ...(latestCost | ||
| ? ["--cost-json", JSON.stringify(latestCost)] | ||
| : []), |
There was a problem hiding this comment.
When a discovery pass exhausts its retries without ever emitting onCost, this branch persists no cost and marks the pass failed without reporting null or raising ScanCostTrackingError; the resume path likewise ignores failed records with no stored cost. A cost-limited parent therefore treats its combined cost as complete and may continue or finish after that pass consumed unmetered tokens, bypassing the requested --max-cost. Treat the absent failed-pass cost as unavailable just as for a completed child.
AGENTS.md reference: sdk/typescript/AGENTS.md:L25-L25
Useful? React with 👍 / 👎.
| const codexHome = await realpath( | ||
| environment.CODEX_HOME ?? join(homedir(), ".codex"), | ||
| ); |
There was a problem hiding this comment.
When the MCP process inherits CODEX_HOME="", this expression passes the empty string to realpath, which fails with ENOENT and prevents every native Deep Scan from starting. The normal SDK path resolver uses the established trimmed-value-or-~/.codex behavior, so this native path should do the same rather than interpreting an empty environment variable as a filesystem path.
AGENTS.md reference: AGENTS.md:L30-L33
Useful? React with 👍 / 👎.
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: f69cb00cc3
ℹ️ 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 (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 after the earlier empty-value report: this replacement still calls realpath when CODEX_HOME contains only whitespace, so native Deep Scan fails with ENOENT while the established configuredCodexHome path trims the value and falls back to ~/.codex. Trim before deciding whether to canonicalize, and preserve the same behavior in worker-launch tests.
AGENTS.md reference: AGENTS.md:L30-L34
Useful? React with 👍 / 👎.
| @@ -2395,6 +3080,7 @@ export class CodexSecurity { | |||
| } finally { | |||
| budgetAbortController.abort(); | |||
| deepProgressTracker?.stop(); | |||
| releaseExecution?.(); | |||
There was a problem hiding this comment.
If the execution-lock release throws—for example, the new Windows release explicitly throws when the native handle close returns an error—this unguarded call runs in finally outside the surrounding catch and replaces either a successfully sealed result or the scan's real failure. Handle this cleanup like the guarded cleanup immediately below so callers do not receive a lock-close error after their scan has already completed.
Useful? React with 👍 / 👎.
Keep client cleanup awaited while preserving the original scan outcome. Remove the optional execution-lock guard after reviewing its native producer. Automatic workflow execution is deferred for this draft update; the skip marker is not a passing CI result. Required verification remains separate.
|
@codex review |
|
Codex Review: Didn't find any major issues. More of your lovely PRs please. Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
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". |
1 participant
Summary
Deep Scan runs ordinary Standard scans through the shared SDK lifecycle, then merges their completed reports. This removes the separate worker and reducer lifecycle while retaining saved settings, stopping rules, original findings and child coverage.
Changes
CODEX_HOMEoverrides as unset. Preserve other paths and the caller's environment.Testing
Validation of cleanup head
6754e4d5:build:cicompiler command passed through the pinned compiler entrypoint. Plugin source compatibility passed, along with all nine tests of that checker.Full regression, the full installed-package smoke suite and Windows verification remain pending. The synthetic child-process fixture skips Windows; the shared home-selection matrix remains enabled there. These checks do not establish performance or detection-quality improvements.
Risk and rollout
This remains a draft. Public options, stopping defaults and saved-scan contracts are preserved. The plugin and bundled SDK must ship together. The whitespace-only home issue also existed in the previous executor; this corrects the replacement path.
The final commit includes
[skip ci]to defer automatic broad-suite execution for this draft update. Only the scoped checks above are claimed; required CI checks remain pending. Existing Windows failures and cancellations have unresolved causes, and no timeout increase is included.Public disclosure review