Repository navigation
refactor: separate scan events, progress, and cost reporting - #1112
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. |
![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: bbdcd084c5
ℹ️ 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".
| clearTimeout(timer); | ||
| expect(controller.signal.aborted).toBe(false); | ||
| pid = Number(await readFile(pidPath, "utf8")); | ||
| const deadline = Date.now() + 5_000; |
There was a problem hiding this comment.
On a loaded CI runner, correct subprocess cleanup can take more than five seconds, so this fixed Date.now() deadline can fail any of the eight parameterized cases even when the worker exits normally; use deterministic process-exit synchronization with only the test runner's outer timeout as a safety bound. The SDK test guidance explicitly requires avoiding timing-sensitive assertions.
AGENTS.md reference: sdk/typescript/AGENTS.md:L35-L35
Useful? React with 👍 / 👎.
|
@codex review |
|
Codex Review: Didn't find any major issues. Bravo. 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
🔵 Needs a closer look
It reorganizes central event, cancellation, budget, and subprocess lifecycle paths that warrant final human validation.
Review effort: Balanced
Findings: None
What changed in this PR
Extracts reusable scan event, preparation, monitoring, and publication logic from the main SDK scan flow.
Changes:
- Adds dedicated modules for event handling, progress/cost reporting, prompt preparation, and result publication.
- Preserves progress, cancellation, final usage, and subprocess lifecycle behavior with expanded tests.
- Includes the extracted modules in package validation.
| File | Description |
|---|---|
sdk/typescript/src/api.ts |
Integrates extracted scan helpers. |
sdk/typescript/src/scan-events.ts |
Handles event streams and observer notifications. |
sdk/typescript/src/scan-monitoring.ts |
Handles cost and progress reporting. |
sdk/typescript/src/scan-preparation.ts |
Builds scan configuration and prompts. |
sdk/typescript/src/scan-publication.ts |
Validates and collects completed results. |
sdk/typescript/tests-ts/api-events.test.ts |
Tests lifecycle, cancellation, usage, and worker settings. |
sdk/typescript/tests-ts/api.test.ts |
Verifies parent Deep Scan progress. |
sdk/typescript/scripts/check-package.mjs |
Adds extracted modules to package checks. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
mldangelo-oai
changed the title
alandelong-oai
left a comment
alandelong-oai
left a comment
There was a problem hiding this comment.
Reviewed bbdcd084c54e28ddf3553792b1c16bf9a9d3b89d against mdangelo/codex/pr939-stack-06-scan-comparison at eb1766c68b1fa8e4b1aef2de3fc92a5324ee5443, scoped to this PR's diff and touched files.
Compared the event, monitoring, preparation, and publication extractions with their base implementations, including failure/cancellation and observer handling. No additional distinct findings beyond the existing review comments.
Review included simplification/deletion opportunities. Full repository and platform test suites were not run.
bbdcd08 to
463f042
Compare
eb1766c to
338d654
Compare
9fabf1d to
efdd324
Compare
0138a07 to
c42ebd9
Compare
faizan-oai
left a comment
faizan-oai
left a comment
There was a problem hiding this comment.
Reviewed event completion ordering, cancellation handling, and the separation of progress and cost callbacks. No issue found in this extraction. Focused event, cost, and cancellation checks passed at the integrated stack tip; this does not claim each intermediate revision was independently exercised.
alandelong-oai
left a comment
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. The extracted event handling, cost/progress state, prompts and artifact collection preserve their prior behavior. Explicitly unknown final usage remains unknown, and the subprocess lifecycle test now synchronizes on process exit. No actionable findings remain.
Validation: 222 event/lifecycle/orchestration tests passed, 4 platform-specific tests skipped, no failures. This includes fresh/resumed worker subprocess settings and completion/cancellation cases; no live model scan was run.
efdd324 to
67bfed2
Compare
c42ebd9 to
07debb1
Compare
alandelong-oai
left a comment
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
07debb1 to
ea6edb3
Compare
There was a problem hiding this comment.
Re-reviewed the real PR diff at ee529e846e2957180a71913c0e400d8b10b5b568. 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. 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.
ea6edb3 to
5bd01d5
Compare
ee529e8 to
3c42923
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 3c42923ec5a4.
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.
5bd01d5 to
54564a6
Compare
3c42923 to
46472d4
Compare
f9431bd to
138f3ef
Compare
alandelong-oai
left a comment
alandelong-oai
left a comment
There was a problem hiding this comment.
Reviewed 87c641dedbcab8720f6061ec1b6b9fe5662a644a against 138f3ef65f849375b5289930e9c87d89ef95dad6 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.
87c641d to
b47933a
Compare
138f3ef to
8a4930a
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, including all test changes, and root verification at this exact head. Approval is withheld for the introduced missing path separator binding described inline. The focused SDK tests passed (237 passed, 4 skipped), but hosted typechecking fails. This assessment is specific to this head; the later stack patch restores the binding in its new module.
| relative, | ||
| resolve, | ||
| sep, | ||
| } from "node:path"; |
There was a problem hiding this comment.
[P1] Retain the path separator import until its remaining consumer moves
This removes sep from node:path, but this head still calls relative(scanDir, result.threatModelPath).split(sep) at line 1990 when post-scan instructions are configured and the completed scan has a threat-model artifact. The exact base imports sep; this head has no other declaration. Hosted typechecking at this commit reports TS2304 (src/api.ts(1990,30): Cannot find name 'sep'). If transpiled without checking, that branch also throws before the post-scan restoration try/catch. Retain the import here; moving the consumer and import in the later #1113 does not fix this PR's own head.
b47933a to
80a852f
Compare
alandelong-oai
left a comment
alandelong-oai
left a comment
There was a problem hiding this comment.
The missing sep import reported in discussion4174999616 is fixed at this head. The configured post-scan threat-model snapshot path now has its node:path binding, and exact-head typechecking passes.
Completed exactly three fresh independent full-diff reviews, including both test diffs, followed by root verification. No serious introduced findings remain. Fresh validation: 237 focused SDK tests passed, four skipped; native host, plugin and SDK builds, types and formatting passed. Reviewed source files are unchanged after validation.
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.
8a4930a to
7b6540d
Compare
80a852f to
1c40f7f
Compare
alandelong-oai
left a comment
alandelong-oai
left a comment
There was a problem hiding this comment.
Source review approved for 1c40f7f5620e7e44e7427b03b86baafcb1d572ce against 7b6540d938c12abb04e31d894fd5e0f3eff0ab5f.
Retained the three completed independent HIGH reasoning full-diff reviews, including every test diff, because this PR's own contribution is unchanged. Independently reconciled the current base and head snapshots and inherited changes in touched files. No additional full review pass was run for this restack, and no actionable introduced finding remains.
Fresh validation passed the native host and SDK/plugin builds, TypeScript and formatting checks, 237 passing SDK tests (4 additional tests skipped), and the 576-entry production archive check.
Hosted CI was still pending at the latest check (2026-10-03T23:28:24.249118+00:00), with no failed jobs observed on this head. This source approval is not a statement that the PR is ready to merge. No model execution, local Windows validation, or full installed-package smoke test is claimed.
7b6540d to
84aa638
Compare
1c40f7f to
0068713
Compare
alandelong-oai
left a comment
alandelong-oai
left a comment
There was a problem hiding this comment.
Source review approved for 0068713e366a467954578c6ac427409383c1015a against 84aa638c07ea8b77ec73e9ae471c8d71201ef634. Completed exactly three fresh independent HIGH full actual-PR-diff passes, including every changed test and fixture, followed by root verification. No supported introduced source finding remains in this contribution.
Fresh focused validation passed 238 SDK tests with 4 skips, builds, source type checks, formatting and the production archive check. Explicit unknown finalized usage and child lifecycle/settings tests passed. Synthetic child-process coverage is not an integrated native model scan.
CI remains pending at the 2026-10-04T06:25:55.396371+00:00 audit; this is source approval only. 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.
84aa638 to
a638837
Compare
0068713 to
72664b9
Compare
There was a problem hiding this comment.
Source approval for 72664b995c54275a7d7d29fb560057bd405edaa4 against a638837dea1aa28f353051055f5bfd45d9359b43. 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.
a638837 to
785e0c3
Compare
72664b9 to
0248325
Compare
alandelong-oai
left a comment
alandelong-oai
left a comment
There was a problem hiding this comment.
Source approval for 02483254899d0723880d897e18fc0e9320b6cdd3 against 785e0c3acd305ebee9fcfffe96f2c145f7bf1445. 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.
785e0c3 to
28ba8ce
Compare
0248325 to
b55bf3b
Compare
alandelong-oai
left a comment
alandelong-oai
left a comment
There was a problem hiding this comment.
Source approval for head b55bf3bdb9d324d97febd3dce9f3c2ca8890e472 against base 28ba8ce9f35cd35d1704c113e925cff8694caa46.
Retained exactly three original independent HIGH full actual-diff reviews, including every changed test and fixture, after current contribution and touched-file reconciliation. The ordered contribution, actual GitHub diff bytes and all touched own/base/instruction modes and bytes are identical to the reviewed contribution. No inherited residual or new source issue was found.
No unchanged full source review or local product validation was repeated. Prior validation retains its original head and execution time. Hosted CI for this new head was pending at 2026-10-04T11:17:33.848692+00:00; older-head success is separate. The actual three-dot equals the own-base diff, and the reviewed prefix contains current main. This is source approval, not whole-stack successful integration, deployment, native Windows or clean-install evidence.
alandelong-oai
left a comment
alandelong-oai
left a comment
There was a problem hiding this comment.
Source approval is withheld following newly observed CI evidence of a P2 in the added worker-launch test. Four discovery/merge fresh/resumed cases reject the intended inherited worker profile ID. This is a bounded follow-up finding; the three original full-review reports and historical validation remain unchanged. No runtime permission regression or complete-fix CI result is established.
| expect(config).not.toHaveProperty("profiles"); | ||
| } | ||
| const profile = config["default_permissions"] as string; | ||
| expect(profile).toStartWith("codex_security_scan_"); |
There was a problem hiding this comment.
[P2] Check the policy-specific worker permission profile
This new assertion runs for ordinary, discovery and merge policies, but the inherited execution preparer intentionally assigns codex_security_deep_scan_worker to discovery/merge. That value cannot start with codex_security_scan_, so all four fresh/resumed Deep-worker cases fail here before the later permission/settings checks. The mismatch is recorded in the Ubuntu shard and the inherited macOS shard. Check the expected ID by policy (codex_security_scan_execution for ordinary; codex_security_deep_scan_worker for discovery/merge), retaining the later settings and permission assertions. The logs establish a test expectation defect, not a worker runtime regression or a guarantee that this adjustment alone makes all CI green.
Withdrawing my approval after the introduced worker-profile test expectation defect was confirmed in hosted CI: #1112 (comment). Original review and validation evidence stays historical and unchanged.
b55bf3b to
7865928
Compare
alandelong-oai
left a comment
alandelong-oai
left a comment
There was a problem hiding this comment.
Reviewed head 7865928ab0b8f06e8bc78bc090e2680925fb9df2 against base 28ba8ce9f35cd35d1704c113e925cff8694caa46 using three fresh 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 policy-specific worker-profile expectation fixes the previously confirmed test defect: ordinary work expects codex_security_scan_execution; discovery and merge expect codex_security_deep_scan_worker. Existing inherited settings and permission checks remain. At this exact head, the selected lifecycle suite passed 20 tests with 39 filtered out; plugin/SDK builds, types and formatting passed. This local macOS result does not establish native Windows behavior or that this change alone makes all CI green.
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.
Consolidate approved Deep Scan changes into the parent topic branch.
Part 7 of 46. Previous: #1111 · Next: #1113 · Stack index
Summary
Move prompt preparation, Codex event handling, progress reporting, and budget callbacks out of the main scan method so Standard scans can share that behavior with later Deep Scan workers.
Changes
Testing
All 59 event and worker-lifecycle tests passed with 325 assertions. The profile assertion correction reproduced four worker failures with two ordinary controls passing before the update. Full SDK and MCP type checks, SDK formatting, and
build:cipassed.See this PR’s Checks tab for CI on the current commit.
Risk and rollout
Existing scans use the extracted modules immediately. Event ordering, cancellation, and budget enforcement are the main review areas. The existing Deep Scan runner remains active until the later integration.
Public disclosure review