Repository navigation
refactor: reuse scan settings when comparing findings - #1111
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 |
There was a problem hiding this comment.
Copilot review overview
🟡 Changes recommended
Selected profiles can bypass read-only restrictions, relative working directories resolve incorrectly, and incomplete turns may be accepted.
Review effort: Balanced
Findings: 1
Open (3)
What changed in this PR
Refactors scan comparison to reuse shared execution setup while preserving provider, environment, and permission settings.
Changes:
- Adds read-only execution preparation and provider propagation.
- Runs MCP discovery from the repository directory.
- Expands comparison execution tests.
| File | Description |
|---|---|
sdk/typescript/src/api.ts |
Connects completed scans to prepared comparison execution. |
sdk/typescript/src/execution-preparation.ts |
Adds read-only permission preparation. |
sdk/typescript/src/runtime.ts |
Adds subprocess working-directory support. |
sdk/typescript/src/scan-comparison.ts |
Uses shared preparation and disables inherited MCP servers. |
sdk/typescript/tests-ts/scan-comparison.test.ts |
Tests providers, permissions, MCP configuration, and propagation. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| config: JsonObject; | ||
| overrides: string[]; | ||
| } { | ||
| const prepared = structuredClone(config); |
| const turn = await readCodexTurn({ | ||
| thread, | ||
| events: (await thread.runStreamed(input, turnOptions)) | ||
| .events, | ||
| }); | ||
| return { finalResponse: turn.finalResponse }; |
| environment, | ||
| undefined, | ||
| options.signal, | ||
| options.workingDirectory, |
![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: eb1766c68b
ℹ️ 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 prepared = structuredClone(config); | ||
| if (permissions === undefined) return { config: prepared, overrides: [] }; | ||
| delete prepared["permissions"]; | ||
| delete prepared["projects"]; | ||
| delete prepared["sandbox_mode"]; | ||
| prepared["default_permissions"] = "codex_security_comparison"; |
There was a problem hiding this comment.
When a scan has inherited permissions and its selected Codex profile defines default_permissions, permissions, or sandbox_mode, this only removes the root-level settings while retaining the profile-level versions. Codex applies the selected profile over the root config, so the generated codex_security_comparison profile can be replaced; the normal permission preflight then rejects the matcher and automatic finding matching is silently skipped via the warning path (and a custom factory without that preflight could run with the writable profile). Sanitize these fields from every profile as scanRuntimeCodexConfig already does before selecting the read-only profile.
AGENTS.md reference: sdk/typescript/AGENTS.md:L20-L23
Useful? React with 👍 / 👎.
| const configured = (config?.["mcp_servers"] ?? {}) as JsonObject; | ||
| const names = new Set([ | ||
| ...Object.keys(configured), | ||
| ...inherited.map(({ name }) => name), | ||
| ]); | ||
| const names = new Set([...Object.keys(configured), ...inherited]); | ||
| return Object.fromEntries( |
There was a problem hiding this comment.
For a normal non-command-auth scan whose active profile declares mcp_servers, this helper enumerates only the root mcp_servers table. In the new prepared API path, the discovery mcp list process reads the managed Codex home and is not given these effective profile overrides, while the unchanged profile/profiles tables are subsequently passed to the comparison process; resolving the selected profile can therefore add an enabled MCP server after the disabled root table is built. This lets the read-only comparison turn invoke a configured MCP tool despite the explicit MCP isolation, so resolve the selected profile before enumerating servers or disable servers in every profile.
AGENTS.md reference: sdk/typescript/AGENTS.md:L20-L23
Useful? React with 👍 / 👎.
mldangelo-oai
changed the title
alandelong-oai
left a comment
alandelong-oai
left a comment
There was a problem hiding this comment.
Reviewed eb1766c68b1fa8e4b1aef2de3fc92a5324ee5443 against mdangelo/codex/pr939-stack-05-worker-permissions at e6b731c52b74e7dc0e3f0ce32b7918248e03cf79, scoped to this PR's diff and touched files.
Reviewed comparison setup, authentication/model forwarding, read-only configuration, and streamed-turn handling. No additional distinct findings; existing comments already cover selected-profile overrides, incomplete turns, and relative working directories.
Review included simplification/deletion opportunities. Full repository and platform test suites were not run.
e6b731c to
3779b93
Compare
338d654 to
0138a07
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 0138a075c4
ℹ️ 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".
| ? undefined | ||
| : await mergedCodexConfig(options.config); | ||
| const configuredModel = | ||
| : resolveCodexProfile(requestedConfig); |
There was a problem hiding this comment.
When a standalone caller supplies a selected profile while CODEX_HOME already selects a different profile, resolving the requested profile here removes its profile key before the configuration is merged with the home config. The ambient selection therefore survives in providerConfig and in the child process, so Codex can use the ambient profile's provider/authentication instead of the explicitly requested one; before this change, the requested profile key overrode it. Resolve the profile after merging with the home configuration, or explicitly neutralize the ambient selection at the process boundary.
AGENTS.md reference: sdk/typescript/AGENTS.md:L20-L23
Useful? React with 👍 / 👎.
0138a07 to
c42ebd9
Compare
faizan-oai
left a comment
faizan-oai
left a comment
There was a problem hiding this comment.
Reviewed comparison setup, provider/authentication forwarding, and conversion of inherited writes to read-only access while retaining denied paths. No issue found in this layer. Comparison and inherited-permission checks passed at the integrated stack tip.
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. No serious introduced findings remain. The current code resolves supplied profiles before applying comparison restrictions, rejects incomplete turns, and normalizes the comparison working directory.
Validation: 70 comparison tests passed, 3 platform-specific tests skipped, no failures. Native fresh/resumed permission preflight accepted the comparison profile even with the parent scan profile present. The ambient legacy-profile candidate did not reproduce as an introduced regression: the bundled Codex rejects that configuration on both base and head before a model turn.
e9bc320 to
fd98dde
Compare
c42ebd9 to
07debb1
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 07debb181e
ℹ️ 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 { | ||
| model, | ||
| config: { ...config, mcp_servers: disabledMcpConfiguration(config, []) }, | ||
| create: options.codex ? () => options.codex! : options.createCodex!, |
There was a problem hiding this comment.
When a caller supplies both an existing codex client and inheritedPermissions, this factory discards the prepared constructor configuration, while startReadOnlyCodexThread also omits sandboxMode: "read-only" because inherited permissions were supplied. The injected client therefore receives neither the comparison permission profile nor the read-only fallback and can run with its original, potentially writable sandbox. Keep the thread-level read-only sandbox for options.codex, or otherwise ensure the inherited profile is actually applied to that client.
AGENTS.md reference: sdk/typescript/AGENTS.md:L23-L23
Useful? React with 👍 / 👎.
alandelong-oai
left a comment
alandelong-oai
left a comment
There was a problem hiding this comment.
The previously completed three independent passes carry forward after verifying the patch is identical; root reconciled the current base and discussion.
No serious outstanding finding was identified in this change. The scoped prior review and current diff remain consistent.
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.
07debb1 to
ea6edb3
Compare
fd98dde to
b437d67
Compare
There was a problem hiding this comment.
Re-reviewed the real PR diff at ea6edb34338f08ad8fce62aa741d207c88a45ceb. Three independent high-reasoning passes covered the complete changed patch and tests; root source checks and focused reproductions informed this decision.
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.
b437d67 to
700642d
Compare
ea6edb3 to
5bd01d5
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 5bd01d54c036.
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.
5bd01d5 to
54564a6
Compare
700642d to
54cb83e
Compare
de9bcb1 to
dfbbf22
Compare
8a4930a to
7b6540d
Compare
alandelong-oai
left a comment
alandelong-oai
left a comment
There was a problem hiding this comment.
Source review approved for 7b6540d938c12abb04e31d894fd5e0f3eff0ab5f against dfbbf2202463992147c6d2bb32d977b8d7a09b6a.
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 actionable introduced finding remains.
Validation is retained for the unchanged touched source: 307 distinct SDK tests passed and 7 were skipped. The suite was not rerun for this restack, and this is not a new whole-tree validation claim.
Hosted CI was still pending at the latest check (2026-10-03T23:28:22.807077+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.
3b9f9b3 to
f4f6f3a
Compare
7b6540d to
84aa638
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". |
There was a problem hiding this comment.
Source review approved for 84aa638c07ea8b77ec73e9ae471c8d71201ef634 against f4f6f3aacd1f466eb5383329c7266b4ebfe45302. Completed exactly three fresh independent HIGH full actual-PR-diff passes, including every changed test and fixture, followed by root verification. No supported introduced source finding remains in this contribution.
Fresh focused validation passed 309 SDK tests with 7 skips, builds, source type checks and formatting. The first archive check rejected two Python bytecode files generated in the disposable validation tree; removing only those untracked files and repacking passed the archive check, with no product-test rerun. The synthetic subprocess tests do not prove native permission enforcement or live provider authentication.
CI update at the 2026-10-04T06:54:54.177389+00:00 audit: the CI-only hold is cleared at this unchanged reviewed head. Run 37181842083 attempt 2 completed successfully with 44 successful and 2 skipped latest-attempt jobs. All 46 job IDs are distinct from attempt 1; the recorded job execution times run from 06:29:38 through 06:50:14 UTC. The earlier attempt’s cancellations and failed coverage checks remain historical; retry success does not establish their cause, flakiness or a source fix. 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.
09d373e to
b61426b
Compare
84aa638 to
a638837
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: a638837dea
ℹ️ 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".
| multi_agent: false, | ||
| multi_agent_v2: false, | ||
| plugins: false, | ||
| shell_tool: false, |
There was a problem hiding this comment.
When CODEX_HOME selects a profile—particularly one preserved for command-auth scans—Codex applies that profile after these root-level overrides. A profile can therefore restore features.shell_tool/unified_exec and replace shell_environment_policy with an unrestricted policy, allowing the repository-influenced comparison turn to execute tools with ambient credential variables despite the intended lockdown. Neutralize the active profile at the child-process boundary or sanitize these fields in the selected profile as well.
AGENTS.md reference: sdk/typescript/AGENTS.md:L22-L24
Useful? React with 👍 / 👎.
alandelong-oai
left a comment
alandelong-oai
left a comment
There was a problem hiding this comment.
Source approval for a638837dea1aa28f353051055f5bfd45d9359b43 against b61426b9b921b05befeda557a383c9fd0fd6bfff. Exactly three fresh independent HIGH reviews covered the full actual GitHub PR diff, including every changed test and fixture. Root synthesis found no supported introduced finding.
Focused current-head validation passed 88 scan-comparison tests with 3 skips, SDK/plugin builds, source type checks and formatting. A bounded native configuration check rejected the legacy profile syntax before execution; it did not establish the reported tool-restoration claim. Historical native versions and other profile paths remain unverified. No model ran.
The prefix contains main at 634532d, while main has separately advanced to 1fb0e5f. This source approval does not establish successful whole-stack integration. Exact-head CI was pending in the 2026-10-04 09:26 UTC receipt and is tracked separately. No clean install, local Windows run or model execution is claimed.
alandelong-oai
left a comment
alandelong-oai
left a comment
There was a problem hiding this comment.
Source approval is withheld after new hosted Windows evidence established a P2 in the newly added provider test fixture. The original three independent reports and focused macOS validation retain their recorded results; this is a later bounded CI finding, not a repeated full review. Three variants fail the new empty-stderr assertion on a native temporary-home warning. No provider runtime regression or complete fix is established.
| undefined, | ||
| home, | ||
| ); | ||
| expect(validation.stderr).toBe(""); |
There was a problem hiding this comment.
[P2] Allow the native temporary-home warning in this fixture
These new home-definition cases create CODEX_HOME with temporaryDirectory, then require the native validation command to write no stderr. Windows Codex emits a warning when it refuses to create PATH helper aliases beneath that temporary home, so all three provider-name variants fail here before the success assertion is reached. This is recorded in the #1111 CI job and the inherited #1114 test-quality job. Remove the empty-stderr requirement while keeping the command-success check and, if needed, expected JSON-output validation. Keep the isolated home and native permission restrictions; do not suppress runtime diagnostics. These logs do not establish a provider runtime failure or guarantee that this change alone makes the entire shard pass.
Withdrawing my approval because newly completed Windows CI established the test-fixture defect recorded at #1111 (comment). Original source-review reports and validation receipts remain unchanged.
a638837 to
785e0c3
Compare
b61426b to
050e4fd
Compare
alandelong-oai
left a comment
alandelong-oai
left a comment
There was a problem hiding this comment.
Source approval for 785e0c3acd305ebee9fcfffe96f2c145f7bf1445 against 050e4fd414c8d9eca3ee19d968d05f9a68e7f16f. Exactly three fresh independent HIGH reviews covered the full actual GitHub diff, including all eight paths and four test paths, followed by root synthesis. No supported new source finding.
The warning-sensitive empty-stderr assertion is removed. Native command success, isolated home and the existing configuration restrictions remain checked. This resolves the test defect in discussion4176986710 at this head; it does not establish Windows success or that this change alone makes all CI green. The three selected home-definition cases passed on macOS (88 other tests filtered out); plugin/SDK builds, types and formatting passed. Existing dependencies and 14 previously built native artifacts were reused after identity checks.
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.
050e4fd to
ec06194
Compare
785e0c3 to
28ba8ce
Compare
alandelong-oai
left a comment
alandelong-oai
left a comment
There was a problem hiding this comment.
Source approval for head 28ba8ce9f35cd35d1704c113e925cff8694caa46 against base ec06194e9e74f00aa1e661efb3f244aa133fe7cc.
Retained exactly three original independent HIGH full actual-diff reviews, including every changed test and fixture, after current contribution and touched-file reconciliation. The ordered own contribution is unchanged. The only inherited touched change is the separately reviewed worker-profile selection from #1110, with identical ordered edits on both base and head; root checked the surrounding provider/configuration composition. Actual current GitHub diff bytes differ in inherited context from the prior diff, which was reconciled rather than treated as byte-identical. No residual source issue was found. The removed warning-sensitive empty-stderr assertion remains removed.
No unchanged full source review or local product validation was repeated. Prior validation retains its original head and execution time. Hosted CI for this new head was pending at 2026-10-04T11:17:33.848692+00:00; older-head success is separate. The actual three-dot equals the own-base diff, and the reviewed prefix contains current main. This is source approval, not whole-stack successful integration, deployment, native Windows or clean-install evidence.
Consolidate approved Deep Scan changes into the parent topic branch.
e391c2f
into
mdangelo/codex/pr939-stack-05-worker-permissions
Part 6 of 46. Previous: #1110 · Next: #1112 · Stack index
Summary
Use the completed scan's prepared Codex settings for its automatic finding comparison, preserving the selected provider, authentication, and environment. Standalone comparisons prepare their own client from the selected profile, including its command-authentication settings.
Changes
Resolve the selected configuration profile before applying explicit caller overrides and comparison restrictions. Explicit provider choices remain authoritative for both authentication and execution, including when managed login selects a different credential home.
Retain denied paths, reduce inherited write access to read access, and disable network access and external tools for the comparison.
Read tool configuration from the resolved repository directory and reject incomplete comparison turns.
Keep the thread-level read-only sandbox for an injected Codex client whose constructor permissions cannot be replaced.
Keep the prepared read-only comparison profile authoritative through the scan’s shared client factory.
Merge the explicitly requested Codex profile with home settings before resolving authentication. Comparison helpers retain the selected provider, model, and API-key behavior when the home selects a different profile. Resolve profile-local command authentication before anchoring its working directory and forwarding the provider override, preserving the selected command and account arguments.
Preserve explicit model and reasoning-effort overrides in component planning and finding matching, while inheriting unspecified settings from the selected home profile.
Carry the selected provider definition when managed login changes credential homes. Serialize provider tables as complete TOML values so literal provider names containing dots or quotes retain their meaning. Omit unset optional feature keys before permission-checked comparison serialization, while preserving explicitly configured false and true values.
Testing
Windows CI exposed an invalid empty-stderr assumption in the native provider fixture. The fixture now retains native success and provider/configuration assertions while allowing nonfatal startup warnings. The full corrected comparison suite passed 88 tests with three platform skips and 568 assertions; the exact-parent comparison controls passed 65 tests with three platform skips. Types and formatting passed.
Literal provider names containing dots or quotes reproduced two CLI configuration failures at the affected head; all three parent controls passed. After the correction, 95 comparison, worker-policy and permission-lifecycle tests passed with three platform skips and 1,270 assertions. Full types and formatting passed.
On the authored correction, the permission-checked and managed-home regressions reproduced two failures with three controls, then passed all five cases after the correction. The full comparison, fresh/resumed worker-policy and permission-lifecycle suites passed 92 tests with three platform skips and 1,204 assertions; full types and formatting passed.
Earlier comparison, API matcher and worker-policy suites passed 86 tests with three platform skips. An actual matcher, native enumeration and child-process regression reproduced the explicit-provider override failure and passed after the correction.
Earlier command-auth regressions cover home-selected and explicitly selected profiles in both credential-environment modes.
All 33 configuration and fresh/resumed discovery/reducer worker cases passed. One configuration fixture was rerun successfully after generating its missing plugin bundle.
Earlier model-precedence validation passed all five cases, including three baseline failures, within 130 component/comparison/worker cases with three platform skips.
Full SDK/MCP/generated types, the SDK CI build, changed-file formatting and a fresh plugin bundle passed for the earlier provider-precedence correction.
See this PR’s Checks tab for CI results on its current commit.
Risk and rollout
This changes post-scan comparisons immediately. Comparison failures remain warnings and do not discard completed scan results. The Deep Scan runner is unchanged at this step.
Public disclosure review