Repository navigation
refactor(plugin): simplify Deep Scan worker launch - #1212
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. 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". |
There was a problem hiding this comment.
Copilot review overview
🔵 Needs a closer look
It changes security-sensitive, cross-platform child-process permission and executable-selection boundaries that still require CI and human validation.
Review effort: Balanced
Findings: None
What changed in this PR
Simplifies Deep Scan worker launch, preflight, and prompt setup while preserving child-process configuration boundaries.
Changes:
- Streamlines worker configuration, permission-profile handling, and prompt rendering.
- Reuses shared validation, version, and temporary-directory helpers.
- Expands coverage for resumed, concurrent, and platform-specific worker launches.
| File | Description |
|---|---|
artifact-writer-main.ts |
Imports the package version directly. |
server.ts |
Imports the package version directly. |
src/version.ts |
Removes the version bridge implementation. |
src/record.ts |
Adds shared non-empty-string validation. |
src/deep-scan/executor.ts |
Simplifies launch configuration, event handling, profiles, and path resolution. |
src/deep-scan/parent-sandbox.ts |
Reuses shared string validation. |
src/deep-scan/permission-profile-preflight.ts |
Consolidates validation and preflight error handling. |
src/deep-scan/templates.ts |
Simplifies template substitution and reducer arguments. |
src/deep-scan/worker-runner.ts |
Reuses computed reducer worker IDs. |
tests/support/reducer-paging/deep-reducer-paging.mjs |
Adapts the reducer evaluation to the new renderer API. |
tests/test_deep_scan_executor.mjs |
Strengthens worker-boundary and configuration assertions. |
tests/test_deep_scan_permission_profile_preflight.mjs |
Reuses shared test helpers and predicates. |
tests/test_deep_scan_templates.mjs |
Validates template placeholders and renderer behavior. |
💡 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.
Worker launch and permission preflight repeat configuration and template setup. Consolidate those paths while preserving the child-process settings boundary.
alandelong-oai
left a comment
alandelong-oai
left a comment
There was a problem hiding this comment.
Reviewed ea826c191dd389a81e741c54e8819200dcb34923 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.
The complete synthetic executor, permission-preflight and template test scripts passed, including fresh/resumed worker launch assertions. 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.
3809b0c to
5f195ee
Compare
ea826c1 to
ac70da9
Compare
|
Codex Review: Didn't find any major issues. Breezy! 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 13 ordered file contributions are unchanged. Root verification reconciled the inherited server changes with the version import and worker launch changes. No actionable introduced findings remain at this exact head.
Fresh validation passed: 3 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. Can't wait for the next one! 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. Breezy! 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 20197a8feda12c09647d8cfb7580d169c133f5e0 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 13 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
Worker launch and permission preflight repeat configuration and template setup. Consolidate those paths while preserving the child-process settings boundary.
Changes
Testing
Risk and rollout
Per-scan isolation, credentials, permission profiles and executable selection remain protected. Child-process tests must continue covering both worker kinds, resumes and concurrent executors.
Targets
main, withdff04368merged. The original refactor contribution is unchanged; conflicts from the landed parent PRs are resolved.Public disclosure review