feat(tui): compliance panel pq_mode gate + export menu (refs #248)#259
feat(tui): compliance panel pq_mode gate + export menu (refs #248)#259peaktwilight merged 3 commits intomainfrom
Conversation
…248) - compliance panel now requires pq_mode, hidden in Scan/Diff/Secrets - new 'e' key opens CBOM/JSON/SARIF export picker - make build_cbom/build_sarif public for TUI reuse
|
Caution Review failedPull request was closed or merged during review 📝 WalkthroughWalkthroughRefactors SARIF and CBOM generation into public builder functions and adds a TUI export modal to write CBOM/JSON/SARIF files; export is blocked when no results, emits success/failure notices, and CBOM export warns on empty cryptographic findings. Also gates CNSA 2.0 compliance panel on PQ-mode and adds unit tests. Changes
Sequence Diagram(s)sequenceDiagram
participant User as "User"
participant TUI as "TuiApp"
participant Builder as "Report Builder(s)"
participant FS as "FileSystem"
participant Notice as "RuntimeNotice"
User->>TUI: press "e"
TUI->>TUI: show Export Modal (CBOM / JSON / SARIF)
User->>TUI: select format + confirm
TUI->>Builder: request payload (build_cbom / build_sarif / serialize)
Builder-->>TUI: JSON payload (or (payload, empty_flag))
TUI->>FS: write pretty JSON to deterministic file path
FS-->>TUI: success / error
alt success
TUI->>Notice: emit success notice with path/count
alt CBOM empty_flag == true
TUI->>Notice: emit "CBOM empty (no crypto findings)" warning
end
else failure
TUI->>Notice: emit "export failed: ..." notice
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
Greptile SummaryThis PR gates the CNSA 2.0 compliance panel ( Key changes:
Issues found:
Confidence Score: 3/5Safe to merge after fixing the thread-unsafe set_current_dir test; the runtime feature itself is sound but the test can cause flaky CI failures in parallel runs. The production logic (pq_mode gate and export flow) is correct and well-tested conceptually. However the export_writes_cbom_file test mutates process-global state (set_current_dir) without coordination, which is a real reliability risk in the default parallel test runner — not merely theoretical. The two P2 items (popup sizing and silent overwrite) are non-blocking but worth addressing before shipping. src/tui.rs — specifically the export_writes_cbom_file test and draw_export_menu popup sizing Important Files Changed
Sequence DiagramsequenceDiagram
participant User
participant TuiApp
participant ExportMenu
participant report_cbom as report::cbom
participant report_sarif as report::sarif
participant FS as std::fs
User->>TuiApp: press 'e'
TuiApp->>TuiApp: open_export_menu()
alt result is None
TuiApp-->>User: push_runtime_notice("no results to export")
else result exists
TuiApp->>ExportMenu: create (Cbom | Json | Sarif, selected=0)
TuiApp-->>User: draw_export_menu()
end
User->>TuiApp: navigate with j/k or arrow keys
TuiApp->>ExportMenu: update selected index
User->>TuiApp: press Enter
TuiApp->>TuiApp: export_findings(format)
alt ExportFormat::Cbom
TuiApp->>report_cbom: build_cbom(findings)
report_cbom-->>TuiApp: (Value, has_pqc_risk)
else ExportFormat::Sarif
TuiApp->>report_sarif: build_sarif(findings)
report_sarif-->>TuiApp: Value
else ExportFormat::Json
TuiApp->>TuiApp: serde_json::to_string_pretty(findings)
end
TuiApp->>FS: write(filename, content)
FS-->>TuiApp: Ok(()) or Err
TuiApp-->>User: push_runtime_notice("exported N findings to …")
User->>TuiApp: press Shift+N
TuiApp->>TuiApp: toggle show_compliance_panel
Note over TuiApp: draw gate: show_compliance_panel && result.is_some() && request.pq_mode
Reviews (1): Last reviewed commit: "feat(tui): gate compliance panel to pqc ..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| std::env::set_current_dir(dir.path()).expect("set_current_dir"); | ||
| app.export_findings(ExportFormat::Cbom); | ||
| std::env::set_current_dir(&original).expect("restore cwd"); |
There was a problem hiding this comment.
set_current_dir in parallel test
std::env::set_current_dir is a process-wide operation, and Rust runs tests within the same module in parallel by default. Between set_current_dir(dir.path()) and set_current_dir(&original) on line 4416, any other concurrently-running test that opens a relative path (e.g., the path: ".".to_string() in tui_app_with_findings, or apply_action_lower_severity_writes_override_and_replaces and apply_action_disable_rule_globally_appends_and_detects_duplicate) will observe the temp directory as cwd — potentially causing spurious failures or incorrect file writes.
The correct fix is to refactor export_findings to accept an explicit output path, or test at a lower level by calling std::fs::write(dir.path().join("findings.cbom.json"), ...) directly — avoiding mutation of shared global state entirely.
| let area = centered_rect(40, 20, frame.area()); | ||
| let items = menu | ||
| .formats | ||
| .iter() | ||
| .map(|fmt| ListItem::new(Line::from(Span::styled(fmt.label(), Style::default())))) | ||
| .collect::<Vec<_>>(); | ||
| let list = List::new(items) | ||
| .block(panel_block(None, PANEL_BG)) | ||
| .highlight_style( | ||
| Style::default() | ||
| .fg(Color::White) | ||
| .bg(DETAIL_BG) | ||
| .add_modifier(Modifier::BOLD), | ||
| ) | ||
| .highlight_symbol("> "); | ||
| let inner = area.inner(Margin { | ||
| vertical: 1, | ||
| horizontal: 1, | ||
| }); | ||
| let layout = Layout::default() | ||
| .direction(Direction::Vertical) | ||
| .constraints([ | ||
| Constraint::Length(1), | ||
| Constraint::Length(menu.formats.len() as u16 + 2), | ||
| Constraint::Length(1), | ||
| ]) | ||
| .split(inner); |
There was a problem hiding this comment.
centered_rect(40, 20, frame.area()) allocates only 20% of the terminal height for the popup. The inner area (after Margin { vertical: 1, horizontal: 1 }) is area_height − 2. The layout requires 1 + (formats.len() as u16 + 2) + 1 = 7 rows (1 header + 5 for the bordered list + 1 footer).
The popup must be at least 9 rows tall (7 inner + 2 for the outer border), so the terminal must be at least ⌈9 / 0.20⌉ = 45 rows. A typical 80×24 terminal gives only ~4 rows — ratatui will silently clip the list and the footer hint ("Enter export Esc cancel") will never render.
Consider raising the percentage (e.g. centered_rect(40, 40, …)) similar to how draw_severity_picker uses centered_rect(44, 34, …).
| fn export_findings(&mut self, format: ExportFormat) { | ||
| let findings = match self.result.as_ref() { | ||
| Some(r) => &r.findings, | ||
| None => return, | ||
| }; | ||
|
|
||
| let filename = format.filename(); | ||
| let content = match format { | ||
| ExportFormat::Cbom => { | ||
| let (cbom, _) = crate::report::cbom::build_cbom(findings); | ||
| serde_json::to_string_pretty(&cbom).expect("Failed to serialize CBOM") | ||
| } | ||
| ExportFormat::Json => { | ||
| serde_json::to_string_pretty(findings).expect("Failed to serialize findings") | ||
| } |
There was a problem hiding this comment.
std::fs::write(filename, &content) unconditionally replaces any pre-existing file. A user who has manually edited or version-controlled their findings.cbom.json / findings.json / findings.sarif.json will lose that data without any warning.
Consider checking for existence first and either (a) surfacing a confirmation notice, or (b) appending a timestamp to make each export unique (e.g. findings.cbom.1714000000.json).
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/tui.rs`:
- Around line 4409-4416: The test must not call std::env::set_current_dir
(process-global); change the test to pass the tempdir path into the export
routine instead: update export_findings (or add a new method like
export_findings_to_dir) to accept a destination Path/PathBuf (or
Option<PathBuf>) and use that in the ExportFormat::Cbom branch, then in the test
call app.export_findings_to_dir(ExportFormat::Cbom, dir.path()) (using
tempfile::tempdir() as before) and remove the std::env::set_current_dir/restore
calls; reference the existing symbols app.export_findings, ExportFormat::Cbom,
tui_app_with_findings and tempfile to locate where to add the parameter/overload
and change the test.
- Around line 530-533: In the ExportFormat::Cbom branch, capture the second
return value from crate::report::cbom::build_cbom (the
empty_but_findings_present flag) instead of discarding it; if
empty_but_findings_present is true, surface the condition (e.g., log a warning
or return an error/result that indicates CBOM cannot be produced) rather than
serializing an empty CBOM, otherwise proceed to
serde_json::to_string_pretty(&cbom); reference the ExportFormat::Cbom match arm,
the build_cbom function and the empty_but_findings_present flag when making the
change so the TUI mirrors the warning behavior in the cbom report code.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 52fda49f-975d-438e-a88a-e37b303bb3b2
📒 Files selected for processing (3)
src/report/cbom.rssrc/report/sarif.rssrc/tui.rs
| ExportFormat::Cbom => { | ||
| let (cbom, _) = crate::report::cbom::build_cbom(findings); | ||
| serde_json::to_string_pretty(&cbom).expect("Failed to serialize CBOM") | ||
| } |
There was a problem hiding this comment.
Surface the empty-CBOM case instead of silently exporting it
build_cbom returns the empty_but_findings_present flag for exactly the "findings exist, but none are cryptographic" case. Dropping it here means Scan/Secrets/Diff exports can write an empty CBOM and still report success, which looks like a broken export flow. src/report/cbom.rs:294-300 already turns this same signal into a warning; the TUI path should do the same or hide the CBOM option when it cannot produce meaningful output.
Possible fix
fn export_findings(&mut self, format: ExportFormat) {
let findings = match self.result.as_ref() {
Some(r) => &r.findings,
None => return,
};
let filename = format.filename();
- let content = match format {
+ let finding_count = findings.len();
+ let mut export_notice = None;
+ let content = match format {
ExportFormat::Cbom => {
- let (cbom, _) = crate::report::cbom::build_cbom(findings);
+ let (cbom, empty_but_findings_present) = crate::report::cbom::build_cbom(findings);
+ if empty_but_findings_present {
+ export_notice = Some(
+ "CBOM export is empty: no cryptographic findings detected".to_string(),
+ );
+ }
serde_json::to_string_pretty(&cbom).expect("Failed to serialize CBOM")
}
ExportFormat::Json => {
serde_json::to_string_pretty(findings).expect("Failed to serialize findings")
}
ExportFormat::Sarif => {
let sarif = crate::report::sarif::build_sarif(findings);
serde_json::to_string_pretty(&sarif).expect("Failed to serialize SARIF")
}
};
+
+ if let Some(notice) = export_notice {
+ self.push_runtime_notice(notice);
+ }
match std::fs::write(filename, &content) {
Ok(()) => {
self.push_runtime_notice(format!(
"exported {} findings to {}",
- findings.len(),
+ finding_count,
filename
));
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| ExportFormat::Cbom => { | |
| let (cbom, _) = crate::report::cbom::build_cbom(findings); | |
| serde_json::to_string_pretty(&cbom).expect("Failed to serialize CBOM") | |
| } | |
| fn export_findings(&mut self, format: ExportFormat) { | |
| let findings = match self.result.as_ref() { | |
| Some(r) => &r.findings, | |
| None => return, | |
| }; | |
| let filename = format.filename(); | |
| let finding_count = findings.len(); | |
| let mut export_notice = None; | |
| let content = match format { | |
| ExportFormat::Cbom => { | |
| let (cbom, empty_but_findings_present) = crate::report::cbom::build_cbom(findings); | |
| if empty_but_findings_present { | |
| export_notice = Some( | |
| "CBOM export is empty: no cryptographic findings detected".to_string(), | |
| ); | |
| } | |
| serde_json::to_string_pretty(&cbom).expect("Failed to serialize CBOM") | |
| } | |
| ExportFormat::Json => { | |
| serde_json::to_string_pretty(findings).expect("Failed to serialize findings") | |
| } | |
| ExportFormat::Sarif => { | |
| let sarif = crate::report::sarif::build_sarif(findings); | |
| serde_json::to_string_pretty(&sarif).expect("Failed to serialize SARIF") | |
| } | |
| }; | |
| if let Some(notice) = export_notice { | |
| self.push_runtime_notice(notice); | |
| } | |
| match std::fs::write(filename, &content) { | |
| Ok(()) => { | |
| self.push_runtime_notice(format!( | |
| "exported {} findings to {}", | |
| finding_count, | |
| filename | |
| )); | |
| } | |
| Err(e) => { | |
| self.push_runtime_error(format!("Failed to export findings: {}", e)); | |
| } | |
| } | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/tui.rs` around lines 530 - 533, In the ExportFormat::Cbom branch, capture
the second return value from crate::report::cbom::build_cbom (the
empty_but_findings_present flag) instead of discarding it; if
empty_but_findings_present is true, surface the condition (e.g., log a warning
or return an error/result that indicates CBOM cannot be produced) rather than
serializing an empty CBOM, otherwise proceed to
serde_json::to_string_pretty(&cbom); reference the ExportFormat::Cbom match arm,
the build_cbom function and the empty_but_findings_present flag when making the
change so the TUI mirrors the warning behavior in the cbom report code.
- Accept explicit output path in export_findings_to, removing thread-unsafe set_current_dir from test - Surface empty-CBOM notice when no cryptographic findings detected - Bump export popup height from 20% to 40% for small terminals - Fix cargo fmt
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/tui.rs (1)
1677-1692:⚠️ Potential issue | 🟡 MinorMark
Shift+Nas Pqc-only in the help copy.The panel is now gated by
request.pq_mode, but the help still advertisesShift+Nas a general toggle. In Scan/Diff/Secrets modes that makes the shortcut look broken.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/tui.rs` around lines 1677 - 1692, Update the help text entry that currently reads Line::from("Shift+N toggle CNSA 2.0 compliance panel") to indicate it's PQC-only (for example "Shift+N (PQC only) toggle CNSA 2.0 compliance panel") so users in Scan/Diff/Secrets modes don't assume the shortcut is available; keep the existing request.pq_mode gating intact (the display logic that uses request.pq_mode should not be changed).
🧹 Nitpick comments (1)
src/tui.rs (1)
4257-4272: This test mirrors the implementation instead of exercising it.
compliance_panel_hidden_outside_pqc_modere-evaluates the same predicate used indraw_body, so it stays green even if rendering stops honoring that gate. Please assert through a shared helper or a rendered buffer so the test covers real behavior.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/tui.rs` around lines 4257 - 4272, The test compliance_panel_hidden_outside_pqc_mode currently re-checks the same predicate used in draw_body instead of exercising rendering; update the test to toggle fields (app.show_launch/app.show_compliance_panel and ensure app.request.pq_mode is false), then call the actual render routine used by the app (e.g., the draw or render_to_buffer method that invokes draw_body) and assert the compliance panel's UI is not present in the rendered buffer (e.g., by checking for the panel title or specific text), rather than re-evaluating the predicate; reference draw_body, show_compliance_panel, and request.pq_mode when locating the code to change.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/tui.rs`:
- Around line 1811-1837: The popup currently uses centered_rect(40, 40,
frame.area()) which can shrink below the required content height; instead
calculate the dialog's required rows from content (e.g. let required_rows = 1
/*header*/ + menu.formats.len() as u16 + 2 /*footer/margins*/ + 2 /*outer
margins*/), clamp it to the terminal height (let term_h = frame.area().height;
let height_rows = required_rows.min(term_h.saturating_sub(0)) ), convert that to
a height percentage (let height_pct = (height_rows.saturating_mul(100) /
term_h).max(1)), and call centered_rect(width_pct, height_pct, frame.area());
keep the inner/layout code using Constraint::Length(menu.formats.len() as u16 +
2) so the list gets its full required length when terminal is large enough and
otherwise is clamped by the computed height_pct.
---
Outside diff comments:
In `@src/tui.rs`:
- Around line 1677-1692: Update the help text entry that currently reads
Line::from("Shift+N toggle CNSA 2.0 compliance panel") to indicate it's
PQC-only (for example "Shift+N (PQC only) toggle CNSA 2.0 compliance
panel") so users in Scan/Diff/Secrets modes don't assume the shortcut is
available; keep the existing request.pq_mode gating intact (the display logic
that uses request.pq_mode should not be changed).
---
Nitpick comments:
In `@src/tui.rs`:
- Around line 4257-4272: The test compliance_panel_hidden_outside_pqc_mode
currently re-checks the same predicate used in draw_body instead of exercising
rendering; update the test to toggle fields
(app.show_launch/app.show_compliance_panel and ensure app.request.pq_mode is
false), then call the actual render routine used by the app (e.g., the draw or
render_to_buffer method that invokes draw_body) and assert the compliance
panel's UI is not present in the rendered buffer (e.g., by checking for the
panel title or specific text), rather than re-evaluating the predicate;
reference draw_body, show_compliance_panel, and request.pq_mode when locating
the code to change.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
| let area = centered_rect(40, 40, frame.area()); | ||
| let items = menu | ||
| .formats | ||
| .iter() | ||
| .map(|fmt| ListItem::new(Line::from(Span::styled(fmt.label(), Style::default())))) | ||
| .collect::<Vec<_>>(); | ||
| let list = List::new(items) | ||
| .block(panel_block(None, PANEL_BG)) | ||
| .highlight_style( | ||
| Style::default() | ||
| .fg(Color::White) | ||
| .bg(DETAIL_BG) | ||
| .add_modifier(Modifier::BOLD), | ||
| ) | ||
| .highlight_symbol("> "); | ||
| let inner = area.inner(Margin { | ||
| vertical: 1, | ||
| horizontal: 1, | ||
| }); | ||
| let layout = Layout::default() | ||
| .direction(Direction::Vertical) | ||
| .constraints([ | ||
| Constraint::Length(1), | ||
| Constraint::Length(menu.formats.len() as u16 + 2), | ||
| Constraint::Length(1), | ||
| ]) | ||
| .split(inner); |
There was a problem hiding this comment.
The export modal still clips on small terminals.
Line 1811 still sizes the popup purely by percentage. This layout needs at least 9 rows end-to-end, but a 20-line terminal only gives the modal 8 rows at 40%, so the list/footer gets truncated. Please size the dialog from its content height and then clamp to the terminal instead of relying on a fixed percentage.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/tui.rs` around lines 1811 - 1837, The popup currently uses
centered_rect(40, 40, frame.area()) which can shrink below the required content
height; instead calculate the dialog's required rows from content (e.g. let
required_rows = 1 /*header*/ + menu.formats.len() as u16 + 2 /*footer/margins*/
+ 2 /*outer margins*/), clamp it to the terminal height (let term_h =
frame.area().height; let height_rows =
required_rows.min(term_h.saturating_sub(0)) ), convert that to a height
percentage (let height_pct = (height_rows.saturating_mul(100) / term_h).max(1)),
and call centered_rect(width_pct, height_pct, frame.area()); keep the
inner/layout code using Constraint::Length(menu.formats.len() as u16 + 2) so the
list gets its full required length when terminal is large enough and otherwise
is clamped by the computed height_pct.
2 participants
Summary
Shift+N) was visible in all scan modes — now gated to Pqc mode only viaself.request.pq_modeekey opens a CBOM / JSON / SARIF export picker, writes findings tofindings.{cbom,sarif}.jsonorfindings.jsonbuild_cbomandbuild_sarifpublic so the TUI can reuse them without stdoutTest plan
cargo test --lib— 378 tests passcargo clippy— cleanShift+Nshows panel; Scan mode →Shift+Ndoes note→ pick CBOM → verify file writtenSummary by CodeRabbit
New Features
Behavior Changes
Bug Fixes
Tests