Repository navigation
refactor: extract scan registration and result saving - #1113
mldangelo-oai merged 2 commits 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. |
|
@codex review |
|
Codex Review: Didn't find any major issues. Already looking forward to the next diff. 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". |
There was a problem hiding this comment.
Copilot review overview
🟢 Approval recommended
The refactor preserves existing lifecycle behavior and introduces no unresolved correctness issues.
Review effort: Balanced
Findings: None
What changed in this PR
Extracts shared scan registration and publication helpers while preserving existing resume, validation, warning, and artifact-recovery behavior.
Changes:
- Moves registration and resume validation into
registerScan. - Moves completion, warning collection, and artifact preservation into publication helpers.
- Updates scan execution and package verification for the new modules.
| File | Description |
|---|---|
sdk/typescript/src/scan-registration.ts |
Adds shared registration and resume handling. |
sdk/typescript/src/scan-publication.ts |
Adds publication and artifact-preservation helpers. |
sdk/typescript/src/scan-events.ts |
Adapts result collection to the shared context. |
sdk/typescript/src/api.ts |
Uses the extracted scan lifecycle helpers. |
sdk/typescript/scripts/check-package.mjs |
Includes the new registration module in package checks. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
alandelong-oai
left a comment
There was a problem hiding this comment.
Reviewed 54d8f2b680016dd2a40dc14c8fa74baa730fa40e against mdangelo/codex/pr939-stack-07-scan-events at bbdcd084c54e28ddf3553792b1c16bf9a9d3b89d, scoped to this PR's diff and touched files.
Compared registration and completion extraction with the base implementation, including fresh/resumed bindings, sealing, restoration, and budget-exhaustion recovery. No new actionable findings.
Review included simplification/deletion opportunities. Full repository and platform test suites were not run.
bbdcd08 to
463f042
Compare
1a70e3c to
dc1f6e8
Compare
9fabf1d to
efdd324
Compare
dc1f6e8 to
3ad3023
Compare
faizan-oai
left a comment
There was a problem hiding this comment.
Reviewed the registration/result-saving extraction and its callers, including saved target/session checks and post-scan handling. No issue found in the incremental change. Publication and sealed-result recovery were exercised at the integrated stack tip.
alandelong-oai
left a comment
There was a problem hiding this comment.
Reviewed the exact base-to-head diff with three independent review passes and root verification. Registration/resume identity checks, prepare/validate/complete ordering, warning classification and failed post-scan artifact restoration retain their behavior. No actionable findings remain.
Validation: 165 orchestration tests passed, 4 platform-specific tests skipped, no failures. Source comparison also covered budget recovery and cancellation through the extracted helpers.
3ad3023 to
a9b405b
Compare
alandelong-oai
left a comment
There was a problem hiding this comment.
The previously completed three independent passes carry forward after verifying the patch is identical; root reconciled the current base and discussion.
No serious outstanding finding was identified in this change. The scoped prior review and current diff remain consistent.
Approval is for this change. Integrated-tip validation passed 1,357 SDK tests (31 skipped), 105 MCP tests (2 skipped), portable checks, types/builds and installed-package smoke. Other stack findings and known CI fixture failures still prevent the stack from being mergeable.
67bfed2 to
ee529e8
Compare
a9b405b to
af0944b
Compare
alandelong-oai
left a comment
There was a problem hiding this comment.
Re-reviewed the real PR diff at af0944baa81e2e38fdfbb87cd88a4178c13cc619. The patch is unchanged from the prior three-pass review. I reconciled the inherited changes in touched files and retained the prior verified findings and tests.
No serious outstanding finding identified in this patch; prior conclusions remain consistent with the unchanged change or the current re-review.
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.
af0944b to
e89bf60
Compare
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 e89bf60c56dd.
No actionable introduced finding survives the independent reviews and root reconciliation of this exact patch, touched-file changes, and current integration checks.
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.
e89bf60 to
71822c8
Compare
alandelong-oai
left a comment
There was a problem hiding this comment.
Retained the three independent passes for the verified identical patch and reconciled inherited touched-file changes at 71822c884412.
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.
71822c8 to
d655625
Compare
46472d4 to
a82ed54
Compare
alandelong-oai
left a comment
There was a problem hiding this comment.
Reviewed c3eeedb245c6c0270cd52131c0aff7d06522d271 against 87c641dedbcab8720f6061ec1b6b9fe5662a644a with exactly three independent full-diff passes, including all changed tests, and independent root verification. No serious introduced findings remain.
Local macOS arm64 validation: 237 SDK tests passed, 4 skipped; fresh native host build, standard plugin build, SDK build, types, and formatting checks passed.
No model execution or local Windows run. This approval covers this exact PR contribution and does not establish validation of the whole stack with current main.
c3eeedb to
d354fe0
Compare
87c641d to
b47933a
Compare
alandelong-oai
left a comment
There was a problem hiding this comment.
Reviewed d354fe0480492fb72e042399ebe78d705d269e4e against b47933a764edc65208bd8e720d0c063225d9cbf0 with exactly three fresh independent full-diff passes, including every test change, and root verification. No serious introduced findings remain.
Local macOS arm64 validation: 237 SDK tests passed, 4 skipped. Fresh native host, plugin and SDK builds, types and formatting passed.
No model execution, full installed-package smoke or local Windows run. This approval covers this exact contribution and does not establish whole-stack validation with current main.
At the last check, current-head hosted CI was still running with no failed latest-attempt jobs. This approval does not assert completed CI or merge readiness.
d354fe0 to
8eed8fd
Compare
b47933a to
80a852f
Compare
alandelong-oai
left a comment
There was a problem hiding this comment.
Completed exactly three fresh independent full-diff reviews and root verification of this contribution, including the updated import removal against the corrected base. No serious introduced findings remain.
The complete head tree is identical to the previously reviewed version, so its validation is retained without duplicate test runs: 237 focused SDK tests passed, four skipped; native host, plugin and SDK builds, types and formatting passed.
Current-head hosted CI was still running with no failed latest-attempt jobs at the prepublication check. This approval covers the reviewed source and does not assert completed CI or whole-stack integration with current main. No model execution, installed-package smoke or local Windows run.
80a852f to
1c40f7f
Compare
8eed8fd to
063e279
Compare
There was a problem hiding this comment.
Reviewed exact head 063e279c2e0e28dde8846af3828c04b5aa847d02 against 1c40f7f5620e7e44e7427b03b86baafcb1d572ce.
Retained exactly three prior independent HIGH reasoning full-diff reviews, including every test diff, after verifying identical ordered contributions and reconciling inherited changes in touched files. No duplicate full review passes were run. No serious introduced finding remains. The package checker inherits the already-reviewed allowlist refactor; the new module requirements remain intact. Fresh native host, plugin and SDK builds, types, formatting, and a 580-entry production archive check passed. The prior 237 passing SDK tests and 4 skips remain scoped to the unchanged reviewed source; that suite was not rerun.
Hosted CI was still pending at the 23:28 UTC check without failed jobs on this head. This review does not establish merge readiness or whole-stack integration. Validation was local macOS arm64 without model execution, source edits, a local Windows run, or a full installed-package smoke test.
CI update after source approval: Windows Node 24 installed-package job 111316560341 in run 37161518495 attempt 1 failed with PluginPythonUnavailableError during CodexSecurity.run session preparation (package-behavior.mjs:165). CPython 3.12.10 setup had succeeded, but the log does not expose interpreter probe diagnostics, so the reason for rejection and source attribution remain unestablished. Job metadata is tied to this PR head; the checkout and package artifact use merge commit 1b509c3. At that observation, this failure blocked merge readiness; investigation or a hosted retry was requested.
https://github.com/openai/codex-security/actions/runs/37161518495/job/111316560341
CI follow-up, 2026-10-04 00:03 UTC: node-ci run 37161518495 attempt 2 completed successfully at this unchanged head. The latest-attempt receipt contains 44 successful and 2 skipped jobs, with no failed or unfinished jobs. This clears the CI hold recorded above. The first-attempt Python-resolution failure remains unexplained; the retry does not establish its cause or whole-stack integration. No duplicate code reviews or local test runs were performed.
063e279 to
cbe37d3
Compare
1c40f7f to
0068713
Compare
alandelong-oai
left a comment
There was a problem hiding this comment.
Source review approved for cbe37d38913e39e5065e679b935b41a08f791d95 against 0068713e366a467954578c6ac427409383c1015a. Completed exactly three historical independent HIGH full actual-PR-diff passes retained after ordered-contribution and inherited touched-file reconciliation, followed by root verification. No supported introduced source finding remains in this contribution.
The unchanged ordered contribution was reconciled against all touched old/new base and head modes and bytes. Three inherited touched-file changes were separately reconciled with reviewed parent/main changes. Prior local validation and the old-head CI clearance keep their original observation times; no duplicate tests were run.
The exact-head latest-workflow CI set was successful at the 2026-10-04T06:25:55.396371+00:00 audit. Source approval and individual CI do not establish whole-stack/current-main successful validation or merge readiness. No model execution, local Windows run, native rebuild or clean installed-package smoke is claimed.
cbe37d3 to
43ea131
Compare
0068713 to
72664b9
Compare
There was a problem hiding this comment.
Source approval for 43ea1314b3a540345632c78a3323557589954513 against 72664b995c54275a7d7d29fb560057bd405edaa4. The ordered actual GitHub contribution is unchanged. All three prior independent HIGH full-diff reviews, including tests and fixtures, are retained after touched own/base modes and bytes were reconciled; inherited touched-file changes match the previously reviewed main changes on both sides. Root synthesis found no new supported finding.
Prior validation retains its original head and execution time; no duplicate local test or build was run for this unchanged contribution.
The prefix contains main at 634532d, while main has separately advanced to 1fb0e5f. This source approval does not establish successful whole-stack integration. Exact-head CI was pending in the 2026-10-04 09:26 UTC receipt and is tracked separately. No clean install, local Windows run or model execution is claimed.
CI update, 2026-10-04 09:39 UTC: current-head Windows Node22 test shard2 failed; this remains source-approved but CI-held. The identical provider-test blob is inherited from #1111, where a temporary-home warning causes three empty-stderr assertions to fail. This PR's job log has not been independently downloaded or assigned that immediate cause. The source finding is #1111 (comment). Original execution times and source-review results remain separate; no local Windows/model run or CI retry was performed.
72664b9 to
0248325
Compare
43ea131 to
e93a32d
Compare
alandelong-oai
left a comment
There was a problem hiding this comment.
Source approval for e93a32d999332a89eec45f104076e4f561067785 against 02483254899d0723880d897e18fc0e9320b6cdd3. The actual GitHub diff, ordered contribution and touched own/base file modes, OIDs and bytes match the prior reviewed contribution. Exactly three prior independent HIGH full-diff reports, including tests and fixtures, are retained after reconciliation; there is no inherited touched residual and root synthesis found no new supported finding.
Historical validation keeps its original head and execution time. No unchanged local source review, test or build was repeated for this contribution.
Exact-head hosted CI remained pending in the 2026-10-04 10:32–10:33 UTC observation; historical failures at older heads are separate.
The prefix contains main 1fb0e5fbf8684c926cac2a36d08389b212d28d5a. Successful whole-stack integration remains unestablished. No clean installation, fresh native compilation, local Windows run or model execution is claimed.
0248325 to
b55bf3b
Compare
e93a32d to
57f13aa
Compare
|
Codex Review: Something went wrong. Try again later by commenting “@codex review”. ℹ️ 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". |
alandelong-oai
left a comment
There was a problem hiding this comment.
Source approval for head 57f13aa74169a4a3fc51d7baae8b48cb34ce8c2f against base b55bf3bdb9d324d97febd3dce9f3c2ca8890e472. Exactly three original independent HIGH full actual-diff reviews are retained after reconciliation: ordered contribution, complete GitHub diff bytes and all touched own/base/instruction modes and bytes are unchanged. No inherited touched residual or new source issue was found. No unchanged full source review, local test or build was repeated; prior validation retains its original head and execution time.
CI remains held. The current macOS shard3 job111425166174 (run37198165749 attempt1) reports 1,014 pass, 6 skip and 4 failures at the inherited worker-profile test assertion introduced in #1112. That test expects every profile to start with codex_security_scan_, while discovery/merge intentionally use codex_security_deep_scan_worker. The identical test blob is inherited here; the finding is #1112 (comment). The separate Ubuntu shard3 failed job remains metadata-only; its immediate cause has not been independently diagnosed. No worker runtime regression or fix-alone-green result is established.
Hosted CI was observed 2026-10-04T11:27:39.083466+00:00: the aggregate was still pending despite failed jobs. The actual three-dot equals the own-base diff and the reviewed prefix contains main1fb. Successful whole-stack integration remains unestablished. No fresh native compilation, local Windows execution, clean installation or model run is claimed.
57f13aa to
cf737e9
Compare
b55bf3b to
7865928
Compare
alandelong-oai
left a comment
There was a problem hiding this comment.
Reviewed head cf737e9a6e02e36f524452f85a03f8066ffe0b12 against base 7865928ab0b8f06e8bc78bc090e2680925fb9df2 using three retained independent HIGH full reviews of the actual GitHub PR diff, including every changed test and fixture, followed by root synthesis. Actual three-dot and own-base diffs match.
The ordered contribution, exact GitHub diff bytes, touched own/base file modes and bytes, and applicable instructions match the earlier reviewed contribution. No inherited touched residual remains; original review and validation timestamps are retained. No unchanged local tests were repeated.
Hosted CI is still pending at the latest saved observation, with no failed jobs observed for this head. Source approval is separate from CI and integration. The reviewed prefix contains main 1fb0e5f, but successful whole-stack integration remains unestablished. No product models, clean installation, local Windows execution or new native compilation were performed.
…1114) Consolidate approved Deep Scan changes into the parent topic branch.
176e438
into
mdangelo/codex/pr939-stack-07-scan-events
Part 8 of 46. Previous: #1112 · Next: #1114 · Stack index
Summary
Extract scan registration and completion from the API so ordinary Standard scans and later Deep Scan passes can use the same database and result-saving flow.
Changes
Testing
The combined lower stack passed SDK/MCP type checks, generated-model checks, SDK formatting, Ruff check and format, SDK CI build, portable source compatibility, and all nine compatibility tests. It also passed 157 focused SDK tests and 24 migration/composition tests, with three Windows-only SDK cases skipped locally.
See this PR’s Checks tab for CI on the current commit.
Risk and rollout
The existing scan API calls these helpers immediately. Deep Scan resume and cost-limit recovery retain their existing paths at this step; the new scheduler is connected later.
Public disclosure review