Repository navigation
refactor(sdk)!: run Deep Scan passes as Standard scans - #1162
Conversation
|
@codex review |
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. |
There was a problem hiding this comment.
Copilot review overview
🟡 Changes recommended
A replacement test references a nonexistent Python fixture, and the resume documentation omits a session-log requirement still enforced by the implementation.
Review effort: Balanced
Findings: 1
Open (2)
What changed in this PR
Refactors Deep Scan to compose Standard SDK scans, reuse shared lifecycle/recovery paths, and persist merged results and accounting.
Changes:
- Routes Deep Scan passes through Standard scan execution, cancellation, publication, and recovery.
- Refreshes runtime/plugin settings and preserves worker configuration across resumed operations.
- Replaces coordinator-focused coverage with composition, recovery, concurrency, and packaging tests.
| File | Description |
|---|---|
sdk/typescript/tsconfig.json |
Includes merge evaluation scripts. |
sdk/typescript/tests-ts/workbench-canonical-paths.test.ts |
Removes retired coordinator path checks. |
sdk/typescript/tests-ts/support/workbench-fixture.ts |
Loads shared Python fixture. |
sdk/typescript/tests-ts/support/workbench-fixture.py |
Provides migrated database fixtures. |
sdk/typescript/tests-ts/support/scan-publication.ts |
Adds draft publication helper. |
sdk/typescript/tests-ts/support/api-events.ts |
Updates event and plugin fixtures. |
sdk/typescript/tests-ts/support/api-client.ts |
Adds execution and cancellation setup. |
sdk/typescript/tests-ts/stopped-scan-results.test.ts |
Replaces stopped-scan coverage with fixture execution. |
sdk/typescript/tests-ts/skeleton.test.ts |
Updates package and Windows matrix assertions. |
sdk/typescript/tests-ts/scan-sealed-resume.test.ts |
Removes superseded recovery tests. |
sdk/typescript/tests-ts/scan-resume-permissions.test.ts |
Updates resumed-worker behavior. |
sdk/typescript/tests-ts/scan-recovery.test.ts |
Updates context caching expectation. |
sdk/typescript/tests-ts/runtime.test.ts |
Tests source-level semantic preparation. |
sdk/typescript/tests-ts/repository-findings.test.ts |
Uses migrated database fixtures. |
sdk/typescript/tests-ts/prompt-only-start-timeout.test.ts |
Removes retired runtime test. |
sdk/typescript/tests-ts/plugin-root.ts |
Removes bundled-source extraction helper. |
sdk/typescript/tests-ts/plugin-report-limits.test.ts |
Updates source contract fixture. |
sdk/typescript/tests-ts/mock-scan.test.ts |
Expects cancellation status. |
sdk/typescript/tests-ts/execution-worker-policy.test.ts |
Removes retired worker-policy tests. |
sdk/typescript/tests-ts/execution-preparation.test.ts |
Removes superseded preparation tests. |
sdk/typescript/tests-ts/deep-scan-worker-shutdown.test.ts |
Removes coordinator worker tests. |
sdk/typescript/tests-ts/deep-scan-windows-executable.test.ts |
Removes retired executable tests. |
sdk/typescript/tests-ts/deep-scan-timestamp-compat.test.ts |
Removes coordinator timestamp tests. |
sdk/typescript/tests-ts/deep-scan-reducer-recovery.test.ts |
Removes retired reducer tests. |
sdk/typescript/tests-ts/deep-scan-parent-denials.test.ts |
Removes retired worker sandbox tests. |
sdk/typescript/tests-ts/deep-progress.test.ts |
Covers externally stopped scans. |
sdk/typescript/tests-ts/deep-config.test.ts |
Removes runtime snapshot-writing tests. |
sdk/typescript/tests-ts/completed-scan-handoff.test.ts |
Removes retired handoff test. |
sdk/typescript/tests-ts/compact-diff-scan.test.ts |
Verifies duplicate identity rejection. |
sdk/typescript/tests-ts/cli-workbench.test.ts |
Tests public recipe projection. |
sdk/typescript/tests-ts/cli-project-config.test.ts |
Tightens fixture permissions. |
sdk/typescript/tests-ts/cli-deep-scan-summary.test.ts |
Reads composition checkpoint summaries. |
sdk/typescript/tests-ts/cli-authentication.test.ts |
Supplies skill working directories. |
sdk/typescript/tests-ts/build-plugin.test.ts |
Adds standalone MCP smoke coverage. |
sdk/typescript/tests-ts/api-surface.test.ts |
Configures isolated state. |
sdk/typescript/tests-ts/api-post-scan.test.ts |
Removes superseded follow-up test. |
sdk/typescript/tests-ts/api-policy.test.ts |
Tests native provider configuration. |
sdk/typescript/tests-ts/api-operation-refresh.test.ts |
Tests reused-client refresh behavior. |
sdk/typescript/tests-ts/api-environment.test.ts |
Covers raw environment values. |
sdk/typescript/tests-ts/api-credentials.test.ts |
Expands startup and concurrency coverage. |
sdk/typescript/tests-ts/api-cancellation-order.test.ts |
Tests failure/cancellation ordering. |
sdk/typescript/tests-ts/api-attribution-concurrency.test.ts |
Covers concurrent child attribution. |
sdk/typescript/tests-support/process-environment.d.mts |
Declares environment test helper. |
sdk/typescript/src/scan-registration.ts |
Separates resume-session validation. |
sdk/typescript/src/scan-publication.ts |
Consolidates publication and result loading. |
sdk/typescript/src/scan-preparation.ts |
Updates shared scan prompts and plugin paths. |
sdk/typescript/src/scan-monitoring.ts |
Maps Deep child progress to discovery. |
sdk/typescript/src/scan-events.ts |
Separates turn execution from result collection. |
sdk/typescript/src/project-config-schema.ts |
Adds cyber_access_program. |
sdk/typescript/src/execution-preparation.ts |
Removes legacy coordinator configuration. |
sdk/typescript/src/deep-progress.ts |
Polls terminal state more frequently. |
sdk/typescript/src/deep-config.ts |
Removes legacy snapshot writing. |
sdk/typescript/src/cli.ts |
Restricts resume and reads composition summaries. |
sdk/typescript/scripts/smoke-package.mjs |
Tests shared installed runtime. |
sdk/typescript/scripts/fixtures/package-behavior.mjs |
Updates cancellation expectations. |
sdk/typescript/scripts/fixtures/mcp-smoke.mjs |
Adds reusable MCP smoke input. |
sdk/typescript/scripts/fixtures/mcp-smoke.d.mts |
Declares MCP smoke types. |
sdk/typescript/scripts/ci-test-durations.json |
Refreshes CI timing data. |
sdk/typescript/scripts/check-package.mjs |
Updates expected package contents. |
sdk/typescript/schemas/project-config.schema.json |
Exposes the new project setting. |
sdk/typescript/README.md |
Documents shared Deep Scan behavior. |
plugins/codex-security/tests/test_workbench_db.py |
Replaces coordinator budget fixtures. |
plugins/codex-security/scripts/workbench_saved_results.py |
Updates legacy draft writes. |
plugins/codex-security/scripts/workbench_db.py |
Completes budget-limited compositions. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| new URL( | ||
| "../../../plugins/codex-security/tests/fixtures/runtime/stopped_scan.py", | ||
| import.meta.url, |
| The scan must still be `running`, with its original checkout, output directory, | ||
| and owning Codex session available in the same Codex Security state directory. | ||
| and Codex Security state directory available. |
![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: 1ee152f6f1
ℹ️ 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".
| """, | ||
| (str(manifest_path), scan_id), | ||
| ) | ||
| mark_deep_aggregate_ready(state_dir, scan_id, scan_dir) |
There was a problem hiding this comment.
This test still calls begin-deep-scan, which creates a legacy deep_scan_runs row, and then this new call writes a v3 composition checkpoint. complete_scan_locked prioritizes the legacy row and therefore rejects completion because the legacy coordinator was never marked successful. Running python -m pytest test_workbench_db.py -q deterministically fails here with “Deep Scan discovery orchestration must finish...”; construct a non-legacy registered scan for this checkpoint test or complete the legacy coordinator instead.
Useful? React with 👍 / 👎.
dbd6160 to
a76663c
Compare
0ff249f to
0eab5dd
Compare
a76663c to
5bfdb5f
Compare
0eab5dd to
2d36b8b
Compare
faizan-oai
left a comment
faizan-oai
left a comment
There was a problem hiding this comment.
Reviewed the SDK activation at 5bfdb5f, including registered output reuse, sealed producer-version handling, and the post-registration sealed-result path. The referenced stopped-scan fixture is present at this head. Integrated SDK composition/checkpoint, typechecking, and workbench recovery checks pass. Approval covers this increment; the separately reproduced shared-install, cancellation, and recovery issues remain tracked on #1141, #1154, #1158, and #1160.
5bfdb5f to
5b35bc5
Compare
2d36b8b to
13b948b
Compare
5b35bc5 to
e7615ea
Compare
13b948b to
864c67b
Compare
There was a problem hiding this comment.
Reviewed this runtime transition with three independent passes and root verification, including fresh and resumed discovery and merge worker processes. One new P2 failure/cancellation ordering regression is reproduced below. The earlier missing stopped-scan fixture and obsolete completion assertion are corrected at this head. The resume documentation still needs the existing clarification about retaining owning session logs.
Validation: 529 SDK tests passed (1 skipped) across 18 files; 137 Python tests passed. CI TypeScript build, plugin build, Ruff check/format with the required 0.16.8 version, source compatibility check, and all 9 compatibility tests passed. Child-process tests covered permissions, credentials, environment snapshots, selected executables, worker limits, and resumed workers. A separate base/head probe confirms that an ordinary or permission failure followed by cancellation is preserved by the base but converted to cancellation by this head. No repository files were changed.
| }; | ||
| } catch (error) { | ||
| throwScanFailure(error, options); | ||
| throwIfAborted(options.signal, options.scanDir); |
There was a problem hiding this comment.
[P2] Preserve the original failure when cancellation arrives during unwind
The unconditional abort check in this catch replaces an already-thrown stream failure with the caller cancellation. I reproduced this with the event iterator rejecting an ordinary error (and separately ScanPermissionError) while its finally handler aborts the caller signal: the base retains the original cause and calls fail-scan, whereas this head returns the later cancellation reason and calls cancel-scan. This loses the actual failure and changes the persisted scan status. Only translate errors caused by cancellation; preserve errors that occurred first.
e7615ea to
f1519ef
Compare
864c67b to
6ff2600
Compare
alandelong-oai
left a comment
alandelong-oai
left a comment
There was a problem hiding this comment.
Three independent restack passes and root verification are complete. The failure/cancellation fix is partial: the original reproduction now correctly persists fail-scan instead of cancel-scan, but the public error still loses the preceding failure.
For both an ordinary execution failure and ScanPermissionError followed by caller cancellation during iterator unwind, this head and the integrated tip return ScanInterruptedError with cause "caller canceled later". The saved pre-change control returned the same wrapper with the original execution/permission failure as its cause. The final API catch still calls throwIfAborted for an aborted signal even when the new canceled classification is false. Preserve that original failure at the public boundary too. Approval remains withheld for this remaining part of the existing P2.
The probe uses the actual SDK failure path and synthetic event/workbench adapters; no model calls. The persisted failure classification is verified fixed and should be retained.
f1519ef to
8b71618
Compare
6ff2600 to
528e821
Compare
There was a problem hiding this comment.
Re-reviewed the real PR diff at 8b7161809baad6afb405d40de5e2093920c5d241. Three independent high-reasoning passes covered the complete changed patch and tests; root source checks and focused reproductions informed this decision.
The persisted failure status is fixed, but the public error still replaces a preceding execution or permission failure with a later caller cancellation reason. The current API probe persists fail-scan in both cases while returning cause="caller canceled later". This file is identical at this PR head and the integrated tip. Approval remains withheld for the existing error-cause finding.
Validation across touched paths at branch tip 06e75cfe: 1,356 SDK tests, 867 Python tests plus 100 subtests, and 108 MCP tests passed; builds, types, portable checks and installed-package checks passed. These are integration checks at the branch tip, not a claim that every intermediate PR or its CI is clean. Latest workflow runs for this unchanged head were verified successful at 2026-10-02 01:26 UTC. Earlier failed or canceled job records are superseded; this CI status correction does not alter the review decision.
8b71618 to
dd10070
Compare
a1582c0 to
f3838de
Compare
dd10070 to
055646b
Compare
alandelong-oai
left a comment
alandelong-oai
left a comment
There was a problem hiding this comment.
Three fresh independent full-diff reviews plus root verification at 055646b1fb69.
The original failure survives later cancellation: the execution probe persists fail-scan and exposes the original execution error as its cause; the permission probe returns the original permission error. This PR API file remains byte-identical to the tested tip. The additional diff removes the legacy preheld-lock configuration branch while activating isolated runtime configuration; three fresh full-diff passes and root reconciliation found no new regression. The prior approval hold is cleared.
Current tip 355ec321 passed 75 report-projection tests, portable source checks, SDK build:ci and plugin build. The immediately preceding tip fec36f1d passed 1,360 SDK tests, 873 Python tests plus 100 subtests, 109 MCP tests, builds/types and the clean 600-entry installed-package check. Only report_projection.py and its test changed between those tips; SDK/MCP runtime files are byte-identical. Both descend from main 008a8b4d. These are integration checks, separate from own-head probes. No native Windows local run.
No serious outstanding finding introduced by this PR. Code review approval does not establish CI or merge readiness.
f3838de to
9e2d61b
Compare
055646b to
6aea655
Compare
alandelong-oai
left a comment
alandelong-oai
left a comment
There was a problem hiding this comment.
Completed three fresh independent full-diff reviews and root verification at 6aea6553b1fa.
No actionable introduced finding survives the independent reviews and root reconciliation of this exact patch and inherited changes in touched files.
Integrated tip 6ef8cbb8, based on 008a8b4d, passed 1,388 SDK tests, 879 Python tests plus 100 subtests, 109 MCP tests, portable checks, builds and the clean 600-entry installed-package check. These are branch-tip integration checks, separate from own-head probes; no local Windows execution was performed.
No serious outstanding finding introduced by this PR. Code approval is separate from CI and merge readiness.
6aea655 to
9095a90
Compare
9e2d61b to
57a9f5c
Compare
alandelong-oai
left a comment
alandelong-oai
left a comment
There was a problem hiding this comment.
Completed three independent full-diff passes and root verification at 9095a90bef6b.
No actionable introduced finding survives the independent reviews and root reconciliation of this exact patch and inherited changes in touched files.
Branch tip 3e0083d9, based on 008a8b4d, passed 1,392 SDK tests, 881 Python tests plus 100 subtests, 109 MCP tests, builds, portable source checks and the 600-entry installed-package check. These are tip checks, separate from own-head probes. No local Windows run was performed. The tip excludes the dependency update merged separately in #1184; combined validation remains outstanding.
No serious outstanding finding introduced by this PR. Code approval is separate from CI and merge readiness.
Consolidate approved Deep Scan changes into the parent topic branch.
1d61154
into
mdangelo/codex/pr939-split-38-completed-results
Part 39 of 46. Previous: #1161 · Next: #1163 · Stack index
Summary
Run SDK Deep Scans as batches of ordinary Standard scans, then combine their saved results into the parent report using the existing finding matcher.
Changes
Testing
Local checks for the implementation:
Five cancellation-order cases and 33 broader event/composition cases passed, including ordinary and permission errors followed by caller cancellation.
Checks against the complete stack passed: SDK and MCP typechecks, formatting, Ruff, the SDK build, portable plugin compatibility and its tests, focused recovery/accounting tests, and local-artifact package smoke using exact cached dependencies. The package check was not a clean registry install. See this PR’s Checks tab for CI results for its current commit.
Risk and rollout
Public
scans resume SCAN_IDremains limited to Deep scans. Live recovery uses version-3 checkpoints; older unfinished Deep Scans require their original version or a new scan. Started merge sessions still need their original Codex session logs. Sealed historical reports remain readable, but completing an older sealed Deep Scan with required cost tracking needs a usable final receipt.The project-config field
codex.cyber_access_program, when supplied, must be a nonempty string and has no default. Deep Scan setting defaults are unchanged. The plugin entrypoint switches in the next PR.Public disclosure review