Repository navigation
refactor(plugin): simplify Deep Scan coordinator ownership - #1213
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: Didn't find any major issues. You're on a roll. 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 ownership simplifications preserve existing semantics and are supported by focused lifecycle, persistence, recovery, and publication tests.
Review effort: Balanced
Findings: None
What changed in this PR
Simplifies Deep Scan coordinator ownership while preserving lifecycle, persistence, recovery, and publication behavior.
Changes:
- Consolidates terminal cleanup, locking, registry delegation, and store typing.
- Removes cancellation persistence from the store in favor of coordinator/server ownership.
- Reuses shared test fixtures and expands resume/publication assertions.
| File | Description |
|---|---|
plugins/codex-security/mcp-app/server.ts |
Uses shared locks, types, and direct coordinator/store ownership. |
plugins/codex-security/mcp-app/src/deep-scan/coordinator.ts |
Consolidates terminal cleanup and scheduler bookkeeping. |
plugins/codex-security/mcp-app/src/deep-scan/registry.ts |
Generalizes locking and simplifies remote observation. |
plugins/codex-security/mcp-app/src/deep-scan/store.ts |
Removes coordinator-external cancellation plumbing. |
plugins/codex-security/mcp-app/src/deep-scan/types.ts |
Derives the coordinator store contract from the implementation. |
plugins/codex-security/mcp-app/src/deep-scan/worker-runner.ts |
Clarifies reducer artifact ownership. |
plugins/codex-security/mcp-app/tests/support/streams.mjs |
Adds shared stdio server test utilities. |
plugins/codex-security/mcp-app/tests/test_workbench_state_fallback.mjs |
Adopts shared stream handling. |
plugins/codex-security/mcp-app/tests/test_deep_scan_store.mjs |
Refactors persistence ordering and retry fixtures. |
plugins/codex-security/mcp-app/tests/test_deep_scan_store_integration.mjs |
Reuses temporary-directory and promise fixtures. |
plugins/codex-security/mcp-app/tests/test_deep_scan_stdio_lifecycle.mjs |
Consolidates lifecycle process fixtures. |
plugins/codex-security/mcp-app/tests/test_deep_scan_coordinator.mjs |
Refactors coordinator fixtures and strengthens publication assertions. |
plugins/codex-security/mcp-app/tests/test_deep_scan_artifact_validation.mjs |
Reuses temporary-directory helpers. |
plugins/codex-security/mcp-app/tests/deep_scan_worker_failure_cases.mjs |
Adds historical prompt recovery coverage. |
plugins/codex-security/mcp-app/tests/deep_scan_publication_cases.mjs |
Updates shared publication synchronization fixtures. |
plugins/codex-security/mcp-app/tests/deep_scan_deadline_cases.mjs |
Updates deadline synchronization and failure assertions. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Artifact storage and server entrypoints repeat context and file-handling work. Consolidate that work at its actual owners while preserving storage boundaries.
The Deep Scan coordinator repeats lifecycle, failure-forwarding and store plumbing. Consolidate those operations at their existing owners.
alandelong-oai
left a comment
alandelong-oai
left a comment
There was a problem hiding this comment.
Reviewed 9831d22f135ec3fab0e4f14b290ebd95fb9c217f against its own base 3809b0c2ef2d5e4dc0169c696c8c80660096da09 with exactly three independent high-reasoning full-diff reviews, including every test diff, plus root verification. No serious introduced code defect identified.
All six complete affected MCP test scripts passed, including coordinator, persistence, process-lifecycle and fallback behavior. Fresh native host build, plugin/SDK builds, types, formatting, Ruff and portable source checks passed.
Local validation was on macOS arm64. No model execution, local Windows run or full installed-package smoke. Reviewed sources remained unchanged. This review covers this exact head; it does not establish combined-stack validation against current main.
9831d22 to
c7fcfd8
Compare
3809b0c to
5f195ee
Compare
|
Codex Review: Didn't find any major issues. 👍 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.
Retained the three earlier independent full-diff reviews after verifying that all 16 ordered file contributions are unchanged. Root verification reconciled the inherited server, worker lifecycle and workbench fixture changes with the coordinator refactor. No actionable introduced findings remain at this exact head.
Fresh validation passed: 6 complete MCP test scripts, native host and plugin/SDK builds, types, formatting, Ruff and portable source checks, including nine source-check tests. Reviewed source files still match the head. Local validation was on macOS arm64; no local Windows run, model execution or installed-package smoke was performed.
At the prepublication check, current-head CI was still running with no reported failures. Approval is not a claim of completed CI or merge readiness.
# Conflicts: # plugins/codex-security/mcp-app/tests/test_compact_artifact_server.mjs
|
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". |
|
Codex Review: Didn't find any major issues. Hooray! 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.
Reviewed exact head 0f907561abfd2933d6eeb2e59cdf262ead87c0a4 against 7989bdbd44c6f3c0b785a7ff4f22ebdddae101c7 with exactly three fresh independent HIGH full-diff reviews, including every test diff across all16 touched files, plus root verification. No actionable introduced findings remain.
Verified the default-clock cancellation change: the promise timer receives the abort signal, preserves the original cancellation reason, and the updated test uses the real default clock and checks that cancellation launches no further retry.
Fresh exact-head validation passed all six complete synthetic MCP scripts, the macOS arm64 native host, plugin and SDK builds, types, formatting, Ruff0.16.9, portable compatibility and nine source-check tests. All809 tracked source files and all16 reviewed paths still match the head. No model execution, source edits, local Windows run or full installed-package smoke.
Hosted CI was still running without failed latest-attempt jobs at23:08UTC. This is source approval; merge readiness remains dependent on hosted checks.
|
Codex Review: Didn't find any major issues. Keep it up! 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.
Source review approved for 4f6b06b896681b9c41f8affdafe3c2bc521c4b07 against dff043688e8e0d7cb9079525eec82ab61eed195f.
Retained exactly three independent HIGH full-diff review passes, including every test diff. Independently verified the actual GitHub three-dot diff, identical ordered contributions, all 16 touched paths at both base and head (bytes and modes), and the three historical report hashes. No actionable introduced source finding remains. No additional full source pass or local test was run for this restack; prior validation retains its original revision, execution time, and limitations.
Hosted CI was still unfinished at the separate 2026-10-04 05:14 UTC observation.
This is source approval, not a merge-readiness or combined-integration claim. No fresh model execution, local Windows validation, native rebuild, or clean installed-package smoke test is claimed.
Summary
The Deep Scan coordinator repeats lifecycle, failure-forwarding and store plumbing. Consolidate those operations at their existing owners.
Changes
Testing
Risk and rollout
Concurrent scans and durable recovery remain compatibility boundaries. This patch depends on the shared MCP fixtures and can be reviewed after the worker-launch patch without depending on that patch’s source.
Targets
main, withdff04368merged. The original refactor contribution is unchanged; conflicts from the landed parent PRs are resolved.Public disclosure review