Repository navigation
fix(imports): distinguish empty reports from failed scanners and exclusions - #2082
mldangelo-oai wants to merge 22 commits into
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. |
![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: 5dc193e771
ℹ️ 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: 7cf18b1012
ℹ️ 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 distinguishing empty imports from scanner failures and excluded evidence, and for fixing the two earlier status-validation gaps. This owner-level merge-safety review found no material issues in the updated head.
The newer zero-run SARIF comment does not establish a defect: SARIF 2.1 section 3.13.4 explicitly permits an empty runs array when the producer has no data; null represents failure to populate it. The current readers reject null, and an empty import does not claim a completed clean scan or close existing findings.
Validation: 250 reader tests and five focused CLI cases passed locally, including failure notifications, negative empty Snyk reports and complete exclusion output. Types, formatting and SDK/plugin builds passed. Current-head CI confirms the publication and local empty-import cases. Local persistence remains limited by environment path/ownership guards; no live vendor or deployed Cloud service was tested.
alandelong-oai
left a comment
alandelong-oai
left a comment
There was a problem hiding this comment.
Thanks, @mldangelo-oai. No material findings in this owner-level merge-safety review of 5b25115.
Clean empty input now avoids unnecessary Cloud discovery and local database creation, while failed scans and exclusions remain distinguishable with complete machine-readable diagnostics. The historical zero-run SARIF concern does not apply: the official SARIF schema explicitly permits an empty runs array. The documented empty status describes the input and does not claim scan coverage or destination validation.
Validation: 22 focused parser/CLI/SDK tests and nine portable checker tests passed locally, along with build, types, generated-artifact and formatting checks. Exact-source CI passed, including all eight installed-package jobs. Full local persistence suites, live scanners and deployed Cloud were not exercised.
alandelong-oai
left a comment
alandelong-oai
left a comment
There was a problem hiding this comment.
Thanks for improving empty-report handling and scanner diagnostics. I reviewed the current head and found one P2 compatibility regression: valid zero-run SARIF is now rejected by all three existing public normalizers. The inline gives the narrow fix; 18 exact-base/head controls confirmed the behavior.
Validation: 274 focused tests, 56 built CLI cases, two built SDK cases, nine portable self-tests, builds, types and formatting passed. The affected suites in current CI also passed; that run uses a newer-main merge and was still in progress when inspected. Full persistent suites were not run locally because of filesystem ownership restrictions. No live vendor or Cloud service was exercised.
| const runs = array(document["runs"], "SARIF runs"); | ||
| if (!runs.length) | ||
| throw new Error("The SARIF report contains no scanner runs."); |
There was a problem hiding this comment.
[P2] Preserve valid zero-run SARIF as an empty selection
{"version":"2.1.0","runs":[]} now throws in the Endor, Snyk and Semgrep public normalizers, whereas all three return an empty selection on the base. SARIF §3.13.4 explicitly uses an empty array for no-data exports and distinguishes it from runs: null when population failed. This breaks existing SDK callers and prevents valid no-data reports from reaching the new CLI empty path. An empty run without any invocation still passes, so the length check does not establish successful scan completion.
Please remove only the nonempty-array requirement, retain null/malformed-array and unsuccessful-invocation/error-notification checks, and cover zero-run acceptance for all three readers.
alandelong-oai
left a comment
alandelong-oai
left a comment
There was a problem hiding this comment.
Thanks for improving empty-report handling and preserving complete exclusion diagnostics. This owner-level merge-safety review still has the existing zero-run SARIF compatibility issue. Fresh built base/head checks confirm that {"version":"2.1.0","runs":[]} changes from an empty selection to an exception in the Endor, Snyk and Semgrep normalizers. SARIF §3.13.4 permits this no-data form. Please remove the nonempty-array requirement while retaining malformed/null-run, failed-invocation and error-notification rejection, and update the tests/docs accordingly. The existing inline remains accurate; no duplicate added.
Validation: exact dependencies, SDK/plugin builds, types/format, ordered portable checks, focused reader/publication tests, 30 base/head cases and 20 built CLI invocations passed with the expected regression reproduced. Clean empty/warning reports avoided state creation; failed/all-excluded reports retained error exits and JSON diagnostics. Local persistence checks remain limited by the environment’s protected-root/ownership guards. Matching-tree CI passes all 96 local adapter tests across Linux/macOS/Windows, but has an unrelated unchanged macOS archive-stream test timeout; no full local suite or live Cloud/vendor qualification is claimed.
alandelong-oai
left a comment
alandelong-oai
left a comment
There was a problem hiding this comment.
Thanks for improving empty-report handling and preserving complete exclusion diagnostics. At 10715af015ff70af92a2e71e5de281af58ca7193, the existing zero-run SARIF compatibility issue remains. All three public normalizers reject {"version":"2.1.0","runs":[]}, which the base accepts as an empty selection. SARIF §3.13.4 permits this no-data form. Remove only the nonempty-array requirement and update its tests/docs, retaining malformed/null-array, failed-invocation and error-notification checks. No duplicate inline added.
All 32 changed files match the preceding review at both base and head; I also checked the incoming dependency changes. Fresh validation passed eight focused publication/reader tests, nine source-checker tests, seven built SDK controls, 16 intended-behavior CLI scenarios, required checks and SDK build/types/format. A 30-observation built base/head matrix and four additional CLI scenarios reproduce the zero-run regression; those are defect probes, not passing product behavior. Current CI passed on the exact base/head merge with an identical full tree.
The full local persistence suite was not rerun because of the documented filesystem restrictions. No live scanner or Cloud service was exercised.
Summary
A genuinely empty scanner report can complete without a prompt, a new workbench database or a Cloud destination lookup. Failed scans and reports containing only excluded rows return a failure the caller can act on.
Changes
status: emptyfor a clean, zero-record report. Local imports validate the checkout before returning. The SDK exposes the selection when it cannot prepare a Cloud upload.Testing
The scanner and publication suites exercise clean reports, failed scans and a batch of 500 excluded rows through the CLI. Local and Cloud cases distinguish zero-result OpenGrep reports with and without scanner errors.
The six scanner and publication test files passed with 604 tests and one skip. Ruff lint and format, SDK build and type checks, MCP app type checking, and both portable plugin compatibility checks passed.
Risk and rollout
CLI clients should recognize the new
emptystatus with exit code 0. It describes the input report. Cloud access is checked when preparing an upload. Previously imported findings stay open. Live vendor tenants and deployed Cloud remain untested.Public disclosure review