test: 14 new tests closing coverage gaps#49
Merged
kienbui1995 merged 6 commits intomainfrom Apr 13, 2026
Merged
Conversation
New tests: - Learned permissions: learn_allow, is_learned, auto-allow after approve (3) - History search: Ctrl+R search with case-insensitive match (1) - FTS: actual search match, unicode safety, multi-session (2 unit + 1 integration) - Smart compaction: importance scoring keeps errors/instructions (1) - ResponseFormat: json_object, json_schema, none omitted (3) - Quit confirmation: pending_quit double-tap flow (1) - Debug tool: all actions validated (1 integration) - Auto-skill: threshold + generate (1 integration) 201 → 215 tests, 0 fail.
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds extensive test coverage across multiple crates (mc-core, mc-provider, mc-tools, mc-tui), introducing unit and integration tests for session compaction, full‑text search, provider serialization/streaming, permissions, TUI behaviors, and related utilities. No public APIs or production signatures changed. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@mc/crates/mc-core/src/compact.rs`:
- Around line 345-373: The test function smart_compact_keeps_important_messages
(and the other standalone tests around lines 284–343) must be moved into a
#[cfg(test)] mod tests { ... } block at the bottom of the file; wrap these test
functions inside that module, add use super::* at the top of the module so
Session, compact_session, ConversationMessage, and Block are in scope, and keep
the #[test] attribute on each test function unchanged so they run as unit tests.
- Around line 370-372: The test's assertions around compact_session are too
weak; update them to assert the exact expected sequence and content: check
session.messages.len() == 5 (summary + 2 important + 2 recent), verify
session.messages[0].contains_text("compacted") for the summary, assert that the
two important messages (the user instruction and the error message) are present
and preserved at the expected positions (e.g., session.messages[1] and
session.messages[2]) by using their .contains_text() checks, and finally verify
the last two messages are the recent messages in the correct order to ensure
importance scoring preserved both content and order.
In `@mc/crates/mc-core/src/fts.rs`:
- Around line 116-117: Replace the temp dir name that uses only
std::process::id() with a collision-safe name that includes a global atomic
counter: add a static AtomicUsize (e.g., static TEST_DIR_COUNTER: AtomicUsize =
AtomicUsize::new(0)) and call TEST_DIR_COUNTER.fetch_add(1, Ordering::SeqCst) to
produce a unique suffix, then build the temp path with both the PID and the
atomic counter (e.g., format!("mc-fts2-{}-{}", std::process::id(),
counter_val)). Apply this change to the two places that create the temp dir (the
statements creating dir with std::env::temp_dir().join(format!("mc-fts2-{}",
std::process::id())) at the shown locations).
In `@mc/crates/mc-core/tests/integration.rs`:
- Around line 390-391: The temp directory name uses only the PID and can
collide; introduce a static AtomicUsize counter (e.g., static TEST_DIR_COUNTER:
AtomicUsize = AtomicUsize::new(0)) and fetch_add(1, Ordering::SeqCst) to
generate a unique suffix, then construct the dir using both PID and counter
(replace the existing format!("mc-fts-int-{}", std::process::id()) with
something like format!("mc-fts-int-{}-{}", std::process::id(), counter_val))
where the variable dir is created, and keep the
std::fs::create_dir_all(&dir).unwrap() call.
In `@mc/crates/mc-provider/src/generic.rs`:
- Around line 433-485: The three standalone test functions
(response_format_json_mode, response_format_json_schema,
response_format_none_omitted) must be moved into a #[cfg(test)] mod tests { ...
} block (or into the file's existing test module) so they run as unit tests per
repo rules; wrap them inside mod tests and import the parent scope with use
super::*; ensure each function keeps its #[test] attribute and still calls
build_request_body(&req) and references CompletionRequest and
crate::types::ResponseFormat as before.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 31bd4742-ba20-41fd-bee2-e3a56b572799
📒 Files selected for processing (7)
mc/crates/mc-core/src/compact.rsmc/crates/mc-core/src/fts.rsmc/crates/mc-core/tests/integration.rsmc/crates/mc-provider/src/generic.rsmc/crates/mc-tools/src/permissions.rsmc/crates/mc-tui/src/app.rsmc/crates/mc-tui/src/history.rs
Comment on lines
+345
to
+373
| #[test] | ||
| fn smart_compact_keeps_important_messages() { | ||
| let mut session = Session::default(); | ||
| // Add user instruction (important, score 2.0) | ||
| session.messages.push(ConversationMessage::user( | ||
| "Please fix the auth bug in login.rs with detailed error handling", | ||
| )); | ||
| // Add tool error (important, score 3.0) | ||
| let mut err_msg = ConversationMessage::assistant("trying fix"); | ||
| err_msg.push_block(Block::ToolResult { | ||
| tool_use_id: "1".into(), | ||
| name: "bash".into(), | ||
| output: "error: compilation failed".into(), | ||
| is_error: true, | ||
| }); | ||
| session.messages.push(err_msg); | ||
| // Add 8 filler messages | ||
| for i in 0..8 { | ||
| session | ||
| .messages | ||
| .push(ConversationMessage::assistant(format!("filler {i}"))); | ||
| } | ||
| // 10 total, compact keeping 2 recent | ||
| compact_session(&mut session, 2); | ||
| // Should have: summary + some important kept + 2 recent | ||
| assert!(session.messages.len() >= 3); // at least summary + 2 recent | ||
| // Summary should exist | ||
| assert!(session.messages[0].contains_text("compacted")); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Move test into #[cfg(test)] mod tests block.
This test is placed outside the mod tests block that ends at line 282. As per coding guidelines, unit tests must use #[cfg(test)] mod tests at bottom of each file. The same issue exists for tests at lines 284-343.
♻️ Proposed fix to move all standalone tests into the `mod tests` block
assert!(session.messages[4]
.blocks
.iter()
.any(|b| matches!(b, Block::Thinking { .. })));
}
-}
-#[test]
-fn estimate_tokens_empty() {
+
+ #[test]
+ fn estimate_tokens_empty() {
- let session = Session::default();
- assert_eq!(estimate_tokens(&session), 0);
-}
+ let session = Session::default();
+ assert_eq!(estimate_tokens(&session), 0);
+ }
-#[test]
-fn micro_compact_trims_long_output() {
+ #[test]
+ fn micro_compact_trims_long_output() {
- let mut session = Session::default();
- session.messages.push(ConversationMessage::tool_result(
- "t1",
- "bash",
- "x".repeat(3000),
- false,
- ));
- micro_compact(&mut session);
- if let Block::ToolResult { output, .. } = &session.messages[0].blocks[0] {
- assert!(output.len() < 3000);
- assert!(output.contains("trimmed"));
+ let mut session = Session::default();
+ session.messages.push(ConversationMessage::tool_result(
+ "t1",
+ "bash",
+ "x".repeat(3000),
+ false,
+ ));
+ micro_compact(&mut session);
+ if let Block::ToolResult { output, .. } = &session.messages[0].blocks[0] {
+ assert!(output.len() < 3000);
+ assert!(output.contains("trimmed"));
+ }
}
-}
-#[test]
-fn collapse_reads_shrinks_large_output() {
+ #[test]
+ fn collapse_reads_shrinks_large_output() {
- let mut session = Session::default();
- session.messages.push(ConversationMessage::tool_result(
- "t1",
- "read_file",
- "line\n".repeat(500),
- false,
- ));
- collapse_reads(&mut session);
- if let Block::ToolResult { output, .. } = &session.messages[0].blocks[0] {
- assert!(output.contains("read_file output"));
- assert!(output.len() < 200);
+ let mut session = Session::default();
+ session.messages.push(ConversationMessage::tool_result(
+ "t1",
+ "read_file",
+ "line\n".repeat(500),
+ false,
+ ));
+ collapse_reads(&mut session);
+ if let Block::ToolResult { output, .. } = &session.messages[0].blocks[0] {
+ assert!(output.contains("read_file output"));
+ assert!(output.len() < 200);
+ }
}
-}
-#[test]
-fn snip_thinking_removes_old() {
+ #[test]
+ fn snip_thinking_removes_old() {
- let mut session = Session::default();
- for _ in 0..5 {
- let mut msg = ConversationMessage::assistant("text");
- msg.push_block(Block::Thinking {
- text: "deep thought".into(),
- });
- session.messages.push(msg);
- }
- snip_thinking(&mut session, 2);
- // First 3 messages should have thinking removed
- assert!(!session.messages[0]
- .blocks
- .iter()
- .any(|b| matches!(b, Block::Thinking { .. })));
- // Last 2 should keep thinking
- assert!(session.messages[4]
- .blocks
- .iter()
- .any(|b| matches!(b, Block::Thinking { .. })));
-}
+ let mut session = Session::default();
+ for _ in 0..5 {
+ let mut msg = ConversationMessage::assistant("text");
+ msg.push_block(Block::Thinking {
+ text: "deep thought".into(),
+ });
+ session.messages.push(msg);
+ }
+ snip_thinking(&mut session, 2);
+ // First 3 messages should have thinking removed
+ assert!(!session.messages[0]
+ .blocks
+ .iter()
+ .any(|b| matches!(b, Block::Thinking { .. })));
+ // Last 2 should keep thinking
+ assert!(session.messages[4]
+ .blocks
+ .iter()
+ .any(|b| matches!(b, Block::Thinking { .. })));
+ }
-#[test]
-fn smart_compact_keeps_important_messages() {
+ #[test]
+ fn smart_compact_keeps_important_messages() {
- let mut session = Session::default();
- // Add user instruction (important, score 2.0)
- session.messages.push(ConversationMessage::user(
- "Please fix the auth bug in login.rs with detailed error handling",
- ));
- // Add tool error (important, score 3.0)
- let mut err_msg = ConversationMessage::assistant("trying fix");
- err_msg.push_block(Block::ToolResult {
- tool_use_id: "1".into(),
- name: "bash".into(),
- output: "error: compilation failed".into(),
- is_error: true,
- });
- session.messages.push(err_msg);
- // Add 8 filler messages
- for i in 0..8 {
- session
- .messages
- .push(ConversationMessage::assistant(format!("filler {i}")));
- }
- // 10 total, compact keeping 2 recent
- compact_session(&mut session, 2);
- // Should have: summary + some important kept + 2 recent
- assert!(session.messages.len() >= 3); // at least summary + 2 recent
- // Summary should exist
- assert!(session.messages[0].contains_text("compacted"));
+ let mut session = Session::default();
+ // Add user instruction (important, score 2.0)
+ session.messages.push(ConversationMessage::user(
+ "Please fix the auth bug in login.rs with detailed error handling",
+ ));
+ // Add tool error (important, score 3.0)
+ let mut err_msg = ConversationMessage::assistant("trying fix");
+ err_msg.push_block(Block::ToolResult {
+ tool_use_id: "1".into(),
+ name: "bash".into(),
+ output: "error: compilation failed".into(),
+ is_error: true,
+ });
+ session.messages.push(err_msg);
+ // Add 8 filler messages
+ for i in 0..8 {
+ session
+ .messages
+ .push(ConversationMessage::assistant(format!("filler {i}")));
+ }
+ // 10 total, compact keeping 2 recent
+ compact_session(&mut session, 2);
+ // Should have: summary + some important kept + 2 recent
+ assert!(session.messages.len() >= 3);
+ // Summary should exist
+ assert!(session.messages[0].contains_text("compacted"));
+ }
}As per coding guidelines: "Unit tests must use #[cfg(test)] mod tests at bottom of each file"
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@mc/crates/mc-core/src/compact.rs` around lines 345 - 373, The test function
smart_compact_keeps_important_messages (and the other standalone tests around
lines 284–343) must be moved into a #[cfg(test)] mod tests { ... } block at the
bottom of the file; wrap these test functions inside that module, add use
super::* at the top of the module so Session, compact_session,
ConversationMessage, and Block are in scope, and keep the #[test] attribute on
each test function unchanged so they run as unit tests.
Comment on lines
+370
to
+372
| assert!(session.messages.len() >= 3); // at least summary + 2 recent | ||
| // Summary should exist | ||
| assert!(session.messages[0].contains_text("compacted")); |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Strengthen test assertions to verify importance scoring behavior.
The current assertions are too weak to verify that compact_session actually preserves important messages:
session.messages.len() >= 3doesn't verify the exact expected count (should be 5: summary + 2 important + 2 recent)- Only checks that a summary exists, not that the important messages (user instruction and error) were preserved
- Doesn't verify the content or order of preserved messages
🔍 Proposed assertions to verify importance scoring
// 10 total, compact keeping 2 recent
compact_session(&mut session, 2);
- // Should have: summary + some important kept + 2 recent
- assert!(session.messages.len() >= 3); // at least summary + 2 recent
- // Summary should exist
+
+ // Should have exactly: summary + 2 important + 2 recent = 5
+ assert_eq!(session.messages.len(), 5);
+ // Summary should exist at position 0
assert!(session.messages[0].contains_text("compacted"));
+ // Verify important messages were preserved
+ assert!(session
+ .messages
+ .iter()
+ .any(|m| m.contains_text("auth bug") && m.contains_text("login.rs")));
+ assert!(session.messages.iter().any(|m| m
+ .blocks
+ .iter()
+ .any(|b| matches!(b, Block::ToolResult { is_error: true, .. }))));
+ // Verify recent messages preserved
+ assert!(session.messages[3].contains_text("filler 6"));
+ assert!(session.messages[4].contains_text("filler 7"));
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| assert!(session.messages.len() >= 3); // at least summary + 2 recent | |
| // Summary should exist | |
| assert!(session.messages[0].contains_text("compacted")); | |
| // Should have exactly: summary + 2 important + 2 recent = 5 | |
| assert_eq!(session.messages.len(), 5); | |
| // Summary should exist at position 0 | |
| assert!(session.messages[0].contains_text("compacted")); | |
| // Verify important messages were preserved | |
| assert!(session | |
| .messages | |
| .iter() | |
| .any(|m| m.contains_text("auth bug") && m.contains_text("login.rs"))); | |
| assert!(session.messages.iter().any(|m| m | |
| .blocks | |
| .iter() | |
| .any(|b| matches!(b, Block::ToolResult { is_error: true, .. })))); | |
| // Verify recent messages preserved | |
| assert!(session.messages[3].contains_text("filler 6")); | |
| assert!(session.messages[4].contains_text("filler 7")); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@mc/crates/mc-core/src/compact.rs` around lines 370 - 372, The test's
assertions around compact_session are too weak; update them to assert the exact
expected sequence and content: check session.messages.len() == 5 (summary + 2
important + 2 recent), verify session.messages[0].contains_text("compacted") for
the summary, assert that the two important messages (the user instruction and
the error message) are present and preserved at the expected positions (e.g.,
session.messages[1] and session.messages[2]) by using their .contains_text()
checks, and finally verify the last two messages are the recent messages in the
correct order to ensure importance scoring preserved both content and order.
Comment on lines
+116
to
+117
| let dir = std::env::temp_dir().join(format!("mc-fts2-{}", std::process::id())); | ||
| std::fs::create_dir_all(&dir).unwrap(); |
There was a problem hiding this comment.
Use an atomic counter for temp test dirs (not only process::id()).
Both new tests create temp paths from PID only; repo test rules require an atomic suffix for collision-safe parallel runs.
As per coding guidelines, Use atomic counter for unique temp file paths in tests to avoid parallel test collisions.
Also applies to: 136-137
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@mc/crates/mc-core/src/fts.rs` around lines 116 - 117, Replace the temp dir
name that uses only std::process::id() with a collision-safe name that includes
a global atomic counter: add a static AtomicUsize (e.g., static
TEST_DIR_COUNTER: AtomicUsize = AtomicUsize::new(0)) and call
TEST_DIR_COUNTER.fetch_add(1, Ordering::SeqCst) to produce a unique suffix, then
build the temp path with both the PID and the atomic counter (e.g.,
format!("mc-fts2-{}-{}", std::process::id(), counter_val)). Apply this change to
the two places that create the temp dir (the statements creating dir with
std::env::temp_dir().join(format!("mc-fts2-{}", std::process::id())) at the
shown locations).
Comment on lines
+390
to
+391
| let dir = std::env::temp_dir().join(format!("mc-fts-int-{}", std::process::id())); | ||
| std::fs::create_dir_all(&dir).unwrap(); |
There was a problem hiding this comment.
Make integration temp dirs atomically unique.
At Line 390, the temp directory name is PID-based only; switch to PID + atomic counter to meet test-collision guidance.
As per coding guidelines, Use atomic counter for unique temp file paths in tests to avoid parallel test collisions.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@mc/crates/mc-core/tests/integration.rs` around lines 390 - 391, The temp
directory name uses only the PID and can collide; introduce a static AtomicUsize
counter (e.g., static TEST_DIR_COUNTER: AtomicUsize = AtomicUsize::new(0)) and
fetch_add(1, Ordering::SeqCst) to generate a unique suffix, then construct the
dir using both PID and counter (replace the existing format!("mc-fts-int-{}",
std::process::id()) with something like format!("mc-fts-int-{}-{}",
std::process::id(), counter_val)) where the variable dir is created, and keep
the std::fs::create_dir_all(&dir).unwrap() call.
Comment on lines
+433
to
+485
|
|
||
| #[test] | ||
| fn response_format_json_mode() { | ||
| let req = CompletionRequest { | ||
| model: "gpt-4".into(), | ||
| max_tokens: 100, | ||
| system_prompt: None, | ||
| messages: vec![], | ||
| tools: vec![], | ||
| tool_choice: None, | ||
| thinking_budget: None, | ||
| response_format: Some(crate::types::ResponseFormat::Json), | ||
| }; | ||
| let body = build_request_body(&req); | ||
| assert_eq!(body["response_format"]["type"], "json_object"); | ||
| } | ||
|
|
||
| #[test] | ||
| fn response_format_json_schema() { | ||
| let schema = serde_json::json!({"type": "object", "properties": {"name": {"type": "string"}}}); | ||
| let req = CompletionRequest { | ||
| model: "gpt-4".into(), | ||
| max_tokens: 100, | ||
| system_prompt: None, | ||
| messages: vec![], | ||
| tools: vec![], | ||
| tool_choice: None, | ||
| thinking_budget: None, | ||
| response_format: Some(crate::types::ResponseFormat::JsonSchema { | ||
| name: "test".into(), | ||
| schema: schema.clone(), | ||
| }), | ||
| }; | ||
| let body = build_request_body(&req); | ||
| assert_eq!(body["response_format"]["type"], "json_schema"); | ||
| assert_eq!(body["response_format"]["json_schema"]["name"], "test"); | ||
| } | ||
|
|
||
| #[test] | ||
| fn response_format_none_omitted() { | ||
| let req = CompletionRequest { | ||
| model: "gpt-4".into(), | ||
| max_tokens: 100, | ||
| system_prompt: None, | ||
| messages: vec![], | ||
| tools: vec![], | ||
| tool_choice: None, | ||
| thinking_budget: None, | ||
| response_format: None, | ||
| }; | ||
| let body = build_request_body(&req); | ||
| assert!(body.get("response_format").is_none()); | ||
| } |
There was a problem hiding this comment.
Move these tests into a #[cfg(test)] mod tests block.
At Line 433, the test module is closed before the newly added tests, so Line 434-485 defines unit tests at module root instead of inside the test module required by repo rules.
Suggested fix
-}
-
-#[test]
-fn response_format_json_mode() {
+ #[test]
+ fn response_format_json_mode() {
let req = CompletionRequest {
model: "gpt-4".into(),
max_tokens: 100,
system_prompt: None,
messages: vec![],
tools: vec![],
tool_choice: None,
thinking_budget: None,
response_format: Some(crate::types::ResponseFormat::Json),
};
let body = build_request_body(&req);
assert_eq!(body["response_format"]["type"], "json_object");
}
-#[test]
-fn response_format_json_schema() {
+ #[test]
+ fn response_format_json_schema() {
let schema = serde_json::json!({"type": "object", "properties": {"name": {"type": "string"}}});
let req = CompletionRequest {
model: "gpt-4".into(),
max_tokens: 100,
system_prompt: None,
messages: vec![],
tools: vec![],
tool_choice: None,
thinking_budget: None,
response_format: Some(crate::types::ResponseFormat::JsonSchema {
name: "test".into(),
schema: schema.clone(),
}),
};
let body = build_request_body(&req);
assert_eq!(body["response_format"]["type"], "json_schema");
assert_eq!(body["response_format"]["json_schema"]["name"], "test");
}
-#[test]
-fn response_format_none_omitted() {
+ #[test]
+ fn response_format_none_omitted() {
let req = CompletionRequest {
model: "gpt-4".into(),
max_tokens: 100,
system_prompt: None,
messages: vec![],
tools: vec![],
tool_choice: None,
thinking_budget: None,
response_format: None,
};
let body = build_request_body(&req);
assert!(body.get("response_format").is_none());
}
+}As per coding guidelines, Unit tests must use #[cfg(test)] mod tests at bottom of each file.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| #[test] | |
| fn response_format_json_mode() { | |
| let req = CompletionRequest { | |
| model: "gpt-4".into(), | |
| max_tokens: 100, | |
| system_prompt: None, | |
| messages: vec![], | |
| tools: vec![], | |
| tool_choice: None, | |
| thinking_budget: None, | |
| response_format: Some(crate::types::ResponseFormat::Json), | |
| }; | |
| let body = build_request_body(&req); | |
| assert_eq!(body["response_format"]["type"], "json_object"); | |
| } | |
| #[test] | |
| fn response_format_json_schema() { | |
| let schema = serde_json::json!({"type": "object", "properties": {"name": {"type": "string"}}}); | |
| let req = CompletionRequest { | |
| model: "gpt-4".into(), | |
| max_tokens: 100, | |
| system_prompt: None, | |
| messages: vec![], | |
| tools: vec![], | |
| tool_choice: None, | |
| thinking_budget: None, | |
| response_format: Some(crate::types::ResponseFormat::JsonSchema { | |
| name: "test".into(), | |
| schema: schema.clone(), | |
| }), | |
| }; | |
| let body = build_request_body(&req); | |
| assert_eq!(body["response_format"]["type"], "json_schema"); | |
| assert_eq!(body["response_format"]["json_schema"]["name"], "test"); | |
| } | |
| #[test] | |
| fn response_format_none_omitted() { | |
| let req = CompletionRequest { | |
| model: "gpt-4".into(), | |
| max_tokens: 100, | |
| system_prompt: None, | |
| messages: vec![], | |
| tools: vec![], | |
| tool_choice: None, | |
| thinking_budget: None, | |
| response_format: None, | |
| }; | |
| let body = build_request_body(&req); | |
| assert!(body.get("response_format").is_none()); | |
| } | |
| #[test] | |
| fn response_format_json_mode() { | |
| let req = CompletionRequest { | |
| model: "gpt-4".into(), | |
| max_tokens: 100, | |
| system_prompt: None, | |
| messages: vec![], | |
| tools: vec![], | |
| tool_choice: None, | |
| thinking_budget: None, | |
| response_format: Some(crate::types::ResponseFormat::Json), | |
| }; | |
| let body = build_request_body(&req); | |
| assert_eq!(body["response_format"]["type"], "json_object"); | |
| } | |
| #[test] | |
| fn response_format_json_schema() { | |
| let schema = serde_json::json!({"type": "object", "properties": {"name": {"type": "string"}}}); | |
| let req = CompletionRequest { | |
| model: "gpt-4".into(), | |
| max_tokens: 100, | |
| system_prompt: None, | |
| messages: vec![], | |
| tools: vec![], | |
| tool_choice: None, | |
| thinking_budget: None, | |
| response_format: Some(crate::types::ResponseFormat::JsonSchema { | |
| name: "test".into(), | |
| schema: schema.clone(), | |
| }), | |
| }; | |
| let body = build_request_body(&req); | |
| assert_eq!(body["response_format"]["type"], "json_schema"); | |
| assert_eq!(body["response_format"]["json_schema"]["name"], "test"); | |
| } | |
| #[test] | |
| fn response_format_none_omitted() { | |
| let req = CompletionRequest { | |
| model: "gpt-4".into(), | |
| max_tokens: 100, | |
| system_prompt: None, | |
| messages: vec![], | |
| tools: vec![], | |
| tool_choice: None, | |
| thinking_budget: None, | |
| response_format: None, | |
| }; | |
| let body = build_request_body(&req); | |
| assert!(body.get("response_format").is_none()); | |
| } | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@mc/crates/mc-provider/src/generic.rs` around lines 433 - 485, The three
standalone test functions (response_format_json_mode,
response_format_json_schema, response_format_none_omitted) must be moved into a
#[cfg(test)] mod tests { ... } block (or into the file's existing test module)
so they run as unit tests per repo rules; wrap them inside mod tests and import
the parent scope with use super::*; ensure each function keeps its #[test]
attribute and still calls build_request_body(&req) and references
CompletionRequest and crate::types::ResponseFormat as before.
Coverage: 41.83% → 45.03% (+3.2%) Tests: 215 → 258 (+43) New tests by module: - commands.rs: 25 tests (help, status, plan, theme, save, load, etc.) - app.rs: 7 tests (tool_call, stream_done, error, scroll, spinner, tab_complete, cap_output) - subagent.rs: 4 tests (permission inherit, budget, shared_context, msg_to_input) - permissions.rs: 4 tests (safe reads, safe bash, dangerous bash, compound) - web.rs: 4 tests (script strip, nested tags, empty, numeric entities) 258 tests, 0 fail.
There was a problem hiding this comment.
🧹 Nitpick comments (3)
mc/crates/mc-tools/src/web.rs (1)
242-248: Tighten the script/style test assertion to be exact.Line 245 currently uses
contains("text"), which can pass even if unwanted content leaks through. Prefer asserting exact normalized output for stronger regression protection.Proposed test tightening
#[test] fn strips_script_and_style() { let html = "<html><style>body{}</style><script>alert(1)</script><p>text</p></html>"; let result = strip_html(html); - assert!(result.contains("text")); + assert_eq!(result, "text"); assert!(!result.contains("alert")); assert!(!result.contains("body{}")); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@mc/crates/mc-tools/src/web.rs` around lines 242 - 248, The test strips_script_and_style should assert the exact normalized output of strip_html rather than using contains; update the test that calls strip_html(html) to compare result == "<p>text</p>" (or the normalized equivalent your strip_html returns) and also assert that result does not contain "alert" or "body{}", referencing the strips_script_and_style test and the strip_html function so the expectation is precise and prevents leakage of script/style text.mc/crates/mc-tui/src/commands.rs (2)
841-846: Strengthen/cleartest to verify actual reset behavior.Current assertion only checks for the “cleared” message, so it could pass even if prior content was not removed.
Proposed test hardening
#[test] fn clear_resets_output() { let mut app = App::new("test".into()); app.output_lines.push("old".into()); cmd(&mut app, "/clear"); - assert!(app.output_lines.iter().any(|l| l.contains("cleared"))); + assert!(!app.output_lines.iter().any(|l| l == "old")); + assert_eq!(app.output_lines.len(), 1); + assert!(app.output_lines[0].contains("Output cleared")); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@mc/crates/mc-tui/src/commands.rs` around lines 841 - 846, Update the test clear_resets_output to assert that prior output content is actually removed: create App via App::new, push a sentinel line (e.g. "old") into app.output_lines, call cmd(&mut app, "/clear"), then assert both that no line contains the original sentinel and that a "cleared" confirmation is present; use the existing identifiers (clear_resets_output, App::new, app.output_lines, cmd) to locate and modify the test.
986-994: Split mixed assertions into separate alias tests.
aliases_workvalidates two independent behaviors (/hand/q) in one test, which makes failures less diagnostic.Proposed split
-#[test] -fn aliases_work() { +#[test] +fn alias_h_maps_to_help() { let mut app = App::new("test".into()); cmd(&mut app, "/h"); assert!(app.output_lines.iter().any(|l| l.contains("Navigation"))); +} +#[test] +fn alias_q_maps_to_quit() { let mut app2 = App::new("test".into()); cmd(&mut app2, "/q"); assert!(app2.should_quit); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@mc/crates/mc-tui/src/commands.rs` around lines 986 - 994, The single test function aliases_work mixes two independent assertions; split it into two focused tests (e.g., aliases_help and aliases_quit): move the help assertion into a test that creates App::new("test".into()), calls cmd(&mut app, "/h"), and asserts the output contains "Navigation"; move the quit assertion into a separate test that creates its own App instance, calls cmd(&mut app2, "/q"), and asserts app2.should_quit; ensure each test is a standalone test function (with the test attribute) and uses its own App instance and cmd call so failures clearly indicate which alias broke.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@mc/crates/mc-tools/src/web.rs`:
- Around line 242-248: The test strips_script_and_style should assert the exact
normalized output of strip_html rather than using contains; update the test that
calls strip_html(html) to compare result == "<p>text</p>" (or the normalized
equivalent your strip_html returns) and also assert that result does not contain
"alert" or "body{}", referencing the strips_script_and_style test and the
strip_html function so the expectation is precise and prevents leakage of
script/style text.
In `@mc/crates/mc-tui/src/commands.rs`:
- Around line 841-846: Update the test clear_resets_output to assert that prior
output content is actually removed: create App via App::new, push a sentinel
line (e.g. "old") into app.output_lines, call cmd(&mut app, "/clear"), then
assert both that no line contains the original sentinel and that a "cleared"
confirmation is present; use the existing identifiers (clear_resets_output,
App::new, app.output_lines, cmd) to locate and modify the test.
- Around line 986-994: The single test function aliases_work mixes two
independent assertions; split it into two focused tests (e.g., aliases_help and
aliases_quit): move the help assertion into a test that creates
App::new("test".into()), calls cmd(&mut app, "/h"), and asserts the output
contains "Navigation"; move the quit assertion into a separate test that creates
its own App instance, calls cmd(&mut app2, "/q"), and asserts app2.should_quit;
ensure each test is a standalone test function (with the test attribute) and
uses its own App instance and cmd call so failures clearly indicate which alias
broke.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 99da29dd-12a2-4d72-b820-d5f6a33d4ad5
📒 Files selected for processing (6)
mc/crates/mc-core/src/subagent.rsmc/crates/mc-tools/src/permissions.rsmc/crates/mc-tools/src/web.rsmc/crates/mc-tui/src/app.rsmc/crates/mc-tui/src/commands.rsmc/tarpaulin-report.json
✅ Files skipped from review due to trivial changes (2)
- mc/crates/mc-core/src/subagent.rs
- mc/crates/mc-tools/src/permissions.rs
🚧 Files skipped from review as they are similar to previous changes (1)
- mc/crates/mc-tui/src/app.rs
Coverage: 45.03% → 48.14% (+3.1%) Tests: 258 → 270 (+12) New integration tests: - debug tool dispatch via MockProvider - edit_plan tool returns formatted plan - codebase_search with repo_map - memory_read/write via runtime - task_create dispatch - session.search() with multiple matches - anthropic wire format (tool_use, thinking, model_info) - model_registry provider coverage - request body structure validation 270 tests, 0 fail.
There was a problem hiding this comment.
♻️ Duplicate comments (1)
mc/crates/mc-core/tests/integration.rs (1)
390-391:⚠️ Potential issue | 🟠 MajorUse atomic suffixes for temp paths to avoid parallel-test collisions.
Line 390, Line 556, and Line 613 use PID-only names, which can collide across concurrently running tests/processes.
As per coding guidelines, `Use atomic counter for unique temp file paths in tests to avoid parallel test collisions`.🔧 Proposed fix (shared helper + atomic counter)
+use std::sync::atomic::{AtomicUsize, Ordering}; + +static TEST_DIR_COUNTER: AtomicUsize = AtomicUsize::new(0); + +fn unique_test_path(prefix: &str) -> std::path::PathBuf { + let n = TEST_DIR_COUNTER.fetch_add(1, Ordering::SeqCst); + std::env::temp_dir().join(format!("{prefix}-{}-{n}", std::process::id())) +} + #[test] fn fts_search_across_sessions() { - let dir = std::env::temp_dir().join(format!("mc-fts-int-{}", std::process::id())); + let dir = unique_test_path("mc-fts-int"); std::fs::create_dir_all(&dir).unwrap(); ... #[tokio::test] async fn codebase_search_with_repo_map() { ... - let tmp = std::env::temp_dir().join(format!("mc-repo-{}", std::process::id())); + let tmp = unique_test_path("mc-repo"); std::fs::create_dir_all(&tmp).unwrap(); ... #[tokio::test] async fn memory_read_write_via_runtime() { ... - let tmp = std::env::temp_dir().join(format!("mc-mem-{}", std::process::id())); + let tmp = unique_test_path("mc-mem");Also applies to: 556-557, 613-613
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@mc/crates/mc-core/tests/integration.rs` around lines 390 - 391, The temp directory name uses only the process ID (std::process::id()) causing collisions in parallel tests; update the three occurrences (the dir variable creation at the shown snippet and the similar sites at lines 556 and 613) to generate a unique suffix—either replace PID-only naming with a shared atomic counter (e.g., a static AtomicUsize that you fetch_add for each test) or use tempfile::TempDir/Builder to create an isolated temp directory; ensure the code that sets dir (the std::env::temp_dir().join(format!("mc-fts-int-{}"...)) expressions) is updated to include the atomic counter value (or the TempDir path) so names are unique across concurrent runs.
🧹 Nitpick comments (2)
mc/crates/mc-core/tests/integration.rs (2)
731-745: Consider consolidating model-registry coverage to reduce duplication.This overlaps with
model_registry_covers_all_providers; merging into one table-driven test will reduce maintenance drift while keeping coverage breadth.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@mc/crates/mc-core/tests/integration.rs` around lines 731 - 745, Two tests duplicate coverage; merge model_registry_covers_new_providers into the existing model_registry_covers_all_providers by making a single table-driven test that constructs ModelRegistry::default() and iterates over a combined list of provider model names, calling reg.context_window(model) and asserting ctx > 0 for each; update or remove the redundant test (model_registry_covers_new_providers) and keep the unique symbol references (ModelRegistry::default, ModelRegistry::context_window, and the test function name model_registry_covers_all_providers) so all providers are checked in one loop to avoid maintenance drift.
709-729: Test name overstates coverage; assertions are construction-only.
anthropic_request_body_structurecurrently validatesCompletionRequestfield assignment, not the provider wire payload shape. Consider either renaming to reflect scope or asserting the transformed outbound request in provider-layer tests.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@mc/crates/mc-core/tests/integration.rs` around lines 709 - 729, The test anthropic_request_body_structure only verifies local CompletionRequest construction but its name implies it validates the provider wire payload; rename the test to something like completion_request_construction or extend it to assert the provider-layer transformed request payload by invoking the function that builds the outbound Anthropics request (locate the provider request builder used for Claude, e.g., the method or function that maps CompletionRequest -> anthropic request struct) and assert expected fields (model, messages, tools serialization, system_prompt mapping) on that produced payload; alternatively, update the test name to reflect it only checks construction (CompletionRequest construction) if you don't add provider-layer assertions.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@mc/crates/mc-core/tests/integration.rs`:
- Around line 390-391: The temp directory name uses only the process ID
(std::process::id()) causing collisions in parallel tests; update the three
occurrences (the dir variable creation at the shown snippet and the similar
sites at lines 556 and 613) to generate a unique suffix—either replace PID-only
naming with a shared atomic counter (e.g., a static AtomicUsize that you
fetch_add for each test) or use tempfile::TempDir/Builder to create an isolated
temp directory; ensure the code that sets dir (the
std::env::temp_dir().join(format!("mc-fts-int-{}"...)) expressions) is updated
to include the atomic counter value (or the TempDir path) so names are unique
across concurrent runs.
---
Nitpick comments:
In `@mc/crates/mc-core/tests/integration.rs`:
- Around line 731-745: Two tests duplicate coverage; merge
model_registry_covers_new_providers into the existing
model_registry_covers_all_providers by making a single table-driven test that
constructs ModelRegistry::default() and iterates over a combined list of
provider model names, calling reg.context_window(model) and asserting ctx > 0
for each; update or remove the redundant test
(model_registry_covers_new_providers) and keep the unique symbol references
(ModelRegistry::default, ModelRegistry::context_window, and the test function
name model_registry_covers_all_providers) so all providers are checked in one
loop to avoid maintenance drift.
- Around line 709-729: The test anthropic_request_body_structure only verifies
local CompletionRequest construction but its name implies it validates the
provider wire payload; rename the test to something like
completion_request_construction or extend it to assert the provider-layer
transformed request payload by invoking the function that builds the outbound
Anthropics request (locate the provider request builder used for Claude, e.g.,
the method or function that maps CompletionRequest -> anthropic request struct)
and assert expected fields (model, messages, tools serialization, system_prompt
mapping) on that produced payload; alternatively, update the test name to
reflect it only checks construction (CompletionRequest construction) if you
don't add provider-layer assertions.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 02014c32-8312-489d-8895-d23e3b885f1f
📒 Files selected for processing (2)
mc/crates/mc-core/tests/integration.rsmc/crates/mc-provider/src/anthropic.rs
✅ Files skipped from review due to trivial changes (1)
- mc/crates/mc-provider/src/anthropic.rs
Add wiremock dev-dependency for HTTP-level provider testing: - streams_text_deltas: SSE text streaming - streams_tool_call: tool_use event parsing - reports_usage: token usage extraction - handles_empty_stream: graceful empty response - json_mode_sent_in_body: ResponseFormat wired to HTTP body Coverage: 48.14% → 50.00% (+1.9%) Tests: 270 → 275 (+5) 275 tests, 0 fail.
|
Review the following changes in direct dependencies. Learn more about Socket for GitHub.
|
There was a problem hiding this comment.
🧹 Nitpick comments (2)
mc/crates/mc-provider/tests/http_mock.rs (2)
25-35: Fail fast on provider stream errors in test helper.Line 34 silently breaks on
Err(_), which can mask failures and weaken test guarantees. In tests, this should fail immediately.Proposed fix
- Some(Err(_)) => break, + Some(Err(e)) => panic!("provider stream error: {e:?}"),🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@mc/crates/mc-provider/tests/http_mock.rs` around lines 25 - 35, In collect_events, don’t silently ignore errors from the ProviderStream; when matching Some(Err(_)) replace the silent break with a hard failure so tests fail fast—e.g., propagate or panic with the error message by matching Some(Err(e)) and calling panic! or unwrap on e; update the match arm in the collect_events function that handles ProviderStream items (ProviderEvent) so errors are surfaced immediately instead of breaking quietly.
128-134: Tighten the done-only assertion.Line 129 currently accepts
events.is_empty(), so the test can pass even if noMessageStopis emitted. Since this fixture sends[DONE], assertProviderEvent::MessageStopexplicitly.Proposed fix
- assert!( - events.is_empty() - || events - .iter() - .any(|e| matches!(e, ProviderEvent::MessageStop)) - ); + assert!(events + .iter() + .any(|e| matches!(e, ProviderEvent::MessageStop)));🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@mc/crates/mc-provider/tests/http_mock.rs` around lines 128 - 134, The test currently allows an empty events vector which can hide missing DONE handling; replace the permissive assertion with one that requires a MessageStop event by asserting that events.iter().any(|e| matches!(e, ProviderEvent::MessageStop)) is true (i.e., remove the events.is_empty() branch and assert explicitly that a ProviderEvent::MessageStop was emitted).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@mc/crates/mc-provider/tests/http_mock.rs`:
- Around line 25-35: In collect_events, don’t silently ignore errors from the
ProviderStream; when matching Some(Err(_)) replace the silent break with a hard
failure so tests fail fast—e.g., propagate or panic with the error message by
matching Some(Err(e)) and calling panic! or unwrap on e; update the match arm in
the collect_events function that handles ProviderStream items (ProviderEvent) so
errors are surfaced immediately instead of breaking quietly.
- Around line 128-134: The test currently allows an empty events vector which
can hide missing DONE handling; replace the permissive assertion with one that
requires a MessageStop event by asserting that events.iter().any(|e| matches!(e,
ProviderEvent::MessageStop)) is true (i.e., remove the events.is_empty() branch
and assert explicitly that a ProviderEvent::MessageStop was emitted).
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: ecff75c5-e193-4eb4-ac7e-70fb49ab75db
⛔ Files ignored due to path filters (1)
mc/Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (2)
mc/crates/mc-provider/Cargo.tomlmc/crates/mc-provider/tests/http_mock.rs
✅ Files skipped from review due to trivial changes (1)
- mc/crates/mc-provider/Cargo.toml
- 5 wiremock HTTP tests for GenericProvider (text, tool_call, usage, empty, json_mode) - 1 Gemini model_info test - 6 Anthropic unit tests (wire format, model_info for opus/haiku/sonnet, tool_use, thinking) - Removed Anthropic HTTP mock tests (wiremock single-chunk delivery incompatible with chunked SSE parser) Coverage: ~50%, 274 tests, 0 fail.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@mc/crates/mc-provider/tests/http_mock.rs`:
- Around line 34-37: The shared test helper currently swallows provider stream
errors in the match arm "Some(Err(e))" by logging and breaking, which can cause
false-positive tests; change that arm to fail fast instead by panicking or
propagating the error (e.g., replace the log+break with panic!("Stream error:
{:?}", e) or return Err(e) and adjust the helper to return a Result), so the
test harness sees the failure immediately rather than continuing on partial
data.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 3e187c9b-c264-4bc0-8046-3e8ac8ac2311
📒 Files selected for processing (1)
mc/crates/mc-provider/tests/http_mock.rs
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Coverage Gaps Closed
201 → 215 tests, 0 fail.
Summary by CodeRabbit