Repository navigation
refactor: extract scan startup and add worker permission checks - #1110
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. 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
🔵 Needs a closer look
Worker process lifecycle and permission enforcement are security-sensitive and warrant final human validation.
Review effort: Balanced
Findings: None
What changed in this PR
Centralizes worker startup and permission enforcement for ordinary scans and future Deep Scan workers.
Changes:
- Adds permission-profile preflight checks for fresh and resumed workers.
- Shares execution setup, environment propagation, and worker configuration.
- Adds coverage for approval policies, cancellation, failures, and inherited settings.
| File | Description |
|---|---|
sdk/typescript/src/api.ts |
Integrates shared execution preparation and inherited permissions. |
sdk/typescript/src/config.ts |
Resolves profile approval policy and worker configuration. |
sdk/typescript/src/execution-preparation.ts |
Adds shared worker launch and configuration helpers. |
sdk/typescript/src/permission-profile.ts |
Implements permission preflight and fallback detection. |
sdk/typescript/src/scan-execution.ts |
Defines the permission rejection error. |
sdk/typescript/scripts/check-package.mjs |
Includes new modules in package verification. |
sdk/typescript/tests-ts/config-approval-policy.test.ts |
Tests profile approval precedence. |
sdk/typescript/tests-ts/execution-worker-policy.test.ts |
Tests worker settings and permission enforcement. |
sdk/typescript/tests-ts/permission-profile-stop.test.ts |
Tests preflight cancellation and process cleanup. |
sdk/typescript/tests-ts/support/codex-process.ts |
Adds synthetic process test support. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
mldangelo-oai
changed the title
alandelong-oai
left a comment
alandelong-oai
left a comment
There was a problem hiding this comment.
Reviewed e6b731c52b74e7dc0e3f0ce32b7918248e03cf79 against mdangelo/codex/pr939-stack-04-execution-inputs at 57ba2bd907957d8fd979d5071dddfcddae157fde, scoped to this PR's diff and touched files.
Reviewed shared launch construction, permission-profile checks, approval precedence, cancellation cleanup, and the added tests. No new distinct actionable findings.
Review included simplification/deletion opportunities. Full repository and platform test suites were not run.
| ): typeof childProcess.spawn { | ||
| return ((...args: Parameters<typeof childProcess.spawn>) => { | ||
| const [command, argv, options] = args; | ||
| if (command !== executable || !Array.isArray(argv)) return spawn(...args); |
There was a problem hiding this comment.
[P2] Compare the Windows executable paths in the same form
createExecutionCodex passes the executable through executablePathForSpawn, which produces a namespaced Windows path, while this fixture compares it with the original unprefixed path. The comparison therefore misses the synthetic executable and attempts to spawn the nonexistent file. All four execution-worker-policy cases fail on Windows before reaching their worker-policy assertions (CI failure). Normalize both sides the same way, or construct the fixture with the actual spawn path, so this PR's fresh/resumed worker coverage runs on Windows as intended.
57ba2bd to
6853db5
Compare
3779b93 to
e9bc320
Compare
![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: e9bc320014
ℹ️ 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".
| return { | ||
| ...session, | ||
| policy: "discovery", | ||
| sessionConfig: deepWorkerConfig(session.sessionConfig), |
There was a problem hiding this comment.
When the selected profile defines mcp_servers.codex-security, prepareDiscoveryExecution leaves that profile intact while disabling the server only at the root. Codex applies the selected profile over the root configuration—as reflected by resolveCodexProfile and the existing profile-MCP tests—so a profile such as { mcp_servers: { "codex-security": { enabled: true } } } re-enables the server for discovery workers, defeating the stated workbench-tool isolation. Resolve the selected profile before calling deepWorkerConfig, as the merge-worker path already does.
AGENTS.md reference: AGENTS.md:L28-L34
Useful? React with 👍 / 👎.
| ...(protectedCredentialHome === undefined | ||
| ? {} | ||
| : { [protectedCredentialHome]: "read" }), | ||
| ...inheritedPermissions?.filesystem, |
There was a problem hiding this comment.
When inheritedPermissions is supplied for a worker, spreading its filesystem entries here does not remove the ordinary scan profile's preexisting :workspace_roots = "write" grant. The Deep Scan parent-sandbox path transports root-read plus explicit denials and intentionally does not transport parent write access, so unless the caller redundantly includes a :workspace_roots override, discovery and merge workers can still modify the repository or other workspace roots despite the inherited read-only policy. Build the worker filesystem profile without the workspace-write default when inherited permissions are present.
AGENTS.md reference: AGENTS.md:L28-L34
Useful? React with 👍 / 👎.
faizan-oai
left a comment
faizan-oai
left a comment
There was a problem hiding this comment.
Reviewed fresh/resumed worker construction and permission-profile preflight. No issue found in the incremental change. At the integrated stack tip, the permission/preflight suites passed and a real Codex 0.159.0 configuration-only preflight accepted the generated profile. That probe did not execute a model turn; Windows was not tested locally.
alandelong-oai
left a comment
alandelong-oai
left a comment
There was a problem hiding this comment.
Reviewed the exact base-to-head diff with three independent review passes and root verification. Approval remains withheld for the existing inherited-permission and selected-profile MCP issues: #1110 (comment) and #1110 (comment). A direct helper probe confirms that a root-read inherited policy acquires workspace writes, and that the selected discovery profile can restore the disabled server setting. I am also tracing these helpers through the later stack integration.
Validation: 9 focused tests passed. Fresh and resumed permission preflight accepted the generated profiles using the bundled Codex binary without starting a model turn. That confirms native compatibility, but does not correct the generated policy.
e9bc320 to
fd98dde
Compare
4decbf9 to
db50f95
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: fd98ddea14
ℹ️ 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".
| ...(codexPathOverride === undefined | ||
| ? {} | ||
| : { codexPathOverride: executablePathForSpawn(codexPathOverride) }), | ||
| ...(externalProvider !== null || apiKey === null ? {} : { apiKey }), |
There was a problem hiding this comment.
When preserveProviderEnvironment is enabled and the invoking environment has only OPENAI_API_KEY with no usable native account, prepareExecutionSource deliberately leaves apiKey null, so this condition omits the SDK apiKey option and the worker receives no CODEX_API_KEY. The existing Deep Scan executor documents and handles this exact case by checking the native account and mapping OPENAI_API_KEY through the SDK because Codex exec reads CODEX_API_KEY (plugins/codex-security/mcp-app/src/deep-scan/executor.ts:94-111); without the equivalent fallback, fresh and resumed workers fail authentication in a common API-key environment even though preparation reports API-key auth.
AGENTS.md reference: AGENTS.md:L30-L34
Useful? React with 👍 / 👎.
alandelong-oai
left a comment
alandelong-oai
left a comment
There was a problem hiding this comment.
Reconciled the unchanged patch and prior three-pass review against this exact restacked head.
The unchanged patch retains the existing inherited-policy finding: an inherited root-read policy still gains workspace write access. The native permission probe continues to accept the broadened configuration for fresh and resumed execution. The selected-profile MCP issue is fixed later; it does not clear this own-head finding.
Approval remains withheld. This review distinguishes the PR's own head from fixes verified later in the stack; it does not duplicate existing inline findings.
fd98dde to
b437d67
Compare
db50f95 to
ab95dba
Compare
alandelong-oai
left a comment
alandelong-oai
left a comment
There was a problem hiding this comment.
Re-reviewed the real PR diff at b437d671d45604558b29d07af06fdd8dfd931867. 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.
The unchanged patch retains the existing inherited-policy finding: an inherited root-read policy still gains workspace write access. The native permission probe continues to accept the broadened configuration for fresh and resumed execution. The selected-profile MCP issue is fixed later; it does not clear this own-head finding.
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. Current CI still contains failed, canceled, or pending checks; this review does not establish merge readiness.
ab95dba to
b31a388
Compare
b437d67 to
700642d
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 700642d99780.
The inherited filesystem policy now keeps workspace access read-only when the caller supplies root-read permissions. The actual native permission preflight accepted both fresh and resumed execution with no generated workspace-write grant. This clears the prior approval hold.
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.
700642d to
54cb83e
Compare
b31a388 to
31271d2
Compare
daf0578 to
85f0cb8
Compare
alandelong-oai
left a comment
alandelong-oai
left a comment
There was a problem hiding this comment.
Verified the contribution is identical to the previously reviewed revision and reconciled both inherited touched-file changes from main. Retained the three independent review passes with root verification. Fresh validation passed: all 9 focused worker-policy tests, native/plugin/SDK builds, types and formatting. No actionable introduced findings remain.
Local validation was on macOS arm64, without model execution, a local Windows run or an installed-package smoke test. Hosted CI and merge readiness are tracked separately.
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.
85f0cb8 to
07fc462
Compare
de9bcb1 to
dfbbf22
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: dfbbf22024
ℹ️ 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".
| ), | ||
| ); | ||
| }); | ||
| child.stderr.resume(); |
There was a problem hiding this comment.
When codex app-server rejects configuration or otherwise reports a startup failure on stderr before returning a response, this unconditional drain discards the only actionable diagnostic and the exit handler replaces it with the generic “ended before its response” error. Preserve the collected stderr in the surfaced SDK failure so users can identify the invalid config or executable problem.
AGENTS.md reference: AGENTS.md:L51-L56
Useful? React with 👍 / 👎.
| join(session.runtime.codexHome, "config.toml"), | ||
| deepMerge({ ...session.runtimeConfig }, config), | ||
| ); | ||
| return release; |
There was a problem hiding this comment.
When session.runtimeConfig is defined for an initialized or native runtime home, this function overwrites CODEX_HOME/config.toml with the worker-specific merged configuration and returns the bare unlock callback, so the first emitted event releases the lock without restoring the prior file. The worker's MCP, permission, and profile settings therefore remain in the shared home and can leak into later scans or ordinary Codex runs; wrap the release callback so it restores the original configuration before unlocking.
AGENTS.md reference: AGENTS.md:L31-L35
Useful? React with 👍 / 👎.
alandelong-oai
left a comment
alandelong-oai
left a comment
There was a problem hiding this comment.
Source review approved for dfbbf2202463992147c6d2bb32d977b8d7a09b6a against 07fc462c51a2d59518fa5f79b4c51a8cf8db5bc5.
Retained the three completed independent HIGH reasoning full-diff reviews, including every test diff, because this PR's own contribution is unchanged. Independently reconciled the current base and head snapshots and inherited changes in touched files. No additional full review pass was run for this restack, and no serious introduced finding remains.
Fresh validation passed the native host and SDK/plugin builds, TypeScript and formatting checks, 9 focused SDK tests, and the 560-entry production archive check.
Hosted CI was still pending at the latest check (2026-10-03T23:28:22.641056+00:00), with no failed jobs observed on this head. This source approval is not a statement that the PR is ready to merge. No model execution, local Windows validation, or full installed-package smoke test is claimed.
Two new discussion claims were assessed before approval. The preflight helper does discard stderr on startup failure when inheritedPermissions activates permission checking; this is a nonblocking diagnostic-quality issue (P3), not evidence that a healthy scan fails. See discussion_r4175372486. The runtimeConfig write helper has no producer through this head's API or worker preparation, so discussion_r4175372488 does not establish an introduced public-path configuration leak at this revision. Directly constructing the internal helper input is a separate condition.
dfbbf22 to
3b9f9b3
Compare
07fc462 to
da92b98
Compare
alandelong-oai
left a comment
alandelong-oai
left a comment
There was a problem hiding this comment.
Source review approved for 3b9f9b3d84949ab826bb6da34d23b32842d31e6e against da92b983da64ca09a0a4814d86093be40db52df1.
Completed exactly three fresh independent HIGH full actual-PR-diff passes, including all ten touched files and all three test files plus the support fixture, followed by root verification. No actionable introduced source finding remains in this contribution. Fresh focused validation passed nine SDK tests, builds, source type checks, formatting and the 564-entry archive check. Synthetic child-process coverage is not live native-worker or integrated model validation.
CI remains held. This head inherits #1105's declaration export defect: generated execution-auth.d.ts re-exports an internal auth helper absent from auth.d.ts; both source and emitted declaration bytes match #1105. Separately, the current Windows Node22 test-shard3 job exits with mprotect failed: 487, without an emitted failing assertion or final test summary. All four changed worker-policy variants passed before that later crash. Its cause and introduction remain unproven.
The 05:51 UTC hosted CI receipt was still pending with failed jobs. Source approval does not establish CI completion, merge readiness or whole-stack integration with current main. No model execution, local Windows validation, native rebuild or clean installed-package smoke test is claimed.
3b9f9b3 to
f4f6f3a
Compare
da92b98 to
9847c8a
Compare
alandelong-oai
left a comment
alandelong-oai
left a comment
There was a problem hiding this comment.
Source review approved for f4f6f3aacd1f466eb5383329c7266b4ebfe45302 against 9847c8a5062debb6a53e28c555658e17ac8bc63f. Completed exactly three historical independent HIGH full actual-PR-diff passes retained after ordered-contribution and inherited touched-file reconciliation, followed by root verification. No supported introduced source finding remains in this contribution.
All ten touched own/base files retain identical modes and bytes. The parent’s declaration-export fix is independently consumer-verified in #1105. No duplicate local suite was run; previous nine SDK outcomes remain historical. The earlier Windows mprotect failure belongs to the old head and its cause remains unexplained.
The exact-head latest-workflow CI set was successful at the 2026-10-04T06:25:55.396371+00:00 audit. Source approval and individual CI do not establish whole-stack/current-main successful validation or merge readiness. No model execution, local Windows run, native rebuild or clean installed-package smoke is claimed.
9847c8a to
dc20dd8
Compare
f4f6f3a to
09d373e
Compare
alandelong-oai
left a comment
alandelong-oai
left a comment
There was a problem hiding this comment.
Source review approved for 09d373e53225cde70015edc71a23de57d491051c against the actual GitHub PR diff's merge base dc20dd82733d1b14a8c1792c00fc0c584829a4a0.
Retained exactly three independent HIGH full actual-diff reviews, including every changed test/fixture, after verifying the unchanged ordered contribution across all 10 touched files. Root reconciled old/current base and head bytes and modes. The inherited changes in check-package.mjs match the exact reviewed and landed #1210 hunks on both sides. Original report hashes and review times are preserved. No additional full source pass was run, and no supported introduced source finding remains.
Historical validation: nine focused SDK tests passed on 3b9f9b3d84949ab826bb6da34d23b32842d31e6e from 2026-10-04T05:49:10.306810Z to 05:49:15.224369Z. Those results remain attached to that revision; no tests were rerun for this head.
The separate 08:27 UTC CI observation still had this head pending. This is source approval only; completed CI, merge readiness and whole-stack/current-main validation are not established. No fresh tests, builds, native compilation, clean installed-package smoke, local Windows run or model execution was performed for this restack.
09d373e to
b61426b
Compare
dc20dd8 to
c00c582
Compare
alandelong-oai
left a comment
alandelong-oai
left a comment
There was a problem hiding this comment.
Source approval for b61426b9b921b05befeda557a383c9fd0fd6bfff against c00c582998fd3238f8d755b7d655cdd9a7df7de6. The actual GitHub three-dot diff, ordered contribution, and touched base/head file modes and bytes match the previously reviewed contribution. All three independent HIGH full-diff reviews, including tests and fixtures, are retained after reconciliation; root synthesis found no new supported finding.
The prefix now contains main 634532d7f7fb21ccca49b4bca9bb9fa1df9308d6.
Exact-head CI was still pending in the 2026-10-04 09:05 UTC observation. This is source approval; whole-stack successful integration is not established. Prior validation remains tied to its original head and execution time; no duplicate local tests, builds, native compilation, clean install, Windows run or model execution were performed for this restack.
b61426b to
050e4fd
Compare
c00c582 to
0c97ad2
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 050e4fd414
ℹ️ 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".
| const permissions = sessionConfig["permissions"] as JsonObject; | ||
| // A stable argv-only profile supports managed allowlists without merging | ||
| // the runtime home's persisted scan defaults into inherited permissions. | ||
| const profileId = `${SCAN_PERMISSION_PROFILE}_execution`; |
There was a problem hiding this comment.
When an organization already allowlists the existing stable codex_security_deep_scan_worker profile, switching worker execution to codex_security_scan_execution makes permissionProfile/list reject every discovery and merge worker until administrators update managed requirements. The existing coordinator defines and documents the former ID in plugins/codex-security/mcp-app/src/deep-scan/permission-profile-preflight.ts, so reuse that shared identifier rather than introducing a second permission-profile name for the replacement path.
AGENTS.md reference: sdk/typescript/AGENTS.md:L16-L16
Useful? React with 👍 / 👎.
alandelong-oai
left a comment
alandelong-oai
left a comment
There was a problem hiding this comment.
Source approval for 050e4fd414c8d9eca3ee19d968d05f9a68e7f16f against 0c97ad273db459b6e224354dd0c1ccd909639812. The actual GitHub diff, ordered contribution and touched own/base file modes, OIDs and bytes match the prior reviewed contribution. Exactly three prior independent HIGH full-diff reports, including tests and fixtures, are retained after reconciliation; there is no inherited touched residual and root synthesis found no new supported finding.
Historical validation keeps its original head and execution time. No unchanged local source review, test or build was repeated for this contribution. The new permission-profile comment describes a conditional managed-allowlist compatibility risk. The touched production API remains ordinary; replacement of existing discovery/merge worker callers is not established at this own head. Explicit inheritedPermissions callers can activate the new ID, and requirements excluding it can reject that path. This is not a guarantee of all managed-policy compatibility.
Exact-head hosted CI remained pending in the 2026-10-04 10:32–10:33 UTC observation; historical failures at older heads are separate.
The prefix contains main 1fb0e5fbf8684c926cac2a36d08389b212d28d5a. Successful whole-stack integration remains unestablished. No clean installation, fresh native compilation, local Windows run or model execution is claimed.
0c97ad2 to
e494301
Compare
050e4fd to
ec06194
Compare
|
Codex Review: Something went wrong. Try again later by commenting “@codex review”. ℹ️ 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 approval for head ec06194e9e74f00aa1e661efb3f244aa133fe7cc against base e494301ccb3ba59bcb0f9f192762d34c63f7795b.
Reviewed the full actual GitHub diff, including every changed test and fixture, with three fresh independent HIGH reviews and root synthesis. No actionable source finding. Explicit discovery and merge executions now use the established codex_security_deep_scan_worker profile ID; ordinary executions retain codex_security_scan_execution. The fail-closed profile checks and inherited filesystem/network restrictions remain.
Validation on this head: all 6 worker-policy SDK tests passed with seed12345; plugin build, SDK build, types and formatting passed. These tests use synthetic child processes to exercise ordinary/discovery/merge forwarding and fresh/resumed accepted/rejected policies. They do not establish deployed organization-policy compatibility or native Codex behavior. Existing dependencies and 14 verified prior native artifacts were reused; no fresh native build, clean install, local Windows execution or product model run. The earlier conditional compatibility concern was not proof of an active production P1.
Hosted CI was observed 2026-10-04T11:10:59.907225+00:00: pending. The actual three-dot diff equals the own-base diff, and this reviewed prefix contains current main. Whole-stack successful integration remains unestablished.
Consolidate approved Deep Scan changes into the parent topic branch.
3ea7582
into
mdangelo/codex/pr939-stack-04-execution-inputs
Part 5 of 46. Previous: #1105 · Next: #1111 · Stack index
Summary
Extract Codex startup from the scan API and prepare the launch path for Deep Scan discovery and merge workers. Worker startup verifies that Codex accepts the required filesystem and network permissions before the turn runs.
Changes
Testing
See this PR’s Checks tab for CI results on its current commit.
Risk and rollout
Ordinary scans use the launch helper here; the new Deep Scan worker setup is connected later. A permission mismatch stops the worker, while a failed preflight connection remains a transport error. No public command or flag is added.
Public disclosure review