fix(plugin): calculate diff digests at completion - #1040
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. |
|
@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
A critical issue may replace the recorded digest of sealed artifacts during stopped-scan recovery.
Review effort: Lite
Findings: 1
What changed in this PR
Updates diff-scan finalization to derive snapshot digests during completion and stopped-scan recovery.
Changes:
- Centralizes committed diff digest generation.
- Adds regression coverage for digest binding and recovery.
- Documents draft and sealed-artifact digest behavior.
| File | Reviewed changes |
|---|---|
plugins/codex-security/tests/test_workbench_db.py |
Updates finalized diff digest expectations. |
plugins/codex-security/tests/test_workbench_completion_binding.py |
Adds completion and recovery regression tests. |
plugins/codex-security/scripts/workbench_target.py |
Provides shared digest derivation and binding logic. |
plugins/codex-security/scripts/workbench_saved_results.py |
Reuses digest generation during recovery. |
plugins/codex-security/scripts/workbench_db.py |
Binds digests during completion finalization. |
plugins/codex-security/references/scan-contract.md |
Documents digest ownership and preservation. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
![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: fa2b697ed9
ℹ️ 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".
| manifest_scan = manifest.get("scan") if manifest is not None else None | ||
| # A previously sealed draft may contain a legacy model-authored digest. | ||
| if not isinstance(manifest_scan, dict) or manifest_scan.get("sealedAt") is None: |
There was a problem hiding this comment.
When a legacy commit/range scan has already been recovered with a nonempty preservedSources map and a later checkpoint is explicitly recovered, _recovery_source_digests excludes the sealed parent while merge_saved_results constructs a new manifest. Returning None here means that new manifest falls back to the derived digest, replacing the previously sealed legacy snapshotDigest; I reproduced this by recovering once, adding a checkpoint, and recovering again. Bind the sealed target's recorded digest explicitly so recovery can add retained results without silently changing the target identity.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in e897064. The binding now carries the validated sealed target's recorded digest into later recovery. The regression covers commit and range scans, a first recovery, a late checkpoint, a second recovery, and byte-stable retries; it failed before the fix and now passes.
|
Codex Review: Didn't find any major issues. Keep them coming! 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". |
mldangelo-oai
changed the title
Summary
Commit and range diff scans can fail to finish when the draft omits
snapshotDigest.Changes
Calculate the digest from the scan’s saved base and head revisions during completion and recovery. Keep the original digest for scans that are already sealed.
Testing
Risk and rollout
No CLI or schema changes. Can merge independently of #1036.
Public disclosure review