Repository navigation
refactor(runtime): share executable and process helpers - #1581
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 |
|
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". |
There was a problem hiding this comment.
🟢 Approval recommended
The refactor preserves existing behavior and includes focused coverage for the newly unified failure path.
0 open findings
What changed in this PR
Refactors executable-path and process contracts into shared plugin/SDK helpers, building on #1577.
Changes:
- Shares executable-path handling and SDK spawn types.
- Consolidates native binding loading and inferred helper types.
- Reuses preflight stdio diagnostics and tests write-callback failures.
| File | Description |
|---|---|
CONTRIBUTING.md |
Documents generated executable-path source. |
evals/secret-discovery/runtime.mts |
Updates the shared helper import. |
plugins/codex-security/mcp-app/src/executable-path.ts |
Adds the canonical path helper. |
plugins/codex-security/mcp-app/src/native.ts |
Consolidates native binding lookup. |
plugins/codex-security/mcp-app/src/deep-scan/executor.ts |
Uses the shared helper and inferred return type. |
plugins/codex-security/mcp-app/src/deep-scan/permission-profile-preflight.ts |
Reuses stdio failure handling. |
plugins/codex-security/mcp-app/tests/test_deep_scan_permission_profile_preflight.ts |
Covers write-callback failures. |
sdk/typescript/scripts/check-package.mjs |
Validates the generated module package contents. |
sdk/typescript/scripts/generate-models.cjs |
Generates the SDK helper copy. |
sdk/typescript/src/deduplication/codex-review.ts |
Uses the shared spawn signature. |
sdk/typescript/src/executable-path.ts |
Adds the generated SDK helper. |
sdk/typescript/src/feedback.ts |
Uses the shared spawn signature. |
sdk/typescript/src/runtime.ts |
Reexports shared paths and defines the spawn contract. |
🧠 Review effort: Balanced
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
…-finding-normalization
…elo/codex/shared-workbench-messages
…/codex/shared-process-contracts # Conflicts: # sdk/typescript/src/runtime.ts
|
@codex review |
|
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". |
…/codex/shared-finding-normalization
…elo/codex/shared-workbench-messages
…/codex/shared-process-contracts
|
@codex review |
|
Codex Review: Didn't find any major issues. Already looking forward to the next diff. 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". |
mldangelo-oai
changed the title
zcrab-oai
left a comment
zcrab-oai
left a comment
There was a problem hiding this comment.
Reviewed a5c465c69b31 in two source-review passes, including relevant callers and contracts.
Approved. No new actionable findings in the reviewed scope.
Validation: CI-helper TypeScript compilation and plugin bundling passed locally; native binaries came from CI with matching compiled source inputs. Focused SDK tests passed: feedback.test.ts. Focused MCP tests passed: test_deep_scan_permission_profile_preflight.ts. The full cross-platform suite was not run locally; Windows execution remains unverified here.
alandelong-oai
left a comment
alandelong-oai
left a comment
There was a problem hiding this comment.
Reviewed the PR diff at 5a5f4e4c7d58 in three independent source passes, followed by verification of the changed runtime paths.
The shared executable-path helper preserves Windows absolute and drive-relative path behavior, and its SDK copy is included in generation and package output. The consolidated stdio handler preserves retryable failures from both stream events and write callbacks. Native binding selection remains unchanged. No blocking source findings.
Static review; I did not rerun the test suites or native platform checks.
Summary
Share executable-path handling and process signatures between SDK and Deep Scan callers.
Depends on #1577.
Changes
Testing
For
a5c465c69b31, source-matched checks passed 34 process, 11 executable-path, and four archive controls, plus 24 preflight functions and eight selected executor controls. Worker checks cover fresh and resumed discovery/reducer launches, concurrent scans, inherited settings, intentional overrides, executable/home origins, and cancellation. All five portable plugin checks passed: Ruff lint and format, SDKbuild:ci, and the source-compatibility checker and its tests.This evidence covers selected executor controls, not a complete executor-suite run. Hosted CI for this candidate remains to be verified.
Risk and rollout
Depends on #1577. Executable lookup, authentication, environment forwarding, working directories, permission overrides, direct-provider routing, and per-scan configuration remain compatible. Existing commands and diagnostic text are preserved. Native packaging and platform behavior remain part of final pre-merge validation.
Public disclosure review
Reviewed the source, synthetic fixtures, commit metadata, description, and public PR artifacts.