Repository navigation
fix(deep-scan): preserve worker coverage during recovery - #1064
Conversation
|
@codex review |
|
@codex security 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. |
🛡️ Codex Security ReviewSecurity review completed. No security issues were found in this pull request. Reviewed commit: ℹ️ About Codex security reviews in GitHubThis is an experimental Codex feature. Security reviews are triggered when:
Once complete, Codex will leave suggestions, or a comment if no findings are found. |
![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: 01e0ff255f
ℹ️ 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".
|
@codex review |
|
@codex security review |
🛡️ Codex Security ReviewSecurity review completed. No security issues were found in this pull request. Reviewed commit: ℹ️ About Codex security reviews in GitHubThis is an experimental Codex feature. Security reviews are triggered when:
Once complete, Codex will leave suggestions, or a comment if no findings are found. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: d42be9b65e
ℹ️ 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".
|
@codex review |
|
@codex security review |
🛡️ Codex Security ReviewSecurity review completed. No security issues were found in this pull request. Reviewed commit: ℹ️ About Codex security reviews in GitHubThis is an experimental Codex feature. Security reviews are triggered when:
Once complete, Codex will leave suggestions, or a comment if no findings are found. |
|
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.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 161c704917
ℹ️ 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".
There was a problem hiding this comment.
Copilot review overview
🔵 Needs a closer look
Canonical ID normalization is incomplete and can duplicate accepted coverage during publication.
Review effort: Lite
Findings: None
What changed in this PR
Preserves Deep Scan coverage and provenance through reduction, retries, publication, resume, and stopped-scan recovery while retaining v1 compatibility.
Changes:
- Separates accepted coverage from reducer findings.
- Adds provenance, retry, receipt, and recovery handling.
- Expands TypeScript and Python regression coverage.
| File | Description |
|---|---|
plugins/codex-security/tests/test_partial_coverage_projection.py |
Tests stopped-scan recovery projections. |
plugins/codex-security/scripts/workbench_saved_results.py |
Recovers and publishes accepted coverage. |
plugins/codex-security/mcp-app/tests/test_retry_coverage_receipts.mjs |
Tests receipt preservation across retries. |
plugins/codex-security/mcp-app/tests/test_retry_coverage_dispositions.mjs |
Tests retry disposition handling. |
plugins/codex-security/mcp-app/tests/test_deep_scan_executor.mjs |
Tests reducer attempt propagation. |
plugins/codex-security/mcp-app/tests/test_deep_scan_artifact_validation.mjs |
Verifies legacy persistence compatibility. |
plugins/codex-security/mcp-app/tests/test_coverage_provenance.mjs |
Tests provenance and completeness. |
plugins/codex-security/mcp-app/tests/test_artifact_scan_draft.mjs |
Tests deferred coverage resolution. |
plugins/codex-security/mcp-app/tests/deep_scan_publication_cases.mjs |
Updates publication expectations. |
plugins/codex-security/mcp-app/tests/deep_scan_coverage_fixture.mjs |
Provides coverage fixtures. |
plugins/codex-security/mcp-app/src/deep-scan/worker-runner.ts |
Passes reducer attempt context. |
plugins/codex-security/mcp-app/src/deep-scan/coordinator.ts |
Carries coverage through reductions. |
plugins/codex-security/mcp-app/src/deep-scan/artifact-validation.ts |
Aggregates and projects source coverage. |
plugins/codex-security/mcp-app/src/artifact-scan-draft.ts |
Preserves retry and archived coverage. |
plugins/codex-security/mcp-app/src/artifact-io.ts |
Extends reducer worker context. |
plugins/codex-security/mcp-app/src/artifact-deep-reducer.ts |
Preserves coverage outside reducer data. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
|
@codex review |
|
@codex security review |
|
Checked the canonical-ID concern in Copilot’s summary against publication and recovery. The 13 existing regressions pass for duplicate worker-local IDs, renamed parent surfaces, linked deferred work, normalized questions, and repeated recovery. Publication preserves explicit deferred IDs. The summary includes no failing example or inline finding, and I could not reproduce a duplicate accepted record. |
🛡️ Codex Security ReviewSecurity review completed. No security issues were found in this pull request. Reviewed commit: ℹ️ About Codex security reviews in GitHubThis is an experimental Codex feature. Security reviews are triggered when:
Once complete, Codex will leave suggestions, or a comment if no findings are found. |
|
Codex Review: Didn't find any major issues. Nice work! 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". |
alandelong-oai
left a comment
alandelong-oai
left a comment
There was a problem hiding this comment.
Thanks for the recovery work. Owner-level merge-safety review of c251e8b: one test regression needs attention before approval. The concurrent-cancellation fixture patches the old module binding, so its simulated publication never runs. The inline comment identifies the hook update; a private control with that update passes all original assertions. This does not demonstrate production checkpoint loss.
Current portable checks, builds, type checks, formatting, package verification and focused Python recovery/finalizer checks passed. The focused MCP core run was 88 passed / 1 failed at that fixture. A broader run on unchanged relevant inputs had 1,021 passes and 202 failures: the same fixture regression plus 201 individually classified trusted-owner environment failures. Full SDK/platform suites are not claimed as passing.
alandelong-oai
left a comment
alandelong-oai
left a comment
There was a problem hiding this comment.
Thanks for the recovery work and follow-up fixes. Reviewed f76b92a: the previous capture-hook finding is addressed, and the unmodified concurrent-cancellation regression now passes. This owner-level merge-safety and simplification review found no new material issues in the recovery, publication, receipt-ownership or history integration.
Fresh checks passed: 89 focused MCP tests, 52 SDK tests, five report tests, all required portable-source checks, builds, type checks, formatting and package-content verification. Six publication cases and 17 history cases stop during setup at this environment’s existing trusted-owner check, before their assertions; those are not claimed as passing. Full current-head CI remains in progress.
alandelong-oai
left a comment
alandelong-oai
left a comment
There was a problem hiding this comment.
Thanks for integrating the latest updates while preserving the recovery fixes. This owner-level merge-safety rereview found no material issues. The core recovery and receipt-ownership changes retain their reviewed behavior, the earlier capture-hook fix still passes, and the manifest preserves all required files and three runtime chunks.
Fresh checks passed: 116 Python tests plus 86 subtests, 41 MCP rendering/recovery tests, the packaged discovery-prompt test, all five portable checks, the 157-file plugin build, types and formatting. Full SDK, protected lifecycle and cross-platform qualification remain unverified locally; native binaries were reused only after input and payload verification. Hosted CI was still running at the checked snapshot.
alandelong-oai
left a comment
alandelong-oai
left a comment
There was a problem hiding this comment.
Thanks for preserving surface-only follow-up while retained work is still pending. The rereview found no material issues at 9ab750d: completed-candidate and fully completed-surface protections remain intact, including #1064’s receipt-ownership behavior.
Fresh validation passed: 62 recovery/candidate cases, 15 receipt/concurrency cases, three additional receipt-integration controls, all five portable checks, a 157-file plugin build, types and formatting. The new predicate is present in the rebuilt runtime. Native binaries were reused only after input and payload verification.
Full SDK/MCP, protected lifecycle and cross-platform qualification remain unverified locally; no fresh native build is claimed. Hosted CI was still running at the checked snapshot.
![github-code-quality[bot]](https://avatars.githubusercontent.com/u/9919?s=60&v=4){kind=small}
alandelong-oai
left a comment
alandelong-oai
left a comment
There was a problem hiding this comment.
Thanks for preserving worker coverage and receipt ownership through recovery. This owner-level merge-safety rereview found no material issues at 2fda7a8. The main integration preserves the reviewed recovery changes, and the added warning/SARIF behavior composes with frozen replay.
Fresh local checks passed: 119 MCP cases, 188 Python tests plus 29 subtests, three warning-export cases, all five portable checks, plugin build, types and formatting. Local protected-lifecycle fixtures remain blocked by environment setup; the identical CI merge tree passed 3,074 Python tests plus 331 subtests (85 skipped).
Full MCP/SDK platform CI was still running at the checked snapshot. Native artifacts were reused after source/toolchain and payload verification; no fresh local native build or full-suite pass is claimed.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 067b95a4b7
ℹ️ 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".
alandelong-oai
left a comment
alandelong-oai
left a comment
There was a problem hiding this comment.
Thanks for preserving worker coverage and receipt ownership through recovery. This owner-level merge-safety rereview found no material issues at 067b95a. The main integration preserves the recovery behavior and executor assertions, arguments and platform guards.
I also checked the latest checkpoint-composition concern: the worker writer already checkpoints the fully composed result before publication, and recovery captures those checkpoints after reading the result.
Fresh portable checks, plugin build, types, formatting, 50 focused MCP recovery/concurrency cases and 41 Python cases passed. Some local executor/database fixtures stopped at ownership prerequisites, and seven CLI authentication fixtures failed Git-root setup; full local suites are not claimed. Verified CI artifacts for the identical tree show 3,073 Linux Python tests passed with 85 skipped, plus 398 macOS Python tests and 30 subtests passed. The remaining platform matrix was still running.
Summary
Keep accepted coverage and receipt ownership intact through concurrent worker writes, retries, and stopped recovery. Preserve completed task IDs and reasons in Deep Scan results, including resumed reduction and recovery without a parent result.
Changes
Testing
Integrated
mainatc3a1e5b8, including the reviewed fixture/authentication changes and expanded CI shards. The sole manual conflict was the executor test: retain the four named test groups and awaited cleanup, keep reducer-attempt coverage under its original non-Windows guard, and pass the startup-classification helper its required worker callback. All other 17 incoming files exactly match main. Independent AST review confirms that all original assertions, function bodies, and platform guards remain intact.Validation for
067b95a4b7ecd34a814d976f5cd6716ec2cc8294:The preceding
2fda7a85integration completed hosted CI successfully, including the unchanged macOS retry. Its local validation passed 131 Python tests and 29 subtests, 57 MCP rendering tests, ten publication/receipt cases, 32 combined SDK interface cases, and 311 release-workflow tests, plus portable/package/type/format checks and independent review. Its eight duplicate-parent conflict resolutions preserved the reviewed recovery versions, and the complete tree matched the expected upstream combination. Recovery projection, receipt provenance, identity validation, and rollback sources are unchanged by this final test-group refresh.Completed full SDK qualification remains bound to these recorded revisions:
c251e8ba, seed12345: 6,979 passed, 83 skipped, zero failed, across 233 files.26d80188, default order with printed seed3011354238: 6,991 passed, 83 skipped, zero failed, across 234 files.40ef0600, default order with printed seed1809773577: 6,953 passed, 83 skipped, zero failed, across 233 files.The later narrow MCP fix and final upstream integration are covered separately by focused checks. No full SDK run on the final SHA, fresh native build, installed-package smoke run, or cross-platform local pass is claimed. Fresh hosted CI is required for the final head. Local validation uses Linux, Node 22.23.2, Bun 1.3.14, and Python 3.12.13.
The preceding
9ab750d8surface-follow-up fix passed 61 focused recovery cases, ten publication/receipt cases, all five portable checks, a fresh 157-file build, MCP types, formatting, and independent review. It retains surface-only follow-up while work remains pending without reopening completed terminal surfaces. Its subsequent macOS CI attempt exhausted the existing job/test deadlines; both affected tests passed unchanged locally and in recent hosted macOS controls. No timeout or source changes were made for those timing failures.The earlier
26d80188refresh passed 93 Python projection tests and 54 subtests, 34 MCP rendering tests, 119 SDK canonical-contract tests, the shipped prompt test, portable/package/type/format checks, and independent review. Thef76b92aintegration passed 21 focused Python history/report tests, 52 SDK coverage/history/verification tests, and ten MCP publication/receipt cases; its full hosted MCP suite also passed. Both MCP fixture repairs remain unchanged and preserve full draft, retry, immutable-source, publication, and authenticated worker-review assertions.Earlier integration evidence includes 303 affected Python tests, 571 identity/publication tests, the full MCP store integration, 64 focused recovery tests, four MCP stdio tests, nine SDK package-consumer/scan-matching tests, and 56 SARIF tests with 33 subtests on their recorded intermediate revisions. These remain revision-bound evidence. The 571-case run covered identity and artifact-publication recovery, not the MCP publication-integration module repaired above. Initial history fixtures rejected group-writable output directories under the host's permissive umask; they passed with standard
022fixture permissions without source changes.Previous refresh validation (historical)
Merged
mainat5f521759e7c6ff8c3fbd5e81fd1a3567ac801b59into this branch at072fdcad9ca0dc9a30563ad3418d3782722a6515. The clean merge preserves the existing branch contribution.git diff --checkpassed.These checks ran on Linux with Node 22.23.3 and Bun 1.3.14, seed
12345. Existing dependencies and generated plugin/native artifacts were reused after matching package/lock bytes, complete plugin/build inputs, and artifact hashes. Plugin source is unchanged by this refresh, so portable plugin checks were not repeated. ShellCheck was unavailable and disabled. No fresh plugin/native build, full SDK suite, installed-package smoke run, or cross-platform pass is claimed. Fresh hosted CI remains required for this head.Earlier feature, integration, and portable-check results carried by
abc60054383e86de7f3dbe7ab08612258a7da7b8remain historical evidence for their recorded revisions. They were not rerun for this refresh unless listed above; existing review, rollout, and disclosure qualifications below remain in force.Risk and rollout
Recovery ordering and ownership span live retries and saved archives. Checkpoint-payload verification, receipt-resolution guards, and the model-facing prohibition on terminal Deep child-task closeouts remain enforced. No public command, flag, or environment variable is added. Local validation uses Node 22.23.2 on Linux; fresh hosted CI must pass for the final head before merging.
Public disclosure review
The current documentation example is explicitly fictional. A removed historical example's provenance remains unestablished, so the second attestation stays unchecked. The current public materials and exact package were reviewed; historical uncertainty remains recorded for maintainer follow-up.