Repository navigation
feat(imports): support OpenGrep reports locally and in Cloud - #1872
mldangelo-oai wants to merge 2 commits into
Conversation
alandelong-oai
left a comment
alandelong-oai
left a comment
There was a problem hiding this comment.
Thanks for adding OpenGrep imports. This owner-level merge-safety review found one packaging blocker: both new modules need entries in the distribution manifest before the package and container checks can pass. Details are inline.
The 141 reader tests, 46 base/head Semgrep parity cases, 13 built API/CLI preview controls, builds, types and formatting passed. Packing succeeds, but archive validation fails locally with the same unexpected-file error as CI. Persistence roundtrips remain unverified locally: filesystem trust guards stopped the direct cases, and the local CLI import test exited 2. Cloud controls used a synthetic transport; no live upload was tested.
alandelong-oai
left a comment
alandelong-oai
left a comment
There was a problem hiding this comment.
Thanks for fixing the package manifest and extending the installed-consumer checks. Both new modules now ship with their declarations and maps, and the earlier packaging finding is fixed. I reviewed all 20 changed files and found no remaining material issue.
Fresh validation passed: 141 reader tests, 100 package regressions, 46 base/head Semgrep parity cases, builds, types and formatting; actual archive validation; and installed exports, native loading, CLI previews and strict TypeScript consumers. The hosted package build and four Ubuntu installed-package jobs also passed.
The remaining hosted matrix is still running. I did not rerun the full local SDK or installed smoke suites, or qualify SQLite/Cloud write roundtrips locally; earlier protected-fixture failures remain a local coverage limit. No live scanner, Cloud account or model was used.
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: 12a3bb5da7
ℹ️ 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 updates. The grouped OpenGrep evidence, report diagnostics, source provenance, and Semgrep compatibility paths look sound. Reader tests, base/head compatibility comparisons, type/format checks, portable checks, and the package archive validation passed. Local storage and persisted Cloud roundtrips remain limited by this runner’s filesystem protections; the broader installed-package smoke stopped at an unrelated Git fixture check.
2e579df to
5019bd1
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 5019bd1099
ℹ️ 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".
| id = stableId("opengrep-location", [ | ||
| parsed.rule, | ||
| parsed.path ?? parsed.rawPath, | ||
| parsed.line, | ||
| parsed.column, | ||
| parsed.endLine, | ||
| parsed.endColumn, |
There was a problem hiding this comment.
When OpenGrep emits multiple matches for the same rule and source range but with different metavariable bindings, their fingerprint bases distinguish separate findings; this key ignores the fingerprint entirely and groups every such result into one record. projectGroup then publishes only one source finding and selects one observation for the typed evidence, so local and Cloud imports silently collapse distinct scanner results. Include the stable portion of the fingerprint in the identity while removing only its unstable occurrence counter.
Useful? React with 👍 / 👎.
alandelong-oai
left a comment
alandelong-oai
left a comment
There was a problem hiding this comment.
Thanks for the OpenGrep support. No material issues found. Location-based grouping preserves the raw observations, imports stay unassessed behind the existing opt-in, and checkpoint identity now separates changed diagnostics while preserving report-free compatibility.
Focused import, preview and checkpoint checks, builds, type/format checks and portable source checks passed. Persisted roundtrips and complete installed-package smoke coverage remain limited by filesystem fixtures here; current hosted CI is still running.
Summary
Import native OpenGrep JSON, plain or gzip-compressed, into the local workbench or publish it to Cloud. Imports preserve scanner evidence, including ignored matches and raw severity. They leave findings unassessed.
Changes
codex-security publish findingsaccepts--provider opengrepwith--to localor--to cloudand requires--source-repository-url. SetCODEX_SECURITY_ENABLE_EXTERNAL_IMPORTS=1in the command environment to enable external imports. Local imports check that URL against configured Git remotes even when no findings are ready. SDK callers can usenormalizeOpenGrepFindingsandreadOpenGrepFindings.semgrepprovider and recordOpenGrepas the scanner origin. The importer groups traces by rule, path and source range, so a change in scanner fingerprints or result order does not create a second record.Errorvalues inside scanner diagnostics. If only a scanner diagnostic changes, Cloud treats the report as a different publication. Retrying the original report resumes its saved upload without sending the findings twice.Testing
mainat796fa130: OpenGrep 1.32.0 scanned two synthetic Python taint flows, one in a subdirectory and one in a filename with a literal backslash. Both import targets preserved the distinct occurrence IDs, paths and raw traces across JSON, gzip and reversed result order. Local SQLite import created two unassessed records; replay left both unchanged. Local import rejected a mismatched source repository for both an empty report and one with all results excluded.2e579df0tested withmainat34c97c82. The regression cases cover gzip input, SQLite import and replay, trace exclusions at the Cloud boundary, and a readback retry that preserves diagnostics without uploading the same finding twice.Risk and rollout
The reader-routing prerequisite has merged; this PR targets
main. Cloud deployments must already support the Semgrep import contract. Moving a match creates a new occurrence; omitting it from a later report does not close the old one. A completed upload does not establish scan coverage or an assessed severity.Public disclosure review
One attestation remains open for existing CI output.