Repository navigation
fix(deep-scan): keep selected results stable during retries - #915
Conversation
![github-code-quality[bot]](https://avatars.githubusercontent.com/u/9919?s=60&v=4){kind=small}
|
@codex review Please review the current head, |
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: Didn't find any major issues. Hooray! 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". |
![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: 85e47f4eaf
ℹ️ 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".
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: d38a5819e4
ℹ️ 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".
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 65af3956b9
ℹ️ 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".
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: cb963ee9a7
ℹ️ 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".
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: c0badd85d4
ℹ️ 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
zcrab-oai
left a comment
zcrab-oai
left a comment
There was a problem hiding this comment.
Reviewed the diff and relevant surrounding code at c0badd8, with an independent second pass. No obvious correctness or regression issues found. Full local platform suites were not rerun. CI still has failed, canceled, or unfinished checks; those results remain separate from this source-review approval.
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: c0badd85d4
ℹ️ 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
mldangelo-oai
added
area:artifacts
|
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". |
mldangelo-oai
removed
area:artifacts
zcrab-oai
left a comment
zcrab-oai
left a comment
There was a problem hiding this comment.
Reviewed the diff and relevant callers at e27346d, with an independent second pass. No actionable findings.
Focused validation: 264 Python tests passed across checkpoint publication authority, checkpoint-head recovery, and saved-source ordering.
Validation limits: Full Windows/native matrix not rerun. Host instrumentation printed shutdown permission warnings after tests; pytest exited 0. Source review only; CI status remains separate.
CI still has failed, canceled, or unfinished checks. This source-review approval does not clear those checks.
|
@codex review |
There was a problem hiding this comment.
🔵 Needs a closer look
Schema-history repair and concurrency-sensitive evidence persistence require the documented rollback and platform acceptance review.
0 open findings
What changed in this PR
Preserves Deep Scan evidence selection across failed publication attempts and explicit recovery.
Changes:
- Persist frozen checkpoint heads through schema migration 48.
- Replay retained evidence, ordering, dispositions, and threat models consistently.
- Add Python and Node migration and publication-retry coverage.
| File | Description |
|---|---|
plugins/codex-security/scripts/workbench_saved_results.py |
Implements checkpoint capture and deterministic replay. |
plugins/codex-security/shared/workbench-migrations.json |
Adds checkpoint-head storage. |
plugins/codex-security/shared/workbench-history-repairs.json |
Repairs migration 47/48 history conflicts. |
plugins/codex-security/tests/test_checkpoint_schema.py |
Tests Python schema upgrades and locking. |
plugins/codex-security/tests/test_checkpoint_publication_authority.py |
Tests publication and recovery authority. |
plugins/codex-security/tests/test_workbench_checkpoint_heads.py |
Covers legacy tied heads. |
plugins/codex-security/tests/test_workbench_completion_binding.py |
Verifies recovered target binding. |
plugins/codex-security/tests/test_workbench_db.py |
Updates migration count. |
plugins/codex-security/tests/test_workbench_saved_source_order.py |
Tests ordering across snapshot failures. |
plugins/codex-security/tests/test_workbench_setup_and_migrations.py |
Registers migration 48. |
plugins/codex-security/mcp-app/tests/test_workbench_database.ts |
Tests Node-side migration compatibility. |
🧠 Review effort: Balanced
💡 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: fe74ff1dbf
ℹ️ 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: 4f9b149477
ℹ️ 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".
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 0b44f1c473
ℹ️ 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".
zcrab-oai
left a comment
zcrab-oai
left a comment
There was a problem hiding this comment.
Reviewed the changes since the previous reviewed head in two passes, using exact source at 0b44f1c473b0 and prior review evidence. Unchanged portions of the large branch were not exhaustively re-audited.
Approved with these considerations still open:
- P2: Explicit recovery can fail while writing auxiliary checkpoint-head/source-order snapshots before persisting its captured selection. If a worker advances before retry, recovery selects different results. Preserve the captured boundary on these failures, as initial preservation already does. plugins/codex-security/scripts/workbench_saved_results.py:664
Validation: CI-helper TypeScript compilation and plugin bundling passed locally; native binaries came from CI with matching compiled source inputs. The full cross-platform suite was not run locally; Windows execution remains unverified here.
alandelong-oai
left a comment
alandelong-oai
left a comment
There was a problem hiding this comment.
Approval withheld for the remaining explicit-recovery persistence gap described in #915 (comment) and #915 (comment).
recover_scan_results still calls _recovery_source_digests before entering preserve_scan_results_locked. A checkpoint-head snapshot failure or source-order write failure escapes before the selected recovery sources are persisted. Retrying after a worker advances can therefore select different evidence. The initial stop path has improved, but the explicit-recovery path still needs the same persistence protection. This is an outstanding gap, not a claim that this revision introduced it.
Static review of the actual diff and current touched files; no fault-injection or runtime suites executed.
alandelong-oai
left a comment
alandelong-oai
left a comment
There was a problem hiding this comment.
Withdrawing the two durability blockers in my earlier review. Both failures occur before an explicit recovery selection has been validated and stored. The exact base already captures source ordering before storing that selection, and this PR explicitly preserves that persistence boundary. Requiring a failed, uncommitted selection to survive retry would strengthen the contract rather than fix an established regression.
Once a validated selection is stored, the publication-failure path retains it for retry; the changed tests cover that behavior separately from initial stop/cancellation capture. No supported P0–P2 source finding remains at 0b44f1c473b05efa284bd777a534d0423d9b5013 after all three review passes.
Static review only; no runtime suites executed. The PR's stated reader-capable rollback and platform-acceptance conditions still apply before merge.
alandelong-oai
left a comment
alandelong-oai
left a comment
There was a problem hiding this comment.
Thanks for the recovery work. Reviewed all 15 changed files at 339048b581ab against 05ff204ee048, including selected-head ordering, legacy recovery, model provenance, publication retry boundaries and schema upgrades. No new actionable issue found. The earlier pre-validation recovery concerns were explicitly withdrawn; the documented boundary is preserved.
Local validation passed 103 merger cases, eight migration cases, SDK/build/type checks and the portable checks. The MCP database suite passed 23 of 24 cases; its directory-mode case passed under umask 022. Full lifecycle and SDK stopped-result runs were blocked during worker setup by this environment's trusted-directory-owner check. Hosted CI passed 2,844 Python cases and 399 MCP cases on a merge tree with all 15 changed files identical to this head; 12 other files differ, so this is qualified integration evidence.
The PR's reader-capable rollback artifact and platform-acceptance prerequisites still need maintainer confirmation before durable writes are enabled.
alandelong-oai
left a comment
alandelong-oai
left a comment
There was a problem hiding this comment.
Thanks for the recovery work. Re-reviewed e6193ca2f7f0 against dd42caa7a22a. All 16 feature patches and files are unchanged from the previous reviewed version; the new owner-inference integration matches the reviewed base. No new actionable issue found.
Fresh plugin build, types, formatting and portable checks pass, alongside seven owner tests and 13 execution-profile migration cases. Prior recovery/database validation is reused only for verified unchanged inputs. The documented rollback and platform-acceptance prerequisites remain maintainer rollout checks.
|
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". |
alandelong-oai
left a comment
alandelong-oai
left a comment
There was a problem hiding this comment.
Thanks for preserving saved-result recovery through the refresh. All 18 feature contributions remain unchanged, and the current owner-report integration preserves selected source bindings and validated retry state. No actionable issue found.
All 11 setup checks and 11 checkpoint-schema cases passed freshly. Earlier snapshot, migration, and platform evidence remains qualified only for unchanged inputs. Current CI is incomplete; the documented rollback and platform-acceptance prerequisites remain maintainer rollout checks.
alandelong-oai
left a comment
alandelong-oai
left a comment
There was a problem hiding this comment.
Thank you for preserving selected results through the recovery integration. I found no material code issue at 9e5df0252f46507309ecc3555caa1bfc4f322c44.
Fresh local validation passed 139 direct Python recovery cases, ten portable merger cases, eleven schema cases, eleven MCP checkpoint cases and nine portable source checks, plus builds, package validation, types and formatting. Four registered fixture cases stop at this host’s trusted-owner guard before their assertions. A verified hosted Python report records 1,448 passes and one platform skip across the changed test modules; their relevant source and fixture inputs match the tested merge. This is not a whole-tree or local platform-suite qualification.
Retain the documented reader-capable rollback artifact and platform-acceptance prerequisites before enabling durable writes, together with the historical disclosure-review condition.
9e5df02 to
4645da2
Compare
Summary
Keep selected findings, coverage, source ordering, and threat models stable when stopped-scan publication is retried or results are explicitly recovered.
Changes
Compare checked scan-file identifiers without numeric precision loss, preserving existing path and ownership checks.
Retain authoritative root and worker checkpoint observations, including legacy selections, timestamp ties, independently registered results, and reopened deferred work.
Bind outcomes and models to their accepted source observations so later worker updates do not replace a pending publication's selected evidence.
Extend explicit recovery to retain its validated selection when checkpoint-head or source-order snapshot writes fail. Retries reuse immutable observations; when a failed snapshot leaves a raw-head fallback, changed raw evidence is rejected until the captured observation is available again.
Preserve main's existing migrations and append checkpoint-selection storage as migration 54. Upgrade Python and Node readers using name-bound repairs for earlier checkpoint migration numbers, including 53.
Keep existing commands, optional live-model fallback behavior, source-size checks, and diagnostic text. Move unchanged source-path helpers into the existing saved-sources module and remove a side effect from a schema-test assertion.
Testing
Merged
mainat5f521759e7c6ff8c3fbd5e81fd1a3567ac801b59into this branch at1d3a04b9355c11681aca4351c06c7999cf928977. The merge is clean, and the branch’s existing contribution is preserved byte for byte relative to the updated base.git diff --checkpassed.These checks ran on Linux with Node 22.23.3 and Bun 1.3.14. Existing dependency installations and generated plugin/native artifacts were reused only after matching dependency bytes, plugin/build inputs, and artifact hashes. Plugin source is unchanged by this refresh, so portable plugin checks were not repeated. No fresh plugin/native build, full SDK suite, installed-package smoke run, or cross-platform execution is claimed. ShellCheck was unavailable. Fresh hosted CI is required for this head.
Earlier validation at
6551afc579e82a60fedb2a13abc9a80b850badedand preceding named revisions remains historical. Prior feature, recovery, migration, worker, portable-plugin, and platform qualifications were not rerun unless listed above; earlier current-head or pending-status wording does not describe this refresh. Existing review, rollout, and disclosure qualifications below remain in force.Risk and rollout
The explicit-recovery change extends the earlier persistence boundary to auxiliary snapshot failures. Prior reviews withdrew those failures as blockers under the old boundary; this change intentionally improves that behavior after source and model selection, rather than claiming a newly introduced regression.
The existing merge hold remains: establish a reader-capable rollback artifact and complete platform acceptance before enabling durable writes. A schema-only rollback is insufficient. Fresh hosted checks and approval for this head remain separate from local validation.
Older interrupted state without a captured model source can still seal without that model; the original unsealed model is not reconstructed. Earlier incomplete binding can retain an older accepted model until explicit recovery selects a newer retained checkpoint. Existing warnings remain, and missing provenance is not inferred from text.
Public disclosure review
The refreshed branch metadata, new commits, changed source and tests, available comments and review history, links, and local validation output were reviewed for public disclosure. Examples and fixtures are synthetic.
The historical qualification remains: fifty earlier artifact archives were unavailable for byte review, and native binary strings inspection was not a full semantic review. That history and future CI outputs are outside this refresh's audit; the broad attestations remain unchecked.