Repository navigation
fix(plugin): close saved review tasks and simplify recovery - #1036
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. |
![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: 1d4aeb7987
ℹ️ 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".
mldangelo-oai
changed the title
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 7e224f0093
ℹ️ 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 review Please review correctness and opportunities to simplify the changed code. Focus on digest binding and sealed artifacts for #1040, and explicit IDs, worker ownership, interrupted writes, and recovery ordering for #1036. Identify concrete bugs or unnecessary complexity and explain the tradeoffs. |
There was a problem hiding this comment.
Copilot review overview
🟡 Changes recommended
Critical recovery and closure-preservation defects remain unresolved.
Review effort: Lite
Findings: 2
Open (2)
What changed in this PR
This PR adds saved deferred-task closures and improves interrupted scan recovery across SDK, MCP, schemas, and workbench persistence.
Changes:
- Adds
resolvedDeferredIDs and completion reasons. - Preserves checkpoint evidence, task identity, and worker state across retries.
- Expands recovery and finalization regression coverage.
Review findings:
- Critical —
workbench_saved_results.py:254: The new live-head digest breaks recovery of existing persisted heads. - Critical —
workbench_saved_results.py:1272: Valid worker-owned closures are omitted from merged coverage. - Moderate —
artifact-scan-draft.ts:702: Equal-time pending evidence may be closed based on filename ordering. - Moderate —
workbench_saved_results.py:1029: Source-order metadata is incorrectly merged as semantic scan data.
| File | Summary |
|---|---|
sdk/typescript/tests-ts/stopped-scan-results.test.ts |
Tests frozen checkpoint recovery. |
sdk/typescript/tests-ts/runtime.test.ts |
Removes relocated coverage test. |
sdk/typescript/src/models.ts |
Adds resolved deferred typing. |
sdk/typescript/src/custom-validation-prompt.ts |
Updates embedded skill hashes. |
plugins/codex-security/tests/workbench_test_support.py |
Adds recovery fixtures. |
plugins/codex-security/tests/test_workbench_standard_deep_results.py |
Adds recovery regression tests. |
plugins/codex-security/tests/test_workbench_saved_source_order.py |
Tests source ordering and retries. |
plugins/codex-security/tests/test_workbench_db_exports.py |
Tests checkpoint publication recovery. |
plugins/codex-security/tests/test_workbench_checkpoint_heads.py |
Tests checkpoint-head selection. |
plugins/codex-security/tests/test_saved_deferred_unicode.py |
Tests Unicode legacy IDs. |
plugins/codex-security/tests/test_saved_deferred_identity.py |
Tests deferred identity preservation. |
plugins/codex-security/tests/test_resolved_deferred_finalization.py |
Tests finalization behavior. |
plugins/codex-security/skills/security-scan/SKILL.md |
Documents saved-task closure. |
plugins/codex-security/skills/security-diff-scan/SKILL.md |
Documents diff-task closure. |
plugins/codex-security/scripts/workbench_saved_results.py |
Implements checkpoint recovery and merging. |
plugins/codex-security/scripts/finalize_scan_contract.py |
Validates resolved deferred entries. |
plugins/codex-security/schemas/tools/scan-draft.schema.json |
Adds draft closure schema support. |
plugins/codex-security/schemas/coverage.schema.json |
Adds coverage closure schema support. |
plugins/codex-security/references/final-report.md |
Updates reporting guidance. |
plugins/codex-security/mcp-app/tests/test_deep_scan_stdio_lifecycle.mjs |
Tests resumed completion. |
plugins/codex-security/mcp-app/tests/test_compact_artifact_server.mjs |
Tests generated IDs and closure validation. |
plugins/codex-security/mcp-app/tests/test_artifact_scan_draft.mjs |
Tests closure validation and IDs. |
plugins/codex-security/mcp-app/tests/test_artifact_scan_draft_recovery.mjs |
Tests interrupted draft recovery. |
plugins/codex-security/mcp-app/tests/scan-draft-fixture.mjs |
Provides shared draft fixtures. |
plugins/codex-security/mcp-app/src/server/compact-artifact-tools.ts |
Updates tool descriptions. |
plugins/codex-security/mcp-app/src/scan-handoff.ts |
Updates continuation instructions. |
plugins/codex-security/mcp-app/src/artifact-scan-draft.ts |
Implements deferred-ID reconciliation. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: a2288085da
ℹ️ 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 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.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 14c588a80d
ℹ️ 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
alandelong-oai
left a comment
There was a problem hiding this comment.
This change gives saved review tasks stable IDs for updates and explicit closure, and preserves their ordering and recovery state across parent and worker drafts. I completed three independent full-diff reviews and root verification at 8bbc5a860202, including reconciliation of all 22 existing review-thread claims. No actionable finding remains at this head.
Validation at this head: 333 Python tests, 167 SDK tests, 113 MCP tests, Ruff check/format, SDK type/build checks, plugin build and portable source checks passed. Latest CI passed. GitHub currently reports merge conflicts; this code approval does not resolve those conflicts or cover a later rebase.
Preserve accepted review checkpoints and task closures while simplifying scan coordination, artifact handling, and shared regression fixtures. Integrate current main without changing its draft normalization or worker configuration behavior.
mldangelo-oai
changed the title
![github-code-quality[bot]](https://avatars.githubusercontent.com/u/9919?s=60&v=4){kind=small}
alandelong-oai
left a comment
alandelong-oai
left a comment
There was a problem hiding this comment.
Re-reviewed df19342fdeefe1204f4709c2d67d3e1992fe3183 against 14fc2f8534e7877aa1dff74f115eec0ca8d29bd8: all 101 changed files, including the full test diffs, received exactly three independent reviews and root verification. No actionable introduced finding remains.
The compact_timestamp import is used: workbench_db.main exports its globals into the Deep Scan dependency namespace, and begin_deep_scan_for_target calls dependencies().compact_timestamp(). Removing that import would break this path.
Local validation on macOS arm64: 357 Python tests; 862 SDK tests passed and 25 skipped; 156 MCP checks passed across the initial run and controlled reruns. The SDK process-group test passed once sandbox permission allowed its read-only ps assertion. The MCP executor failures reproduced at the exact base and passed at this head with a resolved temporary-directory path and Node isolated from a globally installed Codex. No source or test changes were needed. Ruff check/format, SDK and plugin builds, typechecks, portable source checks, and 9 source-check tests passed. No local Windows run.
Latest CI at this exact head passed. GitHub currently reports conflicts with main; this approval covers the reviewed revision, and any changed patch after conflict resolution needs re-review.
soyeon-oai
left a comment
soyeon-oai
left a comment
There was a problem hiding this comment.
Source review focused on simplification; no tests were run.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 3a6684be96
ℹ️ 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
alandelong-oai
left a comment
There was a problem hiding this comment.
Reviewed all 77 changed files, including the full test diffs, with three independent passes and root verification at 3a6684b against c0f5a5b. No actionable introduced findings.
Validation on macOS arm64: 386 Python tests, 869 SDK tests across the initial run and a controlled rerun (25 skipped), 132 MCP checks, Ruff 0.16.9, builds/types, and portable source checks passed. Built the native module from this revision. The SDK rerun permitted the read-only ps assertion denied by the sandbox; portable-check tests passed after correcting my harness environment. Exact-head CI is complete: 5 workflows successful / 1 skipped, 52 jobs successful / 4 skipped. No local Windows run or model execution.
Checked the two latest bot comments against the exact base. The explicit ID/candidateId collision already drops the generic row at base; a distinct-ID control retains it at both revisions. Terminal-marker retention and successful-return acceptance also exist at base. Actual worker retries with a synthetic executor reject thrown interruption and cancellation at both revisions, while the real parent workbench can explicitly seal partial coverage at both. This head additionally retains new progress evidence. These observations do not establish introduced regressions; they do not claim the baseline behaviors are fixed.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 5300c009a2
ℹ️ 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
alandelong-oai
left a comment
There was a problem hiding this comment.
Reviewed 5300c009a20a5b8ee46fdb22ee62c9b33dfd54bc against c0f5a5b9400fa262107349c1600237a7f766d5e5. All 77 changed files, including full test diffs, received exactly three fresh independent reviews plus root verification. No serious introduced finding established.
Verified the deferred-row identity and terminal-marker fixes through the actual worker and parent workbench paths, including retry, interruption, cancellation, rejected completion, and subsequent successful completion controls.
Rechecked the three new discussions against the exact base and head:
- discussion 4171537602: the finding/terminal-surface identity collision reproduces at the base too; distinct-ID controls retain the generic work.
- discussion 4171537607: current-result candidate loss is pre-existing. A manually constructed selected checkpoint exposes a recovery difference, but the actual writer does not produce that state in the tested interrupted-publication path: the pending candidate remains in the selected checkpoint and stopped recovery. Malformed findings are rejected before changing the head or result; valid findings are recovered. All nine base/prior/current cases replay identically. This does not establish the claimed introduced P1; malformed saved-artifact handling remains a narrower limitation, with no supported import or legacy producer established.
- discussion 4171537611: an actual result-replacement failure retains stale coverage/questions at the base too. This PR additionally retains the newer surface, so the duplicate representation is real. The stale retention is pre-existing, and stopped recovery remains partial in all controls; no new completeness regression was established.
Local validation on macOS arm64: 425 Python, 869 distinct SDK, 159 MCP, and 9 source-check tests passed; 25 SDK tests skipped. The initial SDK run had one sandbox denial for read-only ps; the identical test passed with that permission. Ruff 0.16.9, builds, types, portable checks, and a fresh host native build passed. No local Windows or model execution.
At 03:44 UTC, exact-head CI had 4 successful workflows and 1 running, with 45 successful, 3 skipped, and 4 running latest-attempt jobs. No failures were reported. GitHub still reports MERGEABLE/BLOCKED with three unresolved discussions; this code approval does not establish merge readiness.
alandelong-oai
left a comment
alandelong-oai
left a comment
There was a problem hiding this comment.
Reviewed the full 77-file diff at 6505b815edb1e7913bd5663c6d3863fab19a7ece against c0f5a5b9400fa262107349c1600237a7f766d5e5, including all test changes, with three fresh independent review passes and root verification. No serious introduced finding remains.
Verified recovery through actual worker writes and public workbench commands: a newer selected terminal checkpoint replaces stale ordinary coverage and questions after an interrupted result write; newer-result and successful-write controls remain correct. Missing-finding candidates stay pending, malformed finding input is rejected before saved writes, and valid finding controls recover correctly. All 18 public recovery cases replay consistently. Separate reader controls verify the selected-checkpoint malformed-artifact fix; the hand-authored current-result case remains unchanged from the base.
Local macOS arm64 validation: 443 Python tests, 869 distinct SDK tests plus 25 skipped, 159 MCP tests, and 9 portable-source-check tests passed. The SDK run initially had one sandbox denial for read-only ps; the identical test passed on a controlled permission rerun. Ruff 0.16.9, builds, types, portable checks, and a fresh native build passed. All 77 reviewed source files still match the exact head. No implementation/test-source edits, model execution, or local Windows run.
At 04:18:08 UTC, exact-head CI had all 5 latest workflows successful: 53 latest-attempt jobs successful and 3 skipped, with no pending or failed jobs. GitHub reported MERGEABLE/CLEAN and no unresolved review threads. This approval covers this exact head only.
Summary
Final Standard and diff drafts can close saved review tasks by ID and completion reason. Closed tasks stop keeping coverage partial, while independent and reopened work survives retries, interrupted publication, and stopped-scan recovery.
Changes
coverage.resolvedDeferredand return saved coverage so callers can reuse assigned task and surface IDs.main, retain its reducer prompt and shared test helpers, and simplify scheduler failure cleanup using the existing settlement barrier.Testing
build:cipassed.Risk and rollout
The closure field and saved-coverage response are additive. Candidate evidence requirements, worker scope, target binding, seals, and diagnostic text remain intact. Existing file-authored documents without an accepted head retain full-document replacement behavior; incremental drafts use explicit closures.
Validation uses synthetic fixtures on Linux. Full SDK runs used the merged integration bundle before the final recovery fixes; the final fixes passed the focused Python and MCP suites above. Authentication failures came from synthetic Git markers in the managed checkout ancestry; release failures were subprocess timeouts on the shared host. No live scans were performed. Three fresh native reviews and a separate independent verification passed on the final head, with no actionable findings.
Public disclosure review