v0.4.3

[nyudenkov]

No description provided.

[nyudenkov]

@claude review

[claude]

Code Review: v0.4.3

Scope: Major — 4110 additions / 1889 deletions across 21 files
Change type: Feature + Refactor
Risk level: Medium-High (breaking API change, two silent behavioral defaults changed)

---

Summary

This PR delivers a significant improvement to pysentry's output layer: the monolithic report.rs is split into focused modules (model.rs, human.rs, json.rs, markdown.rs, sarif.rs, styles.rs), the SARIF generator migrates from raw serde_json::json\! macros to the type-safe serde-sarif crate, and color/display control is added via --color and --display flags. The new evaluate_fail_and_filter logic correctly fixes a pre-existing bug where --severity set above --fail-on could prevent vulnerabilities from being evaluated for the exit condition. Overall quality is high, but there are a few issues to address before merge.

---

Strengths

• Zero-copy JSON view: JsonReportView<'a> borrows directly from AuditReport instead of cloning — clean and efficient.
• OutputStyles design: Centralising styles into a struct created once and threaded through renderers is a solid pattern. The colorized() vs default() separation correctly delegates the ANSI gate to owo_colors' runtime check when no override is set.
• SARIF type-safety: Moving from json\!({...}) macros to serde-sarif typed builders eliminates whole classes of structural bugs and catches schema errors at compile time.
• Fingerprinting in SARIF: Using SHA-256 of vuln_id:package:version for partialFingerprints gives GitHub Code Scanning stable alert identities across scans.
• CVE alias tags: Emitting external/cve/cve-YYYY-NNNN tags in SARIF rules follows the CodeQL convention and improves GitHub integration.
• evaluate_fail_and_filter fix: Computing effective_min = min(severity, fail_on) upstream and applying the display filter post-hoc correctly handles the edge case where --severity is stricter than --fail-on. The logic and accompanying tests are clear.
• Test coverage: New tests cover OutputStyles, all four output formats, compact/detailed/normal variants, severity display, JSON field shapes, and the fail/filter logic.

---

Issues

1. Breaking change to JSON output format — Medium

VulnerabilityMatch.vulnerability now has #[serde(flatten)], so every field previously nested under "vulnerability" now appears at the top level of each entry in "vulnerabilities":

// Before: json["vulnerabilities"][0]["vulnerability"]["id"] == "GHSA-..."
// After:  json["vulnerabilities"][0]["id"] == "GHSA-..."

Any script or CI pipeline parsing pysentry's --format json output will break silently. This should be documented in a changelog or migration note.

2. Silent default change: show_progress flipped to false — Medium

fn default_http_show_progress() -> bool {
-    true
+    false
}

Users who relied on the progress bar (the previous default) will no longer see one. Changing a default mid-minor-version without a deprecation path is a jarring UX regression.

3. Silent default change: forbid_quarantined flipped to true — High

fn default_maintenance_forbid_quarantined() -> bool {
-    false
+    true
}

This silently causes CI pipelines to start failing for projects with quarantined transitive dependencies when upgrading from v0.4.2. This is a breaking behavioral change and should either ship behind an opt-in flag or be prominently documented.

4. Fix suggestions dropped from SARIF results — Medium

The old create_sarif_results built a fix_map from fix_suggestions and embedded upgrade advice in each SARIF result. The new version drops the parameter entirely. GitHub Code Scanning can surface fix suggestions when they appear in SARIF results via the fixes field or relatedLocations. Removing this data is a regression for users who relied on upgrade guidance in PR annotations. If the old implementation was incomplete, please open a tracking issue rather than silently removing it.

5. severity_str uses an unnecessary serde round-trip — Low

fn severity_str(severity: Severity) -> String {
    serde_json::to_value(severity)
        .ok()
        .and_then(|v| v.as_str().map(str::to_string))
        .unwrap_or_else(|| "unknown".to_string())
}

Since Severity now implements Display (uppercase) and #[serde(rename_all = "lowercase")] is stable, this is equivalent to severity.to_string().to_lowercase() — which is faster, clearer, and does not depend on serde config. The round-trip also allocates a temporary Value.

6. Duplicate SeverityLevel -> Severity conversion in evaluate_fail_and_filter — Low

The match from SeverityLevel to Severity is written out verbatim twice inside the same function (once for fail_on_db, once for severity_db). A small private helper or From impl would eliminate the duplication and reduce the risk of the two branches diverging.

7. --display is silently ignored outside compact mode — Low

The doc comment says display is "compact mode only" but the CLI accepts it unconditionally without any diagnostic. A user who runs pysentry --display table without --compact will see no effect and receive no feedback. Either restrict the flag with requires = "compact" in clap, or emit a warning when the flag is set but compact mode is inactive.

8. enable_ansi_support called twice — Trivial

Called in both main.rs::run() and python.rs::run_cli(). Harmless but redundant; a shared init helper would be cleaner.

---

Minor Observations

• severity_to_sarif_level (returns &'static str) and severity_to_result_level (returns ResultLevel) perform the same logical mapping for different call sites. Both are used, so no change needed — just noting the parallel for future reviewers.
• The removal of in_dev_dependencies tracking from the pyproject.toml location parser appears correct: that variable tracked Cargo-style [dev-dependencies], which is not valid Python pyproject.toml syntax.
• Severity's new #[serde(alias = "High")] etc. annotations preserve backward compatibility with cached data that may have used PascalCase — good defensive practice.

---

Verdict

The architectural direction (output module split, typed SARIF, color abstraction) is correct and the implementation quality is high. Items that should be resolved before merge:

1. Document the JSON flatten change — existing consumers will break silently.
2. Reconsider the forbid_quarantined default flip — this will surprise CI pipelines upgrading from v0.4.2.
3. Restore or explicitly drop fix suggestions from SARIF — if dropped intentionally, open a tracking issue.

The show_progress default change and the --display-without---compact inconsistency are also worth addressing but are lower priority.