Repository navigation
refactor(deep-scan)!: run plugin Deep Scans through the SDK - #1163
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
🔵 Needs a closer look
The large cross-language lifecycle, recovery, authentication, and artifact migration requires final human validation despite extensive tests.
Review effort: Balanced
Findings: None
What changed in this PR
This PR replaces the plugin’s legacy Deep Scan coordinator with the shared SDK scan lifecycle, including execution, recovery, cancellation, completion, and artifact handling.
Changes:
- Routes plugin Deep Scans through SDK-managed Standard child scans and aggregate completion.
- Retires coordinator-specific commands, publication adapters, and tests while preserving historical-result compatibility.
- Updates MCP behavior, documentation, schemas, fixtures, and integration coverage.
| File | Description |
|---|---|
sdk/typescript/tests-ts/native-scan-package.test.ts |
Tests packaged native registration. |
plugins/codex-security/tests/workbench_test_support.py |
Updates Deep Scan fixtures. |
plugins/codex-security/tests/test_workbench_storage_contracts.py |
Revises checkpoint recovery tests. |
plugins/codex-security/tests/test_workbench_scan_usage.py |
Removes legacy usage accounting cases. |
plugins/codex-security/tests/test_workbench_scan_history.py |
Covers SDK-backed history and migration. |
plugins/codex-security/tests/test_workbench_prompt_only_scan.py |
Tests directory errors and scan reuse. |
plugins/codex-security/tests/test_workbench_output_permissions.py |
Updates start-response shape. |
plugins/codex-security/tests/test_workbench_native_indexes.py |
Expands index and publication coverage. |
plugins/codex-security/tests/test_workbench_db_exports.py |
Expands pagination and export tests. |
plugins/codex-security/tests/test_workbench_current_draft.py |
Tests historical checkpoint compatibility. |
plugins/codex-security/tests/test_workbench_completion_binding.py |
Uses aggregate-ready completion fixtures. |
plugins/codex-security/tests/test_workbench_cancellation.py |
Tests deferred stop publication. |
plugins/codex-security/tests/test_legacy_checkpoint_recovery.py |
Removes retired recovery tests. |
plugins/codex-security/tests/test_deep_scan_successful_publication.py |
Reworks publication tests for SDK scans. |
plugins/codex-security/tests/test_deep_scan_stop_conditions.py |
Removes coordinator stop tests. |
plugins/codex-security/tests/fixtures/runtime/stopped_scan.py |
Adds stopped-scan boundary fixture. |
plugins/codex-security/skills/deep-security-scan/SKILL.md |
Documents SDK-owned execution and completion. |
plugins/codex-security/scripts/workbench_scan_usage.py |
Simplifies current usage accounting. |
plugins/codex-security/scripts/workbench_scan_history.py |
Adds SDK-backed Deep Scan helpers. |
plugins/codex-security/scripts/workbench_saved_results.py |
Switches recovery to composition checkpoints. |
plugins/codex-security/scripts/workbench_progress.py |
Removes coordinator lease handling. |
plugins/codex-security/scripts/workbench_db.py |
Routes Deep Scan lifecycle through SDK records. |
plugins/codex-security/scripts/workbench_cli.py |
Removes coordinator-only CLI commands. |
plugins/codex-security/schemas/definitions/discovery-candidate.schema.json |
Rejects whitespace-only candidate IDs. |
plugins/codex-security/references/scan-contract.md |
Documents SDK completion and recovery. |
plugins/codex-security/references/scan-artifacts.md |
Updates Deep Scan artifact ownership. |
plugins/codex-security/references/final-report.md |
Documents SDK-owned finalization. |
plugins/codex-security/references/config-preflight.md |
Updates permission-profile behavior. |
plugins/codex-security/references/artifact-storage.md |
Removes legacy worker guidance. |
plugins/codex-security/plugin-files.json |
Registers expanded MCP bundle parts. |
plugins/codex-security/mcp-app/tests/test_workbench_timeout.mjs |
Tests workbench timeout selection. |
plugins/codex-security/mcp-app/tests/test_workbench_state_fallback.mjs |
Shares bundle configuration. |
plugins/codex-security/mcp-app/tests/test_native_scan.mjs |
Expands native SDK execution tests. |
plugins/codex-security/mcp-app/tests/test_native_scan_stop.mjs |
Tests native completion and stopping. |
plugins/codex-security/mcp-app/tests/test_native_permissions.mjs |
Covers unused relative CWD metadata. |
plugins/codex-security/mcp-app/tests/test_native_pdf.mjs |
Tests packaged PDF preparation. |
plugins/codex-security/mcp-app/tests/test_mcp_app_smoke.mjs |
Updates Deep Scan smoke coverage. |
plugins/codex-security/mcp-app/tests/test_artifact_validation_phase.mjs |
Updates scan-bound context tests. |
plugins/codex-security/mcp-app/tests/test_artifact_storage.mjs |
Shares MCP bundle options. |
plugins/codex-security/mcp-app/tests/test_artifact_discovery.mjs |
Removes worker-layout behavior. |
plugins/codex-security/mcp-app/tests/test_artifact_attack_path.mjs |
Updates scan-bound validation. |
plugins/codex-security/mcp-app/src/python_command.ts |
Centralizes workbench timeouts. |
plugins/codex-security/mcp-app/src/native-scan.ts |
Runs saved Deep Scans through the SDK. |
plugins/codex-security/mcp-app/src/artifact-validation-phase.ts |
Uses shared candidate ledger schema. |
plugins/codex-security/mcp-app/src/artifact-storage.ts |
Reserves Deep Scan artifact paths. |
plugins/codex-security/mcp-app/src/artifact-discovery.ts |
Shares candidate schema definitions. |
plugins/codex-security/mcp-app/src/artifact-candidate.ts |
Defines strict and enriched candidate schemas. |
plugins/codex-security/mcp-app/src/artifact-attack-path.ts |
Uses shared ledger validation. |
plugins/codex-security/mcp-app/server.ts |
Replaces coordinator orchestration with SDK hosting. |
plugins/codex-security/mcp-app/main.ts |
Removes the artifact-writer entrypoint. |
docs/project-configuration.md |
Documents native executable selection. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
|
Codex Review: Didn't find any major issues. What shall we delve into next? 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". |
7a4be8e to
cc83a1f
Compare
dbd6160 to
a76663c
Compare
cc83a1f to
9a5f900
Compare
9a5f900 to
bfa9fd3
Compare
a76663c to
5bfdb5f
Compare
faizan-oai
left a comment
faizan-oai
left a comment
There was a problem hiding this comment.
Holding approval because this activates the native resume path with the still-reproducible reference-document drift reported on #1159: a saved recipe with no knowledgeBasePaths adopts newly configured ambient documents. Both preparation and actual SDK/workbench reproductions fail the preservation invariant. The rejoined-Standard response now correctly removes the private recipe, and the integrated compact-server/native suites pass. I also reran the earlier credential-fixture probe: the native executable-inheritance test still fails when OPENAI_API_KEY is set to a synthetic value and includes that value in its assertion output; it must save/clear/restore both API-key variables as noted on #1123.
5bfdb5f to
5b35bc5
Compare
bfa9fd3 to
0e24db8
Compare
5b35bc5 to
e7615ea
Compare
0e24db8 to
64ac18f
Compare
alandelong-oai
left a comment
alandelong-oai
left a comment
There was a problem hiding this comment.
Three independent passes and root verification found a new P2 recovery regression below. An actual workbench base/head probe injected failure of the immutable checkpoint copy after the pending index write. The base recovered the staged finding on fail-scan; this head returned zero findings while the staged checkpoint still existed.
Validation: the native runtime checks passed, including actual fresh/resumed child-process settings, credentials, safe executable selection, shutdown and stop publication, and PDF extraction through the shipped entrypoint. All 41 SDK recovery/package tests and six changed artifact/smoke/fallback test files passed. The plugin rebuilt successfully. All 244 focused Python tests passed across eight changed suites. No repository files were changed.
| write_scan_local_bytes(scan_dir, f"checkpoints/pending/{checkpoint_name}", b"") | ||
| write_scan_local_bytes(scan_dir, f"checkpoints/{checkpoint_name}", checkpoint_contents) |
There was a problem hiding this comment.
[P2] Keep the staged checkpoint recoverable if its history copy fails
For the still-supported --draft-path/--checkpoint-path input, the pending marker no longer records the staged source, and _saved_result_paths now ignores pending entries. If the next checkpoint-history write raises (for example an I/O error), the validated checkpoint remains in drafts/ but neither the next draft nor stopped recovery can discover it. I injected that write failure and then called the real fail-scan command: the base retained one finding, while this head retained zero. Preserve a recoverable reference until the immutable copy succeeds.
e7615ea to
f1519ef
Compare
64ac18f to
2472bdc
Compare
alandelong-oai
left a comment
alandelong-oai
left a comment
There was a problem hiding this comment.
Three independent re-review passes and root verification are complete for this restacked head. The earlier staged checkpoint copy failure is fixed: the same interrupted-publication probe now recovers its finding. One new P2 remains in native recovery of an already sealed Deep Scan, described inline.
The bounded workbench reproduction accepts unchanged native registration, accepts CLI recovery after a checkout edit, and rejects native registration after that same edit; all four sealed artifacts remain byte-for-byte unchanged. The relevant functions and fixtures are identical at this PR head and the tested stack tip. The existing native sealed-registration and historical-worker migration tests also passed. Approval remains withheld.
| raise SystemExit( | ||
| "Saved scan registration must match its target, directory and mode." | ||
| ) | ||
| if scan_target_identity(repository, None) != ( |
There was a problem hiding this comment.
[P2] Check for a sealed result before rejecting changed checkout contents
This unconditional comparison runs before sealed_scan_producer_version. Native Deep reattachment reaches register-cli-scan for the saved registration, so a scan interrupted after sealing but before database completion cannot recover once the checkout changes. Recovery only needs to publish the verified sealed result; cli_scan_resume already skips the content comparison for that case. Using the existing native completion fixture, binding succeeds before the edit, CLI resume still succeeds after it, but native binding raises the changed-checkout error with all sealed bytes unchanged. Apply this content/revision check only to unsealed continuation while retaining checkout identity, ownership, claim and seal validation.
f1519ef to
8b71618
Compare
2472bdc to
01ac366
Compare
alandelong-oai
left a comment
alandelong-oai
left a comment
There was a problem hiding this comment.
Re-reviewed the real PR diff at 01ac3668a46b9402619f0a43008fd9dce1c0a77c. Three independent high-reasoning passes covered the complete changed patch and tests; root source checks and focused reproductions informed this decision.
The revised failure-persistence tests now match successful staged recovery. Two issues remain: native registration rejects a valid sealed result after checkout edits, and Python stopped-result recovery loses an older staged-path checkpoint. Exact-head native/CLI controls preserve sealed bytes; an exact base/head Standard-scan probe retains one finding at the base and zero at this head despite intact staged bytes. The new inline finding describes the latter Python path, separately from #1156's TypeScript draft-resume path.
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.
| draft = _read_scan_local_json(scan_dir, pending, "Saved scan checkpoint") | ||
| if not (scan_dir / relative).exists(): | ||
| relative = f"checkpoints/pending/{Path(relative).name}" | ||
| draft = _read_scan_local_json(scan_dir, relative, "Saved scan checkpoint") |
There was a problem hiding this comment.
[P2] Keep the staged-path fallback for interrupted Standard scans
Older pending markers can contain drafts/<uuid>.checkpoint.json when saving immutable history failed after accepting a checkpoint. This replacement parses that path as JSON instead of following and validating the intact staged file. In an exact base/head Standard-scan reproduction, fail-scan recovers one finding at the base and zero at this head, while the staged checkpoint bytes remain unchanged. Preserve the previous staged-path/digest fallback alongside the new JSON marker format. This is the Python stopped-result recovery path, separate from the TypeScript next-draft compatibility finding in #1156.
8b71618 to
dd10070
Compare
bf47060 to
8f425bc
Compare
alandelong-oai
left a comment
alandelong-oai
left a comment
There was a problem hiding this comment.
Retained the three independent passes for the identical patch and reconciled inherited touched-file changes at 8f425bc6d4e1.
Both recovery findings are fixed at this own head. Python fail-scan recovers the intact legacy staged marker and retains its finding. Native reattachment of sealed results now succeeds after a checkout edit, with unchanged-checkout and CLI controls passing and all sealed bytes unchanged.
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.
8f425bc to
b69fb2b
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 b69fb2b50081.
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
b69fb2b to
3e95db1
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 3e95db1a0b18.
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.
8da33c4
into
mdangelo/codex/pr939-split-39-sdk-deep-scans
Part 40 of 46. Previous: #1162 · Next: #1164 · Stack index
Summary
Route plugin Deep Scan start, wait, cancel and shutdown through the SDK runner so plugin and SDK scans share ordinary child scans and saved results.
Changes
Testing
Local checks for the implementation and applicable stack boundary:
Stopped-scan recovery passed with JSON and legacy staged-path checkpoints, including rejection of altered staged bytes. Native sealed registration accepts changed checkout contents only after seal verification; the focused tests also reject unsealed changes and corrupted sealed artifacts.
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
Native plugin scans use
CODEX_CLI_PATHor a nativecodexonPATH(codex.exeon Windows). Windows npm shims, managed-package directories and Desktop caches are no longer searched; setCODEX_CLI_PATHwhen the executable is elsewhere.begin-deep-scanreturns the SDK scan record and drops--available-parallelismand--workflow-version. The coordinator workbench commands are removed:get-deep-scan,claim-deep-scan-coordinator,upsert-deep-scan-worker,claim-deep-scan-dedup,commit-deep-scan-dedup,finish-deep-scan,fail-deep-scanandrecord-deep-scan-publication-failure.--coordinator-generationand MCP--artifact-writerdispatch are retired.Unsealed scans using the old engine or checkpoint format require their original version or a new scan. Supported sealed reports remain readable; finishing older sealed results still depends on their available accounting. Later PRs delete the unused implementations.
Public disclosure review