Conversation
Splits the file that grew to 3.6x over its own LOC-gate frozen ceiling into 13 sibling files under src/mcp_server/, following the delegation pattern item.rs already established (each MCP tool method's body moves to a pub fn in its own file; the #[tool_router]-tagged impl block in mcp_server.rs keeps only thin one-line dispatchers, so the router macro's exactly_one_tool_router_block_exists invariant still holds). - types.rs: request/response structs + small shared helpers (856 lines) - artifact.rs, asset.rs, claim.rs, comment.rs, flare_git.rs, handoff.rs, memory_tool.rs, review.rs: one file per tool's method body - tests/: the former ~3580-line monolithic test module, split by domain into mod.rs (shared harness + misc tests), item_tests.rs, asset_tests.rs, artifact_tests.rs, action_tests.rs -- all under the 1500-line limit for new files Widely-shared helpers used across many tools (with_backend_db, resolve_project, claim_db, resolve_repo_or_err, run_git, etc.) stay in mcp_server.rs itself rather than being assigned to one tool's file. Verified: all 619 tests pass (confirmed identical test-name set before and after via diff, after catching and fixing one dropped #[test] attribute at a file-split boundary); cargo clippy with CI's exact flags clean (aside from the pre-existing Windows-only agent_launch.rs import, tracked as #169); scripts/loc-gate.sh passes clean, no allowlist bump needed; .githooks/pre-commit's staged-file LOC check now passes without --no-verify. mcp_server.rs remains on the LOC gate's frozen allowlist at 1521 lines (down from 7249) -- under the 2000 ceiling but not yet under the 1500 line ordinarily required to drop off the allowlist entirely; the remaining content is mostly cross-tool shared infrastructure that doesn't cleanly belong to any single tool's file.
📝 WalkthroughWalkthroughThe MCP server is reorganized into shared types, dedicated handlers for action-based tools, thin routing wrappers, and separate test modules. Artifact, asset, handoff, comment, claim, review, memory, and GitHub workflows retain JSON validation and backend integration coverage. ChangesMCP server modularization
Estimated code review effort: 5 (Critical) | ~120 minutes Possibly related PRs
Suggested labels: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 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: 11
🧹 Nitpick comments (1)
src/mcp_server/tests/item_tests.rs (1)
1036-1047: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winSeed more than 200 items so this test actually verifies the clamp.
The empty fixture always returns zero items, so
len() <= 200passes even ifi64::MAXis never clamped.Proposed test fix
fn item_groom_clamps_limit_to_a_sane_maximum() { let (_tmp, s) = harness(); + for n in 0..201 { + s.item(Parameters(empty_item_create(&format!("Item {n}")))) + .unwrap(); + } + let groomed: serde_json::Value = serde_json::from_str( &s.item(Parameters(ItemRequest { action: "groom".into(), limit: Some(i64::MAX), ..Default::default() })) .unwrap(), ) .unwrap(); - assert!(groomed["items"].as_array().unwrap().len() <= 200); + assert_eq!(groomed["items"].as_array().unwrap().len(), 200); }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/mcp_server/tests/item_tests.rs` around lines 1036 - 1047, Update item_groom_clamps_limit_to_a_sane_maximum to seed more than 200 items through the existing harness or fixture setup before invoking the groom action, then retain the assertion that the returned items length is at most 200. Ensure the test exercises the limit clamp rather than passing with an empty fixture.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/mcp_server/artifact.rs`:
- Around line 105-148: The artifact search loop in the request handler must
avoid unbounded full-body scanning. Add explicit limits for the number of
artifacts examined, results returned, and content bytes read or otherwise use an
indexed store query; apply them around store.list, store.get, and hits
accumulation while preserving name/description matching and bounded snippets.
In `@src/mcp_server/asset.rs`:
- Line 70: Validate AssetRequest.metadata as JSON before persisting it in both
metadata handling locations, rejecting or handling malformed values rather than
storing arbitrary strings unchanged. Preserve valid JSON metadata and ensure the
persistence path only receives serialized, parseable JSON.
- Around line 217-231: Update the deletion flow around
agentflare_backend::asset::delete and delete_file so soft-delete, live-reference
coordination, and unlinking are performed atomically against concurrent
attachments, preventing storage removal when a new live reference appears. Do
not discard delete_file failures; propagate them or persist them for retry, and
ensure the API does not report successful cleanup while bytes remain
inaccessible through live rows.
- Around line 30-68: Update the staged-file handling around `staging_dir`,
`metadata`, and `std::fs::read` to open the validated filename with no-follow
semantics, rejecting symlinks and other unsafe file types. Read from that single
open handle while enforcing `asset_max_attach_bytes()` incrementally,
eliminating the separate path-based size check and preventing replacement or
enlargement between validation and reading.
In `@src/mcp_server/flare_git.rs`:
- Around line 9-22: Validate req.action against the supported actions before the
RepoId resolution and Client::new calls in the request handler, returning the
existing unknown-action error for unsupported values. Preserve normal processing
for valid actions, and add a regression test that submits an unknown action
without credentials or a resolvable repository and verifies it is rejected as
unknown action rather than failing during setup.
- Around line 230-242: Update the git_ref resolution in the GitHub request
handler so a missing git_ref with an overridden req.repo resolves that
repository’s default branch through GitHub instead of returning invalid_params.
Preserve the existing local resolve_default_branch behavior when req.repo is
absent, and propagate any GitHub lookup errors appropriately.
In `@src/mcp_server/memory_tool.rs`:
- Around line 27-28: Update the MCP dispatch error mapping around
handle_remember and handle_compact so delegated input-validation failures,
including blank fields and unsupported scorers, return INVALID_PARAMS rather
than internal errors. Add localized validation before delegation or introduce
typed handler errors, then map only those validation failures to the
invalid-params response while preserving internal-error handling for unexpected
failures.
In `@src/mcp_server/tests/artifact_tests.rs`:
- Around line 16-18: Update the response-reading helper around the
String-building logic to propagate read_to_string failures instead of discarding
them, and assert the actual HTTP status line rather than checking whether the
full response contains “200”. Adjust the helper’s return/error handling and
affected assertions so non-200 status lines cannot pass due to matching headers
or body content.
In `@src/mcp_server/tests/asset_tests.rs`:
- Around line 484-506: In the asset test around the temporary
AGENTFLARE_BACKEND_ASSET_MAX_INLINE_BYTES override, replace manual restoration
after the assertion with a local RAII guard whose Drop implementation restores
the saved environment value, ensuring cleanup also occurs if s.asset or an
assertion panics. Keep the existing with_temp_home serialization and
original-value handling.
In `@src/mcp_server/tests/mod.rs`:
- Around line 55-71: Update the routing suggestion tests around
routing_suggestion_returns_null_for_non_locate and
routing_suggestion_returns_nudge_for_find to inject or explicitly configure a
deterministic router instead of relying on optimize::active_router(). Ensure
both assertions remain stable regardless of the AGENTFLARE_ROUTER environment
setting.
- Around line 73-82: Update check_session_health_unknown_returns_status to
isolate persisted runtime state by using the existing temporary-home test
support or an injectable runtime fixture. Ensure the test’s nonexistent session
ID cannot resolve to an existing record, while preserving the assertion that the
result contains “unknown”.
---
Nitpick comments:
In `@src/mcp_server/tests/item_tests.rs`:
- Around line 1036-1047: Update item_groom_clamps_limit_to_a_sane_maximum to
seed more than 200 items through the existing harness or fixture setup before
invoking the groom action, then retain the assertion that the returned items
length is at most 200. Ensure the test exercises the limit clamp rather than
passing with an empty fixture.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: f993433b-b9ae-42a4-a957-55f6100b9fad
📒 Files selected for processing (15)
src/mcp_server.rssrc/mcp_server/artifact.rssrc/mcp_server/asset.rssrc/mcp_server/claim.rssrc/mcp_server/comment.rssrc/mcp_server/flare_git.rssrc/mcp_server/handoff.rssrc/mcp_server/memory_tool.rssrc/mcp_server/review.rssrc/mcp_server/tests/action_tests.rssrc/mcp_server/tests/artifact_tests.rssrc/mcp_server/tests/asset_tests.rssrc/mcp_server/tests/item_tests.rssrc/mcp_server/tests/mod.rssrc/mcp_server/types.rs
Comment on lines
+105
to
+148
| let (store, base) = self.ensure_artifact_server()?; | ||
| let needle = query.to_lowercase(); | ||
| let mut hits = Vec::new(); | ||
| for summary in store | ||
| .list(req.session_id.as_deref()) | ||
| .map_err(Self::artifact_error)? | ||
| { | ||
| let name_hit = summary.name.to_lowercase().contains(&needle); | ||
| let desc_hit = summary | ||
| .description | ||
| .as_deref() | ||
| .is_some_and(|d| d.to_lowercase().contains(&needle)); | ||
| let content = store | ||
| .get(&summary.id) | ||
| .map(|a| a.content) | ||
| .unwrap_or_default(); | ||
| let content_pos = content.to_lowercase().find(&needle); | ||
| if !(name_hit || desc_hit || content_pos.is_some()) { | ||
| continue; | ||
| } | ||
| let snippet = content_pos.map(|pos| { | ||
| let mut start = pos.saturating_sub(40); | ||
| while !content.is_char_boundary(start) { | ||
| start -= 1; | ||
| } | ||
| let mut end = (pos + needle.len() + 40).min(content.len()); | ||
| while !content.is_char_boundary(end) { | ||
| end += 1; | ||
| } | ||
| content[start..end].to_string() | ||
| }); | ||
| let mut v = serde_json::to_value(&summary).unwrap_or_default(); | ||
| if let Some(obj) = v.as_object_mut() { | ||
| obj.insert( | ||
| "url".into(), | ||
| serde_json::json!(format!("{base}/{}", summary.id)), | ||
| ); | ||
| if let Some(snippet) = snippet { | ||
| obj.insert("snippet".into(), serde_json::json!(snippet)); | ||
| } | ||
| } | ||
| hits.push(v); | ||
| } | ||
| Ok(serde_json::to_string_pretty(&hits).unwrap_or_default()) |
There was a problem hiding this comment.
🚀 Performance & Scalability | 🟠 Major | 🏗️ Heavy lift
Bound artifact search work before scanning every stored body.
Each search loads and lowercases every artifact’s complete content with no result, artifact-count, or content-size limit. A sufficiently large store can block the MCP request and allocate several copies of its contents. Add pagination/result limits and bounded reads, or move this into an indexed store query.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/mcp_server/artifact.rs` around lines 105 - 148, The artifact search loop
in the request handler must avoid unbounded full-body scanning. Add explicit
limits for the number of artifacts examined, results returned, and content bytes
read or otherwise use an indexed store query; apply them around store.list,
store.get, and hits accumulation while preserving name/description matching and
bounded snippets.
Comment on lines
+30
to
+68
| // path traversal guard: reject filename with .. or absolute components | ||
| let staged_rel = std::path::Path::new(&fn_val); | ||
| if staged_rel | ||
| .components() | ||
| .any(|c| !matches!(c, std::path::Component::Normal(_))) | ||
| { | ||
| return Err(ErrorData::invalid_params( | ||
| format!( | ||
| "filename '{fn_val}' contains path separators or parent-refs — not allowed" | ||
| ), | ||
| None, | ||
| )); | ||
| } | ||
| let staging_dir = crate::paths::home().join(".agentflare").join("staging"); | ||
| let staged = staging_dir.join(&fn_val); | ||
| if !staged.exists() { | ||
| return Err(ErrorData::invalid_params( | ||
| format!( | ||
| "file not found at staging path: {} — write the file there before calling attach", | ||
| staged.display() | ||
| ), | ||
| None, | ||
| )); | ||
| } | ||
| let size = std::fs::metadata(&staged) | ||
| .map_err(|e| ErrorData::internal_error(e.to_string(), None))? | ||
| .len(); | ||
| let max_attach = Self::asset_max_attach_bytes(); | ||
| if size > max_attach { | ||
| return Err(ErrorData::invalid_params( | ||
| format!( | ||
| "file is {} bytes, exceeds the {} byte attach limit", | ||
| size, max_attach | ||
| ), | ||
| None, | ||
| )); | ||
| } | ||
| let bytes = std::fs::read(&staged) | ||
| .map_err(|e| ErrorData::internal_error(e.to_string(), None))?; |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | 🏗️ Heavy lift
Prevent staged symlinks and file-swap limit bypasses.
A normal filename can still identify a symlink under staging, and metadata/read follow it outside that directory. The separate size check also permits the file to be replaced or enlarged before the unbounded read. Open the staged file with no-follow semantics and enforce the limit while reading from that same handle.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/mcp_server/asset.rs` around lines 30 - 68, Update the staged-file
handling around `staging_dir`, `metadata`, and `std::fs::read` to open the
validated filename with no-follow semantics, rejecting symlinks and other unsafe
file types. Read from that single open handle while enforcing
`asset_max_attach_bytes()` incrementally, eliminating the separate path-based
size check and preventing replacement or enlargement between validation and
reading.
| let bytes = std::fs::read(&staged) | ||
| .map_err(|e| ErrorData::internal_error(e.to_string(), None))?; | ||
| let hash = Self::content_hash(&bytes); | ||
| let meta = metadata.unwrap_or_else(|| "{}".to_string()); |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick win
Validate the documented JSON metadata before persistence.
AssetRequest.metadata is declared as JSON, but arbitrary strings are stored unchanged, allowing malformed metadata to break downstream consumers.
Proposed fix
let hash = Self::content_hash(&bytes);
let meta = metadata.unwrap_or_else(|| "{}".to_string());
+ serde_json::from_str::<serde_json::Value>(&meta).map_err(|e| {
+ ErrorData::invalid_params(format!("metadata must be valid JSON: {e}"), None)
+ })?;Also applies to: 119-119
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/mcp_server/asset.rs` at line 70, Validate AssetRequest.metadata as JSON
before persisting it in both metadata handling locations, rejecting or handling
malformed values rather than storing arbitrary strings unchanged. Preserve valid
JSON metadata and ensure the persistence path only receives serialized,
parseable JSON.
Comment on lines
+217
to
+231
| // soft-delete the row | ||
| agentflare_backend::asset::delete(conn, &id) | ||
| .map_err(map_backend_err)?; | ||
| // only unlink from disk if no other live row references the same storage_path | ||
| let remaining: i64 = conn | ||
| .query_row( | ||
| "SELECT count(*) FROM assets WHERE storage_path = ?1 AND deleted_at IS NULL", | ||
| rusqlite::params![&asset.storage_path], | ||
| |r| r.get(0), | ||
| ) | ||
| .map_err(|e| ErrorData::internal_error(e.to_string(), None))?; | ||
| if remaining == 0 { | ||
| let base_path = crate::paths::home().join(".agentflare"); | ||
| let _ = agentflare_backend::asset::delete_file(&base_path, &asset.storage_path); | ||
| } |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | 🏗️ Heavy lift
Make reference-safe deletion atomic and observable.
The API soft-deletes the row before unlinking and discards delete_file failures, so it can report success while sensitive bytes remain with no live row available for retry. The count-then-unlink sequence can also race with a concurrent attachment and remove storage referenced by that new row. Coordinate reference creation/deletion and propagate or persist cleanup failures.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/mcp_server/asset.rs` around lines 217 - 231, Update the deletion flow
around agentflare_backend::asset::delete and delete_file so soft-delete,
live-reference coordination, and unlinking are performed atomically against
concurrent attachments, preventing storage removal when a new live reference
appears. Do not discard delete_file failures; propagate them or persist them for
retry, and ensure the API does not report successful cleanup while bytes remain
inaccessible through live rows.
Comment on lines
+9
to
+22
| let repo = match &req.repo { | ||
| Some(r) => RepoId::parse(r) | ||
| .ok_or_else(|| ErrorData::invalid_params(format!("bad repo: {r}"), None))?, | ||
| None => RepoId::resolve_from_remote(&std::env::current_dir().unwrap_or_default()) | ||
| .ok_or_else(|| { | ||
| ErrorData::invalid_params( | ||
| "no repo given and could not resolve origin remote".to_string(), | ||
| None, | ||
| ) | ||
| })?, | ||
| }; | ||
| let client = Client::new().map_err(to_mcp_error)?; | ||
|
|
||
| let out = match req.action.as_str() { |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Validate the action before resolving the repository or creating the client.
An unknown action can currently fail with “no repo,” malformed repository, or client-configuration errors before reaching the unknown-action branch. Reject unsupported actions first and add a credential-independent regression test.
Also applies to: 248-253
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/mcp_server/flare_git.rs` around lines 9 - 22, Validate req.action against
the supported actions before the RepoId resolution and Client::new calls in the
request handler, returning the existing unknown-action error for unsupported
values. Preserve normal processing for valid actions, and add a regression test
that submits an unknown action without credentials or a resolvable repository
and verifies it is rejected as unknown action rather than failing during setup.
Comment on lines
+27
to
+28
| crate::memory::mcp::handle_remember(input) | ||
| .map_err(|e| ErrorData::internal_error(e, None)) |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Return INVALID_PARAMS for delegated input-validation failures.
handle_remember rejects blank title/content, while handle_compact rejects blank queries and unsupported scorers. These currently become internal server errors. Validate those constraints here or introduce typed handler errors before mapping failures.
Proposed localized validation
let r#type = req
.r#type
.ok_or_else(|| ErrorData::invalid_params("type is required", None))?;
+ if title.trim().is_empty() || content.trim().is_empty() {
+ return Err(ErrorData::invalid_params(
+ "title and content are required",
+ None,
+ ));
+ }
...
"compact" => {
+ if !matches!(req.query.as_deref(), Some(q) if !q.trim().is_empty()) {
+ return Err(ErrorData::invalid_params("query is required", None));
+ }
+ if let Some(scorer) = req.scorer.as_deref()
+ && scorer != "fts5"
+ {
+ return Err(ErrorData::invalid_params(
+ format!("unsupported scorer '{scorer}'"),
+ None,
+ ));
+ }
let input = crate::memory::mcp::CompactInput {Also applies to: 75-76
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/mcp_server/memory_tool.rs` around lines 27 - 28, Update the MCP dispatch
error mapping around handle_remember and handle_compact so delegated
input-validation failures, including blank fields and unsupported scorers,
return INVALID_PARAMS rather than internal errors. Add localized validation
before delegation or introduce typed handler errors, then map only those
validation failures to the invalid-params response while preserving
internal-error handling for unexpected failures.
Comment on lines
+16
to
+18
| let mut full = String::new(); | ||
| let _ = stream.read_to_string(&mut full); | ||
| full |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Assert the actual HTTP status line and propagate read failures.
resp.contains("200") can accept a non-200 response whose headers or body contain those digits, while the helper silently ignores read errors.
Proposed fix
let mut full = String::new();
- let _ = stream.read_to_string(&mut full);
+ stream
+ .read_to_string(&mut full)
+ .expect("read HTTP response");
full
}
let resp = http_get(url);
- assert!(resp.contains("200"), "serves published artifact: {resp}");
+ let status = resp.lines().next().unwrap_or_default();
+ assert!(
+ status.starts_with("HTTP/1.0 200 ") || status.starts_with("HTTP/1.1 200 "),
+ "serves published artifact: {resp}"
+ );Also applies to: 44-49
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/mcp_server/tests/artifact_tests.rs` around lines 16 - 18, Update the
response-reading helper around the String-building logic to propagate
read_to_string failures instead of discarding them, and assert the actual HTTP
status line rather than checking whether the full response contains “200”.
Adjust the helper’s return/error handling and affected assertions so non-200
status lines cannot pass due to matching headers or body content.
Comment on lines
+484
to
+506
| // override inline limit to 1 byte so our file exceeds it | ||
| // SAFETY: with_temp_home holds GLOBAL_STATE_LOCK so no concurrent env mutation. | ||
| let saved = std::env::var("AGENTFLARE_BACKEND_ASSET_MAX_INLINE_BYTES").ok(); | ||
| unsafe { std::env::set_var("AGENTFLARE_BACKEND_ASSET_MAX_INLINE_BYTES", "1") }; | ||
| let got: serde_json::Value = serde_json::from_str( | ||
| &s.asset(Parameters(AssetRequest { | ||
| action: "get".into(), | ||
| id: Some(asset_id), | ||
| item_id: None, | ||
| project_id: None, | ||
| filename: None, | ||
| metadata: None, | ||
| })) | ||
| .unwrap(), | ||
| ) | ||
| .unwrap(); | ||
| assert!(got["content"].is_null()); | ||
| assert!(got["content_omitted_reason"].as_str().is_some()); | ||
| // restore to avoid leaking to sibling tests | ||
| match saved { | ||
| Some(v) => unsafe { std::env::set_var("AGENTFLARE_BACKEND_ASSET_MAX_INLINE_BYTES", v) }, | ||
| None => unsafe { std::env::remove_var("AGENTFLARE_BACKEND_ASSET_MAX_INLINE_BYTES") }, | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
Restore the environment variable with a panic-safe guard.
with_temp_home serializes mutation, but any panic between set_var and manual restoration leaks the one-byte limit into subsequent tests. Use a local RAII guard whose Drop restores the original value.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/mcp_server/tests/asset_tests.rs` around lines 484 - 506, In the asset
test around the temporary AGENTFLARE_BACKEND_ASSET_MAX_INLINE_BYTES override,
replace manual restoration after the assertion with a local RAII guard whose
Drop implementation restores the saved environment value, ensuring cleanup also
occurs if s.asset or an assertion panics. Keep the existing with_temp_home
serialization and original-value handling.
Comment on lines
+55
to
+71
| #[test] | ||
| fn routing_suggestion_returns_null_for_non_locate() { | ||
| let s = AgentflareMcp::default(); | ||
| let result = s.get_routing_suggestion(Parameters(GetRoutingSuggestionRequest { | ||
| prompt: "refactor the payment module".to_string(), | ||
| })); | ||
| assert!(result.contains("null")); | ||
| } | ||
|
|
||
| #[test] | ||
| fn routing_suggestion_returns_nudge_for_find() { | ||
| let s = AgentflareMcp::default(); | ||
| let result = s.get_routing_suggestion(Parameters(GetRoutingSuggestionRequest { | ||
| prompt: "find the auth handler".to_string(), | ||
| })); | ||
| assert!(result.contains("cheap-model")); | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
Isolate these assertions from the active router configuration.
Lines 58 and 67 call a router selected by optimize::active_router(), which honors AGENTFLARE_ROUTER; a configured CI or developer environment can legitimately produce a different suggestion. Inject or otherwise fix the router configuration for these tests.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/mcp_server/tests/mod.rs` around lines 55 - 71, Update the routing
suggestion tests around routing_suggestion_returns_null_for_non_locate and
routing_suggestion_returns_nudge_for_find to inject or explicitly configure a
deterministic router instead of relying on optimize::active_router(). Ensure
both assertions remain stable regardless of the AGENTFLARE_ROUTER environment
setting.
Comment on lines
+73
to
+82
| #[test] | ||
| fn check_session_health_unknown_returns_status() { | ||
| let s = AgentflareMcp::default(); | ||
| let result = s | ||
| .check_session_health(Parameters(CheckSessionHealthRequest { | ||
| session_id: "nonexistent-session-id".to_string(), | ||
| })) | ||
| .unwrap(); | ||
| assert!(result.contains("unknown")); | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
Avoid reading shared runtime state in this unit test.
check_session_health loads the persisted runtime state, so an existing nonexistent-session-id record changes this result to healthy or stale. Use the existing temporary-home test support or an injectable runtime fixture.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/mcp_server/tests/mod.rs` around lines 73 - 82, Update
check_session_health_unknown_returns_status to isolate persisted runtime state
by using the existing temporary-home test support or an injectable runtime
fixture. Ensure the test’s nonexistent session ID cannot resolve to an existing
record, while preserving the assertion that the result contains “unknown”.
This was referenced Jul 19, 2026
This was referenced Jul 21, 2026
getappz
added a commit
that referenced
this pull request
Jul 21, 2026
) * fix(search): rename max_results->num_results for rivalsearch; sanitize store FTS query - search_web: rivalsearch web_search expects num_results, not max_results (#253) - search_store: sanitize query via flare_search_kit::fts_query to prevent FTS5 column-name parsing errors like 'no such column: ctx' (#254) Agentflare-Agent: 1 Agentflare-Branch: fix/search-web-store-bugs * fix(search): avoid unsanitized FTS5 fallback in store search fts_query() returns None only when the sanitized query has no tokens (e.g. quote-only input). The unwrap_or_else fallback was resubmitting the raw, unsanitized query to FTS5 MATCH in that case, undermining the sanitization it was meant to guarantee. Return an empty result set instead. Agentflare-Agent: claude-code_2-1-216_agent Agentflare-Branch: fix/search-web-store-bugs * fix(search): clamp web search limit to rivalsearch's num_results bound rivalsearch web_search rejects num_results outside 1..=20 (schema validation). An unclamped limit (e.g. the default max of 50 used elsewhere, or a caller-supplied value) failed the whole search call instead of returning a truncated result set. Agentflare-Agent: claude-code_2-1-216_agent Agentflare-Branch: fix/search-web-store-bugs
This was referenced Jul 24, 2026
feat: on-demand third-party docs — flare-docs crate + flare_docs MCP tool + agentflare docs CLI
#316
Merged
This was referenced Jul 28, 2026
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.
Summary
src/mcp_server.rswas 7249 lines, 3.6x over its own frozen LOC-gate ceiling (2000), and — as of PR feat(ci): wire LOC-gate into pre-commit hook (staged files only) #237 (merged today) — that's no longer just a style concern:.githooks/pre-commitactively blocks any commit touching the file. Closes item feat: asset MCP tool — attach/get/list/delete with storage, dedup, and tests #168.src/mcp_server/, following the exact delegation patternitem.rsalready established (its own header comment literally documents the convention): each#[tool]method's body moves to apub fnin its own file; the#[tool_router]-tagged impl block inmcp_server.rskeeps only thin one-line dispatchers (self.artifact_impl(req)etc.), soexactly_one_tool_router_block_exists— the test that guards this exact invariant — still passes.types.rs(856 lines): request/response structs + small shared helpers, pulled out first.artifact.rs,asset.rs,claim.rs,comment.rs,flare_git.rs,handoff.rs,memory_tool.rs,review.rs: one file per tool's method body.tests/: the former ~3580-line monolithic test module, split by domain intomod.rs(shared harness fns + misc small tests),item_tests.rs,asset_tests.rs,artifact_tests.rs,action_tests.rs— all individually under the 1500-line limit that applies to new (non-allowlisted) files.with_backend_db,resolve_project,claim_db,resolve_repo_or_err,run_git, gateway config helpers, etc.) stay inmcp_server.rsitself rather than being force-assigned to one tool's file — they're genuinely cross-cutting infrastructure.mcp_server.rsis now 1521 lines (down from 7249), still on the LOC gate's frozen allowlist (comfortably under the 2000 ceiling) but not yet under the 1500-line bar that would let it drop off the allowlist entirely — the remainder is mostly that shared infrastructure.Bugs caught and fixed along the way
#[test]attribute (item_create_auto_provisions_workspace_and_projectbecame a plain, never-run function) — caught by diffing the full test-name set before/after the split, not just the pass/fail count.open(path, "w")on Windows silently converts\n→\r\n, which brokeexactly_one_tool_router_block_exists's exact-byteinclude_str!match — invisible togit diffand even a Python read-back check (Python's read mode auto-normalizes CRLF back), only surfaced via a standalonerustcprobe comparing byte length vs codepoint length.Test plan
cargo test --bin agentflare— 619 passed, 0 failed (same count and same test-name set as before the split, verified via diff)cargo clippy --locked --workspace --all-targets --all-features -- -D warnings -A unsafe_code -A clippy::pedantic(CI's exact invocation) clean, aside from the pre-existing Windows-onlyagent_launch.rsunused-import (tracked separately as handoff: assign items + attach versioned assets instead of raw artifacts #169, doesn't occur on CI'subuntu-latestrunner)cargo fmt --checkcleanbash scripts/loc-gate.sh(full repo scan) —LOC gate OK: all non-allowlisted Rust files <= 1500 lines (2 legacy files frozen <= 2000).githooks/pre-commitpasses cleanly on this commit with no--no-verifyneeded — the original motivating problem is resolvedSummary by CodeRabbit