fix: record findings held back by the reportable severity gate - #155
fix: record findings held back by the reportable severity gate#155mldangelo wants to merge 1 commit into
Conversation
report.md filters findings to REPORTABLE_SEVERITIES, which excludes informational. A scan whose findings are all informational therefore sealed them into findings.json and exported them to SARIF and CSV while the human-facing report stated that no findings survived the discovery, validation and reportability gates. The three artifacts of one scan contradicted each other, and the one a person reads was the one missing data. The exclusion itself is deliberate -- src/cli.ts draws the same line for --fail-on-severity and keeps a separate DISPLAY_SEVERITIES that adds informational -- so the report keeps its reportable-only body. It now records how many findings were held back, with their severity mix, and points at the artifacts that retain them. references/final-report.md described the ordering as critical through low without stating that informational is dropped, so it now says so. Fixes openai#48
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 48ef1cd443
ℹ️ 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".
| "-B", | ||
| join(pluginRoot, "scripts", "validate_report_format.py"), | ||
| "--report-md", | ||
| "/dev/stdin", |
There was a problem hiding this comment.
Replace the POSIX-only stdin path in the Windows test
The test workflow runs the full TypeScript suite on windows-latest (.github/workflows/node-ci.yml, lines 23 and 70–76), where the hosted Python interpreter is discoverable but /dev/stdin does not exist. Consequently, validate_report_format.py fails while opening that path and this new test receives a nonzero status, breaking the Windows CI job. Write the rendered report to a temporary file and pass that native path to --report-md instead.
Useful? React with 👍 / 👎.
Fixes #48.
Summary
report.mdfilters findings toREPORTABLE_SEVERITIES, which excludesinformational. A scan whose findings are all informational seals them intofindings.json, exports them to SARIF asnoteand to CSV, and then renders a report stating:The three artifacts of a single scan contradict each other, and the one a human reads is the one missing data.
Which of the issue's two directions I took, and why
The issue offers both: state the exclusion, or add
informationaltoREPORTABLE_SEVERITIESand give it a section.I concluded the exclusion is deliberate and took the first. The evidence is that "reportable" is a load-bearing concept across the codebase, not a one-off in the projection:
src/cli.tsdefines the same four levels asREPORTABLE_SEVERITIESto gate--fail-on-severity(you cannot fail a build on informational) and then defines a separateDISPLAY_SEVERITIESthat adds informational back for display. The report body follows the reportable set; that split is intentional and I did not want to silently reverse it.So the report keeps its reportable-only body and instead records what was held back:
The line appears in both branches — under
### No findings, and beneath the findings table when reportable findings also exist, since the summary counts are computed from the filtered list and would otherwise under-report there too.The count is derived from
not in REPORTABLE_SEVERITIESrather than hardcodinginformational, and the mix is rendered through the existing_severity_mix, so a future non-reportable level is described accurately instead of silently vanishing again.references/final-report.mddescribed the ordering ascritical→lowwithout ever saying informational is dropped — the documentation gap the issue calls out. It now states it.Testing / QA instructions
Baseline before this branch: 470 pass / 6 skip / 0 fail. After: 475 pass / 6 skip / 0 fail (five added tests, new file
tests-ts/report-projection.test.ts).Confirm the tests reproduce the issue
Revert only the plugin files, keeping the tests:
Reproduce by hand, as the issue does
Use
-B(as the workbench itself does viarunWorkbench) — without it Python leaves__pycache__inside_bundled_plugin/scripts/, which is not gitignored and makespnpm run check:packagefail with "npm tarball contains an unexpected file". The added tests spawn Python with-I -Bfor the same reason.New coverage —
tests-ts/report-projection.test.tsThe repo has no Python test suite, so these drive the real projection through
spawnSync, guarded bytest.skipIfon interpreter availability (matching the existingskipIfconvention). They render the bundledexamples/completed-scanbundle with severities forced, exactly as the issue reproduces it.records findings held back by the reportable severity gate— all informational: report still says### No findings, and now also states1 finding is outside the reportable severity set (informational: 1)and namesfindings.json.records held-back findings alongside reportable ones— 1 high + 2 informational: no### No findings, the high finding renders, and2 findings are outside…(informational: 2)appears.stays silent when every finding is reportable— high + low: the note must be absent, so clean reports are unchanged.keeps the rendered report valid for the format validator— pipes both renders throughvalidate_report_format.pyand asserts exit 0, sincereport.mdis contract-validated and the new lines sit inside the## Findingssection.documents the exclusion in the final-report reference— pins the documentation fix.Packaging check
Because this touches
_bundled_plugin, verify the plugin payload is unchanged in shape:No files were added or removed from the plugin, so
plugin-files.jsonneeds no update (it carries paths, not hashes).