feat(deep): share execution and persist recoverable scan results - #907
mldangelo-oai wants to merge 179 commits into
Conversation
![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: 0fa83d8de8
ℹ️ 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".
| for (const path of sessionPaths.get(threadId)!.slice(1)) { | ||
| if (await extendsSessionLog(path, session.path)) session.path = path; |
There was a problem hiding this comment.
When the first-home copy contains the shared scan prefix followed by a later unrelated owner turn, while the recorded-home copy contains that prefix followed by the missing scan-turn events, this raw comparison treats the copies as divergent and retains the truncated first copy; the turn filter runs only afterward, so it removes the unrelated suffix without recovering the recorded scan suffix. Fresh evidence in the exact head is the new extendsSessionLog check, which explicitly rejects every non-prefix duplicate even when attribution makes the recorded copy more complete for this scan; compare or merge the attributed event occurrences instead.
AGENTS.md reference: AGENTS.md:L40-L41
Useful? React with 👍 / 👎.
|
@codex review The saved-log follow-up is now in 4c0039a. Duplicate selection and output use the same existing replay and scan-attribution filter. The reproduced unrelated-owner-tail case now retains the recorded scan suffix, including repeated and non-usage events. Equal, shorter and genuinely divergent attributed streams still keep traversal priority. The 159 affected tests pass. Independent installed readers and the public CLI preserve all ten expected events versus seven on the parent; stream cleanup/error controls pass. Required full SDK suites and this head's hosted platform checks remain pending and are identified separately in the description. Please review the current complete change. |
|
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". |
|
@codex review Please review exact current head Since the previous review, budget-limited publication now assigns missing finding identities without colliding with authored identities, and empty partial reports describe unresolved work accurately. Native Deep completion now belongs to the owning coordinator after selected publication; local observers wait for that finalizer, and completion failures reach active observers. Please review interruption, ownership and result-acceptance behavior, including the new real stdio lifecycle cases. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 652298cd96
ℹ️ 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".
| finish: (input) => this.options.store.finish(input), | ||
| }); | ||
| if (this.canceled || this.externallyFailed) return; | ||
| await this.options.onFinalized?.(cloneState(this.state)); |
There was a problem hiding this comment.
When cancel_codex_security_scan arrives after selected publication has set the Deep run to succeeded but while this callback is awaiting complete-scan, cancellation aborts the publication controllers but cannot interrupt this finalizer. Moreover, cancelAfterPersistence() deliberately postpones the actual cancel-scan command until the coordinator reaches finally, so complete-scan can seal the parent before cancellation is persisted, leaving an explicitly canceled scan reported as complete. Make this finalization cancellation-aware or persist cancellation before allowing parent completion to commit.
AGENTS.md reference: AGENTS.md:L42-L43
Useful? React with 👍 / 👎.
| onFinalized: async (run) => { | ||
| try { await completeSelectedParent(run); } |
There was a problem hiding this comment.
For an SDK Deep scan, this callback runs complete-scan from inside the MCP tool call, before the owning Codex turn emits its final turn.completed usage and before the SDK calls tracker.stop(). The workbench therefore seals the scan and fixes completedAt using only mid-turn rollout data; the later receipt suffix falls outside that attribution window and is represented as an unpriced remainder, so the SDK cannot reconcile a final cost or detect when that suffix crosses maxCostUsd before the scan is already complete. Delay parent completion until the owning turn's usage is final, or provide an authoritative post-completion accounting and budget-enforcement path.
AGENTS.md reference: AGENTS.md:L11-L11
Useful? React with 👍 / 👎.
| const immediate = deepScanTerminalResult(begun.run); | ||
| if (immediate) return { begun, immediate }; | ||
| const completingLocally = begun.run.status === "succeeded" | ||
| && begun.run.finalizationInput | ||
| && deepScanCoordinators.get(begun.run.scanId); | ||
| if (immediate && !completingLocally) return { begun, immediate }; |
There was a problem hiding this comment.
When another MCP process rejoins after finish-deep-scan commits succeeded but before the owning process's complete-scan settles, its registry cannot see the owner's in-memory coordinator, so this branch treats the result as immediately terminal and the rejoiner subsequently runs completeSelectedParent itself. That creates two cross-process parent finalizers and can let the late rejoiner report success while the original active observers receive the owner's completion failure. Distinguish an actively owned finalization from an orphaned succeeded result before permitting recovery completion.
AGENTS.md reference: AGENTS.md:L42-L43
Useful? React with 👍 / 👎.
|
@codex review Please review exact current head |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: ca53a81d67
ℹ️ 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".
| }): Promise<DeepScanRunState> { | ||
| const { run, artifacts, signal } = input; | ||
| const selection = selectedInput(run); | ||
| if (run.status === "succeeded") return run; |
There was a problem hiding this comment.
When the Deep run has reached succeeded but its parent remains running, this early return skips both the selected-result digest check and republication. During the remainder of the SDK owner turn—or after process loss—scan-manifest.json, findings.json, or coverage.json can therefore be replaced with different schema-valid content, and the later complete-scan seals that content because parent readiness checks only the run status and manifest path, not its selected digest. Verify that the canonical draft still derives from finalizationInput, or republish the selected bytes, before accepting this state.
AGENTS.md reference: AGENTS.md:L42-L43
Useful? React with 👍 / 👎.
| if (sdkOwned) return; | ||
| try { | ||
| await runWorkbench([ | ||
| "complete-scan", "--scan-id", run.scanId, "--thread-id", threadId, | ||
| ...optionalArg("--claim-token", handoffClaimToken), |
There was a problem hiding this comment.
For a native Deep scan (sdkOwned === false), this runs complete-scan from inside start_codex_security_deep_scan, so collect_scan_usage fixes its completion window before the owning turn emits the tool result and final response. Those later charged tokens are permanently omitted from the persisted scan cost, and the redundant completion call prompted afterward does not reconcile cost for already-complete scans. Fresh evidence in the exact head is the new if (sdkOwned) return branch, which defers settlement only for SDK scans while leaving native owners finalized mid-turn.
Useful? React with 👍 / 👎.
| const recoverableSelection = begun.run.status === "succeeded" | ||
| && begun.run.finalizationInput && !sdkOwned; | ||
| if (immediate && !completingLocally && !recoverableSelection) return { begun, immediate, sdkOwned }; |
There was a problem hiding this comment.
When a headless Deep coordinator succeeds but its parent remains running and the original tool response is lost, terminal_deep_scan_for_target_snapshot deliberately lets a new thread recover that finished snapshot by calling the start tool with targetPath. This unconditional recoverableSelection now sends that cross-thread result into claimCoordinator, whose ownership check rejects the new thread, so the recovery path fails instead of returning the saved manifest for completion. Keep the immediate terminal return for snapshot reusers that do not own the original scan, while retaining lease-based finalization for actual owners.
Useful? React with 👍 / 👎.
| const saved = JSON.parse(await fs.readFile(path, "utf8")); | ||
| if (saved.version !== 1) { | ||
| throw new Error("This Deep Scan uses an unsupported execution settings version."); | ||
| } | ||
| settings = executionSettings(saved.settings); |
There was a problem hiding this comment.
On coordinator takeover, this trusts execution-settings.json directly for codexPath and codexHome, even though the file lives in the scan output that the scan turn can write and no digest or database-bound copy authenticates it. If model-driven work alters this file before a resume, the recovered coordinator passes the substituted codexPath to the worker launcher and executes it as the current user; a substituted home is also traversed for sessions and credentials. Bind the snapshot bytes or security-sensitive fields to durable trusted state at creation and verify that binding before using them.
AGENTS.md reference: AGENTS.md:L6-L8
Useful? React with 👍 / 👎.
1 participant
Summary
Managed Standard and Deep scans share execution and audit handling. Durable accepted results let replacement workers and publishers recover the same inputs, unresolved coverage, execution settings and result conversation. Native Deep scans also finish publishing when their last observing request disconnects. SDK Deep scans account for the complete owner turn and enforce its budget before publishing.
Changes
Testing
Current source:
ca53a81d673ffd944115bfe3598d4f5efbfee463, tree7b8b098b98e02cc6bb4829b6e0fde97d1990c386. Independent source and QA commits resolve to this exact entire tree.d8c9df4115c39862c9b87978d7221814161e5b5b17a8a6b9f075ed62e02555f8; 515 package members and 142 plugin files verified.loadContractmatches returned artifacts.652298cdreproduces cancellation during finalization, clipped owner usage/budget and competing late completion. The repair passes each process-gated comparison. Parent rejection of corrupted/removed sealed artifacts is retained; the intermediate replay regression is reproduced and fixed.These are Linux checks using Node 24.13, Python 3.12.13 and stock Git 2.43. They do not establish current-head Windows/macOS behavior. Hosted checks for this head remain pending. Preceding hosted verification and its source/package identity remain separate.
Earlier failures remain recorded: an intermediate replay-integrity regression, obsolete prompt assertion, custom Git configuration behavior, missing test extras, inherited umask 0002 and a missing-build-dependency default-order attempt. Final passes use the declared dependencies and preserve existing path and credential protections. No original failure is counted as a pass.
Lifecycle and usage tests use deterministic model substitutes with real workbench, SQLite, accepted artifacts, publication, restart and saved-result access. Full-target natural and matched scans have complete, partial and failed outcomes. Independent quality, cost and latency comparisons remain in progress; whole billing is unavailable. No production equivalence or comparative quality parity is claimed.
Risk and rollout
This PR contains new writers and remains unmerged. Release compatible reader #919 first. It can finish supported selected results, but cannot resume arbitrary v2 discovery/reduction; unsupported state remains intact. Rollback must understand already-written state.
Sparse reader migrations 45–48 followed by writer migration 44 are tested. Open #816 and #829 both allocate 42 to different SQL; their eventual composition still needs reconciliation while preserving published migration numbers. #839's helper port also needs composition against the current entrypoint. No unused migration reservation is added here.
An unowned remote observer retains its aggregate-ready response and can return before the owner's public seal. Completion retains existing claim, continuation and locking authority; no new generation argument is added to complete-scan. POSIX fake-executable stdio tests skip on Windows; hosted platform coverage is reported separately.
The publication fixes preserve distinct repairs and honest partial output; they do not restore findings to earlier measured empty reports or establish comparative quality parity. Checkpoint persistence is separate from terminal artifact acceptance. A file removed after acceptance can still fail publication. Plain log streaming retains existing limits for individual oversized records and aggregate output controls. Credential, unsafe-path, scan-integrity and explicit permission protections remain enforced. No optional scheduler or provenance-format experiment is adopted.
Public disclosure review