Repository navigation
refactor(imports): separate report readers from source providers - #1871
Conversation
alandelong-oai
left a comment
There was a problem hiding this comment.
Thanks for separating reader selection from source identity. No material findings across the seven-file change and its local/Cloud/CLI callers. Exact dependency installs, builds, types, formatting and 18 built-SDK/CLI reader-mapping controls passed. The broader reader run finished with 381 passing and 57 failing cases under existing host directory restrictions; it is not a full integration-suite pass. Current hosted CI is still pending.
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. |
alandelong-oai
left a comment
There was a problem hiding this comment.
Thanks for separating report-reader selection from stored source identity. This owner-level merge-safety rereview found no blocking issues in the complete 21-file diff, including the updated checkpoint recovery integration. Existing reader choices, local/Cloud identities, Wiz retry projections, and Semgrep repository checks remain intact.
Fresh builds, types, formatting, all five portable checks, and 18 built-SDK/CLI reader controls passed. Shared recovery tests and rollback/replay controls were reused after verifying identical source, schemas, tests, native inputs, and Python dependencies. The broader local persistence suite remains limited by filesystem restrictions; previous hosted success is historical, and full current-head hosted qualification is still pending. Merge #1710 first as documented.
Summary
File imports currently use the same provider value to select a report reader and identify stored evidence. Separate those roles so a compatible reader can reuse an existing storage and Cloud provider without changing the normalized import APIs.
Changes
Testing
Fresh checks on
40ef060071a7:git diff --checkpassed.build:ci, source compatibility, and its nine regression tests.This head incorporates the reviewed checkpoint-retention repair from #1710 and current main; #1710 must merge first.
The full seeded package run at
ba8e23e92028completed with 6,912 passed, 83 skipped, 41 failed, and one error across 233 files (seed12345). Its failures were qualified with unchanged-test controls:0002; all six related storage cases passed with the package run's0022umask. No product code, test assertions, or timeouts changed.The first default-order attempt was stopped after 946 passing tests because it used the same configuration-inheriting temporary location. The full default-order package run on
40ef060071a7is now running with the verified clean temporary environment. No complete full-package pass is claimed yet; current hosted CI and the final default-order result remain merge gates.Earlier focused checks at
3713481feea8passed 320 importer/publication tests with one Windows-only skip, plus all 23 local evidence-size/Cloud-limit cases.Risk and rollout
This is a prerequisite refactor for an additional compatible report reader. It changes no wire schema, storage identity, public CLI value, or retry format. No service rollout is required.
Public disclosure review