Repository navigation
fix: keep scan drafts when saving results fails - #1118
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 |
![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: bd3bf330ea
ℹ️ 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".
| "The scan stopped; its saved checkpoint was retained without replacing sealed results." | ||
| ) |
There was a problem hiding this comment.
When fail-scan or cancel-scan acquires the completion lock after the caller has staged these files but before write-scan-draft acquires it, this early status check exits before checkpoint_path is recorded under checkpoints/pending. The terminal publisher therefore cannot discover the staged checkpoint, resultsRecoveryNeeded can remain false, and findings from the last model response are omitted even though the error claims the checkpoint was retained; the stopped-scan path must register and recover that checkpoint rather than leave it as an unreachable draft file.
AGENTS.md reference: AGENTS.md:L42-L43
Useful? React with 👍 / 👎.
| if relative != "parent" and resolved.get(candidate_id) in { | ||
| "rejected", | ||
| "not_applicable", | ||
| }: |
There was a problem hiding this comment.
During composed stopped-scan recovery, independent Standard child scans can reuse a local candidate ID, and project_scan_artifacts() namespaces coverage candidate IDs but leaves finding provenance candidate IDs unchanged. This global lookup therefore lets a rejection from the current parent suppress an unrelated valid finding from another child merely because both used a value such as candidate-1; the finding is moved into rejection history and disappears from the report. Key these decisions by the projected source identity (for example sourceFindingIds/child scan) rather than the unqualified candidate ID.
AGENTS.md reference: AGENTS.md:L42-L43
Useful? React with 👍 / 👎.
| def preserve_stopped_results_after_transition( | ||
| db: Any, connection: Any, scan_id: str, *, stop_children: bool = False |
There was a problem hiding this comment.
For native composed Deep Scans, both fail_scan_locked() and cancel_scan_locked() call this helper without enabling stop_children, and the workbench wrapper does the same, so the newly added stop_composition_children() is never reached. If a parent is failed or canceled while a deep_pass child is still running, that hidden child remains in the running state and can continue modifying or publishing its artifacts after the parent has sealed its recovered result; pass stop_children=True for terminal parent transitions.
AGENTS.md reference: AGENTS.md:L42-L43
Useful? React with 👍 / 👎.
| const draftPath = `drafts/${randomUUID()}.json`; | ||
| const checkpointPath = `drafts/${randomUUID()}.checkpoint.json`; |
There was a problem hiding this comment.
Each publication attempt allocates a new draft/checkpoint pair, while failures intentionally retain that pair and a later successful invocation only removes its own newly allocated paths. Consequently, a transient workbench failure—or every iteration of the automatic scan-draft conflict loop—leaves the earlier .json and .checkpoint.json files permanently under drafts/, even after their checkpoint marker is acknowledged; repeated conflicts can accumulate full copies of scan results without bound. Reuse an attempt's paths across conflict retries or garbage-collect previously retained stages when their checkpoint is successfully reconciled.
Useful? React with 👍 / 👎.
| def _uses_legacy_engine(connection, scan): | ||
| return ( | ||
| scan["mode"] == "deep" | ||
| and connection.execute( | ||
| "SELECT 1 FROM deep_scan_runs WHERE scan_id = ?", (scan["id"],) | ||
| ).fetchone() | ||
| is not None |
There was a problem hiding this comment.
A migrated or hybrid Deep Scan can legitimately have both a deep_scan_runs row and native deep_pass children—the scan-history code and its new tests explicitly combine those sources—but this predicate routes any such scan exclusively through the legacy recovery implementation. On parent failure or cancellation, _legacy_preserve_scan_results_locked() only searches legacy workers and root checkpoints, so even a sealed completed child can be omitted entirely from the parent’s findings and report. Dispatch to composition-aware recovery whenever composition children/checkpoints exist, while incorporating any legacy sources as well.
AGENTS.md reference: AGENTS.md:L42-L43
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Copilot review overview
🔵 Needs a closer look
It substantially changes cross-language publication and recovery ordering, while full runtime suites were not rerun.
Review effort: Balanced
Findings: None
What changed in this PR
Retains staged checkpoints until publication succeeds and improves stopped-scan recovery across the SDK, MCP app, and workbench.
Changes:
- Adds pending-checkpoint indexing, acknowledgment, cleanup, and recovery.
- Preserves completed child findings, coverage, reports, and evidence when parent scans stop.
- Maintains legacy checkpoint recovery and expands cross-platform tests.
| File | Description |
|---|---|
sdk/typescript/tests-ts/scan-draft-publication.test.ts |
Tests shared draft staging behavior. |
sdk/typescript/tests-ts/plugin-finding-detail-contract.test.ts |
Updates recovery helper calls. |
sdk/typescript/src/scan-draft-publication.ts |
Adds shared checkpoint publication logic. |
sdk/typescript/scripts/check-package.mjs |
Verifies the new SDK module. |
plugins/codex-security/tests/workbench_test_support.py |
Creates pending checkpoint markers. |
plugins/codex-security/tests/test_workbench_storage_contracts.py |
Tests publication failure and recovery ordering. |
plugins/codex-security/tests/test_workbench_scan_composition.py |
Tests stopped-parent child preservation. |
plugins/codex-security/tests/test_windows_scan_local_files.py |
Tests Windows missing-file handling. |
plugins/codex-security/tests/test_scan_projection.py |
Tests stopped child projection and coverage merging. |
plugins/codex-security/tests/test_report_projection.py |
Tests retained remediation rendering. |
plugins/codex-security/tests/test_legacy_checkpoint_recovery.py |
Covers pre-index checkpoint recovery. |
plugins/codex-security/scripts/workbench_saved_results.py |
Implements pending, composed, and legacy recovery paths. |
plugins/codex-security/scripts/report_projection.py |
Renders retained source remediation details. |
plugins/codex-security/scripts/finalize_scan_contract.py |
Reuses a preloaded findings schema. |
plugins/codex-security/mcp-app/tests/test_legacy_scan_checkpoint_publication.mjs |
Tests legacy history publication. |
plugins/codex-security/mcp-app/tests/test_artifact_scan_draft.mjs |
Tests marker and staging reconciliation. |
plugins/codex-security/mcp-app/tests/test_artifact_deep_reducer.mjs |
Accounts for pending-index directories. |
plugins/codex-security/mcp-app/src/artifact-scan-draft.ts |
Integrates pending checkpoints with the shared publisher. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
mldangelo-oai
changed the title
alandelong-oai
left a comment
alandelong-oai
left a comment
There was a problem hiding this comment.
Reviewed bd3bf330ea4c9a0ad8a5878c32e84d95eb79d39f against mdangelo/codex/pr939-stack-12-child-artifacts at c485be6a010f5cb7d75577798d7c0d3790e31056, scoped to this PR's diff and touched files.
Reviewed checkpoint retention, publication failures, recovery, and changed tests. No additional distinct bug found. The duplicated legacy publisher is removed later in the stack, so no intermediate refactor is requested. The changed report-projection test file passed 58 tests with the exact report_projection module and local runtime dependencies; this was not the full exact-head integration suite.
Review included simplification/deletion opportunities. Full repository and platform test suites were not run.
bd3bf33 to
e65aed1
Compare
c485be6 to
ca84751
Compare
|
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". |
There was a problem hiding this comment.
💡 Codex Review
codex-security/plugins/codex-security/scripts/workbench_saved_results.py
Lines 753 to 754 in e65aed1
Preserve coverage from pending checkpoints
When an earlier canonical parent draft is marked complete and a later write-scan-draft attempt indexes its checkpoint but fails before replacing the canonical files, that pending checkpoint is newer and unreconciled. During terminal recovery it is nevertheless classified as superseded, and this unconditional branch drops all of its surfaces, exclusions, deferred work, and open questions; the recovered report can therefore omit coverage saved by the final model response even though its pending marker remains valid. Only acknowledged legacy history, not entries from checkpoints/pending, should be skipped as superseded.
AGENTS.md reference: AGENTS.md:L40-L43
ℹ️ 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".
| if re.fullmatch(r"[0-9a-f]{64}\.json", name): | ||
| yield f"{directory}/{name}", kind | ||
|
|
||
| yield from checkpoints("checkpoints") |
There was a problem hiding this comment.
When a legacy-engine Deep Scan encounters an I/O failure after save_pending_checkpoint() writes the pending marker but before it writes checkpoints/<digest>.json, this enumerator never exposes that marker and _legacy_read_saved_result() has no staged-file fallback. Stopping or recovering the scan therefore republishes only the older canonical data and omits the finding even though the validated checkpoint stage was retained; the legacy path needs to consume the pending index just like the composed path.
AGENTS.md reference: AGENTS.md:L40-L43
Useful? React with 👍 / 👎.
| return {"scanId": scan_id, "path": str(scan_dir / output)} | ||
|
|
||
|
|
||
| def stop_composition_children(db: Any, connection: Any, composition: CompositionView) -> None: | ||
| merged = set(composition.checkpoint["mergedScanIds"]) if composition.checkpoint else set() |
There was a problem hiding this comment.
If the saved Deep Scan checkpoint is version 2 but lacks mergedScanIds (or contains a non-iterable value), read_composition_checkpoint() accepts it because it checks only version, and this access raises KeyError/TypeError. fail-scan and cancel-scan have already committed the parent transition when they reach this helper, while preserve_stopped_results_after_transition() does not catch either exception, so the command crashes instead of returning the stopped scan or recording a recovery warning; validate the checkpoint contract or convert malformed fields to a handled ContractError.
AGENTS.md reference: AGENTS.md:L6-L10
Useful? React with 👍 / 👎.
ca84751 to
aefd6e8
Compare
e65aed1 to
1a14c3e
Compare
aefd6e8 to
fa701dd
Compare
1a14c3e to
03f4d6c
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 03f4d6c579
ℹ️ 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".
| merged = set(composition.checkpoint["mergedScanIds"]) if composition.checkpoint else set() | ||
| for child in composition.children: | ||
| if child["id"] not in merged and child["status"] == "running": | ||
| fail_scan( |
There was a problem hiding this comment.
When an unmerged child completes after load_composition() snapshots it as running but before this call acquires the child's completion lock, fail_scan() raises SystemExit because a completed scan cannot be failed. That exception aborts the loop and is caught by preserve_stopped_results_after_transition(), which records a warning but skips publishing the parent's recovered results; for cancellation, recover-scan-results explicitly refuses a later retry. Fresh evidence beyond the earlier stop-children comment is that callers now pass stop_children=True, but the newly reachable helper still turns this normal child-completion race into lost terminal publication. Re-read each child's state or treat an already-completed child as successfully stopped before continuing.
AGENTS.md reference: AGENTS.md:L42-L43
Useful? React with 👍 / 👎.
faizan-oai
left a comment
faizan-oai
left a comment
There was a problem hiding this comment.
Reviewed pending-draft retention, publication ordering, and stopped-scan result recovery. No issue found in this incremental change. Current/pending-draft and composition recovery suites passed at the integrated stack tip; the final recovery implementation includes the later simplification in #1095.
alandelong-oai
left a comment
alandelong-oai
left a comment
There was a problem hiding this comment.
Reviewed the exact diff with three high-reasoning independent passes and root verification. Approval remains withheld. I reproduced the existing child-completion race (#1118 (comment)): completing a child after the composition snapshot but before parent cancellation results in no parent report, and the canceled parent cannot recover later. I also reproduced the separate recovery issue below.
Validation: 93 Python tests, 2 MCP publication tests, and 2 SDK draft-publication tests passed. Both failure scenarios were independently reproduced against this head. The candidate-ID namespacing and missing stop-children invocation comments are fixed in the current code. The pre-index staged-write limitation also exists on the base, so I am not reporting it as an introduced regression.
| if frozen_sources is None: | ||
| save_composed_checkpoint(db, connection, scan, scan_dir, composition) |
There was a problem hiding this comment.
[P2] Retry failed child projection during terminal recovery
A transient failure in _stopped_child_draft is caught by save_composed_checkpoint, which publishes an incomplete aggregate and freezes its sources. On a subsequent recover-scan-results, this condition skips rebuilding the composition, so the child is never retried after the fault clears. I reproduced this with a completed child containing one finding: inject one OSError during parent failure preservation, remove it, then explicitly recover. Both operations return zero findings and resultsRecoveryNeeded=false. Retain enough state to retry failed child projections while preserving already frozen evidence.
fa701dd to
6fbdc35
Compare
03f4d6c to
6a0842d
Compare
alandelong-oai
left a comment
alandelong-oai
left a comment
There was a problem hiding this comment.
Re-reviewed this exact restacked head with three independent passes and root verification. The earlier transient child-read recovery loss and concurrent child-completion stop failures are fixed: explicit recovery now retains the child finding and the stop operation finishes cleanly.
One new P2 is reproduced in the indexed pending-checkpoint path: an incomplete retry can prefer an older final checkpoint by hash filename order and resurrect a finding rejected by a newer final checkpoint. Inline details below. Approval remains withheld for that issue.
Validation: 39 focused Python tests and both focused MCP artifact scripts passed. The root used real recovery commands and the MCP draft API for the reproductions; the same two checkpoint payloads retain zero findings through the existing newest-first legacy reader, but one stale finding through the indexed reader.
| ); | ||
| } | ||
| const checkpoints: Array<{ name: string; input: ScanDraftInput }> = []; | ||
| for (const entry of await fs.readdir(root, { withFileTypes: true })) { |
There was a problem hiding this comment.
[P2] Keep newest-final precedence when reading pending checkpoints
This returns pending checkpoints in filesystem/hash-name order, while preserveScanDraft chooses the first completed source when the incoming draft is incomplete. With no accepted parent draft, retain an earlier final that reports candidate X and a later final that rejects X (for example, two interrupted publications), then retry with complete:false. If the earlier checkpoint's hash sorts first, it becomes the final draft and its finding suppresses the newer rejection. I reproduced one stale finding through this indexed path; the same payloads and timestamps produce zero findings through the existing newest-first legacy reader. Preserve the intended chronological precedence instead of using directory order.
6a0842d to
63f0fba
Compare
6fbdc35 to
418d0f1
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 63f0fba5b3
ℹ️ 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".
| const contents = | ||
| context.layout === "worker" | ||
| ? JSON.stringify(snapshot, null, 2) + "\n" | ||
| : JSON.stringify(snapshot); |
There was a problem hiding this comment.
For non-worker layouts such as reducer and parent checkpoints, an in-progress scan created before this upgrade already has pretty-printed checkpoint files because the previous implementation always used replaceArtifactJson. The filename is nevertheless derived from the compact JSON.stringify(snapshot), so retrying an identical checkpoint after upgrading finds the same path but compares its legacy bytes against the new compact contents and throws existing content does not match its digest. This can block resumed Deep reducers or parent scans despite the stored checkpoint being semantically identical; retain the prior serialization or compare parsed content before rejecting it.
AGENTS.md reference: AGENTS.md:L42-L43
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Re-reviewed the real PR diff at 63f0fba5b38444699a8fe73a61c12067eaf6f478. Three independent high-reasoning passes covered the complete changed patch and tests; root source checks and focused reproductions informed this decision.
Both pending and legacy checkpoint readers now prefer the latest final checkpoint. The exact-head and integrated probes retain the newer rejection and return zero stale findings.
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. Latest workflow runs for this unchanged head were verified successful at 2026-10-02 01:26 UTC. Earlier failed or canceled job records are superseded; this CI status correction does not alter the review decision.
63f0fba to
f170e3a
Compare
|
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". |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: d24d201afb
ℹ️ 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".
| @@ -1452,32 +1415,46 @@ def write_scan_draft(db: Any, connection: Any, args: Any) -> dict[str, Any]: | |||
| filename, | |||
| (json.dumps(document, allow_nan=False, indent=2) + "\n").encode(), | |||
| ) | |||
| for name in acknowledged: | |||
| _remove_scan_local_file_if_exists(scan_dir, f"checkpoints/pending/{name}") | |||
There was a problem hiding this comment.
If the canonical documents have already been written but removing an acknowledged pending marker fails—for example because of a transient filesystem error—this exception escapes, so write-scan-draft reports failure, skips phase advancement, and leaves its staging files even though the result was successfully published. Treat marker removal as best-effort cleanup, as is already done for the staged draft files below, so cleanup failures do not turn a completed publication into an error.
Useful? React with 👍 / 👎.
| if candidate_id is not None: | ||
| provenance["candidateId"] = _project_candidate_id(source_scan_id, candidate_id) |
There was a problem hiding this comment.
When independent child scans reuse an extensions.candidateId, this only writes the namespaced value to provenance and leaves the extension unchanged. The deep report's _deep_candidate_id() still groups by extensions.candidateId first, so two unrelated recovered findings are collapsed into one “Reportable DSS finding” row and count even though their canonical findings remain distinct. Namespace the extension used by presentation or make report grouping prefer the projected provenance identifier.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Retained the three independent passes for the identical patch and reconciled inherited touched-file changes at d24d201afb92.
Two P2 issues remain. A pre-index checkpoint without checkpoints/pending writes the draft documents and then throws ENOENT during acknowledgement; the actual workbench call succeeds at the base and fails here. This confirms existing discussion #discussion_r4162238402 with a concrete legacy layout, so no duplicate inline is added. The following presentation issue is inherited from #1117, rather than introduced by this patch: two independent projected children using extensions.candidateId=candidate-1 retain different child-qualified provenance IDs but render as one Reportable DSS finding. The actual projector and report generator reproduce two canonical findings versus one report group at this head and at tip355ec321; see #discussion_r4162238405. Later #1154 repairs acknowledgement cleanup, but the grouping mismatch still needs a fix.
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.
Approval remains withheld for the findings above.
027b224 to
9a14b2b
Compare
d24d201 to
1722ac3
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 1722ac32345d.
Both outstanding issues are fixed at this own head. A pre-index checkpoint without checkpoints/pending publishes successfully and retains its history. Two independently projected children reusing extensions.candidateId now remain two saved findings and two report groups. The nested-lock candidate was ruled out by actual reentrant lock execution.
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.
No serious outstanding finding introduced by this PR. Code approval is separate from CI and merge readiness.
9a14b2b to
d2dd0d9
Compare
1722ac3 to
d3aefa4
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: d3aefa49ef
ℹ️ 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".
| checkpointPath, | ||
| Buffer.from(JSON.stringify(checkpoint)), | ||
| ); | ||
| await options.workbench([ |
There was a problem hiding this comment.
When write-scan-draft writes findings.json but then hits a transient error writing coverage.json or scan-manifest.json, this call rejects and correctly leaves the stages, but the next invocation cannot consume them: recordCodexSecurityScanDraft() first calls readPreviousScanDraft(), which rejects the incomplete canonical document set before reaching the workbench. Consequently, the newly retained checkpoint cannot repair a running scan after a mid-publication failure; publish the three documents atomically/with rollback, or let retry reconstruction proceed from the pending checkpoint when the canonical set is partial.
Useful? React with 👍 / 👎.
| all_sources = ([("parent", parent)] if parent else []) + sources | ||
| valid_parent = True | ||
| resolved: dict[str, str] = {} |
There was a problem hiding this comment.
When canonical parent documents are unavailable after a failed publication and a newer pending checkpoint rejects a candidate reported by an older checkpoint, resolved is populated only from parent; none of the checkpoint sources contribute their reported/rejected decisions. The stopped-scan merge therefore republishes the obsolete finding alongside the rejection surface, so a rejected vulnerability remains reportable precisely in the recovery path added here. Determine the effective newest checkpoint decisions before merging historical findings, retaining superseded findings only as rejection history.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Retained three prior independent passes for the verified identical patch and reconciled inherited touched-file changes at d3aefa49ef6b.
The prior cleanup and report-grouping fixes remain. New comments about partial draft publication and rejected-checkpoint recovery reproduce at both the exact base and own head, so neither is introduced by this patch. Actual failed-publication/recovery probes pass at the tip, including zero reportable findings after the newer rejection. No new introduced finding remains.
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.
No serious outstanding finding introduced by this PR. Code approval is separate from CI and merge readiness.
Windows package-inspection attempt 1 failed with PluginPythonUnavailableError; its cause remains unproven. Attempt 2 is now running at the same head (04:19 UTC). The previous failure is historical, and this code approval does not establish CI or merge readiness.
4a25e89 to
62be1d6
Compare
d3aefa4 to
17687fa
Compare
alandelong-oai
left a comment
alandelong-oai
left a comment
There was a problem hiding this comment.
Completed exactly three fresh independent HIGH reviews of the full 30-file actual PR diff, including every test/fixture diff, plus independent root verification at 17687faef300 / base 62be1d63d53e.
Approval is withheld for one introduced source-contract failure: the new workbench_result_merge.py does not implement the existing scripts-directory --help behavior. The current Python source-contract CI fails test_all_scripts_support_help; an independent direct control at this exact head also exits 0 with empty stdout. The same helper bytes are inherited by #1119 and #1120; these are one underlying failure, not three separate source defects.
Fresh validation: 35 SDK tests; 589 Python tests with 5 skips; five complete MCP scripts passed (189 outcomes). The sixth, threat-model-document, passed 9 outcomes and failed one unchanged watcher-based test with EMFILE. One isolated rerun of that named test reproduced EMFILE; later asynchronous rename errors are retained separately. This does not establish an introduced projection defect, and that local check remains unresolved. Plugin/SDK builds, types, format, Ruff 0.16.9, compatibility, 9 portable checks and the 603-entry production archive passed. All 852 tracked source bytes/modes match the reviewed head. Native build evidence is retained historical Darwin arm64 evidence after native/dependency and artifact identity checks, not a fresh native build.
The older publication/rejection claims reproduced at the base as well as the prior head and are not new introduced findings here. No models, full installed-package smoke or local Windows run. No whole-stack/current-main success is claimed.
| } | ||
| if not isinstance(parent["findings"], list): | ||
| raise ContractError("Saved parent draft has no findings array") | ||
| return parent |
There was a problem hiding this comment.
[P2] Preserve the existing scripts-directory help contract
This newly added module exits 0 with empty stdout for --help, while the unchanged test_cli_dry_runs.py::test_all_scripts_support_help runs every scripts/*.py file and requires usage output. Current hosted source-contract job 111344166077 fails on this file, and an exact-own-head direct control reproduces it. The same bytes also break the descendant runs. Add the ordinary helper help entry point used by the other modules, or explicitly reconcile this file with the existing contract, before treating this revision as passing CI.
62be1d6 to
b966172
Compare
17687fa to
25d161b
Compare
alandelong-oai
left a comment
alandelong-oai
left a comment
There was a problem hiding this comment.
Reviewed the complete 34-file actual PR diff at 25d161b369ebfacf2b813fb592bf261e34f81eb7 against b966172282677e2628c8899e4a7e543a15475eda, including all 25 test/support paths, with exactly three fresh independent HIGH passes plus root verification. Approval remains withheld for the new test setup defect below.
The earlier workbench_result_merge.py --help defect is fixed: the helper prints usage and exits zero. The existing all-scripts help test also passes with the script-directory import path supplied; its initial local invocation failed on an unchanged publication module's import, and that failure is retained separately.
Fresh local validation: 35 SDK tests passed; 642 Python tests passed with 5 skipped. The complete recovery MCP script has 219 passes and 8 registration setup failures, matching the eight failing cases in hosted CI. A focused real-workbench registration control rejects a 0755 output directory and accepts 0700; these failures occur before the intended recovery assertions. This supports a test setup finding, not a recovery runtime defect. Builds, types, format, Ruff 0.16.9, 9 portable source tests and the 607-member production archive checks passed. Native artifacts were retained only after unchanged source/dependency and artifact identity checks; no fresh native build, clean installation, local Windows or model run.
At the 08:02:48 UTC CI observation, the exact-head workflow remained in progress with the failed MCP job. Source review, completed CI and whole-stack integration remain separate.
| for (const disposition of ["rejected", "not_applicable"]) { | ||
| for (const implicitComplete of [false, true]) { | ||
| test(`conflicted reported checkpoint cannot replace an accepted ${disposition} (implicit complete: ${implicitComplete})`, async (t) => { | ||
| const f = await fixture(t, "standard"); |
There was a problem hiding this comment.
[P2] Make the registered recovery fixtures private
This new block (and the analogous block at line 2616) creates the fixture output directory with the default mode, then passes that existing directory to register-cli-scan. With the normal 022 umask it is 0755; the workbench rejects it with Scan directory must not be accessible to other users (chmod 700). All eight added cases fail during registration before their recovery assertions, both locally and in the hosted MCP job. A bounded control reproduces the rejection at 0755 and succeeds at 0700. Create or chmod these registered fixture directories to 0700 before registering them, while preserving the workbench permission requirement.
f34c154 to
36384d8
Compare
25d161b to
5480e23
Compare
alandelong-oai
left a comment
alandelong-oai
left a comment
There was a problem hiding this comment.
Reviewed head 5480e23d50e7bbc5e8b8503cc30049b386074aaa against base 36384d8ff5dde256f47a791158a09a53391852c6 using three fresh independent HIGH full actual-diff reviews, including all changed tests and fixtures, followed by root synthesis. Original reports and their review times are preserved.
The previous recovery-fixture permission defect is fixed: the shared fixture creates the directory with mode 0700. Eight selected recovery tests and 113 changed Python tests passed. Checkpoint selection/reselection, unchanged child evidence and terminal assessment preservation have no supported introduced finding.
Hosted CI at 2026-10-04T12:53:09.313276+00:00–2026-10-04T12:53:33.556119+00:00 was success with no failed jobs observed for this head. Source approval, CI and successful integration are separate. This prefix contains main 1fb0e5f, but whole-stack successful integration remains unestablished.
No product model, clean installation or local Windows run was performed. Native binaries were retained after source/dependency/artifact identity checks; no fresh native compilation occurred. Individual full-diff reviews qualify selective unchanged long-file context.
Consolidate approved Deep Scan changes into the parent topic branch.
Part 13 of 46. Previous: #1117 · Next: #1119 · Stack index
Summary
Preserve accepted scan drafts and recoverable child evidence when publication is interrupted. Retries keep accepted decisions and original evidence without rerunning discovery; explicit recovery incorporates changed child results.
Changes
Testing
The repeated-coverage selection regression reproduced stale parent coverage after a child changed A to B to A to B. The corrected real-workbench test also checks unchanged repeat recovery and preservation. All 124 cases across four affected recovery suites, SDK types, and all five portable source checks passed.
The accepted-assessment and unreadable-staging corrections passed 27 focused cases, all 126 cases across four related recovery suites, and all five portable source checks. The earlier child-location and retained-evidence changes passed 472 related Python cases. The three MCP draft/recovery files passed all 228 cases under a normal
022umask after the fixture correction; the eight cases that failed in CI also passed in isolation. SDK types, formatting and all five portable source checks passed. Final branch CI is reported in Checks.Risk and rollout
Recovery and interrupted publication are the affected paths. Optional projection failures remain diagnostic warnings when canonical evidence is saved. Frozen retries retain their original input set; explicit recovery selects updated evidence.
Public disclosure review