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
1 change: 1 addition & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -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 <info|minor|major|critical>` 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.
Expand Down
6 changes: 6 additions & 0 deletions src/commands/scan.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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();
Expand Down
64 changes: 61 additions & 3 deletions src/engine/rules/builtin.rs
Original file line number Diff line number Diff line change
Expand Up @@ -610,16 +610,30 @@ 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")
|| p.split('/')
.any(|seg| matches!(seg, "bin" | "examples" | "benches"))
}

/// Is `p` (relative to `base`, the project root) under a `<package>/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,
Expand Down Expand Up @@ -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/"
);
}
}
30 changes: 29 additions & 1 deletion src/engine/rules/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -90,6 +90,26 @@ pub fn validate_custom_rules(rules: &[CustomRule]) -> Vec<String> {
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<std::path::PathBuf> {
if cfg!(test) {
return None;
}
static TOPLEVEL: std::sync::OnceLock<Option<std::path::PathBuf>> = 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<RuleFinding> {
if !config.enabled {
debug!("rule engine is disabled");
Expand All @@ -108,6 +128,8 @@ pub fn run_rules_in(files: &[ScanFile<'_>], config: &RulesConfig) -> Vec<RuleFin
rule.ensure_compiled();
}

let root = config.project_root.clone().or_else(git_toplevel);

debug!(
rule_count = all_rules.len(),
"running rules against diff chunks"
Expand Down Expand Up @@ -144,7 +166,12 @@ pub fn run_rules_in(files: &[ScanFile<'_>], config: &RulesConfig) -> Vec<RuleFin
}

// Post-match filter (e.g., allow localhost URLs)
if builtin::post_match_filter_for_path(&rule.id, content, file_path) {
if builtin::post_match_filter_for_path(&rule.id, content, file_path)
|| (rule.id == "bug-println"
&& root
.as_deref()
.is_some_and(|r| builtin::is_in_binary_only_package(r, file_path)))
{
continue;
}

Expand Down Expand Up @@ -442,6 +469,7 @@ mod tests {
index_skip_files: super::types::default_index_skip_files(),
include_tests: false,
clone_hints: false,
project_root: None,
}
}

Expand Down
6 changes: 6 additions & 0 deletions src/engine/rules/types.rs
Original file line number Diff line number Diff line change
Expand Up @@ -28,6 +28,11 @@ pub struct RulesConfig {
pub include_tests: bool,
/// Run the noisy `qual-clone` hint (`.clone()` in Rust). Off by default (#683).
pub clone_hints: bool,
/// Root the scanned paths are relative to (set by `cora scan` and the MCP
/// scan tools; `None` = the git toplevel of the working directory). Used to
/// find the package a file belongs to (#682). Not configurable.
#[serde(skip)]
pub project_root: Option<std::path::PathBuf>,
}

/// Default skip patterns for index scanners — common false-positive sources.
Expand Down Expand Up @@ -78,6 +83,7 @@ impl Default for RulesConfig {
index_skip_files: default_index_skip_files(),
include_tests: false,
clone_hints: false,
project_root: None,
}
}
}
Expand Down
8 changes: 6 additions & 2 deletions src/mcp/agent_tools.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand Down Expand Up @@ -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,
Expand Down
69 changes: 69 additions & 0 deletions tests/binary_crate_println.rs
Original file line number Diff line number Diff line change
@@ -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"]));
}
Loading