diff --git a/.claude/skills/detail-bugs/SKILL.md b/.claude/skills/detail-bugs/SKILL.md index ceb3266..6e4d919 100644 --- a/.claude/skills/detail-bugs/SKILL.md +++ b/.claude/skills/detail-bugs/SKILL.md @@ -1,11 +1,11 @@ --- name: detail-bugs -description: Interact with Detail bugs for a repository via the CLI — list and filter bugs, inspect reports, close as resolved or dismissed, reopen previously closed bugs, and override a bug's priority. Also covers read-only access to security scan findings (`detail vulns`). +description: Interact with Detail bugs for a repository via the CLI — list and filter bugs, inspect reports, close as resolved or dismissed, reopen previously closed bugs, and override a bug's priority. Also covers security scan findings (`detail vulns`): list, show, close, and reopen. --- # Detail Bugs -The Detail CLI exposes per-repository bugs through five subcommands: `list`, `show`, `close`, `reopen`, and `prioritize`. Findings from Detail's security scans live separately under `detail vulns` (read-only). This skill describes both surfaces so you can pick the right command for whatever the user is trying to do. +The Detail CLI exposes per-repository bugs through five subcommands: `list`, `show`, `close`, `reopen`, and `prioritize`. Findings from Detail's security scans live separately under `detail vulns`. This skill describes both surfaces so you can pick the right command for whatever the user is trying to do. ## Prerequisites @@ -70,10 +70,11 @@ Setting the priority a bug already has is a no-op — the CLI reports "no change ## Security scan findings (`detail vulns`) -Security scans produce findings ("vulns") that are separate from bugs: they have a severity (`critical`, `high`, `medium`, `low`) instead of a priority, and no review state — there is no close, reopen, or prioritize for them. Vuln IDs look like `ssf_…`. When the user asks about security scan results or vulnerabilities from a security scan, use these commands rather than `detail bugs list --vulns`. +Security scans produce findings ("vulns") that are separate from bugs: they have a severity (`critical`, `high`, `medium`, `low`) instead of a priority, so there is no prioritize for them. They can be closed and reopened like bugs. A finding's state can also change without a review: a merged Detail fix PR or a closed tracker issue sets it, and the latest event wins. Vuln IDs look like `ssf_…`. When the user asks about security scan results or vulnerabilities from a security scan, use these commands rather than `detail bugs list --vulns`. ### `detail vulns list [REPO]` +- `--status pending|resolved|dismissed` — one value; default `pending`, so closed findings are hidden unless asked for. To see every state, run it once per state. - `--severity critical|high|medium|low` — comma-separate or repeat the flag (e.g. `--severity critical,high`). Default: every severity. - `--sort newest|oldest|severity` — default `newest`. `severity` puts the most severe first. - `--scan-id ` — limit to a specific security scan. Workflow IDs come from `detail vulns scans`. @@ -81,10 +82,23 @@ Security scans produce findings ("vulns") that are separate from bugs: they have ### `detail vulns show ` -Shows the full finding: severity, file, commit, scan, linked issues, the fix PR if any, and a markdown description of the flaw, how it is reached, and the fix. +Shows the full finding: severity, state (with dismissal reason, notes, and what set it, when present), file, commit, scan, linked issues, the fix PR if any, and a markdown description of the flaw, how it is reached, and the fix. - `--format table|json`. +### `detail vulns close ` + +Marks a finding as resolved or dismissed. The CLI prompts for `--state` interactively in a TTY; pass it explicitly when invoking non-interactively. + +- `--state resolved|dismissed`. +- `--dismissal-reason not-exploitable|wont-fix|duplicate|other` — required when state is `dismissed`. +- `--notes "..."` — optional free-form context, up to 2000 characters. +- `--format table|json`. + +### `detail vulns reopen ` + +Flips a previously resolved or dismissed finding back to `pending`. Takes only the vuln ID. The earlier reason and notes do not carry over. + ### `detail vulns scans [REPO]` Lists recent security scans with per-severity finding counts. diff --git a/docs/HELP.md b/docs/HELP.md index b71b471..c5501b9 100644 --- a/docs/HELP.md +++ b/docs/HELP.md @@ -37,6 +37,8 @@ This document contains the help content for the `detail` command-line program. * [`detail vulns`↴](#detail-vulns) * [`detail vulns list`↴](#detail-vulns-list) * [`detail vulns show`↴](#detail-vulns-show) +* [`detail vulns close`↴](#detail-vulns-close) +* [`detail vulns reopen`↴](#detail-vulns-reopen) * [`detail vulns scans`↴](#detail-vulns-scans) ## `detail` @@ -63,7 +65,7 @@ Common workflow: * `skill` — Install Detail skills (default: detail-bugs, which also covers vulns) * `update` — Update immediately (auto-update also runs in the background) * `version` — Show version information -* `vulns` — List and inspect findings from security scans +* `vulns` — List, show, and close findings from security scans @@ -568,7 +570,7 @@ Show version information ## `detail vulns` -List and inspect findings from security scans +List, show, and close findings from security scans **Usage:** `detail vulns ` @@ -576,6 +578,8 @@ List and inspect findings from security scans * `list` — List security scan findings for a given repository * `show` — Show the report for a security scan finding +* `close` — Close a security scan finding as resolved or dismissed +* `reopen` — Reopen a previously resolved or dismissed finding — flips it back to pending. Useful when a fix is reverted or a dismissal is overturned * `scans` — List recent security scans for a repository @@ -592,6 +596,10 @@ List security scan findings for a given repository ###### **Options:** +* `--status ` — Only show findings in this state. Takes one value. Default: pending (applied by the API), so closed findings stay hidden unless asked for + + Possible values: `pending`, `resolved`, `dismissed` + * `--severity ` — Only show findings at these severities — repeat the flag or comma-separate values (e.g. `--severity critical,high`). Default: all severities Possible values: `critical`, `high`, `medium`, `low` @@ -642,6 +650,48 @@ Show the report for a security scan finding +## `detail vulns close` + +Close a security scan finding as resolved or dismissed + +**Usage:** `detail vulns close [OPTIONS] ` + +###### **Arguments:** + +* `` — Vuln ID + +###### **Options:** + +* `--state ` — Close state (prompted interactively if omitted in a TTY) + + Possible values: `resolved`, `dismissed` + +* `--dismissal-reason ` — Dismissal reason (required if state is dismissed) + + Possible values: `not-exploitable`, `wont-fix`, `duplicate`, `other` + +* `--notes ` — Additional notes (up to 2000 characters) +* `--format ` — Output format + + Default value: `table` + + Possible values: `table`, `json` + + + + +## `detail vulns reopen` + +Reopen a previously resolved or dismissed finding — flips it back to pending. Useful when a fix is reverted or a dismissal is overturned + +**Usage:** `detail vulns reopen ` + +###### **Arguments:** + +* `` — Vuln ID + + + ## `detail vulns scans` List recent security scans for a repository diff --git a/openapi.json b/openapi.json index fdf0570..c24845b 100644 --- a/openapi.json +++ b/openapi.json @@ -572,6 +572,9 @@ "Vuln": { "description": "A vulnerability found by a Detail security scan.", "properties": { + "category": { + "$ref": "#/components/schemas/VulnCategory" + }, "commitSha": { "nullable": true, "type": "string" @@ -583,6 +586,9 @@ "description": "Markdown: the flaw, how an attacker reaches it, and the fix.", "type": "string" }, + "dismissalReason": { + "$ref": "#/components/schemas/VulnDismissalReason" + }, "filePath": { "type": "string" }, @@ -598,15 +604,32 @@ }, "type": "array" }, + "notes": { + "type": "string" + }, "repoId": { "$ref": "#/components/schemas/RepoId" }, "severity": { "$ref": "#/components/schemas/VulnSeverity" }, + "state": { + "$ref": "#/components/schemas/VulnReviewState", + "description": "`pending` until something resolves or dismisses it." + }, + "stateSource": { + "$ref": "#/components/schemas/VulnStateSource" + }, + "stateUpdatedAt": { + "type": "integer" + }, "title": { "type": "string" }, + "trackerStateName": { + "description": "The tracker's own name for the state.", + "type": "string" + }, "workflowRequestId": { "nullable": true, "pattern": "^wr_.*", @@ -614,10 +637,12 @@ } }, "required": [ + "state", "id", "title", "description", "severity", + "category", "filePath", "createdAt", "repoId", @@ -627,6 +652,27 @@ ], "type": "object" }, + "VulnCategory": { + "description": "Vulnerability class of a security scan finding. Null for findings reported before categories existed.", + "enum": [ + "access-control", + "authentication", + "injection", + "xss", + "request-forgery", + "path-traversal", + "sensitive-data-exposure", + "denial-of-service", + "prompt-injection", + "vulnerable-dependency", + "supply-chain", + "abuse-of-functionality", + "other", + null + ], + "nullable": true, + "type": "string" + }, "VulnCounts": { "properties": { "critical": { @@ -654,6 +700,15 @@ ], "type": "object" }, + "VulnDismissalReason": { + "enum": [ + "not_exploitable", + "wont_fix", + "duplicate", + "other" + ], + "type": "string" + }, "VulnId": { "pattern": "^ssf_.*", "type": "string" @@ -684,6 +739,40 @@ ], "type": "object" }, + "VulnReview": { + "properties": { + "createdAt": { + "type": "integer" + }, + "dismissalReason": { + "$ref": "#/components/schemas/VulnDismissalReason" + }, + "id": { + "pattern": "^ssfrv_.*", + "type": "string" + }, + "notes": { + "type": "string" + }, + "state": { + "$ref": "#/components/schemas/VulnReviewState" + } + }, + "required": [ + "id", + "state", + "createdAt" + ], + "type": "object" + }, + "VulnReviewState": { + "enum": [ + "resolved", + "dismissed", + "pending" + ], + "type": "string" + }, "VulnScan": { "properties": { "commitSha": { @@ -752,6 +841,18 @@ ], "type": "string" }, + "VulnStateSource": { + "description": "What set the vulnerability's current state.", + "enum": [ + "review", + "linear", + "jira", + "asana", + "github_issue", + "detail_fix_pr" + ], + "type": "string" + }, "WorkflowStatus": { "enum": [ "in_progress", @@ -1769,6 +1870,16 @@ "$ref": "#/components/schemas/RepoId" } }, + { + "description": "Only vulnerabilities currently in this state.", + "in": "query", + "name": "status", + "required": false, + "schema": { + "$ref": "#/components/schemas/VulnReviewState", + "default": "pending" + } + }, { "description": "Comma-separated severities to include, e.g. \"critical,high\". Omit to include every severity.", "example": "critical,high", @@ -1928,6 +2039,85 @@ ], "summary": "Get a vulnerability" } + }, + "/public/v1/vulns/{vuln_id}/review": { + "post": { + "description": "Records a review on a vulnerability: resolve, dismiss, or reopen (state `pending`). `dismissalReason` is optional, and is kept only when dismissing.", + "operationId": "createPublicVulnReview", + "parameters": [ + { + "in": "path", + "name": "vuln_id", + "required": true, + "schema": { + "$ref": "#/components/schemas/VulnId" + } + } + ], + "requestBody": { + "content": { + "application/json": { + "schema": { + "properties": { + "dismissalReason": { + "$ref": "#/components/schemas/VulnDismissalReason" + }, + "notes": { + "maxLength": 2000, + "type": "string" + }, + "state": { + "$ref": "#/components/schemas/VulnReviewState" + } + }, + "required": [ + "state" + ], + "type": "object" + } + } + }, + "required": true + }, + "responses": { + "200": { + "content": { + "application/json": { + "schema": { + "$ref": "#/components/schemas/VulnReview" + } + } + }, + "description": "OK" + }, + "4XX": { + "content": { + "application/json": { + "schema": { + "$ref": "#/components/schemas/ApiError" + } + } + }, + "description": "Client error" + }, + "5XX": { + "content": { + "application/json": { + "schema": { + "$ref": "#/components/schemas/ApiError" + } + } + }, + "description": "Server error" + } + }, + "security": [ + { + "BearerAuth": [] + } + ], + "summary": "Review a vulnerability" + } } }, "servers": [ diff --git a/src/api/client.rs b/src/api/client.rs index f678e41..36649ed 100644 --- a/src/api/client.rs +++ b/src/api/client.rs @@ -11,11 +11,12 @@ use progenitor::progenitor_client::{Error as ProgenitorError, ResponseValue}; use super::generated::types::CreateRuleBody; use super::types::{ Bug, BugDismissalReason, BugId, BugReview, BugReviewState, BugSortOrder, BugsResponse, - CreatePublicBugReviewBody, CreateRuleInput, CreateRuleResponse, + CreatePublicBugReviewBody, CreatePublicVulnReviewBody, CreateRuleInput, CreateRuleResponse, ListPublicBugsWorkflowRequestId, ListPublicVulnsWorkflowRequestId, Priority, PriorityUpdate, RepoId, ReposResponse, Rule, RuleCreationRequestId, RuleId, RuleRequestStatus, RuleRequestsResponse, RulesResponse, ScansResponse, UpdatePublicBugPriorityBody, UserInfo, - Vuln, VulnId, VulnScansResponse, VulnSortOrder, VulnsResponse, + Vuln, VulnDismissalReason, VulnId, VulnReview, VulnReviewState, VulnScansResponse, + VulnSortOrder, VulnsResponse, }; /// Server-side knobs for `GET /public/v1/bugs` beyond repo, status, and paging. @@ -35,6 +36,8 @@ pub struct VulnListQuery<'a> { /// Comma-separated severity filter, already in the API's wire form. pub severity: Option<&'a str>, pub sort: Option, + /// Omitted when `None`, which the API reads as `pending`. + pub status: Option, pub scan_id: Option<&'a ListPublicVulnsWorkflowRequestId>, } @@ -223,6 +226,7 @@ impl ApiClient { repo_id, query.severity, query.sort, + query.status, query.scan_id, ) .await @@ -238,6 +242,33 @@ impl ApiClient { .map_err(api_error) } + pub async fn update_vuln_close( + &self, + vuln_id: &VulnId, + state: VulnReviewState, + dismissal_reason: Option, + notes: Option<&str>, + ) -> Result { + // The spec caps notes at 2000 chars, so progenitor wraps them in a + // validating newtype — reject over-long notes here rather than letting + // the API do it in a round trip. + let notes = notes + .map(TryInto::try_into) + .transpose() + .map_err(|e| anyhow::anyhow!("Invalid notes: {e}"))?; + let body = CreatePublicVulnReviewBody { + dismissal_reason, + notes, + state, + }; + + self.inner + .create_public_vuln_review(vuln_id, &body) + .await + .map(ResponseValue::into_inner) + .map_err(api_error) + } + pub async fn list_vuln_scans( &self, repo_id: &RepoId, diff --git a/src/api/types.rs b/src/api/types.rs index 72b9a97..e46d70f 100644 --- a/src/api/types.rs +++ b/src/api/types.rs @@ -13,8 +13,9 @@ pub use super::generated::types::{ UpdatePublicBugPriorityBody, WorkflowStatus, }; pub use super::generated::types::{ - ListPublicVulnsWorkflowRequestId, Vuln, VulnCounts, VulnId, VulnLinkedIssue, - VulnLinkedIssueTracker, VulnScan, VulnSeverity, VulnSortOrder, + CreatePublicVulnReviewBody, ListPublicVulnsWorkflowRequestId, Vuln, VulnCounts, + VulnDismissalReason, VulnId, VulnLinkedIssue, VulnLinkedIssueTracker, VulnReview, + VulnReviewState, VulnScan, VulnSeverity, VulnSortOrder, VulnStateSource, }; // Friendlier aliases for the generated response-wrapper names. @@ -68,6 +69,34 @@ pub const fn severity_label(s: &VulnSeverity) -> &'static str { } } +pub const fn vuln_state_label(s: &VulnReviewState) -> &'static str { + match s { + VulnReviewState::Pending => "Pending", + VulnReviewState::Resolved => "Resolved", + VulnReviewState::Dismissed => "Dismissed", + } +} + +pub const fn vuln_dismissal_reason_label(r: &VulnDismissalReason) -> &'static str { + match r { + VulnDismissalReason::NotExploitable => "Not Exploitable", + VulnDismissalReason::WontFix => "Won't Fix", + VulnDismissalReason::Duplicate => "Duplicate", + VulnDismissalReason::Other => "Other", + } +} + +pub const fn vuln_state_source_label(s: &VulnStateSource) -> &'static str { + match s { + VulnStateSource::Review => "Review", + VulnStateSource::Linear => "Linear", + VulnStateSource::Jira => "Jira", + VulnStateSource::Asana => "Asana", + VulnStateSource::GithubIssue => "GitHub Issue", + VulnStateSource::DetailFixPr => "Detail Fix PR", + } +} + pub const fn rule_status_label(s: &RuleStatus) -> &'static str { match s { RuleStatus::Pending => "Pending", @@ -236,6 +265,40 @@ impl clap::ValueEnum for VulnSeverity { } } +impl clap::ValueEnum for VulnReviewState { + fn value_variants<'a>() -> &'a [Self] { + &[Self::Pending, Self::Resolved, Self::Dismissed] + } + + fn to_possible_value(&self) -> Option { + match self { + Self::Pending => Some(PossibleValue::new("pending")), + Self::Resolved => Some(PossibleValue::new("resolved")), + Self::Dismissed => Some(PossibleValue::new("dismissed")), + } + } +} + +impl clap::ValueEnum for VulnDismissalReason { + fn value_variants<'a>() -> &'a [Self] { + &[ + Self::NotExploitable, + Self::WontFix, + Self::Duplicate, + Self::Other, + ] + } + + fn to_possible_value(&self) -> Option { + match self { + Self::NotExploitable => Some(PossibleValue::new("not-exploitable")), + Self::WontFix => Some(PossibleValue::new("wont-fix")), + Self::Duplicate => Some(PossibleValue::new("duplicate")), + Self::Other => Some(PossibleValue::new("other")), + } + } +} + impl clap::ValueEnum for VulnSortOrder { fn value_variants<'a>() -> &'a [Self] { &[Self::Newest, Self::Oldest, Self::Severity] @@ -366,8 +429,12 @@ impl Formattable for Vuln { let mut pairs = vec![ ("Vuln ID", self.id.to_string()), ("Severity", severity_label(&self.severity).to_string()), - ("Created", format_date(self.created_at)), + ("State", vuln_state_label(&self.state).to_string()), ]; + if let Some(reason) = &self.dismissal_reason { + pairs.push(("Dismissal", vuln_dismissal_reason_label(reason).to_string())); + } + pairs.push(("Created", format_date(self.created_at))); if !self.file_path.is_empty() { pairs.push(("File", self.file_path.clone())); } @@ -1042,7 +1109,7 @@ mod tests { "description": "...", "severity": "critical", "filePath": "src/search.rs", "createdAt": 1_736_899_200_000_i64, "repoId": "repo_xyz", "commitSha": null, "workflowRequestId": null, - "linkedIssues": [] + "linkedIssues": [], "state": "pending" })) .expect("valid Vuln JSON") } @@ -1082,8 +1149,63 @@ mod tests { let (header, pairs) = sample_vuln().to_card(); assert_eq!(header, "SQL injection in search"); let keys: Vec<&str> = pairs.iter().map(|(k, _)| *k).collect(); - assert_eq!(keys, ["Vuln ID", "Severity", "Created", "File"]); + assert_eq!(keys, ["Vuln ID", "Severity", "State", "Created", "File"]); assert_eq!(pairs[1].1, "Critical"); + assert_eq!(pairs[2].1, "Pending"); + } + + #[test] + fn vuln_card_shows_dismissal_reason_when_dismissed() { + let mut vuln = sample_vuln(); + vuln.state = VulnReviewState::Dismissed; + vuln.dismissal_reason = Some(VulnDismissalReason::NotExploitable); + let (_, pairs) = vuln.to_card(); + assert!(pairs.contains(&("State", "Dismissed".to_string()))); + assert!(pairs.contains(&("Dismissal", "Not Exploitable".to_string()))); + } + + #[test] + fn vuln_card_handles_dismissal_without_reason() { + // A tracker can dismiss a finding without giving a reason. + let mut vuln = sample_vuln(); + vuln.state = VulnReviewState::Dismissed; + let (_, pairs) = vuln.to_card(); + assert!(pairs.contains(&("State", "Dismissed".to_string()))); + assert!(!pairs.iter().any(|(k, _)| *k == "Dismissal")); + } + + #[test] + fn vuln_state_possible_values() { + let values: Vec = VulnReviewState::value_variants() + .iter() + .map(|v| v.to_possible_value().unwrap().get_name().to_string()) + .collect(); + assert_eq!(values, ["pending", "resolved", "dismissed"]); + } + + #[test] + fn vuln_dismissal_reason_possible_values() { + let values: Vec = VulnDismissalReason::value_variants() + .iter() + .map(|v| v.to_possible_value().unwrap().get_name().to_string()) + .collect(); + assert_eq!( + values, + ["not-exploitable", "wont-fix", "duplicate", "other"] + ); + } + + #[test] + fn vuln_state_and_reason_labels() { + assert_eq!(vuln_state_label(&VulnReviewState::Resolved), "Resolved"); + assert_eq!( + vuln_dismissal_reason_label(&VulnDismissalReason::NotExploitable), + "Not Exploitable" + ); + assert_eq!( + vuln_state_source_label(&VulnStateSource::GithubIssue), + "GitHub Issue" + ); } #[test] diff --git a/src/commands/bugs.rs b/src/commands/bugs.rs index 77f319a..76769ba 100644 --- a/src/commands/bugs.rs +++ b/src/commands/bugs.rs @@ -396,7 +396,7 @@ fn prompt_priority() -> Result { } /// Prompt for optional notes via text input. -fn prompt_notes() -> Result> { +pub(crate) fn prompt_notes() -> Result> { let input: String = Input::new() .with_prompt("Notes (optional)") .allow_empty(true) diff --git a/src/commands/vulns.rs b/src/commands/vulns.rs index 6e2c503..7e13742 100644 --- a/src/commands/vulns.rs +++ b/src/commands/vulns.rs @@ -1,14 +1,19 @@ use std::convert::TryInto; -use anyhow::{Context, Result}; +use anyhow::{bail, Context, Result}; +use clap::builder::{PossibleValuesParser, TypedValueParser}; use clap::Subcommand; -use console::Term; +use console::{style, Term}; +use dialoguer::Select; use crate::api::client::{ApiClient, VulnListQuery}; use crate::api::types::{ - format_fix_pr, format_linked_issue, severity_label, ListPublicVulnsWorkflowRequestId, RepoId, - Vuln, VulnId, VulnScan, VulnSeverity, VulnSortOrder, WorkflowStatus, + format_fix_pr, format_linked_issue, severity_label, vuln_dismissal_reason_label, + vuln_state_label, vuln_state_source_label, ListPublicVulnsWorkflowRequestId, RepoId, Vuln, + VulnDismissalReason, VulnId, VulnReviewState, VulnScan, VulnSeverity, VulnSortOrder, + WorkflowStatus, }; +use crate::commands::bugs::prompt_notes; use crate::output::{clamp_page, output_list, SectionRenderer}; use crate::utils::datetime::{format_datetime, parse_time_spec}; use crate::utils::pagination::page_to_offset; @@ -23,6 +28,11 @@ pub enum VulnCommands { /// If omitted, inferred from the git or jj remote (origin). repo: Option, + /// Only show findings in this state. Takes one value. Default: pending + /// (applied by the API), so closed findings stay hidden unless asked for. + #[arg(long, value_enum)] + status: Option, + /// Only show findings at these severities — repeat the flag or /// comma-separate values (e.g. `--severity critical,high`). /// Default: all severities. @@ -74,6 +84,35 @@ pub enum VulnCommands { format: crate::OutputFormat, }, + /// Close a security scan finding as resolved or dismissed + Close { + /// Vuln ID + vuln_id: String, + + /// Close state (prompted interactively if omitted in a TTY) + #[arg(long, value_parser = close_state_parser())] + state: Option, + + /// Dismissal reason (required if state is dismissed) + #[arg(long, value_enum)] + dismissal_reason: Option, + + /// Additional notes (up to 2000 characters) + #[arg(long)] + notes: Option, + + /// Output format + #[arg(long, value_enum, default_value = "table")] + format: crate::OutputFormat, + }, + + /// Reopen a previously resolved or dismissed finding — flips it back to + /// pending. Useful when a fix is reverted or a dismissal is overturned. + Reopen { + /// Vuln ID + vuln_id: String, + }, + /// List recent security scans for a repository Scans { /// Repository in owner/repo format or just repo name. @@ -108,6 +147,93 @@ pub enum VulnCommands { }, } +// ── Interactive prompt helpers ────────────────────────────────────── + +/// Prompt for close state (Resolved / Dismissed) via arrow-key selection. +fn prompt_close_state() -> Result { + let items = ["Resolved", "Dismissed"]; + let selection = Select::new() + .with_prompt("Close state") + .items(items) + .default(0) + .interact() + .context("Failed to read close state selection")?; + match selection { + 0 => Ok(VulnReviewState::Resolved), + _ => Ok(VulnReviewState::Dismissed), + } +} + +/// Prompt for dismissal reason via arrow-key selection. +fn prompt_dismissal_reason() -> Result { + let items = ["Not Exploitable", "Won't Fix", "Duplicate", "Other"]; + let selection = Select::new() + .with_prompt("Dismissal reason") + .items(items) + .default(0) + .interact() + .context("Failed to read dismissal reason selection")?; + match selection { + 0 => Ok(VulnDismissalReason::NotExploitable), + 1 => Ok(VulnDismissalReason::WontFix), + 2 => Ok(VulnDismissalReason::Duplicate), + _ => Ok(VulnDismissalReason::Other), + } +} + +/// `--state` values `vulns close` accepts, so its help lists only these. +fn close_state_parser() -> impl TypedValueParser { + PossibleValuesParser::new(["resolved", "dismissed"]).map(|state| { + if state == "resolved" { + VulnReviewState::Resolved + } else { + VulnReviewState::Dismissed + } + }) +} + +/// Validate the close-command flags that can be checked without prompting. +/// A returned `None` means the field still needs interactive input. +/// +/// Rules match `detail bugs close`: +/// - `--state pending` is always rejected. +/// - When `is_interactive` is false, `--state` is required. +/// - When state is `Dismissed` and `is_interactive` is false, +/// `--dismissal-reason` is required. +/// - When state is not `Dismissed`, any supplied `--dismissal-reason` is +/// passed through (the API drops it). +fn validate_close_flags( + state: Option, + dismissal_reason: Option, + is_interactive: bool, +) -> Result<(Option, Option)> { + if matches!(state, Some(VulnReviewState::Pending)) { + bail!("'pending' is not a valid close state. Use 'resolved' or 'dismissed'."); + } + + let state = match state { + Some(s) => Some(s), + None if is_interactive => None, // will prompt + None => bail!( + "--state is required in non-interactive mode. Use --state resolved or --state dismissed." + ), + }; + + let dismissal_reason = if matches!(state, Some(VulnReviewState::Dismissed)) { + match dismissal_reason { + Some(r) => Some(r), + None if is_interactive => None, // will prompt + None => bail!( + "--dismissal-reason is required when state is 'dismissed' in non-interactive mode." + ), + } + } else { + dismissal_reason + }; + + Ok((state, dismissal_reason)) +} + /// Join `--severity` values into the comma-separated form the API takes. /// /// Preserves first-seen order and drops repeats. Returns `None` for an empty @@ -188,6 +314,27 @@ fn vuln_show_pairs(vuln: &Vuln) -> Vec<(&'static str, String)> { ("ID", vuln.id.to_string()), ("Title", vuln.title.clone()), ("Severity", severity_label(&vuln.severity).to_string()), + ("State", vuln_state_label(&vuln.state).to_string()), + ]; + if let Some(reason) = &vuln.dismissal_reason { + pairs.push(("Dismissal", vuln_dismissal_reason_label(reason).to_string())); + } + if let Some(notes) = &vuln.notes { + pairs.push(("Notes", notes.clone())); + } + if let Some(source) = &vuln.state_source { + let label = vuln_state_source_label(source); + pairs.push(( + "State Source", + vuln.tracker_state_name + .as_deref() + .map_or_else(|| label.to_string(), |name| format!("{label} ({name})")), + )); + } + if let Some(updated_at) = vuln.state_updated_at { + pairs.push(("State Updated", format_datetime(updated_at))); + } + pairs.extend([ ( "File", if vuln.file_path.is_empty() { @@ -197,7 +344,7 @@ fn vuln_show_pairs(vuln: &Vuln) -> Vec<(&'static str, String)> { }, ), ("Created", format_datetime(vuln.created_at)), - ]; + ]); if let Some(sha) = vuln.commit_sha.as_deref() { let short = sha.get(..7).unwrap_or(sha); pairs.push(("Commit", short.to_string())); @@ -275,6 +422,7 @@ pub async fn handle(command: &VulnCommands, cli: &crate::Cli) -> Result<()> { match command { VulnCommands::List { repo, + status, severity, sort, scan_id, @@ -300,12 +448,13 @@ pub async fn handle(command: &VulnCommands, cli: &crate::Cli) -> Result<()> { let since_ms = resolve_time_flag("--since", since.as_deref(), now)?; let until_ms = resolve_time_flag("--until", until.as_deref(), now)?; - // `--severity`, `--sort` and `--scan-id` are applied by the API; - // only `--all` and the time window force a full fetch. + // `--status`, `--severity`, `--sort` and `--scan-id` are applied by + // the API; only `--all` and the time window force a full fetch. let severity_filter = severity_query(severity); let query = VulnListQuery { severity: severity_filter.as_deref(), sort: Some(*sort), + status: *status, scan_id: scan_id.as_ref(), }; @@ -366,6 +515,83 @@ pub async fn handle(command: &VulnCommands, cli: &crate::Cli) -> Result<()> { .print() } + VulnCommands::Close { + vuln_id, + state, + dismissal_reason, + notes, + format, + } => { + let vuln_id: VulnId = vuln_id + .as_str() + .try_into() + .context("Invalid vuln ID format (expected ssf_...)")?; + let is_interactive = Term::stdout().is_term(); + + let (state, dismissal_reason) = + validate_close_flags(*state, *dismissal_reason, is_interactive)?; + + // Resolve fields that still need interactive prompts + let state = match state { + Some(s) => s, + None => prompt_close_state()?, + }; + + let dismissal_reason = if matches!(state, VulnReviewState::Dismissed) { + match dismissal_reason { + Some(r) => Some(r), + None => Some(prompt_dismissal_reason()?), + } + } else { + dismissal_reason + }; + + let notes = match notes { + Some(n) => Some(n.clone()), + None if is_interactive => prompt_notes()?, + None => None, + }; + + let review = client + .update_vuln_close(&vuln_id, state, dismissal_reason, notes.as_deref()) + .await + .context("Failed to close vuln")?; + + if matches!(format, crate::OutputFormat::Json) { + // Emit only the review JSON — the human-friendly success + // banner would corrupt the structured output. + Term::stdout().write_line(&serde_json::to_string_pretty(&review)?)?; + return Ok(()); + } + + Term::stdout() + .write_line(&format!( + "{}", + style(format!("✓ Vuln closed as: {}", vuln_state_label(&state))).green() + )) + .ok(); + Ok(()) + } + + VulnCommands::Reopen { vuln_id } => { + let vuln_id: VulnId = vuln_id + .as_str() + .try_into() + .context("Invalid vuln ID format (expected ssf_...)")?; + + // A review is a new row, so the earlier reason and notes don't + // carry over: reopening is a pure state flip. + client + .update_vuln_close(&vuln_id, VulnReviewState::Pending, None, None) + .await + .context("Failed to reopen vuln")?; + + Term::stdout() + .write_line(&format!("{}", style("✓ Vuln reopened (pending)").green())) + .ok(); + Ok(()) + } + VulnCommands::Scans { repo, status, @@ -429,7 +655,8 @@ mod tests { "repoId": "repo_1", "commitSha": null, "workflowRequestId": null, - "linkedIssues": [] + "linkedIssues": [], + "state": "pending" })) .unwrap() } @@ -449,6 +676,74 @@ mod tests { vulns.iter().map(|v| v.id.to_string()).collect() } + // ── validate_close_flags ───────────────────────────────────────── + + #[test] + fn close_resolved_non_interactive() { + let (state, reason) = + validate_close_flags(Some(VulnReviewState::Resolved), None, false).unwrap(); + assert!(matches!(state, Some(VulnReviewState::Resolved))); + assert!(reason.is_none()); + } + + #[test] + fn close_dismissed_with_reason_non_interactive() { + let (state, reason) = validate_close_flags( + Some(VulnReviewState::Dismissed), + Some(VulnDismissalReason::NotExploitable), + false, + ) + .unwrap(); + assert!(matches!(state, Some(VulnReviewState::Dismissed))); + assert!(matches!(reason, Some(VulnDismissalReason::NotExploitable))); + } + + #[test] + fn close_rejects_pending_in_both_modes() { + for is_interactive in [false, true] { + let err = validate_close_flags(Some(VulnReviewState::Pending), None, is_interactive) + .unwrap_err(); + assert!(err.to_string().contains("not a valid close state")); + } + } + + #[test] + fn close_no_state_non_interactive_errors() { + let err = validate_close_flags(None, None, false).unwrap_err(); + assert!(err.to_string().contains("--state is required")); + } + + #[test] + fn close_no_state_interactive_defers_to_prompt() { + let (state, _) = validate_close_flags(None, None, true).unwrap(); + assert!(state.is_none()); + } + + #[test] + fn close_dismissed_no_reason_non_interactive_errors() { + let err = validate_close_flags(Some(VulnReviewState::Dismissed), None, false).unwrap_err(); + assert!(err.to_string().contains("--dismissal-reason is required")); + } + + #[test] + fn close_dismissed_no_reason_interactive_defers() { + let (state, reason) = + validate_close_flags(Some(VulnReviewState::Dismissed), None, true).unwrap(); + assert!(matches!(state, Some(VulnReviewState::Dismissed))); + assert!(reason.is_none()); + } + + #[test] + fn close_resolved_passes_reason_through() { + let (_, reason) = validate_close_flags( + Some(VulnReviewState::Resolved), + Some(VulnDismissalReason::Other), + false, + ) + .unwrap(); + assert!(matches!(reason, Some(VulnDismissalReason::Other))); + } + // ── severity_query ─────────────────────────────────────────────── #[test] @@ -523,8 +818,50 @@ mod tests { fn show_pairs_minimal() { let pairs = vuln_show_pairs(&make_vuln("x", 1)); let keys: Vec<&str> = pairs.iter().map(|(k, _)| *k).collect(); - assert_eq!(keys, ["ID", "Title", "Severity", "File", "Created"]); + assert_eq!( + keys, + ["ID", "Title", "Severity", "State", "File", "Created"] + ); assert_eq!(pairs[2].1, "High"); + assert_eq!(pairs[3].1, "Pending"); + } + + #[test] + fn show_pairs_dismissed_by_review() { + let vuln: Vuln = serde_json::from_value(serde_json::json!({ + "id": "ssf_d", "title": "t", "description": "...", "severity": "low", + "filePath": "a.rs", "createdAt": 1, "repoId": "repo_1", "commitSha": null, + "workflowRequestId": null, "linkedIssues": [], "state": "dismissed", + "dismissalReason": "not_exploitable", "notes": "behind auth", + "stateSource": "review", "stateUpdatedAt": 1_736_899_200_000_i64 + })) + .unwrap(); + let pairs = vuln_show_pairs(&vuln); + let get = |k: &str| { + pairs + .iter() + .find(|(key, _)| *key == k) + .map(|(_, v)| v.as_str()) + }; + assert_eq!(get("State"), Some("Dismissed")); + assert_eq!(get("Dismissal"), Some("Not Exploitable")); + assert_eq!(get("Notes"), Some("behind auth")); + assert_eq!(get("State Source"), Some("Review")); + assert!(get("State Updated").is_some()); + } + + #[test] + fn show_pairs_tracker_state_has_name_and_no_reason() { + let vuln: Vuln = serde_json::from_value(serde_json::json!({ + "id": "ssf_t", "title": "t", "description": "...", "severity": "low", + "filePath": "a.rs", "createdAt": 1, "repoId": "repo_1", "commitSha": null, + "workflowRequestId": null, "linkedIssues": [], "state": "dismissed", + "stateSource": "jira", "trackerStateName": "Won't Do" + })) + .unwrap(); + let pairs = vuln_show_pairs(&vuln); + assert!(pairs.contains(&("State Source", "Jira (Won't Do)".to_string()))); + assert!(!pairs.iter().any(|(k, _)| *k == "Dismissal")); } #[test] @@ -533,7 +870,7 @@ mod tests { "id": "ssf_full", "title": "SSRF in fetcher", "description": "...", "severity": "critical", "filePath": "src/fetch.rs", "createdAt": 1, "repoId": "repo_1", "commitSha": "abcdef1234567", - "workflowRequestId": "wr_123", + "workflowRequestId": "wr_123", "state": "pending", "linkedIssues": [{ "tracker": "linear", "issueId": "SEC-1", "url": "https://l/SEC-1" }], "fixPr": { "prNumber": 7, "url": "https://gh/pr/7", "state": "open" } })) diff --git a/src/lib.rs b/src/lib.rs index 6387a71..2475b68 100644 --- a/src/lib.rs +++ b/src/lib.rs @@ -103,7 +103,9 @@ impl Cli { Commands::Vulns { command } => match command { commands::vulns::VulnCommands::List { format, .. } | commands::vulns::VulnCommands::Show { format, .. } + | commands::vulns::VulnCommands::Close { format, .. } | commands::vulns::VulnCommands::Scans { format, .. } => Self::is_json(format), + commands::vulns::VulnCommands::Reopen { .. } => false, }, Commands::Rules { command } => match command { commands::rules::RuleCommands::List { format, .. } @@ -230,7 +232,7 @@ enum Commands { /// Show version information Version, - /// List and inspect findings from security scans + /// List, show, and close findings from security scans Vulns { #[command(subcommand)] command: commands::vulns::VulnCommands, @@ -240,7 +242,9 @@ enum Commands { #[cfg(test)] mod tests { use super::*; - use crate::api::types::{BugReviewState, VulnSeverity, VulnSortOrder}; + use crate::api::types::{ + BugReviewState, VulnDismissalReason, VulnReviewState, VulnSeverity, VulnSortOrder, + }; #[test] fn silent_when_bugs_list_json() { @@ -459,12 +463,122 @@ mod tests { } #[test] - fn vulns_has_no_mutating_subcommands() { - for sub in ["close", "reopen", "prioritize"] { - assert!(Cli::try_parse_from(["detail", "vulns", sub, "ssf_1"]).is_err()); + fn vulns_has_no_prioritize_subcommand() { + assert!(Cli::try_parse_from(["detail", "vulns", "prioritize", "ssf_1"]).is_err()); + } + + #[test] + fn vulns_list_status_defaults_to_unset() { + // Left unset so the API's own default (pending) applies. + let cli = Cli::try_parse_from(["detail", "vulns", "list"]).unwrap(); + if let Commands::Vulns { + command: commands::vulns::VulnCommands::List { status, .. }, + } = cli.command + { + assert!(status.is_none()); + } else { + panic!("expected vulns list command"); } } + #[test] + fn vulns_list_parses_status() { + let cli = + Cli::try_parse_from(["detail", "vulns", "list", "--status", "dismissed"]).unwrap(); + if let Commands::Vulns { + command: commands::vulns::VulnCommands::List { status, .. }, + } = cli.command + { + assert_eq!(status, Some(VulnReviewState::Dismissed)); + } else { + panic!("expected vulns list command"); + } + } + + #[test] + fn vulns_list_status_takes_one_value() { + assert!( + Cli::try_parse_from(["detail", "vulns", "list", "--status", "resolved,dismissed"]) + .is_err() + ); + } + + #[test] + fn vulns_close_parses_flags() { + let cli = Cli::try_parse_from([ + "detail", + "vulns", + "close", + "ssf_1", + "--state", + "dismissed", + "--dismissal-reason", + "not-exploitable", + "--notes", + "behind auth", + ]) + .unwrap(); + if let Commands::Vulns { + command: + commands::vulns::VulnCommands::Close { + vuln_id, + state, + dismissal_reason, + notes, + .. + }, + } = cli.command + { + assert_eq!(vuln_id, "ssf_1"); + assert_eq!(state, Some(VulnReviewState::Dismissed)); + assert_eq!(dismissal_reason, Some(VulnDismissalReason::NotExploitable)); + assert_eq!(notes.as_deref(), Some("behind auth")); + } else { + panic!("expected vulns close command"); + } + } + + #[test] + fn vulns_close_rejects_bug_only_reason() { + assert!(Cli::try_parse_from([ + "detail", + "vulns", + "close", + "ssf_1", + "--state", + "dismissed", + "--dismissal-reason", + "not-a-bug", + ]) + .is_err()); + } + + #[test] + fn vulns_reopen_rejects_notes_flag() { + assert!( + Cli::try_parse_from(["detail", "vulns", "reopen", "ssf_1", "--notes", "x"]).is_err() + ); + } + + #[test] + fn silent_when_vulns_close_json() { + let cli = Cli::try_parse_from([ + "detail", "vulns", "close", "ssf_1", "--state", "resolved", "--format", "json", + ]) + .unwrap(); + assert!(cli.is_silent()); + } + + #[test] + fn not_silent_for_vulns_close_table_or_reopen() { + let close = + Cli::try_parse_from(["detail", "vulns", "close", "ssf_1", "--state", "resolved"]) + .unwrap(); + assert!(!close.is_silent()); + let reopen = Cli::try_parse_from(["detail", "vulns", "reopen", "ssf_1"]).unwrap(); + assert!(!reopen.is_silent()); + } + #[test] fn not_silent_for_auth_status() { let cli = Cli::try_parse_from(["detail", "auth", "status"]).unwrap(); diff --git a/tests/integration.rs b/tests/integration.rs index 1858c6c..ded69cf 100644 --- a/tests/integration.rs +++ b/tests/integration.rs @@ -395,6 +395,12 @@ fn commands_fail_without_auth() { let out = env.run(&["vulns", "list", REPO, "--format", "json"]); assert!(!out.success, "vulns list should fail without auth"); + + let out = env.run(&["vulns", "close", "ssf_1", "--state", "resolved"]); + assert!(!out.success, "vulns close should fail without auth"); + + let out = env.run(&["vulns", "reopen", "ssf_1"]); + assert!(!out.success, "vulns reopen should fail without auth"); } #[test]