Repository navigation
fix(deep-scan): recover checkpoints at discovery deadlines - #1594
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. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: e034d4029a
ℹ️ 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".
…/codex/remaining-pr-1593-refresh
…/codex/simplify-pr-1593
…/codex/simplify-pr-1594
…/codex/simplify-pr-1594
…/codex/simplify-pr-1594
…/codex/simplify-pr-1594
|
@codex review |
There was a problem hiding this comment.
🔵 Needs a closer look
Cross-transaction filesystem recovery and rollback behavior warrants final human review despite comprehensive tests.
0 open findings
What changed in this PR
Preserves valid Deep Scan worker checkpoints when discovery expires before any worker completes.
Changes:
- Recovers checkpoint findings, coverage, provenance, and threat models.
- Rolls back publication artifacts and timestamps after failures.
- Adds extensive Python and MCP integration regressions.
| File | Description |
|---|---|
plugins/codex-security/scripts/workbench_saved_results.py |
Implements recovery, validation, and rollback. |
plugins/codex-security/scripts/deep_scan_workbench.py |
Invokes recovery during deadline completion. |
plugins/codex-security/references/scan-contract.md |
Documents deadline recovery behavior. |
plugins/codex-security/tests/test_saved_threat_model_recovery.py |
Tests malformed and fallback threat models. |
plugins/codex-security/tests/test_deep_deadline_rollback.py |
Tests rollback and chronology preservation. |
plugins/codex-security/tests/test_deep_deadline_recovery_edges.py |
Covers recovery and failure edge cases. |
plugins/codex-security/mcp-app/tests/test_deep_scan_store_integration.ts |
Adds end-to-end checkpoint recovery coverage. |
🧠 Review effort: Balanced
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
|
Codex Review: Didn't find any major issues. 👍 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". |
alandelong-oai
left a comment
There was a problem hiding this comment.
Thanks for preserving checkpoint evidence at discovery deadlines. Owner-level merge-safety review: no blocking findings. Current-contract validation precedes duplicate selection, requested scope remains enforced, invalid optional threat models no longer discard valid findings, and rollback respects publication ownership and committed success. The five previous findings are addressed.
All five portable checks, MCP types and formatting pass. Fourteen pure rollback/model tests and six independent publication probes pass; the probes exercise real merge/finalizer/write functions with an in-memory database adapter. Local affected tests have 119 passes and 113 failures at existing ownership guards; the real store also stops at its initial write. Downloaded hosted validation passes all 232 affected Python cases and the real store integration file on an identical merge tree.
alandelong-oai
left a comment
There was a problem hiding this comment.
Owner-level merge-safety review: one rollback issue needs attention. I reproduced the existing missing-journal-entry finding through the actual deadline publication path: a worker head-snapshot write fails before its directory exists, merging continues, and a later publication update fails. Cleanup then raises FileNotFoundError and leaves three newly created recovery files behind. The control with only the publication failure restores everything. Treat absent journal paths or parents as already removed and continue cleanup; add a regression covering this sequence.
The intended partial-result recovery, scope filtering, current-contract duplicate selection, and committed-success handling otherwise look sound. Fourteen rollback/model tests, six publication probes, MCP typechecking/formatting, and the plugin build pass locally. Portable checks pass after suppressing only an injected proxy warning in the checker tests. Hosted validation passes all 232 affected Python cases on an identical merge tree. Local CLI/store integration stops at the existing trusted-owner guard; the current hosted MCP job was still running when checked.
alandelong-oai
left a comment
There was a problem hiding this comment.
Thanks for fixing the incomplete rollback. Owner-level merge-safety review: no actionable findings at d1611ca. The missing-journal-path failure now cleans up completely; deadline recovery preserves partial findings and pending review without claiming a completed reduction.
Fourteen focused tests, eight publication/failure probes, model-retry checks, MCP typechecking, and the plugin build pass locally. Portable checks pass after filtering an injected runtime warning. Hosted merge coverage passes all 45 deadline cases on Python 3.10, 3.12, and 3.14 plus the real store integration; production code and every PR-changed file match this head. Local CLI/store runs stop during fixture setup, and full hosted CI was still running when checked.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: e203e81cb8
ℹ️ 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".
alandelong-oai
left a comment
There was a problem hiding this comment.
Thanks for fixing scoped deadline recovery. The existing inventory issue is resolved: recovery now derives scoped inventory from the registered scope before finalization, preserving valid worker findings and partial status. No remaining actionable finding after reviewing all eight changed files and the new regression cases.
Twenty actual recovery/finalizer controls and 14 focused rollback/threat-model tests pass, along with frozen installs, portable checks, builds, types and formatting. The new full finish/complete regression stops locally at the unchanged registration ownership guard; the independent controls use a synthetic database adapter. No full protected lifecycle, cross-platform or complete current CI pass is claimed.

Summary
Preserve valid partial worker results when a discovery deadline settles before any discovery is accepted. Recovery retains usable checkpoint evidence and pending review without claiming a completed reduction.
Changes
Testing
Merged
mainatfe9d77e1c3d9; this refresh changes only the two Dockerfile base-image references to Node 26.12.1. The merge is clean,git diff --checkpasses, and every tracked entry outside Dockerfile is identical to the previously published head. The validation below is historical evidence for its named revisions; runtime tests and image builds were not repeated for this Dockerfile-only refresh. Fresh exact-head hosted CI remains required.Merged
mainat5f521759e7c6ff8c3fbd5e81fd1a3567ac801b59into this branch at01daf16439afdf34ce0b04e64e2d082445e9303f. Those preceding merges were clean; the deadline-recovery contribution is preserved.git diff --check.These checks ran on Linux with Node 22.23.3 and Bun 1.3.14. Initial sandbox attempts stopped at temporary-directory permission and ownership checks; the completed native run used a trusted private temporary directory. Existing dependency installations and 149 generated plugin files, including 14 native artifacts, were reused after source/build/lock/artifact comparisons. Plugin source is unchanged, so portable plugin checks were not repeated. No fresh plugin/native build, full SDK suite, installed-package smoke run, or cross-platform execution is claimed. ShellCheck was unavailable.
The earlier Windows failure prompted a separate probe that demonstrated the fixture’s native-child lifetime gap. The upstream fixture change repairs that ordering issue; it does not establish the cause of the Windows SQLite initialization failure. Fresh hosted CI, including Windows, remains required for this head.
Earlier deadline-recovery qualification is historical, including 61 recovery/rollback tests, the five portable plugin checks, and preceding Python/MCP integration checks. Those records apply to their earlier revisions and were not repeated for this refresh.
Risk and rollout
Recovery applies only at a settled discovery deadline with no accepted discovery, and recovered output remains partial. Existing source validation, coordinator ownership, cancellation, and acceptance checks govern publication. The creation journal is local to the publication attempt; rollback removes only files that attempt created. No public command or configuration surface changes.
Public disclosure review
Reviewed the latest-main merges, their thirteen changed files, synthetic fixture and workflow tests, generic commit and branch metadata, captured local evidence, comments, links, and this description. Existing disclosure attestations retain their prior scope.