Repository navigation
feat(deep-scan): resume interrupted scans from saved worker results - #907
mldangelo-oai wants to merge 250 commits into
Conversation
mldangelo-oai
changed the title
mldangelo-oai
added
area:artifacts
![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: e72cb2ee9b
ℹ️ 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".
| ? { nativeServiceTierAbsent: true as const } | ||
| : {}), | ||
| providerConfig: selected.model_providers as JsonObject | undefined, | ||
| runtimeSettings: workerRuntimeSettingsFromConfig(config), |
There was a problem hiding this comment.
When a Deep Scan resumes in a new SDK/MCP runtime, this captures settings only from CODEX_SECURITY_CONFIG_PATH; it never reads the isolated CODEX_SECURITY_DEEP_SCAN_CONFIG_PATH whose worker_runtime contains the original provider environment, native profile, web-search/context/instructions settings, and other worker overrides. CodexSdkWorkerExecutor.workerRuntimeSettings() therefore reads those values from the new process's snapshot instead, so remaining workers can run with a different provider configuration or fail authentication. Fresh evidence in this exact head is that workerRuntimeSettingsFromConfig(config) persists only the small base-config subset while executor.ts:695-708 still sources the rest from the current isolated snapshot. Persist the non-secret per-scan worker runtime selections and intentionally refresh only credentials.
AGENTS.md reference: AGENTS.md:L31-L35
Useful? React with 👍 / 👎.
| } | ||
| if (directories.has(directory)) continue; | ||
| directories.add(directory); | ||
| for await (const path of sessionFiles(directory)) { |
There was a problem hiding this comment.
When a completed worker rollout has been compacted to Codex's supported .jsonl.zst format before an SDK resume, this traversal now searches both active and archived directories but calls sessionFiles(directory) with its default compressed = false, so the worker is never loaded. Attribution then marks its usage missing, but its tokens cannot contribute to the priced lower bound, allowing a resumed --max-cost scan to continue even when that compressed worker already crossed the cap. Fresh evidence beyond the earlier archived-session issue is that the exact head added archived_sessions while still excluding compressed files; use the compressed-session reader already used by scan-logs.ts rather than treating these rollouts as absent.
AGENTS.md reference: sdk/typescript/AGENTS.md:L28-L28
Useful? React with 👍 / 👎.
mldangelo-oai
removed
area:artifacts
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: d1870d3dd9
ℹ️ 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 savedCompletion = resumeThreadId | ||
| ? await recoverSelectedCompletion() | ||
| : null; |
There was a problem hiding this comment.
When a Deep Scan has durably saved finalizationInput but its owner rollout is unavailable (for example, history was disabled or the log was lost after process failure), this recovery is reached only after the earlier findScanSession() check rejects the resume. No model turn is needed once a selection exists—the new event generator explicitly skips runStreamed()—so the accepted result remains stranded despite being sufficient for local publication. Check for a saved selection before requiring the rollout, and apply the same ordering to bulk resume.
AGENTS.md reference: sdk/typescript/AGENTS.md:L28-L28
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
🛡️ Codex Security Review · Automatically triggered
Here are some automated security review suggestions for this pull request.
Reviewed commit: d1870d3dd9
ℹ️ About Codex security reviews in GitHub
This is an experimental Codex feature. Security reviews are triggered when:
- You comment "@codex security review"
- A regular code review gets triggered (for example, "@codex review" or when a PR is opened), and you’re opted in so security review runs alongside code review
Once complete, Codex will leave suggestions, or a comment if no findings are found.
| draft = _read_staged_scan_draft(scan_dir, args.draft_path) | ||
| _require_current_deep_publication(db, connection, scan_id, draft) |
There was a problem hiding this comment.
🛡️ Codex Security Review · Automatically triggered
With an SDK-managed Deep scan and a writer in the model-writable scan directory, publish the exact draft checked here. _read_staged_scan_draft opens and validates one inode, but line 3090 reopens the path and lines 3128-3140 publish and hash that second document. A writer notified on the first open can atomically replace the file, so schema-valid attacker findings become the recorded publicationSha256, pass completion, and can flow to downstream publication. Fresh evidence at this head is the new first read plus the unchanged second read. Reuse the parsed draft or bind both reads by identity/digest.
SECURITY.md reference: SECURITY.md:L11-L18
Dismiss this finding: Reply with @codex security dismiss <reason> [context]. Codex will resolve this conversation automatically; GitHub may require a page refresh to show the result.
Valid reasons: false-positive, duplicate, out-of-scope, compensating-control, risk-accepted, or other. Example: @codex security dismiss duplicate Already flagged by another review
What each reason means
false-positive— Not a vulnerabilityduplicate— Already tracked elsewhereout-of-scope— Outside this review's scopecompensating-control— Mitigated by another controlrisk-accepted— Risk intentionally acceptedother— Another reason; context required
Useful? React with 👍 / 👎.
zcrab-oai
left a comment
zcrab-oai
left a comment
There was a problem hiding this comment.
Reviewed d1870d3. These issues remain after independent validation:
- [P2] Recover selected finalization before requiring the old rollout (sdk/typescript/src/api.ts:2099): A valid running CLI scan with a durable finalizationInput fails the earlier findScanSession check when its owner rollout is archived or removed. findScanSession only enumerates sessions, so control never reaches this model-free selected-result recovery. The outer catch only retries selected finalization for a ScanCostLimitExceededError with activeScan and budgetRecovery.threadId populated; neither applies to this earlier unavailable-session error. After validating the saved scan identity and ownership, check and recover an existing finalizationInput before requiring an active owner rollout or constructing a resumed Codex thread. Keep the rollout requirement for paths that need further model work.
Validation used the real session reader and cost tracker with synthetic saved rollouts. The API recovery paths were traced from the exact head. No model-backed scan was run.
Approval is withheld on these blockers; the remaining large diff has not been exhaustively cleared.
This branch has not been deployed
2 participants
Summary
Resume interrupted Deep scans from accepted worker results, preserve worker settings and receipt ownership, and enforce requested budgets from known priced usage when child usage is incomplete.
Changes
Testing
d1870d3d/9cfdc336. This includes portable checks, types/format/build, all 21 profile tests, the complete worker executor module and both installed-package consumers. Fresh/resumed discovery and reducer workers, concurrent settings, protected provider homes, cancellation, diagnostics and schema cleanup remain covered. The real npm launcher with a synthetic native process failed both child-reaping cases before correction; compatible exact-main controls passed, then both cases passed after correction without changing their deadlines.55a88668, including 405 runtime/knowledge/CLI tests with 23 skips and no failures on Bun 1.3.14. Action inputs passed 17 tests plus six standard/Deep dry-run controls against the owned CLI build. Those imported source and locked inputs remain unchanged by the cleanup correction.6ce07dc4/981c9289. Those six feature files remain byte-identical. Prior whole-contribution/material reviews retain that source attribution; proportional independent review of the new main import, cleanup correction and package delta completed with no findings. The exact-head three-pass native review and fresh independent verifier completed cleanly with no findings.e72cb2ee/7e396e79: fixed seed 12345 and randomized seed 2217608745 each passed 4,447 tests with 68 skips and no failures, with types/format between them. The successor's full pair has not run. Original local/hosted failures and the previous cleanup blocker remain recorded separately.142cf6eeCI compilation origin. SDK pin differences and the existing test-only Windows proof difference are accounted separately. No new native compilation or Windows execution is claimed.Risk and rollout
Existing commands, settings, diagnostics, credential/configuration protections and artifact-integrity checks remain compatible. Private profiles stay isolated; accepted completion authority and migration history remain preserved.
Public disclosure review
Maintainers should confirm the provenance of previously imported example and rendering-fixture material. The related attestation remains unchecked; current review does not establish historical erasure or approved provenance.