Repository navigation
refactor: remove retired Deep Scan recovery routes - #1165
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
🟢 Approval recommended
The removed routes have no remaining references, and retained behavior and compatibility handling remain covered.
Review effort: Balanced
Findings: None
What changed in this PR
Removes retired Deep Scan recovery and accounting paths after migration to the SDK.
Changes:
- Deletes legacy recovery, publication, and worker-accounting code.
- Preserves historical review counts and current recovery helpers.
- Updates affected fixtures and context structures.
| File | Description |
|---|---|
plugins/codex-security/tests/workbench_test_support.py |
Removes a retired coordinator helper. |
plugins/codex-security/tests/test_workbench_scan_composition.py |
Updates the composition fixture. |
plugins/codex-security/scripts/workbench_scan_usage.py |
Removes legacy worker-thread accounting. |
plugins/codex-security/scripts/workbench_scan_history.py |
Deletes obsolete recovery routes. |
plugins/codex-security/scripts/workbench_saved_results.py |
Retains current helpers while removing legacy recovery logic. |
plugins/codex-security/scripts/workbench_db.py |
Removes obsolete saved-results dependencies. |
plugins/codex-security/scripts/workbench_composition.py |
Drops legacy run state while preserving review counts. |
plugins/codex-security/scripts/project_scan_artifacts.py |
Removes an unused coverage merge helper. |
💡 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. 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". |
e3495da to
768c85c
Compare
6c14f0c to
2060525
Compare
768c85c to
57ce934
Compare
2060525 to
5fdd7ca
Compare
57ce934 to
ef0ec7d
Compare
5fdd7ca to
1566bb0
Compare
faizan-oai
left a comment
faizan-oai
left a comment
There was a problem hiding this comment.
Reviewed retirement of the old recovery routes at 1566bb0 against the active composition/recovery paths. The integrated recovery, migration, and compact-server checks pass. No new issue found in this cleanup increment; upstream findings remain tracked separately.
ef0ec7d to
ccb5ac7
Compare
611b0c6 to
98092da
Compare
ccb5ac7 to
413d90e
Compare
alandelong-oai
left a comment
alandelong-oai
left a comment
There was a problem hiding this comment.
All three independent reviewers identified the same historical-accounting regression. Root verification reproduced it using the actual base/head composition loader, usage collector, SQLite session discovery, and rollout parser: the same scan changes from 110 tokens across two sessions to 10 tokens across one, and both receipts are labeled complete. Historical sealed results and interrupted sealed completion remain supported, so removing old execution machinery must not erase their worker associations.
All 79 composition tests passed; the plugin build and required Ruff check passed. AST comparison confirms that retained Python functions otherwise keep their behavior apart from the composition loader and thread selection. No repository files were changed.
| SELECT DISTINCT sdk_thread_id | ||
| FROM deep_scan_workers | ||
| WHERE scan_id = ? AND sdk_thread_id IS NOT NULL |
There was a problem hiding this comment.
[P2] Retain historical worker sessions when removing the old runner
Older Deep Scans recorded their independent SDK session IDs only in deep_scan_workers, not scan_execution_threads or child scan rows. Removing this lookup drops those sessions from both usage collection and saved execution-thread metadata. Using the actual revision-specific composition and usage modules with an owner session of 10 tokens and a legacy worker of 100, the base reports 110 tokens/two sessions; this head reports 10 tokens/one session with coverage still complete. Preserve or migrate these associations for supported historical results and interrupted sealed completion.
98092da to
86aa193
Compare
413d90e to
42b4e95
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 reconciliation are complete for this restacked head.
The real composition/accounting probe retains 110 input tokens across both historical worker threads at the restacked base, this head and the integrated tip.
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.
86aa193 to
098811d
Compare
42b4e95 to
10ce572
Compare
There was a problem hiding this comment.
Re-reviewed the real PR diff at 098811d4e36cf3297e161943d3e03506c2c90cf3. 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.
10ce572 to
4a6f0eb
Compare
098811d to
0ca998d
Compare
4a6f0eb to
f30ab3b
Compare
0ca998d to
8818d1e
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 8818d1ea40b2.
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.
8818d1e to
e9666ca
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 verified identical patch and reconciled inherited touched-file changes at e9666ca1ff7a.
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.
e9666ca to
7bf154b
Compare
23fbdf0 to
0aa1b39
Compare
alandelong-oai
left a comment
alandelong-oai
left a comment
There was a problem hiding this comment.
Retained three prior independent passes for the verified identical patch and reconciled inherited touched-file changes at 7bf154b6f647.
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.
a61565a
into
mdangelo/codex/pr939-split-41-unused-result-helpers
Part 42 of 46. Previous: #1164 · Next: #1166 · Stack index
Summary
Remove retired Deep Scan recovery and progress dispatch after the plugin entrypoint moves to the SDK, while retaining historical session accounting.
Changes
Testing
Local checks for the implementation:
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
The preceding plugin switch already rejects unsupported unfinished scans. This removes unused legacy execution routes while preserving historical reports, review counts and measured parent/worker usage.
Public disclosure review