Repository navigation
refactor(deep-scan)!: simplify scan recovery and result saving - #1095
mldangelo-oai wants to merge 1 commit into
Conversation
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. |
Keep a committed scan draft for saving results and recovery. Consolidate scan execution settings, child completion records, and report preparation. This commit preserves the code from PR #1095 through its last upstream sync.
2a5796d to
3065926
Compare
mldangelo-oai
changed the title
|
@codex review |
There was a problem hiding this comment.
Copilot review overview
🟡 Changes recommended
Post-turn result collection no longer preserves the public cancellation error contract.
Review effort: Balanced
Findings: 1
Open (1)
What changed in this PR
Simplifies Deep Scan recovery and result publication by persisting reusable drafts, retaining original evidence, recording execution sessions, and sharing prepared worker settings.
Changes:
- Reworks checkpoint, merge, evidence, and recovery persistence.
- Adds execution-session accounting and reusable worker preparation.
- Updates workbench publication, MCP completion, schemas, and tests.
| File | Description |
|---|---|
sdk/typescript/src/deep-scan.ts |
Revises pass recovery, persistence, merging, and publication. |
sdk/typescript/src/deep-scan-checkpoint.ts |
Introduces checkpoint v3 and aggregate hydration. |
sdk/typescript/src/scan-merge.ts |
Retains source evidence and reconciles accepted revisions. |
sdk/typescript/src/scan-publication.ts |
Centralizes draft publication and result completion. |
sdk/typescript/src/scan-events.ts |
Separates turn processing from result collection. |
sdk/typescript/src/runtime.ts |
Adds batched restoration and child projection support. |
sdk/typescript/src/execution-preparation.ts |
Reuses prepared execution settings across workers. |
sdk/typescript/tests-ts/support/scan-publication.ts |
Removes the obsolete staged-file publication helper. |
sdk/typescript/tests-ts/support/api-events.ts |
Adds synthetic plugin-variant setup. |
sdk/typescript/tests-ts/support/api-client.ts |
Updates the artifact-restorer test double. |
sdk/typescript/tests-ts/scan-resume-permissions.test.ts |
Verifies resumed launch configuration. |
sdk/typescript/tests-ts/scan-projection-fixtures.test.ts |
Updates projection tests for retained evidence references. |
sdk/typescript/tests-ts/scan-merge-reconciliation.test.ts |
Adds reconciliation and validator-isolation coverage. |
plugins/codex-security/scripts/workbench_saved_results.py |
Commits drafts before exporting canonical results. |
plugins/codex-security/scripts/workbench_schema.py |
Adds execution-session migration and backfill. |
plugins/codex-security/scripts/workbench_scan_usage.py |
Accounts for persisted execution sessions. |
plugins/codex-security/mcp-app/server.ts |
Updates native completion and stopped-result handling. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| const completed = await runScanTurn(options); | ||
| return collectResult( |
|
Codex Review: Didn't find any major issues. Keep them coming! 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". |
| throwScanFailure(error, options); | ||
| } | ||
| const completed = await runScanTurn(options); | ||
| return collectResult( |
There was a problem hiding this comment.
Remove the unused event/publication facade
runScanEvents has no production callers left: the API uses runScanTurn and publishScan, while only the event tests call this combined path. It is internal and absent from the package exports. Delete this wrapper and its api.ts re-export, and have the event tests exercise runScanTurn directly. That also removes collectResult/ScanResult, pluginRoot, and workbenchValidated from the event layer and lets those tests stop copying completed-report fixtures merely to test event callbacks. I verified completion, thread callbacks, finalization, and null finalized usage through runScanTurn with nonexistent artifact directories; artifact validation/publication can remain covered by the existing contract/API tests.
| await input.writer.restoreMany([ | ||
| ...revisions, | ||
| ...(contents === undefined | ||
| ? [] | ||
| : [{ path: aggregatePath!, contents: Buffer.from(contents) }]), | ||
| { path: DEEP_SCAN_CHECKPOINT, contents: Buffer.from(snapshot) }, |
There was a problem hiding this comment.
Keep metadata saves from rewriting the unchanged aggregate
Reserving or registering a pass changes the checkpoint snapshot, so the snapshot comparison above does not prevent this block from rewriting the same content-addressed aggregate. Running the existing composition fixture with one worker and three passes produced three distinct aggregate files but nine aggregate writes. Each metadata save also serializes and hashes the full retained findings and coverage again. Separate aggregate persistence from metadata saves: retain the successfully persisted aggregate reference/path, update it when merge acceptance or coverage replacement changes the aggregate, and otherwise write only checkpoint.json. The aggregate is already replaced rather than mutated, so this can remove report-sized work from pass bookkeeping while preserving the existing write queue and failure semantics.
4eca17a to
e30421d
Compare
3065926 to
95bbfbe
Compare
![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: 95bbfbe176
ℹ️ 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".
| elif version == 43: | ||
| backfill_composition_children(connection) |
There was a problem hiding this comment.
For a database that already applied migration 43, apply_migrations skips this branch, leaving existing child rows with parent_scan_role = NULL. Since composition_children() now selects only rows whose role is deep_pass (workbench_composition.py:105-110), resuming or recovering an existing Deep Scan can omit all prior children, including their findings and costs. Move this backfill into a new migration version so previously upgraded databases execute it.
Useful? React with 👍 / 👎.
e30421d to
2348b94
Compare
95bbfbe to
121c362
Compare
121c362 to
24713fb
Compare
2348b94 to
e29cdc2
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 24713fb088
ℹ️ 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".
| FROM scan_severity_classifications AS classification | ||
| JOIN json_each(classification.finding_ids_json) AS selected | ||
| JOIN finding_severity_assessments AS assessment ON assessment.finding_id = selected.value | ||
| JOIN findings AS finding ON finding.id = assessment.finding_id""" |
There was a problem hiding this comment.
When an older database contains an unindexed assessment and the same scan's classification was restarted with a different rubric or knowledge-base digest before migration, this join still selects the stale global assessment solely because its occurrence ID matches the scan. Migration 46 then persists that assessment as the scan's result, whereas migration 42 deliberately required both hashes to match. Add the same rubric and knowledge-base predicates here so an interrupted reassessment cannot corrupt the recovered classification.
Useful? React with 👍 / 👎.
faizan-oai
left a comment
faizan-oai
left a comment
There was a problem hiding this comment.
Reviewed final draft commit ordering, immutable source findings/revisions, checkpoint persistence, child/session accounting, and reuse of prepared execution settings. I did not confirm another runtime defect in those paths, and did not find a further deletion that I could justify without removing recovery behavior. The credential-fixture issue inherited from #1123 is now posted there.
At 24713fb, local checks produced 680 passes and four skips across focused SDK, native lifecycle, Python migration, result-saving, composition, cost, and resume suites. The 28 initial native-dependent failures passed after building this source's macOS module with Rust 1.97.1. build:ci and types also passed; the pnpm launcher used Node 25.8.1 outside the declared engine range. A real Codex 0.159.0 permission preflight passed without executing a model turn.
Leaving COMMENT because this was a focused review of a large change, with universal package, local Windows, and live-model validation incomplete. The checkpoint-v2 compatibility break, retained-ID requirement for draft updates, and failed-pass slot behavior are explicit in the description and should stay visible in release notes.
faizan-oai
left a comment
faizan-oai
left a comment
There was a problem hiding this comment.
Completed the follow-up review at 24713fb. The earlier platform/package validation gap is closed: the current CI run succeeded, including Windows test shards, standalone plugin checks and installed packages. I also built the universal bundled plugin locally on Node 24.21.0 using this run's native artifact.
The remaining issue is the saved-recipe reference-document drift from #1125, which still reproduces here. A scan started without a knowledge base can resume with newly configured documents because prepareNativeScan falls back to the current environment when the saved field is absent, and the digest comparison only runs when an old digest exists. A synthetic SDK/database reproduction completed one worker without documents, then ran another worker with a new knowledge snapshot under the same scan ID. Reproduction details and a verified minimal correction. I am leaving this head unapproved until that case is fixed.
Two other concerns are resolved by this validation:
- The relative-evidence-link probe that fails on #1117/#1125 passes here because the child tree is retained.
- Migration 46 can restore stale rows, but
SeverityStore.readfilters their rubric/knowledge hashes and rejects an incomplete classification. Both interrupted-reclassification probes passed; I did not reproduce stale assessments being accepted for publication.
The focused severity, projection, resume-permission and installed-native-registration suites passed all 26 cases. One initial permission-test failure was caused by my restricted test PATH omitting python; both permission cases passed after restoring the Python path. These results supplement the earlier composition/recovery/accounting validation; no live model scan was used.
Summary
Deep Scan runs ordinary Standard scans and combines their results. Saving and resuming it still involves copying evidence, rebuilding prior results, and preparing the same worker environment repeatedly. Keep one saved draft, store original findings once, and share the prepared worker setup.
Changes
write-scan-draftcommand read{documents, checkpoint}from stdin when--draft-pathis omitted. Existing staged-file inputs remain supported.Testing
The complete stack passed these checks on Linux:
--seed 12345: 3,443 passed, 46 skipped.Each earlier PR also passed source and type checks at its own rebuilt commit. Focused worker, comparison, cancellation, and publication tests are recorded on the relevant PRs. Windows execution remains for CI.
Risk and rollout
CODEX_CLI_PATHor normalPATHlookup. They no longer search installer or package-cache directories. Create a new client after editing a custom plugin's source.Part 23 of 23. Previous PR · Stack index. Review against the base branch.
Public disclosure review