fix(plugin): keep resolved checkpoint deferrals closed - #771
fix(plugin): keep resolved checkpoint deferrals closed#771rohanpoudel2 wants to merge 10 commits into
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: bbbaca48e8
ℹ️ 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".
sylvesterkaczmarek
left a comment
There was a problem hiding this comment.
resolvedCoverageSurfaces() trusts any terminal current surface's authored id. Since the live draft schema allows coverage.surfaces[].id, a later draft can reuse a historical deferred surface ID on an unrelated surface and clear that deferral. Match the ID to the historical surface identity, or strip/reject authored IDs, and add a spoofed-ID regression.
|
Addressed the surface-identity review and subsequent recovery feedback in 0a60d3a. An explicit surface ID now resolves a deferral only when its saved label and risk area match unambiguously. Inconsistent authored IDs are reconciled in the working copy, preserving the original canonical identity across repeated submissions. Missing identities, including partially identified groups, are recovered from the combined checkpoint context while retaining existing canonical IDs. Regression coverage exercises changed labels and risk areas, repeated submissions, later legitimate resolution, inherited ID reservations, unrelated follow-up surfaces, and recovery after an interrupted canonical write. The new cases failed before the corresponding fixes and now pass. Existing legitimate-resolution, immutable-history, and explicit duplicate-surface cases remain passing. The branch is updated with @mldangelo-oai, could you review the updated implementation and authorize the fork's CI workflows so the required platform checks can run? @codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: ed240e99b1
ℹ️ 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".
|
Current head b8d2ccc also addresses the review about historical ID reservations. The correction preserves saved canonical identities instead of projecting each raw checkpoint independently. The reported three-draft collision case and the repeated reused-ID cases now pass, including a later legitimate resolution. The PR description reflects the final behavior and validation in progress. @codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: b8d2cccbab
ℹ️ 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".
|
Current head 4fb055d addresses both latest review findings: unrelated follow-up work stays open when one linked deferral resolves, and an interrupted canonical write can recover surface identities from the combined checkpoint context. Regression coverage also preserves the earlier ID-collision and repeated-ID-reuse fixes. Focused artifact-writer and runtime tests, typechecking, formatting, and portable source checks pass; the full SDK/MCP reruns are in progress. @codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 4fb055de98
ℹ️ 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".
|
Current head 819d4ef addresses the partially identified duplicate-group recovery finding. The new regression resolves both IDs on the first terminal submission, with and without an earlier canonical snapshot, and verifies a repeated terminal submission. The earlier interrupted-write, ID-reservation, ID-reuse, and unrelated follow-up cases remain passing. Validation details are recorded in the PR description. @codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 819d4ef675
ℹ️ 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".
sylvesterkaczmarek
left a comment
There was a problem hiding this comment.
The canonical-ID reconciliation now makes terminal semantic resolution stable across repeated submissions, including the unseen-ID alias case. The regression directly covers the re-opened deferral failure. No remaining blocker from me.
Summary
Fixes #741.
Finalizing a scan could restore an interim deferral even after the final draft resolved every referenced surface, leaving otherwise complete coverage marked partial. Reconcile those terminal surface outcomes while retaining the original checkpoint history.
Changes
Testing
Validation uses Node 24.11.1, pnpm 11.19.0, and Bun 1.3.14.
node --test tests/test_artifact_scan_draft.mjsfrom the MCP app: passed. The new reused-ID cases failed before the fix and pass afterward, including repeated submissions.pnpm run typesandpnpm run formatfrom the TypeScript SDK: passed.pnpm run test:mcp: all 23 test files passed.pnpm packand archive inspection: passed, 415 entries.pnpm run test --seed 12345on 819d4ef: 2,404 passed, 43 skipped, 0 failed. The final canonical-ID correction in 0a60d3a passed all 23 MCP test files, the full artifact-writer regression file, typechecking, and portable checks.python .github/scripts/check_plugin_source_compatibility.py: passed.git diff --check: passed.Risk and rollout
The change is limited to checkpoint reconciliation. Saved canonical identities take precedence over inconsistent authored IDs; missing or ambiguous references stay deferred. The existing surface-ID projection is unchanged, and historical checkpoint files are preserved. No CLI, schema, stored-artifact format, dependency, or migration changes are required.
Public disclosure review