Repository navigation
fix: save scan drafts before exporting results - #1154
Conversation
|
@codex review |
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.
Copilot review overview
🟡 Changes recommended
The new stdin command path can expose internal tracebacks for malformed public input.
Review effort: Balanced
Findings: 1
Open (1)
What changed in this PR
Commits scan drafts before export so recovery and completion consistently use the accepted snapshot.
Changes:
- Adds committed draft snapshots and stdin publication.
- Recovers newer canonical results after interrupted exports.
- Expands completion, legacy, and custom-validation coverage.
| File | Description |
|---|---|
sdk/typescript/tests-ts/custom-validation.test.ts |
Tests committed custom-validation drafts. |
sdk/typescript/src/scan-publication.ts |
Commits drafts before completion. |
sdk/typescript/src/custom-validation.ts |
Publishes validated captured documents. |
sdk/typescript/src/api.ts |
Forwards stdin to the workbench. |
plugins/codex-security/tests/workbench_test_support.py |
Updates checkpoint fixture behavior. |
plugins/codex-security/tests/test_workbench_storage_contracts.py |
Tests checkpoint marker contracts. |
plugins/codex-security/tests/test_workbench_db.py |
Tests committed budget-exhaustion drafts. |
plugins/codex-security/tests/test_workbench_current_draft.py |
Covers interrupted exports and recovery. |
plugins/codex-security/scripts/workbench_saved_results.py |
Implements committed draft storage and merging. |
plugins/codex-security/scripts/workbench_db.py |
Completes scans from committed documents. |
plugins/codex-security/scripts/workbench_cli.py |
Adds stdin draft input. |
plugins/codex-security/scripts/reserved_artifact_paths.json |
Reserves the snapshot path. |
plugins/codex-security/mcp-app/src/artifact-scan-draft.ts |
Reads committed snapshots and digests. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| incoming = json.load(sys.stdin) | ||
| draft = incoming["documents"] | ||
| checkpoint = incoming.get("checkpoint") | ||
| reconciled = incoming.get("reconciledCheckpointIds", []) | ||
| checkpoint_contents = _encoded(checkpoint) if checkpoint is not None else None |
![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: 6bd22a2e09
ℹ️ 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".
| names = dict.fromkeys( | ||
| _children(scan_dir, "checkpoints") + _children(scan_dir, "checkpoints/pending") |
There was a problem hiding this comment.
When a complete committed draft has intentionally removed a finding from an earlier acknowledged checkpoint, this union still enumerates that checkpoint because its immutable file remains after the pending marker is retired. The stopped-scan path does not skip superseded sources, so fail-scan or cancel-scan resurrects the removed finding and can publish it alongside a no_issue_found surface. Only pending or otherwise unreconciled checkpoints should participate in this recovery.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Reproduced at this PR's head 76aa937 and at stack tip 835f0d1 using the workbench fixture: publish a complete draft containing finding F; publish a later complete draft with no findings and the same surface set to no_issue_found; verify there are no pending checkpoint markers; run cancel-scan. The resulting findings.json contains F again, while coverage still marks that surface no_issue_found. The immutable acknowledged checkpoint is being treated as new evidence during stopped recovery.
| isinstance(value, str) | ||
| and value in resolved_surfaces | ||
| and value not in pending_surfaces | ||
| for value in item["surfaceIds"] |
There was a problem hiding this comment.
When a complete committed parent has resolved a surface and a concurrent pending checkpoint adds a plain deferred item for that same surface, pending_surfaces contains only the canonical/current coverage, not the pending checkpoint's coverage. This predicate therefore removes the newly appended deferred item, and completion seals partial coverage with no record of the outstanding review. Pending-source surface IDs need to be retained when deciding whether deferred work is superseded.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Reproduced at 76aa937 and at stack tip 835f0d1. After committing a complete parent draft with a resolved surface, add a valid pending checkpoint that marks the same surface needs_follow_up and adds {id:'synthetic-pending-review', reason:'A new review is still needed.', surfaceIds:[surfaceId]}. prepare-scan-completion produces completeness: 'partial' but deferred: []. The pending review item must survive even though the older parent resolved that surface.
| await writePreparedScanDraft(options.workbench, scanId, { | ||
| manifest, | ||
| findings: findingsDocument, | ||
| coverage, | ||
| }); |
There was a problem hiding this comment.
If the custom-validation turn edits the canonical files and this write-scan-draft call then fails because the workbench or database is unavailable, the function exits without restoring the captured pre-validation documents. This is especially damaging for an SDK-authored scan with no earlier committed snapshot: the outer failure path can recover from the validator's corrupted files and lose the original provisional findings. Apply the same captured-document restoration used for invalid validation output when publication fails.
AGENTS.md reference: sdk/typescript/AGENTS.md:L23-L23
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Confirmed with the existing custom-validation fixture at stack tip 835f0d1: capture a draft with one finding, let the synthetic validation turn overwrite findings.json with {} while returning valid reportable validation output, then have the workbench publication throw Synthetic publication failure. runCustomValidation propagates that error and leaves findings.json equal to {} rather than restoring the captured finding. This exercises the publication path introduced here, separately from the existing invalid-output restoration tests.
| write_scan_local_bytes(scan_dir, "artifacts/scan-draft.json", _encoded(draft)) | ||
| _retire_checkpoints(scan_dir, draft["reconciledCheckpointIds"]) |
There was a problem hiding this comment.
If unlinking a newly acknowledged pending marker fails after artifacts/scan-draft.json has been replaced, this call throws before exporting the canonical files or advancing the phase even though the draft is already committed. The caller therefore observes a failed publication, and an MCP retry using its prior digest conflicts with the accepted snapshot. Because _pending_result_paths already excludes IDs acknowledged by the committed snapshot, this post-commit marker removal should be best-effort rather than changing the publication outcome.
Useful? React with 👍 / 👎.
6bd22a2 to
76aa937
Compare
4766892 to
44e6158
Compare
mldangelo-oai
changed the title
faizan-oai
left a comment
faizan-oai
left a comment
There was a problem hiding this comment.
Holding approval. Targeted probes reproduce three remaining issues: (1) canceling a scan resurrects an acknowledged finding removed by a later complete draft; (2) a pending checkpoint's deferred work for an already-resolved surface is dropped during completion; (3) if custom-validation publication fails after the validator overwrites findings.json, the captured original is not restored. The first two reproduce at this PR's own head and the current stack tip; the third reproduces at the tip in the publication path introduced here. Reproduction details added to the existing threads. The normal recovery/migration suites pass, so these need dedicated regression cases.
alandelong-oai
left a comment
alandelong-oai
left a comment
There was a problem hiding this comment.
Reviewed draft persistence at 76aa9370ef9d84a16b4eb0d35f20d2bd53519889 with three independent passes and root verification. Approval is withheld. In addition to the new SDK snapshot-overwrite finding below, I reproduced these existing issues through the exact-head code:
- Acknowledged findings resurrect during stopped recovery: no pending markers remain, but cancellation restores the removed finding beside a
no_issue_foundsurface. - Pending deferred work is lost: an intentionally conflicting publication leaves a pending review; preparation returns partial coverage with an empty deferred list.
- Custom-validation publication failure leaves corrupted exports: the captured finding is not restored after the validator writes
{}and publication fails. - Post-commit marker cleanup can abort publication: an injected unlink failure leaves the new snapshot committed, exports stale, and the acknowledged marker already excluded from pending work.
Validation: exact plugin build, 142 Python tests, and 192 SDK tests pass; four SDK platform cases are skipped. The failures above are separate disposable probes using synthetic data. I classify the reproduced recovery issues as P2 rather than relying on the earlier automated P1 labels.
| const manifest = await read("scan-manifest.json"); | ||
| if (manifest.scan?.sealedAt == null) { | ||
| await writePreparedScanDraft(workbench, scanId, { | ||
| manifest, | ||
| findings: await read("findings.json"), | ||
| coverage: await read("coverage.json"), | ||
| }); |
There was a problem hiding this comment.
[P2] Preserve the committed snapshot after an interrupted export
The workbench commits artifacts/scan-draft.json before exporting the three root files. If export fails at coverage.json, those files contain new findings but the old coverage and manifest. This SDK path then republishes that mixture and replaces the intact committed snapshot before completion can recover it. I reproduced the full path with the actual workbench: before publishScan, the snapshot had complete: true and the new coverage; afterward it had the stale complete: false and old coverage, and completion failed. Distinguish a newer file-authored result from an incomplete export before replacing the committed draft, and cover SDK completion after partial export.
44e6158 to
a299e98
Compare
76aa937 to
c469e17
Compare
alandelong-oai
left a comment
alandelong-oai
left a comment
There was a problem hiding this comment.
Re-reviewed this restacked head with three independent passes and root verification. The original snapshot/recovery/validation/cleanup reproductions now pass at the integrated stack tip. One own-head P2 remains: the pending-checkpoint writer now stores JSON, while this PR's live draft reader still requires a staged path. The exact-head writer/reader probe reproduces the mismatch after an interrupted immutable copy. The later reader in #1156 handles this payload, but this PR boundary still needs the matching reader change. Approval remains withheld.
| re.fullmatch(r"[0-9a-f]{64}\.json", item) for item in _children(scan_dir, "checkpoints") | ||
| ): | ||
| # Retain bytes before copying history so interrupted publication stays recoverable. | ||
| write_scan_local_bytes(scan_dir, f"checkpoints/pending/{name}", payload) |
There was a problem hiding this comment.
[P2] Update the live checkpoint reader with the new pending format
This writes the checkpoint JSON into the pending marker before copying immutable history, but artifact-scan-draft.ts still treats marker contents as a drafts/<uuid>.checkpoint.json path when the immutable copy is missing. After that copy fails, retrying the live draft API raises invalid staged checkpoint path instead of recovering the retained JSON. I reproduced this with this exact writer and reader; the later #1156 reader accepts the same payload. Ship the reader change at the same boundary so a failed history copy does not prevent live continuation.
c469e17 to
976f84b
Compare
alandelong-oai
left a comment
alandelong-oai
left a comment
There was a problem hiding this comment.
Re-reviewed the real PR diff at 976f84b3e0bdedc4fc664f842580068131df51e5. Three independent high-reasoning passes covered the complete changed patch and tests; root source checks and focused reproductions informed this decision.
The new pending checkpoint contains JSON, but this head's MCP retry reader still expects a staged pathname when immutable history is absent. A transient history-write failure therefore prevents the next draft publication. The Python test corrections do not change that reader. #1156 fixes this own-head mismatch later in the stack.
Validation across touched paths at branch tip 06e75cfe: 1,356 SDK tests, 867 Python tests plus 100 subtests, and 108 MCP tests passed; builds, types, portable checks and installed-package checks passed. These are integration checks at the branch tip, not a claim that every intermediate PR or its CI is clean.
34928b2 to
d2f24f3
Compare
976f84b to
6d44799
Compare
d2f24f3 to
ceaa71a
Compare
6d44799 to
6490f01
Compare
alandelong-oai
left a comment
alandelong-oai
left a comment
There was a problem hiding this comment.
Retained the three independent passes for the identical patch and reconciled inherited touched-file changes at 6490f0140175.
The pending-checkpoint reader now accepts both raw JSON and legacy staged paths. An actual workbench publication recovers a raw pending payload with no immutable history copy. The previous retry-reader mismatch is fixed at this own head.
Current tip 355ec321 passed 75 report-projection tests, portable source checks, SDK build:ci and plugin build. The immediately preceding tip fec36f1d passed 1,360 SDK tests, 873 Python tests plus 100 subtests, 109 MCP tests, builds/types and the clean 600-entry installed-package check. Only report_projection.py and its test changed between those tips; SDK/MCP runtime files are byte-identical. Both descend from main 008a8b4d. These are integration checks, separate from own-head probes. No native Windows local run.
No serious outstanding finding introduced by this PR. Code review approval does not establish CI or merge readiness.
6490f01 to
fbdec7f
Compare
ceaa71a to
84f2889
Compare
alandelong-oai
left a comment
alandelong-oai
left a comment
There was a problem hiding this comment.
Completed three fresh independent full-diff reviews and root verification at fbdec7f60971.
One P2 remains at this own head: committed checkpoint acknowledgments survive a cleanup PermissionError, but the MCP reader still replays the acknowledged pending markers. An accepted later draft removes a finding; the next MCP save restores it only in the cleanup-failure case. The clean control stays at zero findings. The same leftover-marker control stays at zero at the current tip, so the fix exists later in the stack.
Integrated tip 6ef8cbb8, based on 008a8b4d, passed 1,388 SDK tests, 879 Python tests plus 100 subtests, 109 MCP tests, portable checks, builds and the clean 600-entry installed-package check. These are branch-tip integration checks, separate from own-head probes; no local Windows execution was performed.
Approval remains withheld for the findings above.
| try: | ||
| _retire_checkpoints(scan_dir, draft["reconciledCheckpointIds"]) | ||
| except (ContractError, OSError): | ||
| pass # The committed acknowledgment already excludes these pending markers. |
There was a problem hiding this comment.
[P2] Honor committed acknowledgments in the MCP checkpoint reader
Cleanup failure is now nonfatal after saving reconciledCheckpointIds, but readCurrentCheckpoints in artifact-scan-draft.ts still merges every remaining pending marker. With an actual injected PermissionError during retirement, a later accepted draft can remove a finding and the next MCP save restores it from an already acknowledged checkpoint. The no-failure control retains zero findings. Filter committed acknowledgments in the MCP path as well; the current stack tip already avoids the replay, but this own head does not.
84f2889 to
db5196e
Compare
fbdec7f to
c612e66
Compare
alandelong-oai
left a comment
alandelong-oai
left a comment
There was a problem hiding this comment.
Retained three prior independent passes for the verified identical patch and reconciled inherited touched-file changes at c612e66dbc32.
One P2 remains at this own head: committed checkpoint acknowledgments survive a cleanup PermissionError, but the MCP reader still replays the acknowledged pending markers. An accepted later draft removes a finding; the next MCP save restores it only in the cleanup-failure case. The clean control stays at zero findings. The same leftover-marker control stays at zero at the current tip, so the fix exists later in the stack.
Branch tip 3e0083d9, based on 008a8b4d, passed 1,392 SDK tests, 881 Python tests plus 100 subtests, 109 MCP tests, builds, portable source checks and the 600-entry installed-package check. These are tip checks, separate from own-head probes. No local Windows run was performed. The tip excludes the dependency update merged separately in #1184; combined validation remains outstanding.
Approval remains withheld for the outstanding findings.
Consolidate approved Deep Scan changes into the parent topic branch.
…1158) Consolidate Deep Scan changes and their reviewed follow-up fixes into the parent topic branch.
Preserve the child source tree while recording its consolidated parent.
8120b65
into
mdangelo/codex/pr939-split-22-prepared-worker-settings
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 208b0727d3
ℹ️ 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".
| for (const finding of input.findings) | ||
| finding.identity ??= { anchor: randomUUID() }; |
There was a problem hiding this comment.
When successive draft submissions describe the same finding through the same provenance.candidateId but omit the optional identity, this assigns a fresh UUID on every call. preserveDraft then keys the new and committed findings by different identities, appending the committed finding instead of refining it, so an ordinary iterative scan can publish duplicate findings for one candidate. Reuse the matching saved candidate/location identity before synthesizing a new one.
AGENTS.md reference: AGENTS.md:L11-L13
Useful? React with 👍 / 👎.
Part 31 of 46. Previous: #1153 · Next: #1155 · Stack index
Summary
Commit each accepted draft as one snapshot before exporting its manifest, findings, and coverage. Completion and recovery use that snapshot after an interrupted or not-yet-started export, preserving acknowledged edits and pending evidence.
Changes
artifacts/scan-draft.json. Import later file-authored edits when the exported manifest’s completion timestamp matches the committed snapshot, so partial exports cannot replace that snapshot.write-scan-draft --scan-id <id>to read{documents, checkpoint}from stdin when--draft-pathis omitted.checkpointis optional; existing staged-file input remains supported.Testing
Local checks for the implementation and applicable stack boundary:
The complete draft script and MCP typecheck passed at part 31 with both staged-path and JSON payload markers.
Checks against the complete stack passed: SDK and MCP typechecks, formatting, Ruff, the SDK build, portable plugin compatibility and its tests, focused recovery/accounting tests, and local-artifact package smoke using exact cached dependencies. The package check was not a clean registry install. See this PR’s Checks tab for CI results for its current commit.
Risk and rollout
Snapshot acceptance and file export remain separate steps: file export can fail after the snapshot is saved, and retry uses that accepted snapshot. Present malformed manifests still fail validation. Recovery preserves the exact saved evidence, including older staged-file markers during the transition; optional marker cleanup does not determine acceptance.
Public disclosure review