Skip to content

fix(plugin): keep resolved checkpoint deferrals closed - #771

Open
rohanpoudel2 wants to merge 10 commits into
openai:mainfrom
rohanpoudel2:fix/resolved-checkpoint-deferrals
Open

fix(plugin): keep resolved checkpoint deferrals closed#771
rohanpoudel2 wants to merge 10 commits into
openai:mainfrom
rohanpoudel2:fix/resolved-checkpoint-deferrals

Conversation

@rohanpoudel2

@rohanpoudel2 rohanpoudel2 commented Sep 1, 2026

Copy link
Copy Markdown
Contributor

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

  • Resolve a linked deferral only when every referenced surface has an unambiguous terminal outcome.
  • Match explicit surface IDs against historical labels and risk areas. A reused ID on an unrelated surface cannot clear the original deferral.
  • Preserve saved canonical ID reservations across drafts. Drop an inconsistent authored ID from the working copy before reconciliation, keeping the saved ID bound to its original surface through repeated submissions.
  • Recover missing identities from the combined checkpoint context when a canonical write was interrupted. Reuse the existing surface projection and preserve known canonical IDs, including collision suffixes reserved by earlier drafts.
  • Recover missing members of partially identified duplicate groups. Preserve each group's recorded multiplicity without deriving extra IDs from repeated copies of an already-known surface.
  • Retain the canonical ID when an otherwise matching terminal surface supplies a new ID, including across repeated submissions and historical checkpoint copies.
  • Keep unrelated follow-up surfaces when a linked deferral resolves; remove ID-less duplicate copies only after all of their historical IDs resolve.
  • Preserve unresolved and ambiguous work; retain support for resolving duplicate semantic surfaces through distinct explicit IDs.
  • Add regression cases for changed labels and risk areas, repeated submissions, later valid resolution, inherited ID reservations, unrelated follow-up work, and recovery after an interrupted canonical write. Recovery cases also cover attempted ID reuse before legitimate resolution.

Testing

Validation uses Node 24.11.1, pnpm 11.19.0, and Bun 1.3.14.

  • node --test tests/test_artifact_scan_draft.mjs from the MCP app: passed. The new reused-ID cases failed before the fix and pass afterward, including repeated submissions.
  • pnpm run types and pnpm run format from the TypeScript SDK: passed.
  • pnpm run test:mcp: all 23 test files passed.
  • pnpm pack and archive inspection: passed, 415 entries.
  • pnpm run test --seed 12345 on 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.
  • Required portable source checks: Ruff lint and formatting with the repository-pinned Ruff 0.16.1, plus 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

  • No customer, partner, prospect, or user identities, data, or identifying details are included.
  • No credentials, personal data, private source, scan findings, or nonpublic links or tickets are included.
  • I reviewed the branch name, title, description, commits, changes, comments, logs, screenshots, attachments, and links for public disclosure.

@github-actions github-actions Bot added the bug Something isn't working label Sep 1, 2026
@chatgpt-codex-connector

chatgpt-codex-connector Bot commented Sep 1, 2026

Copy link
Copy Markdown

Codex Review Summary

This comment shows the latest Codex review activity on this pull request.

Review Status Commit Review trigger
📝 Code Review Completed 2026-09-05T22:58:17.785022Z 819d4ef Manual request
🔒 Security Review Completed 2026-09-05T23:14:59.290735Z 0a60d3a New commits
ℹ️ 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" or "@codex security review".

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 chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 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".

Comment thread plugins/codex-security/mcp-app/src/artifact-scan-draft.ts

@sylvesterkaczmarek sylvesterkaczmarek left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@rohanpoudel2

rohanpoudel2 commented Sep 5, 2026

Copy link
Copy Markdown
Contributor Author

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 main. The full SDK suite on 819d4ef passed with 2,404 passed, 43 skipped, and 0 failed tests. The final canonical-ID correction in 0a60d3a passed all 23 MCP test files, typechecking, required Ruff/source checks, and the focused regressions. Package build/archive inspection passed before that final correction.

@mldangelo-oai, could you review the updated implementation and authorize the fork's CI workflows so the required platform checks can run?

@codex review

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 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".

Comment thread plugins/codex-security/mcp-app/src/artifact-scan-draft.ts Outdated
@rohanpoudel2

Copy link
Copy Markdown
Contributor Author

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

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 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".

Comment thread plugins/codex-security/mcp-app/src/artifact-scan-draft.ts Outdated
Comment thread plugins/codex-security/mcp-app/src/artifact-scan-draft.ts
@rohanpoudel2

Copy link
Copy Markdown
Contributor Author

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

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 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".

Comment thread plugins/codex-security/mcp-app/src/artifact-scan-draft.ts Outdated
@rohanpoudel2

Copy link
Copy Markdown
Contributor Author

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

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 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".

Comment thread plugins/codex-security/mcp-app/src/artifact-scan-draft.ts

@sylvesterkaczmarek sylvesterkaczmarek left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug Something isn't working

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Codex Security 0.1.22: finalization resurrects resolved checkpoint deferrals and produces inconsistent durable coverage

2 participants