diff --git a/CHANGELOG.md b/CHANGELOG.md index fe1aa4e..e624938 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -15,6 +15,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ### Fixed +- **`bug-println` treats a binary crate as program code (#682).** Every module of a package that has `src/main.rs` and no `src/lib.rs` is exempt (stdout is its interface), not only `main.rs`, `src/bin/`, `examples/` and `benches/`. A library package, or one with both `main.rs` and `lib.rs`, keeps the rule, so a core crate can still guard against writing to stdout. Together with the `eprintln!` and `.expect("...")` changes this closes #682. - **`--help` no longer prints `CORA_BASE_URL`, `CORA_MODEL` or `CORA_PROVIDER` values (#687).** A base URL can carry credentials (`user:pass@host`, `?key=...`) and the other two name internal deployments; like the API key, their `[env: ...]` values are hidden (clap `hide_env_values`). - **`cora scan` is quieter and says what it found (#683).** The `qual-clone` hint (`.clone()` in Rust, 306 info findings on one workspace) is now opt-in via `rules_engine.clone_hints: true`. New `cora scan --min-severity ` hides lower findings in every output format and in the count (blocking is still decided by `hook.min_severity`), and the summary counts by severity: `found 457 issues (1 critical, 150 minor, 306 info)`. - **Models that reject `temperature` work (#701).** cora always sent `temperature` (default `0.0`); newer models such as `claude-opus-5-5` answer HTTP 400 "`temperature` is deprecated for this model" and the review ended as "could not complete". A 400 that names `temperature` is now retried once without it and remembered for the model for the rest of the process; every other 400 is returned unchanged, and providers that accept it keep the configured value. diff --git a/src/commands/scan.rs b/src/commands/scan.rs index 8650615..2aa4e04 100644 --- a/src/commands/scan.rs +++ b/src/commands/scan.rs @@ -93,6 +93,12 @@ pub async fn execute_scan( anyhow::bail!("scan path '{}' is not a directory", root.display()); } + // Scanned paths are relative to `root`, which need not be the working + // directory (`--path`): rules that look at the project layout use it (#682). + let mut rooted = config.clone(); + rooted.rules_config.project_root = Some(root.canonicalize().unwrap_or_else(|_| root.clone())); + let config = &rooted; + // Merge include/exclude with config ignore patterns let include = opts.include.clone(); let mut exclude = config.ignore.files.clone(); diff --git a/src/engine/rules/builtin.rs b/src/engine/rules/builtin.rs index 1441d9a..26b151f 100644 --- a/src/engine/rules/builtin.rs +++ b/src/engine/rules/builtin.rs @@ -610,9 +610,9 @@ fn is_invariant_expect(line: &str) -> bool { } /// `println!` is the intended output of a program's entry point: `main.rs`, -/// `src/bin/`, `examples/`, `benches/` (#663). Other files keep the rule; a -/// crate's own `commands/` modules can be silenced with `ignore.rules` or -/// `bug-println` in a profile. +/// `src/bin/`, `examples/`, `benches/` (#663). Every module of a binary-only +/// package is exempt as well, but that needs the project root, so the rule +/// engine checks it ([`is_in_binary_only_package`], #682). fn is_cli_output_path(path: &str) -> bool { let p = path.replace('\\', "/"); p.rsplit('/').next() == Some("main.rs") @@ -620,6 +620,20 @@ fn is_cli_output_path(path: &str) -> bool { .any(|seg| matches!(seg, "bin" | "examples" | "benches")) } +/// Is `p` (relative to `base`, the project root) under a `/src/` +/// whose package has `src/main.rs` and no `src/lib.rs`? In such a package +/// stdout is the program's interface (#682). +pub fn is_in_binary_only_package(base: &std::path::Path, p: &str) -> bool { + let p = p.replace('\\', "/"); + let segments: Vec<&str> = p.split('/').collect(); + let Some(src_idx) = segments.iter().position(|seg| *seg == "src") else { + return false; + }; + let package: std::path::PathBuf = segments[..src_idx].iter().collect(); + let src_dir = base.join(package).join("src"); + src_dir.join("main.rs").is_file() && !src_dir.join("lib.rs").is_file() +} + /// Check if an `http://` URL match is a false positive. /// /// Suppresses: XML/SVG namespaces, Docker internal hostnames, comment/docstring lines, @@ -1743,4 +1757,48 @@ mod tests { let line = r#"query += " AND name = '" + name + "'""#; assert!(!post_match_filter_for_path("sec-sql-concat", line, "a.py")); } + + #[test] + fn every_module_of_a_binary_only_package_is_program_code() { + let d = tempfile::tempdir().unwrap(); + let w = |rel: &str| { + let p = d.path().join(rel); + std::fs::create_dir_all(p.parent().unwrap()).unwrap(); + std::fs::write(p, "").unwrap(); + }; + w("crates/cli/src/main.rs"); + w("crates/cli/src/commands/review.rs"); + w("crates/core/src/lib.rs"); + w("crates/core/src/engine.rs"); + w("crates/both/src/main.rs"); + w("crates/both/src/lib.rs"); + w("crates/both/src/util.rs"); + w("src/main.rs"); + w("src/cmd.rs"); + let b = d.path(); + assert!(is_in_binary_only_package( + b, + "crates/cli/src/commands/review.rs" + )); + assert!( + is_in_binary_only_package(b, "src/cmd.rs"), + "package at the root" + ); + assert!( + !is_in_binary_only_package(b, "crates/core/src/engine.rs"), + "library" + ); + assert!( + !is_in_binary_only_package(b, "crates/both/src/util.rs"), + "lib + bin keeps the rule" + ); + assert!( + !is_in_binary_only_package(b, "crates/unknown/src/a.rs"), + "no package files" + ); + assert!( + !is_in_binary_only_package(b, "docs/readme.md"), + "outside src/" + ); + } } diff --git a/src/engine/rules/mod.rs b/src/engine/rules/mod.rs index c610d45..01ddc84 100644 --- a/src/engine/rules/mod.rs +++ b/src/engine/rules/mod.rs @@ -90,6 +90,26 @@ pub fn validate_custom_rules(rules: &[CustomRule]) -> Vec { errs } +/// The git toplevel of the working directory: diff paths are relative to it. +/// Unit tests run inside this repository, so they never probe it. +fn git_toplevel() -> Option { + if cfg!(test) { + return None; + } + static TOPLEVEL: std::sync::OnceLock> = std::sync::OnceLock::new(); + TOPLEVEL + .get_or_init(|| { + let out = std::process::Command::new("git") + .args(["rev-parse", "--show-toplevel"]) + .output() + .ok()?; + out.status + .success() + .then(|| std::path::PathBuf::from(String::from_utf8_lossy(&out.stdout).trim())) + }) + .clone() +} + pub fn run_rules_in(files: &[ScanFile<'_>], config: &RulesConfig) -> Vec { if !config.enabled { debug!("rule engine is disabled"); @@ -108,6 +128,8 @@ pub fn run_rules_in(files: &[ScanFile<'_>], config: &RulesConfig) -> Vec], config: &RulesConfig) -> Vec, } /// Default skip patterns for index scanners — common false-positive sources. @@ -78,6 +83,7 @@ impl Default for RulesConfig { index_skip_files: default_index_skip_files(), include_tests: false, clone_hints: false, + project_root: None, } } } diff --git a/src/mcp/agent_tools.rs b/src/mcp/agent_tools.rs index a2638db..33a3d14 100644 --- a/src/mcp/agent_tools.rs +++ b/src/mcp/agent_tools.rs @@ -412,10 +412,12 @@ pub fn scan_prepare(params: &Value) -> ToolResult { Ok(r) => r, Err(e) => return e, }; - let config = match load_project_config() { + let mut config = match load_project_config() { Ok(c) => c, Err(e) => return ToolResult::error(format!("Failed to load config: {e}")), }; + // Scanned paths are relative to the scan root (#682). + config.rules_config.project_root = Some(root.clone()); let files = match scan_files_for(&config, &root) { Ok(f) => f, Err(e) => return e, @@ -536,10 +538,12 @@ pub fn scan_finalize(params: &Value) -> ToolResult { Ok(v) => v, Err(e) => return ToolResult::error(e), }; - let config = match load_project_config() { + let mut config = match load_project_config() { Ok(c) => c, Err(e) => return ToolResult::error(format!("Failed to load config: {e}")), }; + // Scanned paths are relative to the scan root (#682). + config.rules_config.project_root = Some(root.clone()); let files = match scan_files_for(&config, &root) { Ok(f) => f, Err(e) => return e, diff --git a/tests/binary_crate_println.rs b/tests/binary_crate_println.rs new file mode 100644 index 0000000..bbca7bd --- /dev/null +++ b/tests/binary_crate_println.rs @@ -0,0 +1,69 @@ +//! `bug-println` treats every module of a binary-only package as program code, +//! while a library package keeps the rule (#682). +use std::path::Path; +use std::process::Command; + +fn layout(root: &Path) { + for (file, body) in [ + ("crates/cli/src/main.rs", "mod commands;\nfn main() {}\n"), + ( + "crates/cli/src/commands.rs", + "pub fn run() { println!(\"out\"); }\n", + ), + ( + "crates/core/src/lib.rs", + "pub fn run() { println!(\"leak\"); }\n", + ), + ( + "crates/core/src/engine.rs", + "pub fn go() { eprintln!(\"diag\"); }\n", + ), + ] { + let p = root.join(file); + std::fs::create_dir_all(p.parent().unwrap()).unwrap(); + std::fs::write(p, body).unwrap(); + } +} + +fn scan(cwd: &Path, extra: &[&str]) -> String { + let out = Command::new(env!("CARGO_BIN_EXE_cora")) + .args(["scan", "--no-llm", "--format", "compact"]) + .args(extra) + .current_dir(cwd) + .env_clear() + .env("PATH", "/usr/bin:/bin") + .env("HOME", cwd) + .output() + .unwrap(); + String::from_utf8_lossy(&out.stdout).to_string() + &String::from_utf8_lossy(&out.stderr) +} + +fn assert_expected(text: &str) { + assert!( + text.contains("crates/core/src/lib.rs"), + "library println must fire: {text}" + ); + assert!( + !text.contains("crates/cli/src/commands.rs"), + "binary module must not: {text}" + ); + assert!( + !text.contains("crates/core/src/engine.rs"), + "eprintln is never flagged: {text}" + ); +} + +#[test] +fn println_is_flagged_in_a_library_package_but_not_in_a_binary_only_one() { + let d = tempfile::tempdir().unwrap(); + layout(d.path()); + assert_expected(&scan(d.path(), &[])); +} + +#[test] +fn the_package_probe_uses_the_scan_root_not_the_working_directory() { + // `cora scan --path proj` from the parent: paths are relative to `proj`. + let d = tempfile::tempdir().unwrap(); + layout(&d.path().join("proj")); + assert_expected(&scan(d.path(), &["--path", "proj"])); +}