Repository navigation
fix(deep-scan): retain unfinished deadline checkpoints - #1710
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: 1bab58f510
ℹ️ 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".
jameshiester-oai
left a comment
There was a problem hiding this comment.
No blocking findings from this owner-level design/code and merge-safety review of 1bab58f51094af7e2c8123f18f4c977bfc0afdca.
The change addresses a real loss of canceled discovery evidence after another discovery has completed reduction; the regression reproduces on the base and passes here. The deadline path waits for workers to settle, imports canceled discovery artifacts, preserves the reduced parent's findings authority, and reuses existing finalization. The source mapping also preserves surface/task relationships across copied checkpoints, reused IDs, and archived attempts. I checked all nine changed files and traced coordinator publication through store finish, locked SQLite recovery, strict canonical validation, and final completion, including Standard/stopped callers and the unchanged zero-accepted, saturation, and run-limit behavior. I did not identify a substantive simplification or integration defect requiring a change.
Validation:
- 252 focused repository tests and eight independent crash/retry probes passed; source compatibility and nine source-check regression tests also passed.
- Two additional integration scenarios exercised the actual TypeScript publisher, store, Python workbench, and SQLite/filesystem completion: accepted discoveries plus a successful reducer and canceled-worker evidence; and coordinator adoption/resume with archived and current worker attempts. Both preserved reduced findings, recovered checkpoint findings, receipt binding, pending surface links, original source bytes, and byte-stable finish/completion replay.
- Inspected CI run 39074127639: 2,032 Python tests and 393 MCP tests passed. CI used merge checkout
d613b38e9668f132443557b4d35af817631000e3, associated with the reviewed head.
Limits: integration workers were synthetic; no live model/provider execution or MCP transport/server was exercised. I did not rerun the full SDK/MCP builds or suites locally. Local Python dependencies differ from the CI pins, and pinned Ruff validation relies on CI. Concurrent independent finish/complete execution of the mixed recovery case was not separately tested; the relevant locking and ownership code is unchanged.
alandelong-oai
left a comment
There was a problem hiding this comment.
Owner-level merge-safety review. Thanks for the recovery work. I reviewed all nine files and the coordinator/store/finalization paths at 1bab58f51094af7e2c8123f18f4c977bfc0afdca. One material issue remains: the deadline-marker collision already reported here still reproduces at this head, although the thread is marked resolved.
I constructed valid canonical coverage with a resolved task named scan-deadline and an unfinished worker checkpoint. Both the original parent and recovered documents pass the real finalizer. Adding this PR’s deadline marker then fails with coverage.resolvedDeferred[0].id: deferred work is still active. The locked finish path rolls back, and the coordinator treats the otherwise valid completion as failed. Please omit the optional annotation on collision or use a deterministic unused ID that remains stable on replay, with a regression for an existing resolved ID. I am referencing the existing finding rather than posting another inline.
The independent recovery smoke otherwise passed across three replays, preserving reduced finding authority, archived receipt binding, source bytes and task/surface links through report generation. All five portable plugin checks passed; the finalizer suite passed 174 tests plus 105 subtests (2 skipped). Of 252 focused local cases, 99 passed; the integration cases were blocked by scan-parent ownership checks and subsequent fixture errors, also reproduced with an unchanged baseline fixture. Matching hosted CI reports have all 252 affected Python cases and 393 MCP cases passing. No live provider execution was performed.
alandelong-oai
left a comment
There was a problem hiding this comment.
Owner-level merge-safety review: thanks for preserving unfinished discovery evidence while retaining the reduced findings. The existing deadline-marker collision still reproduces on 41160365d6778e43a3cfd482fdb04cdcc4fd9571, despite the resolved thread: valid canonical resolvedDeferred containing scan-deadline passes recovery, then the optional marker makes finalization fail with deferred work is still active. Please omit the marker on collision or choose a stable unused ID. I am referencing the existing inline rather than duplicating it.
Fresh validation passed 303 finalizer/recovery tests plus 185 subtests (2 skipped), a stopped-recovery link case, all five portable checks, and a three-replay recovery smoke through Markdown report generation. Real SQLite deadline completion remains locally blocked by the unchanged scan-parent ownership fixture restriction; no full integration pass is claimed.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 41160365d6
ℹ️ 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".
alandelong-oai
left a comment
There was a problem hiding this comment.
Thanks for the coverage-module cleanup and surface-link fixes. The existing deadline-marker collision still reproduces on ada1ff7097a00999e744ff5e4f16ed1bd19714ca: a valid resolved task named scan-deadline passes recovery, but appending the optional marker makes finalization fail with deferred work is still active. Please omit the marker on collision or choose a stable unused ID. I am referencing the existing inline rather than duplicating it.
Fresh validation passed 132 recovery tests, the stopped-recovery link case, a three-replay recovery smoke through Markdown generation, and all five portable checks. Two deadline integration setups remain blocked by the unchanged scan-parent ownership fixture restriction; no full integration pass is claimed.
alandelong-oai
left a comment
There was a problem hiding this comment.
Reviewed 900c35bd9d34 against its exact updated base. The full feature patch and dependency inputs are unchanged from the independently reviewed ada1ff7097a0; the intervening changes match the separately reviewed main integration. The previously verified issues remain in unchanged code and still need attention. See the existing detailed review for the reproduction and requested corrections.
- Optional deadline marker collides with valid resolved work
No duplicate inline comments added.
Validation: exact feature/dependency comparisons support reusing the prior focused tests and reproductions with their recorded limitations; those suites were not rerun on this rebase. The shared main integration was independently reviewed and its 189 owner/configuration/CLI checks passed. This does not claim a complete current-head integration or platform run.
alandelong-oai
left a comment
There was a problem hiding this comment.
Thanks for preserving unfinished discovery evidence while retaining the reduced findings. I reviewed all 13 changed files at 5ca8c67. The existing deadline-marker collision still reproduces: valid canonical resolvedDeferred containing scan-deadline passes recovery, then the optional marker makes finalization fail with deferred work is still active. Please omit the marker on collision or choose a deterministic unused ID, with a regression. No duplicate inline comment added.
Fresh checks passed all five portable checks, plugin/SDK builds, types/formatting, 337 focused Python tests plus 185 subtests, four MCP checks, and a three-replay recovery smoke through report generation. Additional integration and worker-cancellation checks hit the unchanged trusted-owner fixture restriction; the latter surfaced as a stderr-buffer overflow. Full integration/platform coverage is not claimed; broader CI remains incomplete.
alandelong-oai
left a comment
There was a problem hiding this comment.
Thanks for preserving unfinished discovery evidence while retaining the reduced findings. The existing deadline-marker collision still reproduces at this head, although the thread is marked resolved. A valid resolved task named scan-deadline passes recovery; appending the optional marker then fails finalization with deferred work is still active. Please omit the marker on collision or choose a deterministic unused ID, with a regression. No duplicate inline comment added.
All 13 feature contributions and the main integration were rechecked. Fresh setup/build/type/format checks and the three-replay recovery smoke passed. The prior focused suites remain qualified by unchanged plugin and dependency inputs; they were not rerun. Protected SQLite completion, worker-cancellation and full platform qualification remain limited by the established local ownership restriction.
alandelong-oai
left a comment
There was a problem hiding this comment.
Thanks for fixing the deadline-marker collision. The optional marker is now omitted when pending or resolved work already uses its identity, preserving those tasks and allowing strict finalization to succeed. The new completion regressions cover both collisions and a noncolliding control.
All 13 changed files remain covered by the prior full review plus the complete two-file repair. Current builds, types, formatting and portable checks pass. Fresh merger/finalizer controls pass for all three task cases, and the three-replay recovery smoke preserves findings, source links and checkpoint bytes. Full protected SQLite completion and platform tests remain locally limited by the established ownership guard; those suites were not claimed as rerun.
alandelong-oai
left a comment
There was a problem hiding this comment.
Thanks for the update. The deadline recovery and collision fix remain intact, and the fixture now waits for its native child to exit before returning one-shot configuration results or errors. I found no additional issue.
All build, type, formatting and compatibility checks passed. Six lifecycle controls and four built-preflight checks passed on Node/Bun; 21 stored-auth tests stopped at the existing filesystem ownership guard. Prior recovery checks remain applicable to the unchanged production inputs. Full native/cross-platform execution was not rerun.
alandelong-oai
left a comment
There was a problem hiding this comment.
Thanks for preserving unfinished deadline work while retaining the reduced findings. Owner-level merge-safety review: no actionable findings after reviewing all 14 changed files and their recovery/finalization callers, including the latest helper integration. The marker-collision and source-link fixes remain intact; canonical validation, requested scope and publication rollback are preserved.
Fresh validation passed 351 Python tests plus 185 subtests (2 platform skips), all five portable checks, plugin build, types and formatting, three-replay recovery/report checks and six publication/rollback controls. The latter use a synthetic database adapter with the real recovery/finalizer. Full protected SQLite, process-death, SDK/MCP and cross-platform suites were not rerun locally; prior hosted results are historical, not current-head passes.
Summary
When Deep Scan reached its discovery deadline after a review had already been reduced, findings and pending work saved by another unfinished review were omitted from the completed scan. Retain those checkpoints in the final results while keeping the reduced findings authoritative.
Changes
Testing
Integrated
mainat00b0ac57dc5d5878908b51e31761332f89c1a24eintod5ebdbf5f09d3c22fc2cae781313a68823eaac29. The saved-result helper consolidation from main is retained in the extracted coverage module, including the existing source mapping, worker-qualified finding history, canonical-parent handling, and replay behavior. Independent review found no remaining integration issues and confirmed that the three new shared helpers match main exactly.Fresh validation on this head:
test_deep_scan_deadline_surface_links.py,test_deep_scan_deadline_recovery.py, andtest_workbench_recovery_outcome_validation.pypassed in a serial run.build:ci, source compatibility, and all 9 source-compatibility regression tests.git diff --checkpassed.Historical validation: the preceding output-helper integration at
c1a945f7a5ca53cf68ddf3d1ff2c4d9d28c35185passed 45 critical deadline, crash-replay, and rollback regressions, all five portable checks, and independent review. The earlier integration at5ad582139f6f51c83ceff9a3b7929645d533f934passed 207 deadline/recovery tests and 522 existing coverage, checkpoint, saved-result, and publication-authority tests, plus all five portable checks and two independent reviews. These broader suites were not repeated in full for this refresh. The earlier crash-replay run emitted 18 Python fork deprecation warnings.Tests ran on Linux in private temporary directories with the actual filesystem owner. Earlier sandbox attempts stopped at filesystem-owner and temporary-directory protection checks; the completed native runs passed. No fresh plugin/native build, full SDK or MCP suite, installed-package smoke run, live provider execution, or native cross-platform execution is claimed for this refresh. Fresh hosted CI is required on this head. Earlier validation records remain historical and are not counted as fresh execution here.
Risk and rollout
The primary change affects saved-result recovery at discovery deadlines; the shared surface-link mapping also fixes collisions in existing stopped and zero-accepted recovery. A completed reduction remains authoritative, and unfinished reviews retain their worker provenance and pending coverage. Malformed imported records remain in the original artifacts and produce diagnostics; valid sibling records survive. Worker writeups remain available in their source directories, while recovered findings render inline in the parent report. Source bytes and digests remain unchanged. No public commands, flags, defaults, schemas, or text/path policies change. Existing scan-integrity checks and final sealing remain in place.
Public disclosure review