From 068058ab6bf04aac0323d01a5d6ffe5705efba99 Mon Sep 17 00:00:00 2001 From: ajianaz Date: Thu, 15 Oct 2026 13:37:54 +0700 Subject: [PATCH] fix(mcp): report capped deterministic findings to the agent (not_shown) review_prepare listed all deterministic findings but review_finalize showed only rules_engine.max_findings of them with no explanation, found by running a real agent on a real project. prepare now reports the total and the cap; finalize, scan_finalize and review_diff report not_shown. Co-Authored-By: Claude Sonnet 5.5 Signed-off-by: ajianaz --- CHANGELOG.md | 2 ++ src/mcp/agent_tools.rs | 52 ++++++++++++++++++++++++++++++++++++++++++ src/mcp/tools.rs | 1 + 3 files changed, 55 insertions(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index be978fb..706be67 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -39,6 +39,8 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ### Fixed +- **Agent-mode review no longer hides capped findings without saying so.** `cora.review_prepare` listed every deterministic finding but `cora.review_finalize` showed only `rules_engine.max_findings` (default 5) of them with no explanation; found by running a real agent on a real project. `review_prepare` now reports `deterministic_findings_total` and the cap, and `review_finalize` / `scan_finalize` / `cora.review_diff` report `not_shown` (with a note). + - **The MCP `cora.review_diff` tool no longer crashes the server (#647).** Every call panicked with "Cannot start a runtime from within a runtime" because the handler blocked on a second runtime from a tokio runtime thread. Handlers now go through `block_in_place`. - **`cora scan` now runs the rules engine (#645).** Built-in rules (`bug-todo`, `sec-sql-concat`, ...) and custom `rules_engine.custom_rules` were only run by `cora review`; scan silently skipped them. Expect more findings from `cora scan` after upgrading (e.g. `bug-todo` on every TODO), still under the `rules_engine.max_findings` cap (default 5); `rules_engine.enabled: false` turns them off. One set of deterministic families (`deterministic::run_files` for whole files, `deterministic::run` for diffs) now serves both, and the MCP `cora.check_snippet` tool picks up rules too. Scan also has a judge seam (`judge::ScanJudge`, with `HttpScanJudge` and `NoJudge`) so the batch loop is tested without a network, and `DEFAULT_BATCH_TOKEN_BUDGET` is renamed `DEFAULT_BATCH_CHAR_BUDGET` (it was always a character count). `docs/cli-reference.md` now states which review flags scan does not have. diff --git a/src/mcp/agent_tools.rs b/src/mcp/agent_tools.rs index 444d938..1384c1b 100644 --- a/src/mcp/agent_tools.rs +++ b/src/mcp/agent_tools.rs @@ -157,6 +157,18 @@ fn parse_agent_issues(value: Option<&Value>) -> Result<(Vec, usize) Ok((ok, rejected)) } +/// Tells the agent when `rules_engine.max_findings` hid deterministic findings +/// that `*_prepare` listed, instead of letting them vanish unexplained. +fn not_shown_note(dropped: usize, config: &crate::config::schema::Config) -> Value { + if dropped == 0 { + return Value::Null; + } + Value::String(format!( + "{dropped} more deterministic findings are not shown (rules_engine.max_findings = {}; raise it or set 0 to show all).", + config.rules_config.max_findings + )) +} + fn diff_param(params: &Value) -> Result<&str, ToolResult> { let diff = params .get("diff") @@ -215,7 +227,12 @@ pub fn review_prepare(params: &Value) -> ToolResult { "system_prompt": packet.prompts.system, "user_prompt": packet.prompts.user, "valid_files": packet.valid_files, + "deterministic_findings_total": deterministic.len(), "deterministic_findings": deterministic, + "deterministic_findings_note": format!( + "cora.review_finalize shows at most rules_engine.max_findings = {} deterministic findings (most severe first) and reports the rest as `not_shown`.", + config.rules_config.max_findings + ), "response_schema": response_schema(), })) } @@ -287,6 +304,8 @@ pub fn review_finalize(params: &Value) -> ToolResult { "should_block": should_block, "rejected_items": rejected, "dropped_by_validation": dropped_by_validation, + "not_shown": response.dropped_findings, + "not_shown_note": not_shown_note(response.dropped_findings, &config), "issues": response.issues.iter().map(|i| json!({ "title": i.title, "severity": i.severity.label(), @@ -506,6 +525,7 @@ pub fn scan_finalize(params: &Value) -> ToolResult { "rejected_items": rejected, "dropped_by_validation": dropped_by_validation, "not_shown": dropped, + "not_shown_note": not_shown_note(dropped, &config), "issues": all.iter().map(|i| json!({ "title": i.title, "severity": i.severity.label(), @@ -773,4 +793,36 @@ mod tests { "+def add(a, b):\n", "+ return a + b\n", ); + + #[test] + fn capped_deterministic_findings_are_reported_not_silently_hidden() { + // 7 TODO comments, default cap 5: prepare lists all, finalize says 2 are hidden. + let mut diff = String::from(concat!( + "diff --git a/lib.rs b/lib.rs\n", + "new file mode 100644\n", + "--- /dev/null\n", + "+++ b/lib.rs\n", + "@@ -0,0 +1,7 @@\n", + )); + for n in 0..7 { + diff.push_str(&format!("+// TODO item {n}\n")); + } + let prep = as_json(&review_prepare(&json!({ "diff": diff }))); + assert_eq!(prep["deterministic_findings_total"], 7); + assert!( + prep["deterministic_findings_note"] + .as_str() + .unwrap() + .contains("not_shown") + ); + let out = as_json(&review_finalize(&json!({ "diff": diff, "issues": [] }))); + assert_eq!(out["total_issues"], 5); + assert_eq!(out["not_shown"], 2); + assert!( + out["not_shown_note"] + .as_str() + .unwrap() + .contains("max_findings") + ); + } } diff --git a/src/mcp/tools.rs b/src/mcp/tools.rs index 9b16163..a56e1f1 100644 --- a/src/mcp/tools.rs +++ b/src/mcp/tools.rs @@ -824,6 +824,7 @@ fn handle_review_diff(params: &serde_json::Value) -> ToolResult { "summary": response.summary, "total_issues": response.issues.len(), "should_block": should_block, + "not_shown": response.dropped_findings, "issues": response.issues.iter().map(|i| serde_json::json!({ "title": i.title, "severity": i.severity.label(),