Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 2 additions & 1 deletion skills/corgea/SKILL.md
Original file line number Diff line number Diff line change
Expand Up @@ -160,7 +160,8 @@ or `null` when any advisory has no known fix.

Recency gating needs no token; the vuln verdict uses the configured Corgea token when
present. Overrides for testing: `CORGEA_PYPI_REGISTRY`, `CORGEA_NPM_REGISTRY`,
`CORGEA_VULN_API_URL`.
`CORGEA_VULN_API_URL`; `CORGEA_NO_NPM_AUDIT=1` disables the warn-only `npm audit`
second opinion.

<!-- BEGIN GENERATED CORGEA DEPS SKILL -->
### Deps — `corgea deps <command>`
Expand Down
52 changes: 49 additions & 3 deletions src/precheck/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -186,6 +186,9 @@ pub enum TreeReport {
resolved_count: usize,
/// Verdicts for resolved packages beyond the named targets.
transitive: Vec<TreeOutcome>,
/// Warn-only `npm audit` second opinion (npm only; `None` when
/// unavailable, disabled, or failed). Never consulted for blocking.
audit: Option<tree::AuditSummary>,
},
/// Resolution unavailable or failed — only named targets were verified.
NamedOnly { reason: String },
Expand Down Expand Up @@ -364,6 +367,20 @@ fn run_parsed_install(
"warning: transitive dependencies not checked ({reason}); only named packages were verified."
);
}
// Warn-only npm audit second opinion: never blocks, never changes
// exit codes (`should_block_install` ignores it by design).
if let Some(TreeReport::Full {
audit: Some(audit), ..
}) = &tree
{
if audit.total > 0 {
eprintln!(
"note: npm audit reports {} advisories ({} high/critical) — supplementary signal, not blocking",
audit.total,
audit.high + audit.critical
);
}
}
// The requirements note only matters when the tree pass did *not* cover
// those files (fallback to named-only, or recency-only mode).
if !matches!(&tree, Some(TreeReport::Full { .. })) {
Expand Down Expand Up @@ -431,8 +448,11 @@ fn run_tree_pass(
outcomes: &mut [TargetOutcome],
opts: &PrecheckOptions,
) -> TreeReport {
let set = match tree::resolve_tree(manager, rest) {
Ok(Some(set)) => set,
let tree::TreeResolution {
packages: set,
audit: audit_rx,
} = match tree::resolve_tree(manager, rest) {
Ok(Some(resolution)) => resolution,
Ok(None) => {
run_verdict_pass(manager, outcomes, opts);
return TreeReport::NamedOnly {
Expand Down Expand Up @@ -474,10 +494,15 @@ fn run_tree_pass(
.as_ref()
.expect("tree pass requires verdict config");
let results = verdict_pool(jobs, cfg, manager, opts.concurrency);
// Collect the warn-only npm audit second opinion only after the verdict
// pool so the two truly overlap; any failure (timeout, disconnected
// sender) is a silent skip.
let audit = audit_rx.and_then(|rx| rx.recv_timeout(Duration::from_secs(2)).ok());

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Blocking: after this 2s timeout, the code drops the only audit handle and proceeds, but the audit thread/subprocess is still running. spawn_audit returns only an mpsc::Receiver, so the main path cannot cancel or join the thread; if the real install is fast, the corgea process can exit before run_audit reaches its 5s kill path, leaving a hidden npm audit child doing network work and leaking the temp dir. The new hang test only asserts the wrapper returns quickly; it does not prove the audit child is killed or cleaned up. Fix by returning a cancellable/joinable audit handle whose timeout/drop path kills the child process (ideally the process group on Unix) and joins/cleans the temp dir before the wrapper exits, and add a test that observes the hung audit process is actually terminated.

let transitive = apply_verdicts(manager, results, outcomes);
TreeReport::Full {
resolved_count,
transitive,
audit,
}
}

Expand Down Expand Up @@ -768,6 +793,7 @@ fn print_text(report: &PrecheckReport) {
Some(TreeReport::Full {
resolved_count,
transitive,
..
}) => {
println!(
" tree: {} packages resolved, {} transitive checked",
Expand Down Expand Up @@ -875,6 +901,23 @@ fn verdict_json(verdict: &VerdictStatus) -> serde_json::Value {
}
}

/// JSON shape for the warn-only npm audit second opinion in the tree arm.
fn npm_audit_json(audit: &tree::AuditSummary) -> serde_json::Value {
use serde_json::json;
json!({
"total": audit.total,
"critical": audit.critical,
"high": audit.high,
"moderate": audit.moderate,
"low": audit.low,
"info": audit.info,
"top": audit.top.iter().map(|(name, severity)| json!({
"name": name,
"severity": severity,
})).collect::<Vec<_>>(),
})
}

fn print_json(report: &PrecheckReport, opts: &PrecheckOptions) {
use serde_json::json;
let outcomes: Vec<_> = report
Expand Down Expand Up @@ -930,7 +973,7 @@ fn print_json(report: &PrecheckReport, opts: &PrecheckOptions) {
"verdict_mode": if opts.verdict.is_some() { "full" } else { "recency-only" },
"results": outcomes,
"tree": report.tree.as_ref().map(|t| match t {
TreeReport::Full { resolved_count, transitive } => json!({
TreeReport::Full { resolved_count, transitive, audit } => json!({
"mode": "full",
"reason": serde_json::Value::Null,
"resolved_count": resolved_count,
Expand All @@ -939,12 +982,14 @@ fn print_json(report: &PrecheckReport, opts: &PrecheckOptions) {
"version": o.version,
"verdict": verdict_json(&o.verdict),
})).collect::<Vec<_>>(),
"npm_audit": audit.as_ref().map(npm_audit_json),
}),
TreeReport::NamedOnly { reason } => json!({
"mode": "named-only",
"reason": reason,
"resolved_count": 0,
"transitive": [],
"npm_audit": serde_json::Value::Null,
}),
}),
});
Expand Down Expand Up @@ -1161,6 +1206,7 @@ mod tests {
version: "0.4.2".to_string(),
verdict: VerdictStatus::Vulnerable(vec![]),
}],
audit: None,
});

assert_eq!(report.vulnerable_count(), 1);
Expand Down
Loading
Loading