Repository navigation
fix(plugin): require evidence before completing scans - #1875
mldangelo-oai wants to merge 67 commits into
Conversation
Codex Review Summary: ✅ CompletedThis 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. |
![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: 914c3815f2
ℹ️ 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
alandelong-oai
left a comment
There was a problem hiding this comment.
Thanks for the change. The completion gate preserves repairable drafts and historical sealed reports in the focused checks, but three integration issues need attention at 914c3815f2fa39e5d324b8ab9df816494751ec15:
- The existing custom-validation finding is confirmed: a schema-valid provisional finding can finish custom validation without the newly required source fields. The current projection leaves those fields absent; its output seals with the base finalizer and fails with this one. The discovery/validation handoff needs to supply the evidence before SDK completion.
- The existing version mismatch remains: the freshly built bundle contains plugin
0.1.96, while the SDK constant reports0.1.95. - The installed-package smoke fixture still creates a new scan from the sparse historical example. Current Linux/macOS package checks and both container builds fail at the new evidence requirement; the inline comment identifies the fixture update needed.
Validation: 170 local finalizer/schema tests passed (2 skipped, plus 152 subtests), along with 15 direct finalizer rejection/repair/representation/stopped-result controls and the required lint/build/source checks. Digest-verified hosted tests from an exact-head-equivalent merge pass all 13 new Standard/Deep completion and stopped-result cases. Local workbench completion and the full custom-validation bridge stop at the existing output-owner guard; the custom-validation comparison used the actual compiled projection plus real finalizers, not a full SDK end-to-end run. This is a COMMENT review pending the integration fixes.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 3f30f71b23
ℹ️ 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
alandelong-oai
left a comment
There was a problem hiding this comment.
Thanks for updating the completed-scan example. At 3f30f71b23979db1a53eb90b6b6763965d622208, that repairs the package-fixture issue; current Linux/macOS installed-package lifecycle and amd64 container checks pass. The sparse sealed-history compatibility check still passes.
Four existing integration findings remain confirmed:
- CSV imports produce findings without the newly required source evidence and have no repair turn.
- Custom validation accepts a sparse provisional finding but does not populate the required source fields before SDK completion.
- Mock scans lack validation evidence and dataflow. Current SDK tests and the installed Action CLI mock check fail on this path.
- The version mismatch remains: the built SDK reports
0.1.95while its bundled plugin is0.1.96.
I also found a separate Deep deadline regression, detailed inline: preparation of an explicitly unsealed checkpoint uses a completed binding, so the new gate rejects valid partial worker evidence before capped finish can commit. The rollback preserves the files.
Validation: 170 local finalizer/schema tests passed (2 skipped, plus 152 subtests), along with lint, build, type, format, source-compatibility checks and direct rejection/repair controls. Actual compiled producers and real base/head finalizers reproduce the CSV, mock and custom-validation differences; real saved-result merging reproduces the deadline preparation difference. Digest-verified hosted tests from the exact-head-equivalent merge pass all 13 new Standard/Deep completion and stopped-result cases. Local workbench integration stops at the existing output-owner guard, so these bounded local probes do not establish full SDK/SQLite execution. Current CI remains red, including the hosted deadline failure. COMMENT pending these integration fixes.
alandelong-oai
left a comment
alandelong-oai
left a comment
There was a problem hiding this comment.
Thanks for the compatibility fixes and clearer completion-repair guidance. This owner-level merge-safety review of 371bf4db5561c2d3a18dd6f0ab1bc9369693c7c7 still finds the existing custom-validation handoff issue unresolved.
At workbench_saved_results.py:2925, the new guard requires manifest.scan.complete is True, but recordCodexSecurityScanDraftViaWorkbench() serializes only complete: false; a complete: true request omits that manifest field. The actual Standard/diff MCP path therefore accepts a sparse completed discovery draft, and independent validation still reaches finalization without source evidence after the repair turn has ended. The new test writes complete: true directly and misses this representation. Match the producer's default-complete semantics (for example, is not False) and test through the semantic MCP handoff. The new completion-tool repair message does not cover this SDK path, which calls Python prepare-scan-completion directly. I am linking the existing finding rather than adding a duplicate inline comment.
At this head, exact-lock installs, builds, types, formatting, Ruff, portable source checks and six compiled diagnostic controls passed. A fresh isolated reproduction using the actual MCP producer, Python draft writer and finalizer confirms the handoff failure; registration/claim/lock are fixtures, so this is not full SDK end-to-end execution. All three affected local MCP suites stopped at the existing output-owner guard before their new assertions. Current hosted Windows/macOS source suites pass; SDK/MCP CI remains pending. COMMENT pending the handoff correction.
|
@codex review Please review current head |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 534d5920d3
ℹ️ 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".
|
@codex review Please review current head |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: aef9ed4ff5
ℹ️ 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
alandelong-oai
left a comment
There was a problem hiding this comment.
Thanks for correcting the default-complete custom-discovery handoff and adding checkpoint-recovery coverage. This owner-level merge-safety review of aef9ed4ff5d3f92e12908e04f988d0fa2de0fc34 confirms the two existing integration findings:
- Ordinary SDK completion accepts sparse terminal Standard/diff drafts, then rejects them after the model turn has ended; the new-scan error path calls
fail-scan. - Deep completion repair rejects an admitted worker's evidence after canonical publication, when public draft writes are already closed.
COMMENT pending those corrections; no duplicate inline comments added. Exact-head builds, types, Ruff, portable checks and changed-test formatting passed. Bounded probes confirmed the producer/admission/finalizer behavior; these are not full model runs. The prior head's hosted Python suites passed with unchanged inputs. Local MCP integration remains blocked by the existing output-owner check, and current full SDK/MCP CI remains pending.
|
@codex review Please review current head Validation: 272 Python completion/publication/checkpoint/export tests; 91 SDK import, custom-validation, and mock-scan tests; source and shipped MCP submission tests; Deep artifact, coordinator retry, and store integration suites; required build, type, formatting, and source-compatibility checks all pass. |
|
Codex Review: Didn't find any major issues. Keep them coming! Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
alandelong-oai
left a comment
alandelong-oai
left a comment
There was a problem hiding this comment.
Thanks for moving evidence checks into terminal Standard/diff submission and the fresh Deep worker repair loop. This owner-level merge-safety review of cc6f84467c1b9b40f62e6690d975436e2ef6ede0 confirms those earlier handoff issues are fixed. New successful scans require complete evidence, while imports, stopped checkpoints and historical sealed reports retain their existing contracts.
One material upgrade/resume issue remains, detailed inline: a previously accepted Deep worker with sparse evidence now fails during coordinator recovery, before the repair loop runs. The coordinator marks the scan failed, preventing another resume. COMMENT pending a repair path for those retained results.
Validation: exact-head builds, types, formatting, Ruff and portable checks passed. The Deep artifact-validation suite, both isolated tracked worker-repair tests, and 35 evidence-contract controls passed. A bounded real-coordinator probe reproduces the resume failure with zero executor calls; its complete-evidence control succeeds. This uses a fixture store/executor, not a live model or actual app upgrade. Digest-verified current CI Python reports pass on an identical merge tree, including all 93 completion-binding and 9 deadline cases in each Ubuntu run. Local integration suites failed, including explicit output-owner rejections and subprocess failures without captured diagnostics; no full local SDK/MCP pass is claimed. Full hosted SDK/MCP checks remain pending at the verification snapshot.
|
@codex review Please review current head The 18 reducer/publication tests and all 15 SDK admission tests now pass, as do types, formatting, and required source checks. The full local MCP run passed the corrected cases; its two unrelated host failures were traced to Git telemetry writing into a fixture (passed with telemetry directed outside it) and the local filesystem rejecting a raw-byte directory name before application code executes. Fresh hosted CI is running. |
|
Codex Review: Didn't find any major issues. 🚀 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". |
alandelong-oai
left a comment
alandelong-oai
left a comment
There was a problem hiding this comment.
Thanks for completing the synthetic evidence in the three test fixtures. This owner-level merge-safety review of aa9d7cbcf3f8a511760d175d6cf08389071880d3 still confirms the existing Deep upgrade/resume issue; production code is unchanged from the previous reviewed head.
A previously accepted discovery that omits validation passes the base contract but fails current coordinator recovery before any discovery or reducer executes. Recovery then persists a failed parent scan, preventing another resume. The complete-evidence control succeeds. Please route those retained results through repair before successful publication while preserving strict checks for newly completed work. COMMENT pending that correction; no duplicate inline comment added.
Validation: 170 local finalizer/schema tests and 152 subtests passed (2 skipped), as did the focused artifact/fixture checks, both isolated worker-retry branches, 35 evidence-contract controls, builds, types, formatting, Ruff and portable checks. The resume reproduction uses the real coordinator with a fixture store/executor, not a live model or actual app upgrade. Digest-verified current CI reports on an identical merge tree pass MCP and Python suites and the available SDK shards, including all 15 admission tests. Local SDK/MCP integration suites failed at output-owner checks, with one coordinator subprocess failure lacking inner diagnostics. Remaining hosted SDK shards were pending at the verification snapshot; no full-suite pass is claimed.
|
@codex review Please review the current head |
|
Codex Review: Didn't find any major issues. Swish! 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". |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 35739515dc
ℹ️ 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".
|
Codex Review: Didn't find any major issues. You're on a roll. 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". |
alandelong-oai
left a comment
alandelong-oai
left a comment
There was a problem hiding this comment.
Thanks for adding the evidence-repair worker launch regression. The new case and comparison control pass through the real executor with a synthetic child, verifying inherited scan settings, environment and the intended read-only policy arguments. Production code is unchanged from the preceding approved head.
Types, portable checks and changed-file formatting pass. The broader runtime-settings group encountered the existing trusted-owner fixture limitation; no full runtime-suite or native-enforcement pass is claimed.
|
@codex review the latest head. The reducer fixture now retains its complete validation and dataflow when specializing the dispatch example. All 39 iteration tests passed locally. |
|
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 correcting the reducer fixture. The updated validation/attack-path case and the neighboring identity/control regression both pass, along with formatting. Production code and dependencies are unchanged from the preceding approved head; its broader verification retains the previously documented limits.
|
@codex review the current head against main. Evidence-repair attempts now use the same host-bound receipts as comparison workers. The regression tests check retry attribution, token accounting and the read denial for the private usage directory. |
|
Codex Review: Didn't find any major issues. Already looking forward to the next diff. 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". |
alandelong-oai
left a comment
alandelong-oai
left a comment
There was a problem hiding this comment.
Thanks for integrating evidence-repair usage into the existing worker receipt path. Repair retries retain separate attempt identities, and saved usage includes evidence workers without double-counting cumulative snapshots. No remaining findings.
Current validation passed three recovery scenarios, 18 executor cases, four saved-usage cases, plugin/SDK builds, types, formatting and portable checks. The previously blocked context-snapshot fixture was excluded; no full-suite, native enforcement or cross-platform pass is claimed.
alandelong-oai
left a comment
alandelong-oai
left a comment
There was a problem hiding this comment.
No blocking findings. Owner-level merge-safety pass on 10434bb: new successful findings require recorded source and validation evidence; historical sealed reports, imports, and stopped checkpoints retain their existing treatment. The merged shared-argument and empty-target changes preserve the evidence-repair ownership and cutoff checks.
Fresh builds, types, formatting, portable checks, focused evidence-repair/resume and worker-launch tests, usage-accounting tests, and current-example validation passed. I did not rerun the full suites affected by the previously observed fixture-ownership limitations or perform live-model/cross-platform execution. Thanks for preserving the original checkpoints through repair.
alandelong-oai
left a comment
alandelong-oai
left a comment
There was a problem hiding this comment.
The current code review found no new material code issue. I am withholding overall approval while the PR’s existing disclosure qualification and explicit completion/recovery rollout hold remain unresolved. Please complete the outstanding disclosure review and confirm that the stated rollout prerequisite is satisfied before merging.
Fresh verification passed 173 finalizer/schema tests with 175 subtests and two skips, three evidence-resume cases, three coordinator cases and 18 worker launch/repair leaf cases, along with portable checks, builds, types and formatting. Broader custom-validation fixtures remain limited by trusted-owner checks, so this does not establish a passing full local integration or cross-platform suite. This COMMENT corrects the overall disposition of my previous approval while preserving its code-review evidence.
|
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". |
1 similar comment
|
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.
The current code review found no new material issue. Overall approval remains withheld while the PR’s existing completion/recovery rollout hold and disclosure qualification remain unresolved. Please confirm the stated rollout prerequisite and complete the outstanding disclosure review before merging.
All 78 feature-file edit sequences match the preceding reviewed contribution; the current base integration preserves evidence-repair ownership, original cutoff, and usage attribution. Fresh verification passed 173 finalizer/schema tests with 175 subtests and two skips, three evidence-resume scenarios, four coordinator controls, and the worker runtime/repair checks (20 reported tests). Ordered portable checks, builds, types, and formatting also passed. Broader guarded integration and cross-platform execution remain unverified.
alandelong-oai
left a comment
alandelong-oai
left a comment
There was a problem hiding this comment.
Thanks for the update. No material issues found in this review. The completion and recovery follow-ups are present, and the evidence gates preserve accepted artifacts while allowing owned repairs.
Validation passed: 793 Python tests, 1,169 MCP tests, 213 SDK tests, the built SDK/plugin compatibility probe, portable checks, builds, types, and formatting. Six tests were skipped; no live-provider or Windows/macOS execution is claimed.
|
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 the evidence-completeness work and the ownership recheck under the completion lock. This owner-level merge-safety review of 418ef5c found one correctness issue: recovered findings can lose their selected source association, allowing an incomplete finding to disappear while its surface remains reported. The inline comment includes a reproducer and the normalization needed before matching.
CI also needs a fixture update: checks current ownership before rejoining saved scan tasks fails on Linux, macOS, and Windows. Its deep-terminal fixture supplies a terminal scan without a deep_scan_runs row, so the new lookup raises “Deep Scan orchestration state not found.” Supplying the missing fixture dependency restores the expected result while retaining the ownership refusals; this does not call for weakening the production check.
Verification: portable source checks, builds, type checking, and formatting passed. Focused Python tests passed (836 tests plus 175 subtests; 4 skips). Across the initial runs and environment-only targeted retries, 1,169 unique MCP and 258 unique SDK cases passed (SDK: 2 skips, 1 todo); the initial runs were not entirely green. Two validation-survival evals and the built SDK/plugin compatibility smoke passed. The normalization regression remains reproducible. I did not run the full SDK suite or a live-provider scan locally.
|
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 fixing the normalized finding association and the ownership fixture; those regressions now pass. I reviewed all 80 changed files at 0eb4656b72f2f92555a1cbdb2efbd941129ef0d0. Two issues still need attention before approval: selected recovery can lose a collision-adjusted surface association and seal it as reported after deferring its incomplete finding, and the current merge exceeds the workbench source-size limit. Both are detailed inline.
Verification passed: 855 Python tests plus 175 subtests (4 skips), 1,169 MCP tests, and 280 SDK tests (2 skips, 1 existing todo), along with Ruff, source checks and 9 source self-tests, builds, types, formatting, two validation-survival evals, and a built SDK/plugin completion check using a synthetic provider. The surface-collision probe reproduces the failure; distinct-ID and corrected-association controls pass.
These were focused suites, not the full repository suite. I did not run a live-provider scan, compile native code afresh, or execute locally on Windows/macOS; the native artifact's matching source/toolchain and payload hashes were verified. The standalone head's source check passes, while the current merge's hosted source-size check fails.
| if surface_id := saved_surfaces.get( | ||
| _digest(_coverage_comparison_row(projected, "surfaces")) | ||
| ): | ||
| surface_ids.add(surface_id) |
There was a problem hiding this comment.
[P2] Match collision-adjusted surface IDs during selected repair
Legacy deadline recovery can retain successive checkpoints that reuse a worker-local surface ID and rename one parent surface to worker/entry-<digest>. This lookup hashes only the unsuffixed projection, so that selected surface loses its association. Extending the sparse selected-deadline regression with an earlier independent no_issue_found observation and a later reported finding, each in its own valid checkpoint using entry, reproduces successful sealing after the incomplete finding is removed, while its renamed surface remains reported and the deferred task has no surfaceIds. The distinct-ID control passes; restoring only the correct association produces needs_follow_up with checkpoint bytes and the selected cutoff unchanged. Recognize the collision-adjusted parent ID here, as the deferred-surface replay path already does, and cover this case through sealing.
| if scan["recipe_json"] is not None: | ||
| recipe = json.loads(scan["recipe_json"], parse_constant=reject_non_finite_json) | ||
| binding["imported"] = isinstance(recipe.get("import"), dict) | ||
| binding["customValidation"] = recipe.get("validationMode") == "custom" |
There was a problem hiding this comment.
[P2] Keep the merged workbench file within the source-size limit
The current plugin source-contract job fails before running the Python tests: workbench_db.py is 150,031 bytes, above the existing 150,000-byte maximum. The standalone head is 149,750 bytes; combining these binding fields with the in-memory connection change on main exceeds the limit. Current main still has the same file content, so rerunning alone will not fix it. Shortening the existing completion-binding docstring, for example, is enough to restore the check without changing behavior or raising the limit.
Summary
A new scan could seal a finding with a location and verdict alone. The CLI and plugin now check each finding for source snippets, a root cause, the input-to-sink dataflow, and evidence for its validation method before marking the scan complete.
Changes
Testing
The five portable plugin checks and the SDK and MCP type checks passed. Focused tests covered custom-validation handoff, evidence repair after a checkout change, and replay of deferred deadline findings. The tests verify that recovery preserves the selected report and emits each deferred reason once.
All 48 distinct SDK tests that failed in the earlier CI run passed locally after updating their fixtures. The installed-package smoke and the Linux amd64 container package stage also passed with the corrected compatibility fixture.
Risk and rollout
Hold this PR until the scan completion and recovery follow-up has been validated and landed on this branch.
Static source review can supply the required evidence. These checks establish that the finding records the evidence behind its conclusion. They do not establish that the conclusion is correct.
Public disclosure review