Repository navigation
fix: preserve native scan settings when resuming - #1159
Conversation
|
@codex review |
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. |
There was a problem hiding this comment.
Copilot review overview
🟡 Changes recommended
The Standard scan start/join response can still expose persisted recipe secrets.
Review effort: Balanced
Findings: 1
What changed in this PR
Restores saved scan settings and ownership metadata when resuming scans while limiting recipe exposure.
Changes:
- Persists and restores configuration, permissions, provider environment, and ownership tokens.
- Adds resume-boundary and CLI restoration tests.
- Removes saved recipes from most MCP tool responses.
| File | Description |
|---|---|
sdk/typescript/tests-ts/scan-resume-permissions.test.ts |
Tests saved settings and permissions across resume. |
sdk/typescript/tests-ts/completed-scan-handoff.test.ts |
Updates bundled-function extraction for response filtering. |
sdk/typescript/src/scan-registration.ts |
Adds saved recipe and continuation metadata handling. |
sdk/typescript/src/cli.ts |
Restores saved settings during CLI resume. |
sdk/typescript/src/api.ts |
Persists effective settings and forwards ownership claims. |
plugins/codex-security/mcp-app/tests/test_compact_artifact_server.mjs |
Tests recipe privacy in MCP responses. |
plugins/codex-security/mcp-app/server.ts |
Filters recipes from shared response helpers. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| function scanResponseContext(result: JsonObject) { | ||
| const { recipe: _recipe, ...context } = result; | ||
| return context; |
![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: 9cb82b36da
ℹ️ 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".
| scanOptions.resumeScanId !== undefined && | ||
| scanOptions.registeredScan === undefined |
There was a problem hiding this comment.
When prepareNativeScan re-enters an interrupted scan, it supplies registeredScan rather than resumeScanId. This branch now binds that operation to the existing database row, but prepareScanOutputDir still permits a non-empty directory only for resumeScanId or archival, and _run likewise calls prepareOutputDir unless resumeScanId is set. Once the registered scan has written any artifact, its continuation therefore fails with “Scan output directory must be empty” before reaching this registration; handle registeredScan as a continuation throughout output-directory preparation.
AGENTS.md reference: AGENTS.md:L30-L34
Useful? React with 👍 / 👎.
| if (session.inheritedPermissions !== undefined) { | ||
| const savedConfig = structuredClone(effectiveConfig); | ||
| removeManagedPluginRegistration(savedConfig); | ||
| recipe["config"] = { |
There was a problem hiding this comment.
For normal CLI/SDK scans, session.inheritedPermissions is undefined, so this condition leaves recipe.config as the preflight projection; unless a configured provider is detected, settings used by the original Codex process such as mcp_servers and shell_environment_policy are discarded. scans resume then constructs its client solely from that reduced saved configuration, causing an interrupted scan with those explicit settings to resume under different tools or environment values. Save the managed-plugin-stripped effective configuration independently of whether native permissions were inherited.
AGENTS.md reference: sdk/typescript/AGENTS.md:L23-L23
Useful? React with 👍 / 👎.
| } | ||
|
|
||
| function scanResponseContext(result: JsonObject) { | ||
| const { recipe: _recipe, ...context } = result; |
There was a problem hiding this comment.
When a headless Standard scan is rejoined after the native SDK has bound its launch recipe, _start_prompt_driven_scan returns scan_context, including that top-level recipe. The recipe can now contain model-provider headers and MCP environment credentials, but start_codex_security_standard_scan still builds its response with only redactHandoffClaimToken rather than this new recipe-removing helper, making those values model-visible. Apply the same recipe redaction to the Standard-scan response before returning it.
AGENTS.md reference: AGENTS.md:L6-L11
Useful? React with 👍 / 👎.
| const resumeThreadId = | ||
| scanOptions.resumeScanId === undefined | ||
| scanOptions.resumeScanId === undefined && | ||
| scanOptions.registeredScan === undefined |
There was a problem hiding this comment.
This change classifies registeredScan executions as continuations, but the later assignment of registration.sealedProducerVersion to expectation.pluginVersion remains gated on resumeScanId only. If a registered native scan was sealed before interruption and the installed plugin was upgraded before continuation, the workbench correctly returns the original producer version, yet contract validation still compares the artifact against the new runtime version and rejects otherwise recoverable completed results. Apply the sealed producer version for registered continuations as well.
AGENTS.md reference: sdk/typescript/AGENTS.md:L26-L26
Useful? React with 👍 / 👎.
3eaca35 to
c44918f
Compare
0555db1 to
c491057
Compare
c44918f to
d7ded9c
Compare
mldangelo-oai
changed the title
c491057 to
ec4ca94
Compare
d7ded9c to
1ef7e65
Compare
faizan-oai
left a comment
faizan-oai
left a comment
There was a problem hiding this comment.
The saved-recipe omission bug from the previous stack still reproduces. Starting with no reference documents saves neither knowledgeBasePaths nor knowledgeBaseSha256; after interruption, adding ambient CODEX_SECURITY_KNOWLEDGE_BASE makes a resumed worker receive the new documents under the same scan. The native preparation probe and an actual SDK/workbench resume fixture both confirm this at the current tip. The digest guard added here skips the absent saved hash. Preserve omission when a recipe exists and compare the saved absence as part of resume integrity. No approval yet.
| (scanOptions.resumeScanId !== undefined || | ||
| scanOptions.registeredScan !== undefined) && | ||
| isRecord(savedRecipe) && | ||
| typeof savedRecipe["knowledgeBaseSha256"] === "string" && |
There was a problem hiding this comment.
[P2] Treat an omitted saved knowledge-base hash as part of the saved configuration. This condition does not run for a scan originally started without documents. Native preparation currently falls back from the omitted recipe.knowledgeBasePaths to ambient CODEX_SECURITY_KNOWLEDGE_BASE, so the resumed worker can receive newly supplied documents while this check accepts the same scan. A synthetic SDK/workbench reproduction at the current tip records {savedKnowledgeBasePaths: null, savedKnowledgeBaseSha256: null, initialWorkers: 1, resumedWorkersWithKnowledge: 1}. Use ambient document settings only for a new scan with no saved recipe, and cover the absent-to-present transition in the resume check.
ec4ca94 to
d91bbf0
Compare
1ef7e65 to
5477f91
Compare
d91bbf0 to
4258cde
Compare
5477f91 to
253cbca
Compare
alandelong-oai
left a comment
alandelong-oai
left a comment
There was a problem hiding this comment.
Reviewed this native-resume change with three independent passes and root verification. The restack left every touched base/head file byte-for-byte identical; I also reran the resume tests on the updated head. I found one introduced P2 test failure on macOS, detailed inline.
Validation: 169 SDK tests passed with four platform skips. All three focused resume/handoff tests passed again on the updated head. The new MCP fixture fails immediately under the default macOS temporary directory, while the entire MCP file passes with TMPDIR set to a physical directory. The bundled plugin build passed. Existing continuation and saved-recipe comments are also retained for verification through the later native activation change; ordinary configuration reduction already existed on the base. No repository files were changed.
| const fixtureRoot = path.join( | ||
| temporaryRoot, | ||
| `private-recipe-${runtimeLabel}`, | ||
| ); | ||
| const repository = path.join(fixtureRoot, "repository"); | ||
| const scanDirectory = path.join(fixtureRoot, "scan"); |
There was a problem hiding this comment.
[P2] Resolve the temporary fixture path before registering the scan
On macOS, os.tmpdir() normally uses a symlinked path. This new fixture passes that unresolved scanDirectory straight to register-cli-scan, which rejects it with "Scan directory must be an existing canonical non-symlink directory." The MCP test file therefore aborts before exercising the new recipe check. I reproduced this under the default environment; the full file passes when TMPDIR points to a physical directory. Resolve the created fixture directory with realpath() and derive the repository/scan paths from it, as the other registration fixtures do.
4258cde to
f2a9a7a
Compare
253cbca to
c7800a5
Compare
alandelong-oai
left a comment
alandelong-oai
left a comment
There was a problem hiding this comment.
Three independent restack passes and root reconciliation are complete.
The saved-no-knowledge-base continuation fix is verified. The existing default macOS test failure remains at this own head in test_compact_artifact_server.mjs: the direct registration fixture passes a scan directory beneath an unresolved tmpdir alias. The final-tip MCP tests pass because #1163 replaces that fixture; their success does not validate the fixture at this PR boundary.
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.
c7800a5 to
837c840
Compare
f2a9a7a to
12cb54c
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 12cb54c1e605e43d7deb3bb57011d77bfab90761. 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 saved-no-knowledge-base continuation fix is verified. The existing default macOS test failure remains at this own head in test_compact_artifact_server.mjs: the direct registration fixture passes a scan directory beneath an unresolved tmpdir alias. The final-tip MCP tests pass because #1163 replaces that fixture; their success does not validate the fixture at this PR boundary.
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.
837c840 to
ea3b8ae
Compare
12cb54c to
5e37560
Compare
ea3b8ae to
eef4c96
Compare
5e37560 to
ccb3db5
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 ccb3db57514d.
The default macOS compact-artifact fixture now resolves the temporary directory before registration. Its actual own-head MCP test passes after building that plugin. The saved no-knowledge-base continuation fix remains intact. This clears the prior 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.
eef4c96 to
a7392c9
Compare
ccb3db5 to
252f40a
Compare
alandelong-oai
left a comment
alandelong-oai
left a comment
There was a problem hiding this comment.
Completed three fresh independent full-diff reviews and root verification at 252f40a9e849.
No actionable introduced finding survives the independent reviews and root reconciliation of this exact patch and inherited changes in touched files.
Integrated tip 6ef8cbb8, based on 008a8b4d, passed 1,388 SDK tests, 879 Python tests plus 100 subtests, 109 MCP tests, portable checks, builds and the clean 600-entry installed-package check. These are branch-tip integration checks, separate from own-head probes; no local Windows execution was performed.
No serious outstanding finding introduced by this PR. Code approval is separate from CI and merge readiness.
252f40a to
3d6f054
Compare
a7392c9 to
2be15f7
Compare
alandelong-oai
left a comment
alandelong-oai
left a comment
There was a problem hiding this comment.
Retained three prior independent passes for the verified identical patch and reconciled inherited touched-file changes at 3d6f054bb3ec.
No actionable introduced finding survives the independent reviews and root reconciliation of this exact patch and inherited changes in touched files.
Branch tip 3e0083d9, based on 008a8b4d, passed 1,392 SDK tests, 881 Python tests plus 100 subtests, 109 MCP tests, builds, portable source checks and the 600-entry installed-package check. These are tip checks, separate from own-head probes. No local Windows run was performed. The tip excludes the dependency update merged separately in #1184; combined validation remains outstanding.
No serious outstanding finding introduced by this PR. Code approval is separate from CI and merge readiness.
Consolidate approved Deep Scan changes into the parent topic branch.
e6a6c3c
into
mdangelo/codex/pr939-split-35-saved-merge-results
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 1e46ef9d9b
ℹ️ 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".
| config: { | ||
| ...scanPreflightCodexConfig(config), |
There was a problem hiding this comment.
When scans show --json retrieves a scan whose saved recipe contains native execution settings, this projection replaces recipe.config with the preflight subset, silently dropping fields such as mcp_servers, shell_environment_policy, and custom provider headers/auth configuration. The command explicitly promises the saved configuration, so users and automation can no longer inspect or round-trip the settings actually used by the scan; return the stored configuration unchanged rather than sanitizing the history response.
AGENTS.md reference: AGENTS.md:L51-L56
Useful? React with 👍 / 👎.
| const resumeThreadId = | ||
| scanOptions.resumeScanId === undefined | ||
| scanOptions.resumeScanId === undefined && | ||
| scanOptions.registeredScan === undefined | ||
| ? undefined | ||
| : registration["threadId"]; |
There was a problem hiding this comment.
When a registered native scan is re-entered after its rollout logs were removed, moved to another CODEX_HOME, or record a different working directory, this branch accepts the database thread ID and later calls codex.resumeThread without the findScanSession validation performed for resumeScanId. The newly added requireScanResumeSession helper is never invoked, so the continuation can fail only after claiming and launching the scan—and the normal failure path can then mark it terminal—instead of rejecting the unavailable original session up front. Apply that validation to registered continuations before starting the resumed process.
AGENTS.md reference: AGENTS.md:L30-L34
Useful? React with 👍 / 👎.
Part 36 of 46. Previous: #1158 · Next: #1160 · Stack index
Summary
Persist native execution settings and continuation ownership so resumed operations can reuse their saved configuration and permissions.
Changes
Testing
Local checks for the implementation:
The complete source and shipped compact-artifact script passed with a symlinked temporary root after fixture canonicalization.
Checks against the complete stack passed: SDK and MCP typechecks, formatting, Ruff, the SDK build, portable plugin compatibility and its tests, focused recovery/accounting tests, and local-artifact package smoke using exact cached dependencies. The package check was not a clean registry install. See this PR’s Checks tab for CI results for its current commit.
Risk and rollout
No CLI flags are added. Full effective configuration is saved for inherited native permissions; ordinary scan recipes still use the existing preflight projection. Registration checks reference-document identity and continuation metadata before resuming.
Public disclosure review