feat: Complete MCP Implementation with 70+ Tools, Agent Skills & Workflow Improvements #1
Conversation
Workflow Improvements: - Update release workflow to trigger on successful CI after main branch merge - Add workflow_dispatch for manual releases with version input - Auto-create and push tags when releasing via workflow_run - Add dependabot.yml for Cargo, GitHub Actions, and Docker updates - Add sdk-sync.yml for weekly Augment SDK sync checks MCP Enhancements: - Add prompt templates support (prompts/list, prompts/get methods) - Implement PromptRegistry with built-in prompts: - code_review: Review code for quality, bugs, and best practices - explain_code: Explain what a piece of code does - write_tests: Generate test cases for code - Declare prompts capability in server initialization Documentation: - Add docs/MCP_IMPROVEMENTS.md with feature roadmap
WalkthroughAdds MCP features (progress notifications, prompt templating, resources, skills), many new and expanded tool modules (language, navigation, workspace, specialised search, memory, skills), CI/workflows (release, dependabot, sdk-sync), docs and tests, startup wiring to load skills/prompts, and manifest/tooling updates. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant Client
participant McpServer
participant PromptRegistry
participant SkillRegistry
participant ProgressManager
rect rgba(220,235,255,0.7)
Client->>McpServer: initialize(roots)
McpServer->>McpServer: set session_id, roots, capabilities
McpServer-->>Client: capabilities (prompts, resources, tools)
end
Client->>McpServer: list_prompts()
McpServer->>PromptRegistry: list()
PromptRegistry-->>McpServer: prompts (including skill:*)
McpServer-->>Client: prompts
Client->>McpServer: get_prompt(name,args)
McpServer->>PromptRegistry: get(name,args)
alt skill-backed prompt
PromptRegistry->>SkillRegistry: fetch skill(id)
SkillRegistry-->>PromptRegistry: skill instructions
end
PromptRegistry-->>McpServer: rendered messages
McpServer-->>Client: prompt messages
Client->>McpServer: call_tool(tool, params)
McpServer->>ProgressManager: create_reporter()
ProgressManager-->>McpServer: reporter(token)
par Tool execution & progress
McpServer->>McpServer: execute tool logic (may query resources/context)
McpServer->>ProgressManager: reporter.report(percent,msg)
ProgressManager-->>Client: ProgressNotification
end
McpServer-->>Client: final ToolResult
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 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: 2
🧹 Nitpick comments (5)
docs/MCP_IMPROVEMENTS.md (2)
15-17: Documentation is inconsistent with the code changes in this PR.Line 17 states prompts are "Capability declared but no prompts defined", but this PR adds a full
PromptRegistrywith three built-in prompts (code_review,explain_code,write_tests) and implementsprompts/listandprompts/gethandlers.Consider updating this section to reflect the new implementation status.
🔎 Suggested fix
### 🔶 Partially Implemented - **Resources** - Capability declared but not actively used -- **Prompts** - Capability declared but no prompts defined + +### ✅ Fully Implemented (This PR) +- **Prompts** - Three built-in prompt templates with argument substitution
202-207: Phase 1 checklist should be updated.Line 205 shows "Prompt templates (basic set)" as unchecked (
🔲), but this PR implements prompt templates. Consider marking it as complete.🔎 Suggested fix
### Phase 1 (Next Release) 1. ✅ Workflow improvements (PR-based releases) 2. ✅ Dependabot configuration -3. 🔲 Prompt templates (basic set) +3. ✅ Prompt templates (basic set) 4. 🔲 find_references tool 5. 🔲 go_to_definition toolsrc/mcp/server.rs (1)
176-186: Consider moving the import to the top of the file.The
use crate::mcp::prompts::ListPromptsResult;import at line 178 is inside the function. While this works, it's more idiomatic in Rust to place imports at the module level for consistency and discoverability.🔎 Suggested fix
At the top of the file (after line 11):
use crate::mcp::prompts::ListPromptsResult;Then remove line 178.
src/mcp/prompts.rs (1)
252-277: Tests cover the basic happy path.Consider adding tests for edge cases such as:
- Missing required arguments
- Empty argument values (to verify conditional behaviour)
- Non-existent prompt names
These would help ensure the template substitution logic behaves as expected.
.github/workflows/release.yml (1)
149-149: Updatesoftprops/action-gh-releaseto version 2.Version 1 is outdated; version 2.5.0 is the latest stable release and is recommended for use. Update the action reference to
softprops/action-gh-release@v2.Suggested fix
- name: Create Release - uses: softprops/action-gh-release@v1 + uses: softprops/action-gh-release@v2 with:
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (7)
.github/dependabot.yml.github/workflows/release.yml.github/workflows/sdk-sync.ymldocs/MCP_IMPROVEMENTS.mdsrc/mcp/mod.rssrc/mcp/prompts.rssrc/mcp/server.rs
🧰 Additional context used
🧬 Code graph analysis (2)
src/mcp/prompts.rs (1)
src/mcp/server.rs (1)
new(25-32)
src/mcp/server.rs (1)
src/mcp/prompts.rs (1)
new(75-79)
🪛 actionlint (1.7.9)
.github/workflows/release.yml
149-149: the runner of "softprops/action-gh-release@v1" action is too old to run on GitHub Actions. update the action's version to fix this issue
(action)
🪛 LanguageTool
docs/MCP_IMPROVEMENTS.md
[duplication] ~192-~192: Possible typo: you repeated a word.
Context: ...; } ``` ### 2. Multi-Workspace Support Support multiple workspace roots simultaneously...
(ENGLISH_WORD_REPEAT_RULE)
🔇 Additional comments (10)
.github/dependabot.yml (1)
1-67: Well-structured Dependabot configuration.The configuration sensibly groups Rust minor/patch updates together while keeping major updates separate for careful review. The consistent schedule and timezone across all ecosystems is good practice.
One consideration: the Docker ecosystem entry (lines 51-66) will only be active if a Dockerfile exists in the repository root. The comment on line 51 acknowledges this, which is helpful.
.github/workflows/sdk-sync.yml (2)
46-47: Verify the test filter syntax.The command
cargo test --lib -- sdkwill run tests whose names contain "sdk", not tests in thesdkmodule. If you intend to run tests specifically fromsrc/sdk/, consider using a more specific filter likecargo test --lib sdk::or adjusting the filter pattern to match your test naming convention.
1-115: Workflow structure looks good overall.The weekly SDK sync workflow is well-designed with sensible duplicate issue detection and a helpful checklist for manual review. The caching strategy and permission scoping are appropriate.
.github/workflows/release.yml (2)
137-146: Consider tag creation failure handling.The tag is created and pushed before the release is created. If the release step fails, the tag will already exist in the repository. On retry,
workflow_runwould skip becausegit tag -l "v$VERSION"would find the existing tag.Consider either:
- Moving tag creation after successful release, or
- Adding force-push for tags in retry scenarios, or
- Accepting this as intended behaviour (manual cleanup on failure).
31-85: Release condition logic is well-structured.The check job correctly handles the three trigger types (workflow_dispatch, tag push, workflow_run) with appropriate version extraction and duplicate tag detection. The conditional flow is clear and maintainable.
src/mcp/mod.rs (1)
12-22: LGTM!The module structure follows the existing patterns. The prompts module is correctly added and
PromptRegistryis appropriately re-exported for public use.src/mcp/server.rs (2)
188-209: Handler implementation is correct.The
handle_get_promptmethod properly deserialises parameters, handles missing prompts with an appropriate error, and returns the result. The inlineGetPromptParamsstruct is acceptable given it's only used here.Minor note: Line 198 uses
Error::InvalidToolArgumentsfor a prompt-related error. Consider adding a dedicated error variant if prompt operations become more complex, but this is fine for now.
23-46: Constructors are well-designed.The dual constructor pattern (
newfor defaults,with_promptsfor customisation) provides good flexibility whilst maintaining a simple default API. TheArcwrapping is appropriate for the server's concurrent usage pattern.src/mcp/prompts.rs (2)
73-79: LGTM!The registry initialisation correctly creates an empty registry and populates it with built-in prompts. The
Defaultderive onPromptRegistry(line 62) complements this nicely.
32-43: Good use of serde for JSON serialisation.The
#[serde(tag = "type", rename_all = "lowercase")]onPromptContentcorrectly produces the MCP-compatible format with a discriminator field. The enum variants are well-structured.
Resources: - Add resources/list, resources/read, resources/subscribe handlers - Create ResourceRegistry for browsing indexed files - Support file:// URI scheme for resources Progress Notifications: - Add ProgressManager and ProgressReporter for long-running ops - Support notifications/progress with token tracking Completions API: - Add completion/complete handler - Support file path and prompt name completions Request Cancellation: - Track active requests with HashSet - Handle notifications/cancelled to cancel in-progress ops Roots Support: - Parse client roots from initialize params - Store workspace roots for file path resolution New Navigation Tools (3): - find_references: Find all usages of a symbol - go_to_definition: Find where a symbol is defined - diff_files: Compare two files with unified diff Total tools: 52 (was 49)
rishitank
changed the title
- Add bump_type option (patch/minor/major) for workflow_dispatch - Auto-calculate next version when version input is empty - Add bump-version job to update Cargo.toml - Ensure build uses latest code after version bump - Add duplicate tag protection
There was a problem hiding this comment.
Actionable comments posted: 9
🧹 Nitpick comments (6)
src/tools/navigation.rs (1)
462-523: Consider using thesimilarcrate for robust diff generation.The current line-by-line comparison produces basic output but won't handle insertions, deletions, or moved blocks optimally. The
similarcrate (already in dependencies) provides proper diff algorithms with unified diff formatting.🔎 Example using similar crate
use similar::{ChangeTag, TextDiff}; fn generate_diff( name1: &str, name2: &str, content1: &str, content2: &str, context: usize, ) -> String { let diff = TextDiff::from_lines(content1, content2); diff.unified_diff() .context_radius(context) .header(name1, name2) .to_string() }src/mcp/server.rs (1)
317-320: Silent fallback on malformed params is acceptable but consider logging.Using
unwrap_or_default()silently ignores malformed cursor params. For a list operation this is reasonable, but a debug log might help troubleshoot client issues.let list_params: ListParams = params - .map(|v| serde_json::from_value(v).unwrap_or_default()) + .map(|v| serde_json::from_value(v).unwrap_or_else(|e| { + debug!("Invalid list params, using defaults: {}", e); + ListParams::default() + })) .unwrap_or_default();src/mcp/progress.rs (2)
100-106:complete()silently does nothing whentotalisNone.When a reporter is created without a known total, calling
complete()has no effect. Consider either documenting this limitation or sending a completion notification regardless:pub async fn complete(&self, message: Option<&str>) { if let Some(total) = self.total { self.report(total, message).await; + } else { + // Send a final notification even without a known total + self.report(u64::MAX, message).await; } }Alternatively, document that callers should use
report()directly for indeterminate progress.
144-147: Consider moving the receiver rather than sharing it.Wrapping the receiver in
Arc<Mutex>adds locking overhead and prevents efficient streaming. If only one consumer is expected, consider taking ownership:- pub fn receiver(&self) -> Arc<tokio::sync::Mutex<mpsc::Receiver<ProgressNotification>>> { - self.receiver.clone() + pub fn take_receiver(&mut self) -> Option<mpsc::Receiver<ProgressNotification>> { + Arc::get_mut(&mut self.receiver) + .and_then(|mutex| mutex.get_mut().ok()) + .map(std::mem::take) }If multiple accessors are truly needed, the current approach is acceptable.
src/mcp/resources.rs (2)
168-227: Consider adding a depth limit to prevent excessive recursion.The stack-based traversal has no depth limit. While unlikely in typical codebases, deeply nested structures could cause high memory usage. Consider adding a configurable max depth:
async fn discover_files( &self, dir: &std::path::Path, limit: usize, after: Option<&str>, + max_depth: usize, ) -> Result<Vec<PathBuf>> {This is a minor concern for most use cases.
229-236: Minor:.gitin ignore list is redundant.Hidden files (starting with
.) are already skipped at line 200, so.gitin thematches!is unreachable.matches!( name, - "node_modules" | "target" | "dist" | "build" | "__pycache__" | ".git" + "node_modules" | "target" | "dist" | "build" | "__pycache__" ) || name.ends_with(".lock")
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
src/mcp/mod.rssrc/mcp/progress.rssrc/mcp/resources.rssrc/mcp/server.rssrc/tools/mod.rssrc/tools/navigation.rs
🧰 Additional context used
🧬 Code graph analysis (4)
src/tools/mod.rs (1)
src/tools/navigation.rs (3)
new(22-24)new(97-99)new(157-159)
src/tools/navigation.rs (2)
src/mcp/handler.rs (3)
error_result(100-105)get_string_arg(108-118)success_result(92-97)src/service/context.rs (1)
workspace(124-126)
src/mcp/resources.rs (2)
src/mcp/server.rs (1)
new(34-44)src/service/context.rs (1)
workspace(124-126)
src/mcp/server.rs (4)
src/mcp/resources.rs (1)
new(63-68)src/mcp/prompts.rs (1)
new(75-79)src/reactive/mod.rs (1)
new(55-61)src/mcp/transport.rs (2)
serde_json(83-83)serde_json(88-88)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: build
🔇 Additional comments (15)
src/tools/navigation.rs (5)
1-15: LGTM!Imports are appropriate and well-organised. The module documentation clearly states the purpose.
27-89: LGTM!The
FindReferencesToolimplementation is solid with appropriate error handling and result limiting. The text-based search approach is a reasonable trade-off for simplicity.
102-149: LGTM!The
GoToDefinitionToolimplementation correctly uses language-aware patterns for definition lookup. The context extraction provides useful surrounding code.
238-315: LGTM!The recursive file search implementation is correct with appropriate early termination when
max_resultsis reached. The skip patterns for common directories are sensible defaults.
395-435: LGTM!The definition patterns cover common cases for supported languages. The dual patterns for structs (with and without trailing space) handle different formatting styles.
src/tools/mod.rs (3)
3-10: LGTM!Documentation correctly updated to reflect the new tool count and navigation category.
14-14: LGTM!Module declaration follows the existing pattern.
93-98: LGTM!Navigation tools are correctly registered. The final tool taking ownership of
context_service(rather than cloning) is idiomatic since it's the last usage.src/mcp/mod.rs (1)
12-28: LGTM!The new module declarations and re-exports are well-organised and follow the existing patterns in this file. The documentation accurately reflects the new capabilities.
src/mcp/server.rs (2)
271-304: LGTM!The prompt handlers are well-implemented with proper error handling and use of the
PromptRegistryabstraction.
104-135: Request lifecycle tracking is correctly implemented.The pattern of inserting the request ID before processing and removing it after ensures proper cancellation support. The use of
RwLockis appropriate for this read-heavy workload.src/mcp/progress.rs (2)
1-55: LGTM!The progress notification types are well-designed with correct serde attributes for MCP protocol compliance. The
ProgressTokenenum with#[serde(untagged)]correctly handles both string and numeric tokens.
156-173: Good test coverage for the basic flow.Consider adding tests for
report_percentandcompleteedge cases (e.g., whentotalisNone).src/mcp/resources.rs (2)
15-53: LGTM!The resource data structures are well-designed with appropriate
camelCaseserialisation and optional field handling for MCP protocol compliance.
238-269: LGTM!Comprehensive MIME type mapping covering common source file extensions. Good fallback to
text/plainfor unknown extensions.
Tests: - Add 23 new tests for resources.rs (MIME types, serialization) - Add 8 new tests for progress.rs (reporter, manager, tokens) - Add 13 new tests for navigation.rs (patterns, diff, structs) - Add 3 new tests for server.rs (LogLevel) MCP Features: - Add logging/setLevel handler per MCP spec - Add LogLevel enum with proper level handling - Total tests: 137 (was 111)
New workspace tools (3): - workspace_stats: Get file counts, lines of code, language breakdown - git_status: Get staged, unstaged, untracked files with optional diff - extract_symbols: Extract functions, classes, structs from source files Features: - Symbol detection for Rust, Python, TypeScript/JavaScript, Go - Recursive directory traversal with hidden file filtering - Skips common non-code directories (node_modules, target, etc.) Tests added (15): - Extension to language mapping - Symbol detection for each language - Symbol extraction from content - Name extraction helper Total tools: 55 (was 52) Total tests: 152 (was 137)
rishitank
changed the title
There was a problem hiding this comment.
Actionable comments posted: 8
♻️ Duplicate comments (9)
src/mcp/server.rs (4)
126-129: Inverted cancellation logic may cause false positives.This concern was raised in a previous review. The method returns
trueif the ID is not inactive_requests, meaning never-started or completed requests would also appear "cancelled".
268-273: File URIs with percent-encoded characters may not resolve correctly.This was flagged in a previous review — paths with spaces or special characters won't decode properly.
425-427: Hardcoded session ID will cause issues with multiple clients.This was flagged in a previous review — using
"default"means all clients share subscriptions.Also applies to: 448-448
509-548: Path traversal risk in file path completion.This was flagged in a previous review — the prefix is joined directly without sanitisation.
src/mcp/resources.rs (2)
83-85: File URI construction may be incorrect on Windows or with special characters.This was flagged in a previous review —
path.display()doesn't handle Windows backslashes or percent-encoding.
121-131: Security check may fail if workspace path is not canonical.This was flagged in a previous review — the workspace should also be canonicalised before comparison.
src/tools/navigation.rs (3)
40-43: Glob pattern support discrepancy already flagged.The mismatch between the documented glob pattern support (
'src/**/*.ts') and the actual implementation inmatches_patternwas already raised in a previous review. The implementation only handles simple*.extpatterns.
199-201: Path traversal vulnerability already flagged.The path traversal concern with
workspace.join(&file1)andworkspace.join(&file2)was already raised in a previous review.
352-357: Language hint normalisation issue already flagged.The comparison logic that may skip Rust files when
language="rs"is provided was already raised in a previous review.
🧹 Nitpick comments (8)
.github/workflows/release.yml (3)
181-182: Silent failure ongit pullmay mask issues.The
|| truesuppresses all errors, including network failures or merge conflicts. If the pull fails, the build will proceed with stale code, potentially causing version mismatches.Consider failing explicitly or at least logging the failure:
🔎 Suggested improvement
- name: Pull latest changes - run: git pull origin ${{ github.ref_name }} || true + run: | + if ! git pull origin ${{ github.ref_name }}; then + echo "::warning::Failed to pull latest changes, proceeding with current state" + fi
131-151: Version bump commit may trigger CI recursively.When
bump-versionpushes a commit tomain, this could trigger the CI workflow, which upon success triggers this release workflow again viaworkflow_run. The duplicate tag protection at lines 109-112 and 230-231 should prevent duplicate releases, but consider adding[skip ci]to the commit message to avoid unnecessary CI runs.🔎 Suggested improvement
- name: Commit version bump run: | VERSION="${{ needs.check.outputs.version }}" git config user.name "github-actions[bot]" git config user.email "github-actions[bot]@users.noreply.github.com" git add Cargo.toml - git commit -m "chore: bump version to $VERSION" + git commit -m "chore: bump version to $VERSION [skip ci]" git push
214-215: Duplicate silent failure pattern.Same concern as the build job — silent
|| trueon git pull could mask problems during release.src/mcp/server.rs (1)
377-379: Silently defaulting on parse failure may hide malformed requests.If
serde_json::from_valuefails, the code silently usesDefault. Consider logging a warning when falling back to defaults for easier debugging.🔎 Suggested improvement
let list_params: ListParams = params - .map(|v| serde_json::from_value(v).unwrap_or_default()) + .map(|v| serde_json::from_value(v).unwrap_or_else(|e| { + tracing::debug!("Failed to parse list params, using defaults: {}", e); + ListParams::default() + })) .unwrap_or_default();src/mcp/resources.rs (2)
186-189: Silently ignoring read_dir errors may hide permission issues.When
read_dirfails (e.g., permission denied), the error is silently swallowed. For a resources API, consider logging these failures for debugging.🔎 Suggested improvement
let mut entries = match read_dir(¤t).await { Ok(e) => e, - Err(_) => continue, + Err(e) => { + tracing::debug!("Cannot read directory {:?}: {}", current, e); + continue; + } };
229-236: Consider adding more common ignore patterns.The current list covers major patterns, but you may want to include other common directories like
vendor,.venv,.tox,coverage,.idea,.vscode, and.cache.src/tools/workspace.rs (2)
150-178: Consider renaming fallback from "binary" to "other" or "unknown".The fallback
"binary"for unrecognised extensions is used to exclude files from statistics, but it could exclude text-based files like.txt,.cfg, or.env. If this is intentional, consider renaming to"other"for clarity, or document that only recognised source file types are counted.
456-471: TypeScript function detection may produce false positives.Using
line.contains("function ")(line 458) could match occurrences in comments, strings, or function calls rather than just declarations. For example,// function foo is deprecatedorcallMyfunction (x)(with space before paren) could produce incorrect results. Consider requiring the line to start withfunctionor common modifiers likeexport function,async function.🔎 Proposed improvement
fn detect_ts_symbol(line: &str, line_num: usize) -> Option<Symbol> { // Function declarations - if line.contains("function ") { - let parts: Vec<&str> = line.split("function ").collect(); - if parts.len() > 1 { - let name = parts[1].split('(').next().unwrap_or("").trim().to_string(); + let fn_patterns = ["function ", "async function ", "export function ", "export async function "]; + for prefix in fn_patterns { + if let Some(rest) = line.trim_start().strip_prefix(prefix) { + let name = rest.split('(').next().unwrap_or("").trim().to_string(); if !name.is_empty() { return Some(Symbol { name, kind: "function".to_string(), line: line_num, signature: Some(line.to_string()), }); } } }
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
.github/workflows/release.ymlsrc/mcp/progress.rssrc/mcp/resources.rssrc/mcp/server.rssrc/tools/mod.rssrc/tools/navigation.rssrc/tools/workspace.rs
🚧 Files skipped from review as they are similar to previous changes (1)
- src/mcp/progress.rs
🧰 Additional context used
🧬 Code graph analysis (5)
src/tools/mod.rs (2)
src/tools/workspace.rs (3)
new(22-24)new(186-188)new(296-298)src/tools/navigation.rs (3)
new(22-24)new(97-99)new(157-159)
src/tools/workspace.rs (2)
src/mcp/handler.rs (3)
error_result(100-105)get_string_arg(108-118)success_result(92-97)src/service/context.rs (2)
workspace(124-126)status(134-152)
src/tools/navigation.rs (2)
src/mcp/handler.rs (3)
error_result(100-105)get_string_arg(108-118)success_result(92-97)src/service/context.rs (1)
workspace(124-126)
src/mcp/server.rs (3)
src/mcp/handler.rs (1)
default(68-70)src/mcp/resources.rs (1)
new(63-68)src/mcp/prompts.rs (1)
new(75-79)
src/mcp/resources.rs (6)
src/mcp/server.rs (3)
new(79-90)serde_json(232-232)serde_json(266-266)src/mcp/progress.rs (3)
new(38-54)new(67-77)new(117-124)src/tools/navigation.rs (4)
new(22-24)new(97-99)new(157-159)content(371-375)src/service/context.rs (1)
workspace(124-126)src/config/mod.rs (1)
from(96-112)src/mcp/prompts.rs (1)
text(235-238)
🪛 actionlint (1.7.9)
.github/workflows/release.yml
240-240: the runner of "softprops/action-gh-release@v1" action is too old to run on GitHub Actions. update the action's version to fix this issue
(action)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: build
🔇 Additional comments (17)
.github/workflows/release.yml (2)
156-159: Verify thealways()conditional logic.The
always()modifier means this job will run even ifbump-versionis skipped. However, the condition correctly gates onneeds_bump != 'true' || needs.bump-version.result == 'success', which should handle skipped jobs properly.The logic is sound: build runs when release is approved AND (no bump needed OR bump succeeded).
240-240: Updatesoftprops/action-gh-releaseto v2.The static analysis tool flags that the runner of
softprops/action-gh-release@v1is too old to run on GitHub Actions. The latest version is v2.5.0, with v2 recommended for continued compatibility and new features.🔎 Suggested fix
- name: Create Release - uses: softprops/action-gh-release@v1 + uses: softprops/action-gh-release@v2src/mcp/server.rs (4)
19-60: LogLevel implementation looks good.The enum covers standard log levels with sensible parsing (case-insensitive) and a reasonable default (
Info). Thefrom_strgracefully falls back toInfofor unknown values.
164-165: Request tracking uses write lock for both insert and remove.The active request tracking is correct, but acquiring a write lock on every request start/end could become a bottleneck under high concurrency. For this use case it's likely acceptable, but worth noting if performance issues arise.
Also applies to: 193-194
330-340: Prompt list handler is well-structured.The handler correctly wraps the registry's list in the expected result format with optional pagination cursor.
572-601: Good test coverage for LogLevel.The tests cover parsing (including case variations and unknown values), string conversion, and default behaviour.
src/mcp/resources.rs (3)
15-25: Resource struct is well-designed.Appropriate use of
camelCaseserialisation andskip_serializing_iffor optional fields aligns with MCP protocol expectations.
238-269: MIME type guessing is reasonable with a sensible fallback.The implementation covers common programming languages and configuration formats, with
text/plainas the default. Good approach for this use case.
272-371: Good test coverage for serialisation and MIME type guessing.The tests verify camelCase field names, optional field omission, and round-trip serialisation.
src/tools/workspace.rs (4)
88-143: LGTM - async recursive traversal is correctly implemented.The use of
Box::pinfor async recursion is the correct pattern. The hardcoded directory skip list and binary file filtering are reasonable defaults. Consider making the skip list configurable in the future if needed.
241-258: Git status parsing handles common cases but may miss some edge cases.The parsing logic correctly handles the main cases (
??for untracked, staged vs unstaged modifications). However, some edge cases like renamed files (R), copied files (C), or files with merge conflicts (U) are not explicitly handled and would fall through to the catch-all branch. This is acceptable for a basic tool but worth noting.
569-723: LGTM - comprehensive test coverage.The test suite covers all the symbol detection functions, language mapping, and name extraction. Good coverage of edge cases for each supported language.
52-56: Theworkspace_path()method is correctly defined insrc/service/context.rs:129-131and returns&Path. The original concern questioning its existence is unfounded. The method is used consistently throughout the codebase in multiple files (retrieval.rs, workspace.rs).Regarding the
unwrap()onserde_json::to_string_pretty(): whilst the concern is technically valid, it is not problematic in this context. TheWorkspaceStatsstruct is a simple derived type containing only serializable fields (usize and HashMap), making serialisation failure highly unlikely. Theunwrap()pattern is standard here.Likely an incorrect or invalid review comment.
src/tools/navigation.rs (2)
102-149: LGTM - GoToDefinitionTool implementation is well-structured.The tool correctly handles the optional language hint and formats output with code snippets and language tags. The underlying
find_definitionhelper handles the search logic.
525-641: LGTM - good test coverage for helper functions.The tests cover pattern matching, language detection, definition patterns, and basic diff generation. The struct tests verify field access.
src/tools/mod.rs (2)
3-11: LGTM - documentation updated to reflect new tool categories.The module documentation correctly lists the new navigation (3 tools) and workspace (3 tools) categories, bringing the total to 55 tools as stated.
96-104: LGTM - tool registration follows established patterns.The new navigation and workspace tools are registered consistently with existing tools. The final registration correctly moves
context_servicerather than cloning, since it's the last usage.
Security fixes: - Add path traversal protection in extract_symbols tool - Add path traversal protection in resources/read handler - Canonicalize both workspace and target paths before comparison - Fix file URI construction for Windows paths Bug fixes: - Fix generic impl block parsing (impl<T> Foo<T> now extracts 'Foo') - Fix prompt template mid-line conditionals (no longer drops entire lines) - Fix is_cancelled logic with separate cancelled_requests tracking - Use environment variable for SDK sync template injection Workflow improvements: - Update softprops/action-gh-release from v1 to v2 - Add [skip ci] to version bump commit to prevent recursive CI Documentation: - Update MCP_IMPROVEMENTS.md to reflect all implemented features Tests: - Add edge case tests for prompts (missing args, empty values, non-existent) - Total tests: 157 (was 152)
|
@coderabbitai generate docstrings |
✅ Actions performedInitiated docstring generation; will generate only if new commits exist. |
|
Note Docstrings generation - SUCCESS |
Docstrings generation was requested by @rishitank. * #1 (comment) The following files were modified: * `src/mcp/progress.rs` * `src/mcp/prompts.rs` * `src/mcp/resources.rs` * `src/mcp/server.rs` * `src/tools/mod.rs` * `src/tools/navigation.rs` * `src/tools/workspace.rs`
📝 Add docstrings to `feature/workflow-improvements`
New tools: - git_blame: Show blame info for files with line range support - git_log: Show commit history with filtering (author, date, grep) - dependency_graph: Analyze file/module dependencies with Mermaid output - file_outline: Get structured outline of symbols in a file Features: - Multi-language import detection (Rust, Python, TypeScript, Go) - Mermaid diagram generation for dependency visualization - Git porcelain format parsing for blame - Path traversal protection on file_outline Tests added: - test_extract_imports_rust - test_extract_imports_python - test_extract_imports_typescript - test_extract_imports_go - test_sanitize_mermaid_id - test_generate_mermaid_graph - test_parse_git_blame_porcelain - test_parse_git_blame_empty Total tools: 59 (was 55) Total tests: 165 (was 157)
There was a problem hiding this comment.
Actionable comments posted: 9
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/tools/mod.rs (1)
1-122: Runcargo fmtto fix formatting issues.The CI pipeline detected formatting violations in this file. Please run
cargo fmt --allto automatically fix them before merging.#!/bin/bash # Verify formatting issues cargo fmt --all -- --check
♻️ Duplicate comments (12)
src/tools/workspace.rs (1)
410-415: Add timeout for git command execution to prevent indefinite hangs.The
git statuscommand is executed without a timeout. If the repository is in a broken state or the command hangs (e.g., waiting for credentials), this could block indefinitely.src/tools/navigation.rs (6)
69-72: Pattern matching implementation doesn't support the documented glob patterns.The description mentions patterns like
'src/**/*.ts'butmatches_patternonly handles simple*.extprefixes.
338-340: Potential path traversal vulnerability with user-supplied file paths.Using
workspace.join(&file1)without validation could allow paths like../../../etc/passwdto escape the workspace directory.
435-437: Blocking I/O in async context.
path.is_dir()andpath.is_file()perform synchronous filesystem calls that can block the Tokio runtime.
523-525: Same blocking I/O pattern asfind_symbol_in_files.The
path.is_dir()andpath.is_file()calls perform blocking filesystem operations in an async context.
529-534: Language hint comparison may incorrectly skip files.If the user provides
language="rs"butget_languagereturns"rust"for.rsfiles, the condition may skip Rust files unexpectedly.
723-764: Line-by-line diff algorithm misses changes within hunk context ranges.The current implementation skips to the end of a hunk after finding a single difference, causing subsequent changes within the same context range to be missed.
src/mcp/resources.rs (2)
235-241: Subscription allows duplicate session entries.The
subscribemethod unconditionally pushes the session_id without checking if it already exists, leading to duplicate entries if a client subscribes multiple times.
265-324: File discovery traversal order is non-deterministic.The depth-first traversal with
stack.pop()processes directories in reverse order, and entries are returned in filesystem-dependent order, which may cause pagination issues.src/mcp/server.rs (3)
464-469: File URIs with percent-encoded characters may not resolve correctly.The URI is stripped of the
file://prefix without URL decoding. Paths containing spaces (%20) or other encoded characters will not be resolved correctly.
737-739: Hardcoded session ID will cause issues with multiple clients.Using a placeholder
"default"session ID means all clients share subscriptions. When one client unsubscribes, it may inadvertently affect others' subscriptions.
882-902: Path traversal risk in file path completion.The
prefixparameter is joined directly to workspace roots without sanitisation. A malicious prefix like../../../etc/could leak filesystem structure outside the workspace.
🧹 Nitpick comments (3)
src/mcp/progress.rs (1)
120-128: Consider logging send failures for observability.The send error on Line 127 is silently ignored with
let _ =. While this prevents blocking operations, it means progress updates can be lost without any indication. For a production observability system, consider logging failed sends at DEBUG level.🔎 Optional enhancement to log send failures
pub async fn report(&self, progress: u64, message: Option<&str>) { let notification = ProgressNotification::new( self.token.clone(), progress, self.total, message.map(String::from), ); - let _ = self.sender.send(notification).await; + if let Err(e) = self.sender.send(notification).await { + tracing::debug!("Failed to send progress notification: {}", e); + } }src/tools/workspace.rs (1)
220-234: Extensionless files are silently excluded from statistics.Files without extensions (e.g.,
Makefile,Dockerfile,LICENSE) are skipped entirely becauseentry_path.extension()returnsNone. If these files should be counted, consider adding a fallback for extensionless files.🔎 Suggested approach
- if let Some(ext) = entry_path.extension() { - let ext_str = ext.to_string_lossy().to_lowercase(); - let lang = extension_to_language(&ext_str); - - if lang != "binary" { + let ext_str = entry_path + .extension() + .map(|e| e.to_string_lossy().to_lowercase()) + .unwrap_or_default(); + let lang = extension_to_language(&ext_str); + + if lang != "binary" { + stats.total_files += 1; + let lines = count_lines(&entry_path).await.unwrap_or(0); + stats.total_lines += lines; + + let lang_stats = stats.languages.entry(lang.to_string()).or_default(); + lang_stats.files += 1; + lang_stats.lines += lines; + }And update
extension_to_languageto handle empty string:fn extension_to_language(ext: &str) -> &'static str { match ext { + "" => "other", // extensionless files "rs" => "rust",src/mcp/resources.rs (1)
301-303: Blocking I/O in async context.
path.is_dir()andpath.is_file()perform synchronous filesystem calls. Consider usingentry.file_type().awaitfor consistency with async patterns.🔎 Proposed fix
- if path.is_dir() { - stack.push(path); - } else if path.is_file() { + let file_type = match entry.file_type().await { + Ok(ft) => ft, + Err(_) => continue, + }; + + if file_type.is_dir() { + stack.push(path); + } else if file_type.is_file() {
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (10)
.github/workflows/release.yml.github/workflows/sdk-sync.ymldocs/MCP_IMPROVEMENTS.mdsrc/mcp/progress.rssrc/mcp/prompts.rssrc/mcp/resources.rssrc/mcp/server.rssrc/tools/mod.rssrc/tools/navigation.rssrc/tools/workspace.rs
🚧 Files skipped from review as they are similar to previous changes (1)
- .github/workflows/sdk-sync.yml
🧰 Additional context used
🧬 Code graph analysis (6)
src/tools/workspace.rs (2)
src/mcp/handler.rs (3)
error_result(100-105)get_string_arg(108-118)success_result(92-97)src/service/context.rs (2)
workspace(124-126)status(134-152)
src/tools/mod.rs (3)
src/service/context.rs (1)
workspace(124-126)src/tools/navigation.rs (3)
new(34-36)new(159-161)new(268-270)src/tools/workspace.rs (3)
new(32-34)new(327-329)new(497-499)
src/mcp/server.rs (2)
src/mcp/prompts.rs (1)
new(82-86)src/mcp/resources.rs (1)
new(78-83)
src/tools/navigation.rs (2)
src/mcp/handler.rs (3)
error_result(100-105)get_string_arg(108-118)success_result(92-97)src/service/context.rs (1)
workspace(124-126)
src/mcp/resources.rs (2)
src/tools/workspace.rs (3)
new(32-34)new(327-329)new(497-499)src/service/context.rs (1)
workspace(124-126)
src/mcp/prompts.rs (1)
src/mcp/server.rs (1)
new(117-129)
🪛 GitHub Actions: CI
src/tools/workspace.rs
[error] 1-1: cargo fmt --all -- --check detected formatting changes. Run 'cargo fmt --all' to fix code style issues.
src/tools/mod.rs
[error] 1-1: cargo fmt --all -- --check detected formatting changes. Run 'cargo fmt --all' to fix code style issues.
src/mcp/progress.rs
[error] 1-1: cargo fmt --all -- --check detected formatting changes. Run 'cargo fmt --all' to fix code style issues.
src/mcp/server.rs
[error] 1-1: cargo fmt --all -- --check detected formatting changes. Run 'cargo fmt --all' to fix code style issues.
src/tools/navigation.rs
[error] 1-1: cargo fmt --all -- --check detected formatting changes. Run 'cargo fmt --all' to fix code style issues.
src/mcp/resources.rs
[error] 1-1: cargo fmt --all -- --check detected formatting changes. Run 'cargo fmt --all' to fix code style issues.
src/mcp/prompts.rs
[error] 1-1: cargo fmt --all -- --check detected formatting changes. Run 'cargo fmt --all' to fix code style issues.
🪛 LanguageTool
docs/MCP_IMPROVEMENTS.md
[duplication] ~196-~196: Possible typo: you repeated a word.
Context: ...; } ``` ### 2. Multi-Workspace Support Support multiple workspace roots simultaneously...
(ENGLISH_WORD_REPEAT_RULE)
🔇 Additional comments (33)
docs/MCP_IMPROVEMENTS.md (1)
1-238: LGTM!The documentation is comprehensive, well-structured, and accurately reflects the implemented features. The roadmap provides clear guidance for future development phases.
.github/workflows/release.yml (4)
135-140: Version update shares the same workspace concern.The sed pattern
s/^version = \".*\"/version = \"$VERSION\"/will update the first matching line. In a multi-package workspace, this could update the wrong package version.This can be verified with the same script provided in the previous comment.
153-201: LGTM!The build job correctly handles the multi-platform build matrix and properly sequences after the version bump. The
git pull || truepattern safely handles cases where no new commits exist.
53-58: No action required. The version extraction is correct for the current repository structure.This is a single-package repository with only one version declaration in Cargo.toml. The pattern
grep '^version = ' Cargo.toml | head -1correctly extracts the version string. The concern about multi-package workspaces is not applicable to the current codebase.Likely an incorrect or invalid review comment.
240-240: Thesoftprops/action-gh-release@v2action version is appropriate and supports theprereleaseparameter used on line 248. No action required.src/mcp/progress.rs (3)
9-71: LGTM!The type definitions are well-designed with appropriate serde attributes for JSON-RPC compatibility. The untagged enum for
ProgressTokencorrectly handles both string and numeric tokens per MCP spec.
180-291: LGTM!The
ProgressManagerimplementation is thread-safe and well-designed. The use ofAtomicI64for ID generation andArc<Mutex<Receiver>>for shared receiver access is appropriate for concurrent scenarios.
293-403: LGTM!The test suite provides excellent coverage of the progress notification system, including token serialization, manager behavior, and reporter functionality. The tests properly use async/await and verify JSON-RPC format compliance.
src/mcp/prompts.rs (4)
8-71: LGTM!The type definitions correctly implement the MCP prompt specification with appropriate serde annotations. The tagged
PromptContentenum properly supports both text and resource content types.
100-210: LGTM!The built-in prompts are well-designed with appropriate required and optional arguments. The template structure with mid-line conditionals (e.g., Line 164, Line 197) is handled by the rendering logic below.
264-329: LGTM!The template rendering logic correctly handles mid-line conditionals by processing them character-position-wise rather than line-by-line. The sequential approach (conditionals first, then substitutions, then cleanup) ensures proper rendering for all template patterns.
332-431: LGTM!The test suite comprehensively covers the template rendering logic, including edge cases for missing arguments, empty values, and conditional blocks. The tests verify both the presence and absence of conditional content, confirming the fix for mid-line conditionals.
src/tools/mod.rs (1)
44-122: LGTM!The tool registration function correctly wires up all 55 tools with their required services. The new navigation and workspace tools follow the same pattern as existing tools, and the final use of
context_servicewithout.clone()on Line 121 is a minor optimisation.src/tools/workspace.rs (5)
1-15: LGTM on module structure and imports.The imports are well-organised and the module documentation is clear. The use of
tokio::process::Commandfor async subprocess execution is appropriate.
16-107: LGTM on WorkspaceStatsTool implementation.The tool definition and execution logic are well-structured. The optional
include_hiddenparameter handling is correct, and error handling uses theerror_resulthelper appropriately.
562-599: Path traversal protection is correctly implemented.The canonicalisation of both workspace and file path, followed by the
starts_withcheck, properly prevents directory traversal attacks. Good work addressing the previous review comment.
721-743: Generic impl block parsing is correctly implemented.The fix properly handles generic parameters like
impl<T> Foo<T>by stripping the leading angle-bracket section before extracting the type name. Good work addressing the previous review comment.
998-1152: Comprehensive test coverage for symbol detection.The tests cover the key functionality including extension-to-language mapping, Rust/Python/TypeScript/Go symbol detection, and the
extract_namehelper. Good coverage.src/tools/navigation.rs (3)
1-15: LGTM on module structure and imports.The imports are appropriate for async file operations and tool handling.
104-139: LGTM on FindReferencesTool execution logic.The execution flow correctly handles the case of no references found and formats results in a readable markdown format.
769-885: Comprehensive test coverage for navigation helpers.The tests cover pattern matching, language detection, definition patterns, diff generation, and data structures. Good coverage.
src/mcp/resources.rs (6)
1-14: LGTM on module structure and imports.The module documentation and imports are well-organised for implementing MCP resource support.
15-53: LGTM on resource data structures.The
Resource,ResourceContents,ListResourcesResult, andReadResourceResultstructs are well-defined with appropriate serde attributes for camelCase serialisation and conditional field skipping.
103-140: LGTM on list() implementation with pagination.The listing logic correctly discovers files, builds resources with proper URIs and MIME types, and implements cursor-based pagination using the last resource name.
170-211: Path traversal protection is correctly implemented.The
read()method properly canonicalises both the workspace and the requested path before verifying containment. This addresses the previous security concern.
360-424: LGTM on helper methods.The
path_to_file_urimethod correctly handles Windows paths with backslash conversion and drive letter prefixing. Theguess_mime_typemethod provides reasonable defaults for common file types.
427-527: Good test coverage for resource serialisation.The tests verify MIME type inference, resource serialisation with camelCase, optional field skipping, and round-trip parsing.
src/mcp/server.rs (6)
19-85: LGTM on LogLevel enum implementation.The
LogLevelenum withfrom_strandas_strmethods provides a clean API for log level management. The RFC 5424 severities are properly represented, and the default toInfofor unknown values is sensible.
87-162: LGTM on McpServer struct and constructors.The addition of
prompts,resources,roots,active_requests,cancelled_requests, andlog_levelfields provides comprehensive MCP feature support. Bothnew()andwith_features()constructors are well-documented.
229-262: LGTM on request lifecycle tracking.The
is_cancelled,cancel_request, andcomplete_requestmethods properly manage the request lifecycle using separate sets for active and cancelled requests. This correctly addresses the previous review concern about inverted cancellation logic.
473-500: LGTM on capabilities construction.The dynamic construction of capabilities based on configured features (resources, prompts, logging) is correct. Resources capability is only advertised when the resources registry is present.
764-844: LGTM on completion handling logic.The completion handler correctly dispatches to file path or prompt name completions based on the argument type. The response structure with
valuesandhasMorefollows the expected format.
954-983: Good test coverage for LogLevel.The tests verify case-insensitive parsing, string conversion, and default value behaviour.
…nto feature/workflow-improvements Add 4 new workspace/git tools: - git_blame: Show blame info for files with line range support - git_log: Show commit history with filtering (author, date, grep) - dependency_graph: Analyze file/module dependencies with Mermaid output - file_outline: Get structured outline of symbols in a file Total tools: 59 (was 55) Total tests: 165 (was 157)
- Fix empty_line_after_doc_comments in progress.rs, server.rs, navigation.rs, workspace.rs - Fix doc_lazy_continuation in server.rs notification handler docs
- Update MCP_IMPROVEMENTS.md to reflect 59 tools (was 55) - Mark implemented tools in potential tools table - Add MAX_DISCOVERY_DEPTH (20) to prevent excessive recursion - Track depth in discover_files stack for safety Addresses CodeRabbit review comments
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (14)
src/tools/navigation.rs (5)
401-472: Blocking filesystem checks and simple pattern matching in reference searchTwo points here:
find_symbol_in_filesusespath.is_dir()/path.is_file()inside an async traversal. These are blocking filesystem calls and can stall the Tokio runtime when walking large trees. Prefer gettingentry.file_type().awaitand branching onis_dir()/is_file()from that, as in your workspace stats traversal.File filtering relies on
matches_pattern(&name, pattern), but you only hand the bare filename tomatches_pattern. This means patterns likesrc/**/*.tsmentioned in the docs are not actually supported; only simple*.extsuffixes and substring matches on the filename work.Either upgrade
matches_patternto use a proper glob implementation over the relative path, or narrow the documented examples to what’s actually supported.
654-672: Documented glob support does not matchmatches_patternimplementationThe documentation advertises glob-like patterns such as
'src/**/*.ts', butmatches_patternonly:
- Treats patterns starting with
"*."as simple extension checks, and- Otherwise does a substring match on the filename.
It does not support directory components or
**. Either:
- Enhance this helper to use a glob crate against the full relative path, or
- Update the docs for
find_references/matches_patternto describe the actual, simpler semantics.
146-161: GoToDefinitionTool constructor doc still instantiates FindReferencesToolThe example under
GoToDefinitionTool::newconstructsFindReferencesTool::new(service.clone())instead ofGoToDefinitionTool::new(service.clone()). This looks like a copy–paste error and will confuse readers of the docs; please update the example to useGoToDefinitionTool.
255-270: DiffFilesTool constructor doc also references FindReferencesToolLikewise, the
DiffFilesTool::newexample constructsFindReferencesTool::new(service.clone()). This should beDiffFilesTool::new(service.clone())so that the documentation matches the actual type.
330-352: Path traversal risk in DiffFilesTool file reads
DiffFilesTool::executebuildspath1 = workspace.join(&file1)andpath2 = workspace.join(&file2)and reads them directly. Without canonicalising and checking that the resolved paths stay under the workspace root, inputs such as../../../etc/passwdcan escape the workspace and expose arbitrary files.Please mirror the canonicalise-and-verify pattern you already use in workspace tools (e.g.
FileOutlineTool) before callingread_to_string.src/mcp/resources.rs (2)
268-333: Async file discovery uses blockingpath.is_dir/path.is_fileand non-deterministic order
discover_filestraverses usingpath.is_dir()/path.is_file(), which are blocking filesystem calls inside an async loop. It also relies on the filesystem’s iteration order plus a stack-based DFS, making the traversal order unstable for cursor-based pagination.Not strictly a functional bug, but for better scalability and predictable paging:
- Use
entry.file_type().await(or Tokio metadata) instead ofpath.is_dir()/path.is_file(), and- Optionally collect and sort entries per directory before pushing onto the stack to achieve deterministic ordering.
235-241: Duplicate entries possible in subscriptions map
subscribealways pushessession_id.to_string()into the vector for a URI without checking for existing entries. Repeated subscriptions from the same session will accumulate duplicates and could inflate notification fan-out unnecessarily.Consider checking
if !sessions.contains(&session_id.to_string())before pushing.src/mcp/server.rs (4)
214-255: Request cancellation tracking is consistent with explicit cancellations
is_cancelled,cancel_request, andcomplete_requestnow operate over a dedicatedcancelled_requestsset, which addresses the earlier inversion issue where “not active” implied “cancelled”. The semantics are now clear: only IDs explicitly marked via cancellation notifications are treated as cancelled.
441-491: Initialise handler is reasonable, but file URIs are taken as raw paths
handle_initializecorrectly extractsfile://roots and updates the internalrootslist, and capabilities reflect configured resources/prompts/logging.Note that URIs are currently stripped of the
file://prefix and passed directly intoPathBuf::fromwithout percent-decoding. If clients send properly encoded URIs (e.g. with%20for spaces), these roots may not resolve as intended. If this becomes an issue in practice, you may want to decode URI components before constructing thePathBuf.
695-735: Hard-coded session ID for resource subscriptions breaks multi-client isolation
handle_subscribe_resourceandhandle_unsubscribe_resourcepass the literal"default"session ID into the registry. This means:
- All clients effectively share the same subscription identity.
- One client unsubscribing a URI will remove the shared
"default"entry, potentially affecting other clients’ expectations.To support multiple clients, you’ll need a per-session identifier (from the transport or connection context) and use that instead of a hard-coded value in both subscribe and unsubscribe paths.
860-899: File-path completion can traverse outside client roots via..
complete_file_pathuses each client root, doesroot.join(prefix), thenread_dir(parent)and constructs completion strings without canonicalising or ensuring the parent stays within the root. Ifprefixcontains path traversal segments (e.g."../../.."), this can cause the server to list directories outside the intended workspace roots, leaking filesystem structure.To harden this:
- Canonicalise the root once.
- Canonicalise the candidate parent path, and only proceed if it still starts with the canonical root.
- Use that canonical parent for
read_dir.src/tools/workspace.rs (3)
316-327: GitStatusTool constructor docstring still references WorkspaceStatsToolThe
GitStatusTool::newdocs and example still talk aboutWorkspaceStatsTooland constructWorkspaceStatsTool::new(service)instead ofGitStatusTool::new(service). This is confusing for users of the API; please update the summary line and example to refer toGitStatusTool.
401-465: GitStatusTool runs git commands without any timeout
git statusandgit diffare executed with.output().awaitand no timeout. If git hangs (e.g. waiting for credentials or on a broken repo), this tool call can hang indefinitely and tie up the caller.Consider wrapping both invocations in
tokio::time::timeoutwith a reasonable duration and returning a clear error if the timeout elapses, as suggested in the earlier review.
486-497: ExtractSymbolsTool constructor docstring also misnames the toolSimilarly to
GitStatusTool, theExtractSymbolsTool::newdocs still say “Create a new WorkspaceStatsTool…” and the example constructsWorkspaceStatsTool::new(service). This should be corrected toExtractSymbolsToolto keep the documentation consistent with the type.
🧹 Nitpick comments (3)
src/tools/workspace.rs (1)
1492-1533: Synchronouspath.is_dir()/path.is_file()in async traversal
collect_source_filesusespath.is_dir()andpath.is_file()in an async loop. These perform blocking filesystem calls and can block Tokio worker threads when traversing large trees.You can mirror the approach in
collect_stats_recursiveby usingentry.file_type().await(ortokio::fs::metadata) to avoid blocking the async runtime. This is a performance/responsiveness improvement rather than a functional bug.src/mcp/server.rs (1)
164-197: Public log level getters/setter are fine, but don’t yet affect tracing output directly
log_level()andset_log_level()maintain an internalLogLevel, andhandle_set_log_levelupdates it. Unless something outside this module readslog_level()and reconfigures thetracingsubscriber, changing the level won’t actually alter what gets logged.If the intention is that
logging/setLevelshould affect runtime logging, consider wiring this value into a reloadabletracing_subscriberfilter, or document that it’s an informational setting for higher layers to consume.src/mcp/prompts.rs (1)
67-71: Consider making the template field private.The
templatefield is currently public, which exposes the internal representation. Consider making it private and providing a getter method if external access is needed, though this is a minor encapsulation improvement.🔎 Optional refactor
#[derive(Debug, Clone)] pub struct PromptTemplate { - pub template: String, + template: String, +} + +impl PromptTemplate { + pub fn new(template: String) -> Self { + Self { template } + } }
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
docs/MCP_IMPROVEMENTS.mdsrc/mcp/progress.rssrc/mcp/prompts.rssrc/mcp/resources.rssrc/mcp/server.rssrc/tools/mod.rssrc/tools/navigation.rssrc/tools/workspace.rs
🧰 Additional context used
🧬 Code graph analysis (6)
src/tools/navigation.rs (2)
src/mcp/handler.rs (2)
error_result(100-105)get_string_arg(108-118)src/service/context.rs (1)
workspace(124-126)
src/mcp/resources.rs (1)
src/service/context.rs (1)
workspace(124-126)
src/mcp/prompts.rs (1)
src/mcp/server.rs (1)
new(117-129)
src/tools/workspace.rs (2)
src/mcp/handler.rs (3)
error_result(100-105)get_string_arg(108-118)success_result(92-97)src/service/context.rs (1)
workspace(124-126)
src/tools/mod.rs (2)
src/tools/navigation.rs (3)
new(34-36)new(159-161)new(268-270)src/tools/workspace.rs (7)
new(32-34)new(327-329)new(497-499)new(994-996)new(1133-1135)new(1267-1269)new(1586-1588)
src/mcp/server.rs (4)
src/mcp/handler.rs (1)
default(68-70)src/mcp/transport.rs (1)
default(45-47)src/mcp/resources.rs (1)
new(78-83)src/mcp/prompts.rs (1)
new(82-86)
🪛 LanguageTool
docs/MCP_IMPROVEMENTS.md
[duplication] ~199-~199: Possible typo: you repeated a word.
Context: ...; } ``` ### 2. Multi-Workspace Support Support multiple workspace roots simultaneously...
(ENGLISH_WORD_REPEAT_RULE)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: build
🔇 Additional comments (11)
docs/MCP_IMPROVEMENTS.md (1)
5-233: Roadmap content aligns with current implementation and tool countsThe feature list (resources, prompts, progress, completions, cancellation, navigation/workspace tools) and the “59 tools” breakdown are consistent with the code changes in this PR. The document is clear and accurate enough as-is.
src/tools/mod.rs (1)
3-126: Tool registry wiring for navigation/workspace tools looks consistentThe expanded
register_all_toolssignature and registrations correctly cover the documented 59 tools, including the new navigation and workspace tools. The categorisation and cloning of the shared services are clear and idiomatic; just ensure all existing call sites have been updated to passmemory_serviceandplanning_service.src/mcp/progress.rs (1)
9-344: Progress notifications and manager/reporter API look soundThe
ProgressToken/ProgressParams/ProgressNotificationtypes,ProgressReporterhelpers, andProgressManagerchannel wiring are cohesive and well-tested. Ignoring send failures is documented and acceptable here, and the token generation and receiver-sharing strategy are reasonable for the intended usage.src/mcp/server.rs (4)
19-85: LogLevel API and tests are coherent and match documented behaviourThe
LogLevelenum,from_str/as_strhelpers, and associated tests cover the expected RFC‑style severities and reasonable aliases (warn→warning). Defaulting unknown strings toInfois sensible and clearly tested.Also applies to: 948-976
631-648: List resources handler correctly gates on resources support
handle_list_resourcesgracefully errors withResources not enabledwhen no registry is configured, and otherwise deserialises an optionalcursorand delegates toResourceRegistry::list. The behaviour and error shaping look good.
785-838: Completion routing logic looks sensible for prompts and path-like argumentsThe
handle_completiondispatcher cleanly separates:
- Path/file/uri arguments →
complete_file_path.- Prompt/name arguments with
ref.type == "ref/prompt"→ prompt-name prefix matching.- Everything else → empty completion set.
Struct deserialisation and error handling around missing/invalid params are appropriate.
927-945: Set-level handler correctly parses, stores, and acknowledges log level changes
handle_set_log_leveldeserialises a{ "level": "<string>" }object, maps it viaLogLevel::from_str, updates the internal level, logs the change, and returns{}. Error handling for missing params and deserialisation failures is appropriate.src/mcp/prompts.rs (4)
8-59: Well-structured data models with appropriate serde attributes.The data structures correctly implement the MCP prompt specification with proper serialization attributes. The use of
#[serde(default)]for arguments and#[serde(skip_serializing_if)]for next_cursor demonstrates good API design for backward compatibility and optional fields.
73-210: Solid implementation of prompt registry with built-in prompts.The registry correctly initialises with three useful built-in prompts (code_review, explain_code, write_tests) that demonstrate good template patterns with both required and optional arguments. The templates make effective use of conditional blocks for optional parameters.
332-431: Comprehensive test coverage.The tests thoroughly exercise the prompt registry functionality including edge cases (missing arguments, empty values, conditionals with/without values, non-existent prompts). The test suite validates both the happy path and error scenarios effectively.
264-329: Template rendering logic correctly handles conditionals and substitutions.The template processing is well-structured:
- Processes
{{#if var}}...{{/if}}conditionals first, properly handling mid-line conditionals- Performs simple
{{variable}}substitution for provided arguments- Cleans up any remaining placeholders with regex
The conditional processing (lines 268-307) correctly removes or includes blocks based on argument presence, and the
regexcrate dependency is properly declared inCargo.toml.
- Percent-encoded URI decoding (requires percent-encoding crate) - Proper session management (unique IDs, cleanup) - Progress reporting improvements (logging) - TypeScript function detection accuracy - Extensionless file handling (Makefile, Dockerfile, etc.) - Language category naming (binary -> other) - Async I/O improvements - Silent fallback logging - Known limitations table
Add canonicalization and workspace boundary checking to DependencyGraphTool to prevent path traversal attacks (e.g., ../../../etc/passwd). The fix follows the same pattern used in ExtractSymbolsTool and FileOutlineTool: - Canonicalize both workspace and joined path - Verify the resolved path stays within workspace using starts_with() - Return error if path escapes workspace Addresses CodeRabbit security review comment.
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
docs/MCP_IMPROVEMENTS.md (1)
223-233: Consider referencing latest MCP spec updates for async support (Phase 3).The latest MCP specification (2025-11-25) introduces Tasks as an experimental feature for async/call-now-fetch-later patterns. This could inform future enhancements to the Phase 3 roadmap, particularly for long-running operations currently tracked via progress notifications.
You may want to review whether Tasks provide a better abstraction for the work being performed by the server, allowing clients to query status and retrieve results compared to the current progress-based approach planned for Phase 2.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
docs/MCP_IMPROVEMENTS.md
🧰 Additional context used
🧠 Learnings (2)
📚 Learning: 2026-01-02T04:42:02.381Z
Learnt from: rishitank
Repo: rishitank/context-engine PR: 1
File: src/mcp/server.rs:731-733
Timestamp: 2026-01-02T04:42:02.381Z
Learning: For future MCP server session management improvements: proper implementation requires (1) generating unique session IDs during initialize, (2) passing session context through the request handling chain, and (3) cleaning up subscriptions when sessions end. This is planned for a future version beyond v2.0.0.
Applied to files:
docs/MCP_IMPROVEMENTS.md
📚 Learning: 2026-01-02T04:41:50.839Z
Learnt from: rishitank
Repo: rishitank/context-engine PR: 1
File: src/mcp/server.rs:458-463
Timestamp: 2026-01-02T04:41:50.839Z
Learning: In src/mcp/server.rs, the file URI handling in handle_initialize intentionally does not perform percent-decoding of file:// URIs. This is a documented known limitation for v2.0.0, and proper URI decoding with the percent-encoding crate is planned as a future enhancement.
Applied to files:
docs/MCP_IMPROVEMENTS.md
🪛 LanguageTool
docs/MCP_IMPROVEMENTS.md
[duplication] ~199-~199: Possible typo: you repeated a word.
Context: ...; } ``` ### 2. Multi-Workspace Support Support multiple workspace roots simultaneously...
(ENGLISH_WORD_REPEAT_RULE)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: build
🔇 Additional comments (1)
docs/MCP_IMPROVEMENTS.md (1)
236-378: Excellent documentation of future enhancements and known limitations.The "Future Enhancements" section properly captures identified improvements from code review, including percent-encoded URI decoding and session management enhancements. This aligns well with the documented learnings and provides clear guidance for future work. The distinction between high, medium, and low priority enhancements is helpful for prioritisation.
- Add canonicalize() + starts_with() check for workspace containment - Skip paths that escape the workspace root - Prevents enumeration of files outside workspace via malicious prefix Addresses CodeRabbit security review comment.
- Add percent-encoding crate for proper URI decoding in file:// URIs - Implement proper session management with UUID-based session IDs - Add filename_to_language() for extensionless files (Makefile, Dockerfile, etc.) - Rename 'binary' to 'other' for unknown file extensions - Expand extension_to_language() with more language mappings - Update documentation to reflect completed enhancements - Add test for filename_to_language function Resolves CodeRabbit review comments: - Percent-encoded URIs are now decoded correctly - Session IDs are generated during initialize - Extensionless files are properly categorized - Language categories use accurate naming
CodeRabbit correctly identified that the README claimed 73 tools but actual registrations in src/tools/mod.rs total 69. Updated both occurrences (line 24 metrics table and line 77 section heading).
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
README.md (1)
79-80: Add blank lines before tables per markdown linting rules.Markdown linting (MD058) requires blank lines surrounding tables. Five tables are missing preceding blank lines:
- Line 80 (Retrieval Tools table)
- Line 100 (Memory Tools table)
- Line 152 (Navigation Tools table)
- Line 159 (Workspace Tools table)
- Line 170 (Specialised Search Tools table)
🔎 Proposed fixes
Add blank lines before each table:
### Retrieval Tools (7) + | Tool | Description |### Memory Tools (6) + | Tool | Description |### Navigation Tools (3) + | Tool | Description |### Workspace Tools (7) + | Tool | Description |### Specialised Search Tools (7) + | Tool | Description |Also applies to: 99-100, 151-152, 158-159, 169-170
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
README.md
🧰 Additional context used
🪛 markdownlint-cli2 (0.18.1)
README.md
80-80: Tables should be surrounded by blank lines
(MD058, blanks-around-tables)
100-100: Tables should be surrounded by blank lines
(MD058, blanks-around-tables)
152-152: Tables should be surrounded by blank lines
(MD058, blanks-around-tables)
159-159: Tables should be surrounded by blank lines
(MD058, blanks-around-tables)
170-170: Tables should be surrounded by blank lines
(MD058, blanks-around-tables)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: build
- Add 3 Agent Skills (planning, code_review, search_patterns) in skills/ directory - Add 3 skills tools (list_skills, search_skills, load_skill) for Tool Search Tool pattern - Add SkillRegistry for loading and managing SKILL.md files with YAML frontmatter - Add AGENTS.md for cross-platform agent compatibility (Claude Code, Cursor, Copilot, etc.) - Update README with skills documentation and client compatibility table - Total tools: 72 (69 original + 3 skills tools) This reduces token overhead by ~75% by loading skill instructions on-demand instead of including all tool definitions upfront.
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/tools/mod.rs (1)
1-177: Run cargo fmt to fix formatting issues.The CI pipeline detected formatting changes at line 166. Run
cargo fmt --allto apply standard Rust formatting before merging.#!/bin/bash # Verify formatting compliance cargo fmt --all -- --check
🧹 Nitpick comments (4)
src/mcp/skills.rs (1)
60-80: Consider logging skill parse failures for better observability.The method silently ignores parse errors at line 72. Whilst this prevents one malformed skill from blocking the entire registry, it makes debugging difficult when skills fail to load.
💡 Suggested improvement: Log parse failures
if skill_file.exists() { - if let Ok(skill) = self.parse_skill(&skill_file).await { - self.skills.insert(skill.id.clone(), skill); + match self.parse_skill(&skill_file).await { + Ok(skill) => { + self.skills.insert(skill.id.clone(), skill); + } + Err(e) => { + eprintln!("Warning: Failed to parse skill at {:?}: {}", skill_file, e); + } }AGENTS.md (2)
23-32: Add language specifiers to code blocks.Several fenced code blocks throughout the document are missing language identifiers, which affects syntax highlighting and rendering. Add appropriate language tags (e.g.,
text,bash,plaintext) to all code blocks.🔎 Locations requiring language specifiers
- Line 23: Skills usage examples
- Lines 36-59: Architecture diagram
- Lines 120-124: ToolResult structure
For example:
-``` +```text # List all available skills list_skills()
136-136: Use British English spelling for "Licence".Since the review is requested in British English, the noun form should be "Licence" rather than "License".
🔎 Proposed fix
-## License +## Licencesrc/main.rs (1)
68-79: Consider making skills directory configurable.The skills directory is hardcoded to
"skills"relative to the current working directory. Consider making this configurable via CLI argument or deriving it from the workspace path to improve flexibility and deployment scenarios.🔎 Example configuration approach
Add to
Argsstruct:#[arg(long, default_value = "skills")] skills_dir: PathBuf,Then use:
let skills_dir = config.skills_dir.clone(); let skill_registry = Arc::new(RwLock::new(SkillRegistry::new(skills_dir)));
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (10)
AGENTS.mdREADME.mdskills/code_review/SKILL.mdskills/planning/SKILL.mdskills/search_patterns/SKILL.mdsrc/main.rssrc/mcp/mod.rssrc/mcp/skills.rssrc/tools/mod.rssrc/tools/skills.rs
🧰 Additional context used
🧬 Code graph analysis (4)
src/tools/skills.rs (2)
src/mcp/handler.rs (3)
get_optional_string_arg(121-123)get_string_arg(108-118)success_result(92-97)src/mcp/skills.rs (1)
new(52-57)
src/mcp/skills.rs (2)
src/tools/skills.rs (3)
new(24-26)new(85-87)new(150-152)src/tools/navigation.rs (1)
content(597-601)
src/tools/mod.rs (5)
src/service/context.rs (2)
workspace(124-126)workspace_path(129-131)src/mcp/resources.rs (1)
new(90-95)src/tools/navigation.rs (3)
new(35-37)new(162-164)new(273-275)src/tools/workspace.rs (7)
new(38-40)new(331-333)new(506-508)new(669-671)new(814-816)new(954-956)new(1289-1291)src/tools/memory.rs (1)
new(27-29)
src/main.rs (5)
src/mcp/skills.rs (1)
new(52-57)src/tools/skills.rs (3)
new(24-26)new(85-87)new(150-152)src/mcp/handler.rs (2)
new(28-32)tool_count(62-64)src/tools/mod.rs (1)
register_skills_tools(169-177)src/mcp/server.rs (1)
with_features(172-190)
🪛 GitHub Actions: CI
src/tools/skills.rs
[error] 200-200: Formatting changes detected by cargo fmt. Run 'cargo fmt --all' to apply standard formatting.
src/mcp/skills.rs
[error] 161-161: Formatting changes detected by cargo fmt. Run 'cargo fmt --all' to apply standard formatting.
src/tools/mod.rs
[error] 166-166: Formatting changes detected by cargo fmt. Run 'cargo fmt --all' to apply standard formatting.
🪛 LanguageTool
AGENTS.md
[misspelling] ~15-~15: This word is normally spelled as one.
Context: ...cution workflow | Breaking down complex multi-step tasks | | code_review | Comprehensive...
(EN_COMPOUNDS_MULTI_STEP)
[typographical] ~121-~121: Do not use a colon (:) before a series that is introduced by a preposition (‘with’). Remove the colon or add a noun or a noun phrase after the preposition.
Context: ...ndling All tools return a ToolResult with: - content: Array of content items (te...
(RP_COLON)
[locale-violation] ~136-~136: License must be spelled with a “c” when used as a noun in British English. Use “licence”.
Context: ...andcargo clippy` 4. Submit a PR ## License MIT
(LICENCE_LICENSE_NOUN_SINGULAR)
skills/planning/SKILL.md
[misspelling] ~15-~15: This word is normally spelled as one.
Context: ...en you need to plan and execute complex multi-step tasks. ## When to Use - Breaking down...
(EN_COMPOUNDS_MULTI_STEP)
[uncategorized] ~107-~107: Did you mean the formatting language “Markdown” (= proper noun)?
Context: ...tage | | export_plan | Export plan as markdown | | import_plan | Import plan from ma...
(MARKDOWN_NNP)
[uncategorized] ~108-~108: Did you mean the formatting language “Markdown” (= proper noun)?
Context: ...wn | | import_plan | Import plan from markdown | ## Best Practices 1. **Start with r...
(MARKDOWN_NNP)
README.md
[misspelling] ~196-~196: This word is normally spelled as one.
Context: ...Task planning and execution for complex multi-step tasks | | code_review | quality | Com...
(EN_COMPOUNDS_MULTI_STEP)
skills/search_patterns/SKILL.md
[typographical] ~40-~40: Two consecutive commas
Context: ... Searches in patterns: - tests/**/*, test/**/* - **/*test*.*, **/*_test.*, **/*Test*.* - `**/*.te...
(DOUBLE_PUNCTUATION)
[typographical] ~57-~57: Two consecutive commas
Context: ...- **/*.yaml, **/*.yml, **/*.json, **/*.toml - **/*.ini, **/*.cfg, **/*.conf - **/.env*, `...
(DOUBLE_PUNCTUATION)
[grammar] ~204-~204: Did you mean “to Verify”?
Context: ...h small limits, increase if needed 5. Verify findings: Cross-reference with `find_...
(MISSING_TO_BEFORE_A_VERB)
🪛 markdownlint-cli2 (0.18.1)
AGENTS.md
30-30: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
41-41: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
54-54: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
61-61: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
69-69: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
79-79: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
120-120: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
README.md
81-81: Tables should be surrounded by blank lines
(MD058, blanks-around-tables)
101-101: Tables should be surrounded by blank lines
(MD058, blanks-around-tables)
153-153: Tables should be surrounded by blank lines
(MD058, blanks-around-tables)
160-160: Tables should be surrounded by blank lines
(MD058, blanks-around-tables)
171-171: Tables should be surrounded by blank lines
(MD058, blanks-around-tables)
182-182: Tables should be surrounded by blank lines
(MD058, blanks-around-tables)
skills/search_patterns/SKILL.md
31-31: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
42-42: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
55-55: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
69-69: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
79-79: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
138-138: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
🔇 Additional comments (11)
skills/code_review/SKILL.md (1)
1-172: LGTM! Well-structured code review skill documentation.The documentation is comprehensive and follows best practices. The workflow is logical (diff review → risk analysis → invariants → specialized reviews → summary), the checklist covers key areas (correctness, security, performance, maintainability, testing), and the examples provide clear guidance.
skills/planning/SKILL.md (1)
1-144: LGTM! Clear and comprehensive planning skill documentation.The five-step workflow (Create Plan → Add Steps → Execute Steps → Track Progress → Adapt) is logical and practical. The documentation provides good coverage of planning operations, step types, and best practices. The end-to-end example effectively demonstrates the workflow.
Note: Static analysis hints about "multi-step" and "markdown" capitalisation can be safely ignored—these are minor style points that don't affect clarity.
src/mcp/skills.rs (3)
83-101: LGTM! Robust path handling.The method correctly extracts the skill ID from the parent directory name with a sensible fallback to "unknown". The async file reading and frontmatter parsing are properly structured.
104-126: LGTM! Correct frontmatter parsing logic.The YAML frontmatter extraction is correctly implemented. The index arithmetic properly accounts for the opening delimiter, closing delimiter, and content offsets. Error messages are clear and actionable.
139-162: LGTM! Comprehensive search implementation.The search logic correctly filters across all relevant metadata fields using case-insensitive matching. The use of
is_some_andfor optional category matching is idiomatic Rust.Note: The pipeline failure on line 161 is a formatting issue that will be resolved by running
cargo fmt.skills/search_patterns/SKILL.md (1)
1-205: LGTM! Comprehensive search patterns documentation.The documentation clearly describes seven specialised search patterns with practical examples, file patterns, and workflows. The structure is consistent with the other skill documentation files and provides excellent guidance for semantic code search operations.
Note: Static analysis hints about fenced code blocks lacking language specifiers and grammar can be safely ignored—the examples intentionally use pseudocode syntax rather than a specific programming language, and the grammar is correct as written.
src/mcp/mod.rs (1)
18-32: LGTM! Well-structured module additions.The new MCP modules (progress, prompts, resources, skills) and their corresponding re-exports follow Rust conventions and are properly documented. The public API expansion is clear and intentional.
src/main.rs (1)
92-100: LGTM! Clean integration of prompt and skills registries.The server initialisation with
with_features()cleanly integrates the new prompt and resource capabilities. The skills tools registration and logging provide good observability of the startup process.src/tools/mod.rs (1)
67-163: Excellent tool organisation and documentation.The tool registrations are well-organised by category, and the counts match the documentation exactly (72 tools total). The separation of
register_skills_toolsprovides clean modularity for the progressive disclosure pattern.src/tools/skills.rs (2)
1-16: LGTM!The module documentation clearly describes the skills tooling pattern, and the imports are well-organised and appropriate for async tool handling with thread-safe shared state.
18-77: LGTM!The
SearchSkillsToolimplementation follows best practices with proper async locking, clear error handling via the?operator, and well-structured JSON responses with helpful hints.
New skills: - debugging: Systematic debugging workflow for identifying and fixing bugs - refactoring: Safe code refactoring workflow with impact analysis - documentation: Documentation generation and maintenance workflow - testing: Comprehensive test writing and maintenance workflow Skills as prompts: - Skills are now also available as MCP prompts (e.g., skill:debugging) - Clients that support MCP prompts can access skills natively - Each skill prompt accepts an optional 'task' argument for context Total skills: 7 (planning, code_review, search_patterns, debugging, refactoring, documentation, testing)
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
README.md (1)
308-309: Update test count comment to match metrics.The comment states "Run all unit tests (170 tests)" but the metrics table (line 22) and Test Categories table (line 325) both specify 201 unit tests. This inconsistency was flagged in previous reviews but line 308 wasn't updated.
🔎 Proposed fix
-# Run all unit tests (170 tests) +# Run all unit tests (201 tests) cargo test --lib
🧹 Nitpick comments (2)
src/mcp/prompts.rs (2)
75-143: Built-in prompts are well-defined.The code_review, explain_code, and write_tests prompts cover common use cases with appropriate required/optional arguments. The conditional template syntax is correctly used.
Note: The docstring examples reference
crate::mcp::prompts::PromptRegistry::new()which may not compile in doctests unless--document-private-itemsis used or the path is adjusted.
316-321: Consider caching the regex for better performance.The regex is compiled on every call to
get(). For frequently-called methods, this could be optimised by usinglazy_staticoronce_cellto compile the regex once.🔎 Suggested improvement
use once_cell::sync::Lazy; static PLACEHOLDER_RE: Lazy<regex::Regex> = Lazy::new(|| { regex::Regex::new(r"\{\{[a-zA-Z_][a-zA-Z0-9_]*\}\}").unwrap() }); // Then in get(): text = PLACEHOLDER_RE.replace_all(&text, "").to_string();
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
AGENTS.mdREADME.mdskills/debugging/SKILL.mdskills/documentation/SKILL.mdskills/refactoring/SKILL.mdskills/testing/SKILL.mdsrc/main.rssrc/mcp/prompts.rs
🧰 Additional context used
🧬 Code graph analysis (1)
src/main.rs (6)
src/mcp/prompts.rs (2)
new(84-88)list(247-249)src/mcp/skills.rs (2)
new(52-57)list(129-131)src/tools/skills.rs (3)
new(24-26)new(85-87)new(150-152)src/mcp/handler.rs (2)
new(28-32)tool_count(62-64)src/tools/mod.rs (1)
register_skills_tools(169-177)src/mcp/server.rs (1)
with_features(172-190)
🪛 GitHub Actions: CI
src/main.rs
[error] Rustfmt check failed. Run 'cargo fmt' to format code.
🪛 LanguageTool
skills/testing/SKILL.md
[grammar] ~91-~91: You’ve repeated a verb. Did you mean to only write one of them?
Context: ...cution - High coverage ### Integration Tests - Test component interactions - Use real depen...
(REPEATED_VERBS)
[grammar] ~97-~97: You’ve repeated a verb. Did you mean to only write one of them?
Context: ...cution - Critical paths ### End-to-End Tests - Test full workflows - Simulate user actions ...
(REPEATED_VERBS)
[duplication] ~176-~176: Possible typo: you repeated a word.
Context: ...2. Test Cases: List of scenarios to test 3. Test Code: The actual test implementation ...
(ENGLISH_WORD_REPEAT_RULE)
skills/documentation/SKILL.md
[grammar] ~136-~136: It seems that “to” is missing before the verb.
Context: ...tdated information ### Be Consistent - Follow existing style - Use same terminology -...
(MISSING_TO_BETWEEN_BE_AND_VB)
README.md
[misspelling] ~196-~196: This word is normally spelled as one.
Context: ...Task planning and execution for complex multi-step tasks | | code_review | quality | Com...
(EN_COMPOUNDS_MULTI_STEP)
AGENTS.md
[misspelling] ~15-~15: This word is normally spelled as one.
Context: ...cution workflow | Breaking down complex multi-step tasks | | code_review | Comprehensive...
(EN_COMPOUNDS_MULTI_STEP)
[typographical] ~126-~126: Do not use a colon (:) before a series that is introduced by a preposition (‘with’). Remove the colon or add a noun or a noun phrase after the preposition.
Context: ...ndling All tools return a ToolResult with: - content: Array of content items (te...
(RP_COLON)
[locale-violation] ~141-~141: License must be spelled with a “c” when used as a noun in British English. Use “licence”.
Context: ...andcargo clippy` 4. Submit a PR ## License MIT
(LICENCE_LICENSE_NOUN_SINGULAR)
🪛 markdownlint-cli2 (0.18.1)
skills/testing/SKILL.md
32-32: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
38-38: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
43-43: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
57-57: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
79-79: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
skills/refactoring/SKILL.md
32-32: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
38-38: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
45-45: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
50-50: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
57-57: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
74-74: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
86-86: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
91-91: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
🔇 Additional comments (10)
README.md (3)
188-223: Well-documented Agent Skills section.The skills documentation provides clear guidance on discovery, loading, and execution workflows. The client compatibility table is a helpful addition for users integrating with different MCP clients.
321-348: Good additions for test documentation and coverage.The Test Categories table and Code Coverage section provide useful guidance for contributors. The cargo-tarpaulin instructions are accurate and helpful.
78-186: Tool documentation is accurate and well-organised. The category breakdown (7+5+6+20+14+3+7+7+3 = 72) is mathematically consistent and matches the 72 tool registrations in the codebase. The Navigation, Workspace, Specialised Search, and Skills sections are clearly documented and properly integrated.src/mcp/prompts.rs (3)
1-73: Well-structured data types for MCP prompts.The struct definitions follow MCP protocol conventions with appropriate serde attributes. The
PromptContentenum correctly uses tagged representation for type discrimination.
333-371: Clean integration between skills and prompts.The
register_skills()method correctly converts skills to MCP prompts with theskill:prefix convention. The template escaping for nested braces ({{{{...}}}}) is handled properly.
374-473: Good test coverage for prompt functionality.The tests cover the essential scenarios including edge cases for missing arguments, empty values, and conditional blocks with/without values. This addresses the concerns from previous reviews.
AGENTS.md (1)
1-143: Comprehensive agent guidance document.The AGENTS.md file provides clear, actionable guidance for AI coding agents. The skills table, architecture overview, and key patterns sections are well-organised and consistent with the README documentation.
The tool count (72) and category breakdown match the README, maintaining documentation consistency.
skills/documentation/SKILL.md (1)
1-153: Well-structured documentation skill definition.The skill follows the expected SKILL.md format with proper YAML frontmatter. The four-phase workflow is practical and the MCP tool examples (
workspace_stats(),file_outline(),get_file(), etc.) correctly reference available tools. The documentation types and best practices sections provide actionable guidance.src/main.rs (2)
68-79: Good error handling for skills loading.The skills loading uses appropriate error handling - logging a warning and continuing if skills fail to load. This ensures the server can start even if the skills directory is missing or malformed.
103-117: No action required. The HTTP transport intentionally limits support to tools only, as confirmed by the explicitprompts: Nonedeclaration in theinitializeendpoint's capabilities. This is a deliberate design choice distinguishing the HTTP transport from the stdio transport, not an oversight.Likely an incorrect or invalid review comment.
Unit tests added: - SkillRegistry: new, add, get, list, search (by name, description, tag, category) - SkillRegistry: parse_frontmatter (valid, minimal, missing markers, invalid yaml) - SkillRegistry: load_skills (from directory, creates dir, ignores invalid, multiple) - Skills tools: ListSkillsTool, SearchSkillsTool, LoadSkillTool - Prompts: register_skills, skill prompt naming, descriptions, arguments Integration tests added: - test_mcp_list_skills: List all skills via MCP - test_mcp_search_skills: Search skills by query - test_mcp_load_skill: Load skill with full instructions - test_mcp_load_skill_not_found: Handle missing skill - test_mcp_list_skills_filter_by_category: Filter by category - test_mcp_skill_prompts_available: Skills exposed as MCP prompts Bug fixes: - Use workspace skills directory instead of hardcoded relative path - Remove unused PathBuf import Total: 244 unit tests + 15 integration tests passing
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
src/main.rs (1)
96-99: Verify formatting compliance.A past review flagged formatting issues with the multi-line
info!macro call. Whilst the logic is correct, please ensurecargo fmthas been run and the formatting passes CI checks.#!/bin/bash # Verify formatting compliance cargo fmt --all -- --check
🧹 Nitpick comments (2)
src/main.rs (1)
67-78: Consider making the skills directory configurable.The skills directory is hardcoded to
workspace/skills. Whilst this is a reasonable default, consider adding a configuration option to allow users to specify a custom skills directory path.💡 Proposed enhancement
Add a
skills_dirfield to theConfigstruct and use it here:- // Initialize skills registry - use workspace skills dir - let skills_dir = config.workspace.join("skills"); + // Initialize skills registry + let skills_dir = config.skills_dir.clone(); let skill_registry = Arc::new(RwLock::new(SkillRegistry::new(skills_dir)));src/mcp/skills.rs (1)
60-80: Consider logging individual skill parsing failures.The
load_skillsmethod silently ignores parse errors on line 72 withif let Ok(skill). This could make it difficult to debug why specific skills fail to load. Consider logging a warning for each failed skill parse.💡 Proposed enhancement
let skill_file = path.join("SKILL.md"); if skill_file.exists() { - if let Ok(skill) = self.parse_skill(&skill_file).await { - self.skills.insert(skill.id.clone(), skill); + match self.parse_skill(&skill_file).await { + Ok(skill) => { + self.skills.insert(skill.id.clone(), skill); + } + Err(e) => { + tracing::warn!("Failed to parse skill at {:?}: {}", skill_file, e); + } } }
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
src/main.rssrc/mcp/prompts.rssrc/mcp/skills.rssrc/tools/skills.rstests/mcp_integration_test.rs
🚧 Files skipped from review as they are similar to previous changes (2)
- tests/mcp_integration_test.rs
- src/tools/skills.rs
🧰 Additional context used
🧬 Code graph analysis (2)
src/mcp/skills.rs (1)
src/tools/skills.rs (3)
new(24-26)new(85-87)new(150-152)
src/mcp/prompts.rs (1)
src/mcp/skills.rs (3)
new(52-57)list(129-131)get(134-136)
🔇 Additional comments (11)
src/main.rs (1)
5-28: LGTM! Clear documentation and appropriate imports.The Skills Architecture documentation provides a helpful overview of the "Tool Search Tool" pattern, and the new imports (RwLock, PromptRegistry, SkillRegistry) are properly scoped for the functionality introduced.
src/mcp/skills.rs (4)
1-48: LGTM! Well-designed data structures.The skill data structures are well-organised with appropriate serde defaults for optional fields. The registry pattern with HashMap storage provides efficient lookup by skill ID.
138-162: LGTM! Search implementation is correct.The search logic correctly matches queries against name, description, tags, and category with case-insensitive comparison. For large skill registries, consider caching lowercased strings if search performance becomes a concern, but the current implementation is appropriate for the expected scale.
176-278: Excellent test coverage for basic operations.The unit tests comprehensively cover registry operations and search functionality, including edge cases like nonexistent skills, case-insensitive matching, and searching across different fields (name, description, tags, category).
280-503: Comprehensive test coverage for parsing and async operations.The tests thoroughly cover frontmatter parsing (valid formats, missing markers, invalid YAML), asynchronous file loading (directory creation, invalid skills, multiple skills), and serialisation. The use of
tempfileensures test isolation. Excellent work!src/mcp/prompts.rs (6)
1-73: LGTM! Well-designed MCP-compliant data structures.The prompt data structures are properly designed with appropriate serde annotations for MCP protocol compliance. The tagged enum for
PromptContentand optional field handling withskip_serializing_ifdemonstrate good API design.
75-212: Well-structured built-in prompts.The built-in prompt templates (
code_review,explain_code,write_tests) are comprehensive and well-designed. The past issue with mid-line conditional handling has been addressed, and the current templates correctly use{{#if var}}...{{/if}}syntax for optional content.
266-331: Template rendering logic is correct.The rewritten template substitution correctly handles mid-line conditionals by:
- Processing all
{{#if var}}...{{/if}}blocks first (lines 270-309)- Including content when variable is present and non-empty (lines 297-300)
- Removing entire blocks (not lines) when variable is absent (lines 305-307)
- Then substituting simple
{{variable}}placeholders (lines 312-314)- Finally removing any remaining unsubstituted placeholders (lines 318-321)
This approach correctly preserves mid-line text when optional arguments are omitted. Well done on the fix!
333-372: Excellent skills integration.The
register_skillsmethod elegantly converts skills into MCP prompts with:
- Prefixed naming convention (
skill:<id>)- Clear description formatting
- Optional
taskargument for contextualisation- Properly escaped template markers (line 364) to enable conditional task sections
This provides seamless access to skills through both the MCP prompts API and the tool search pattern.
374-435: Good coverage of basic prompt operations.The tests validate core functionality including listing, retrieval, handling of nonexistent prompts, and the critical case of missing required arguments (where placeholders should be removed). The test at lines 408-423 confirms the template substitution fix works correctly.
437-633: Comprehensive test coverage for conditionals and skills integration.Excellent test coverage including:
- Conditional block handling with and without values (validating the past fix)
- Skills registration as prompts (naming, descriptions, arguments)
- Template rendering with optional task arguments
- Edge cases like empty registries and nonexistent skill prompts
The tests thoroughly validate that mid-line conditionals work correctly, addressing the previously identified issue.
- Fix load_skill error handling: use error_result with is_error=true instead of success_result for 'skill not found' case - Fix parse_frontmatter bounds checking: add safer string slicing with explicit bounds checks to prevent potential panics - Run cargo fmt to fix formatting issues - Remove TYPESCRIPT_RUST_GAP_ANALYSIS.md - port is feature-complete with no remaining gaps - Update tests to verify is_error behavior for load_skill not found case
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (5)
.augment/commands/check-pr-build.md (1)
95-99: Expand error parsing patterns to improve coverage.The error detection patterns (lines containing "Error:", "FAIL", "✕", "✖") are limited and may miss legitimate build failures (e.g., warnings treated as errors, custom test frameworks, non-English output). Consider a more robust pattern-matching approach or machine learning-based error extraction for fuller coverage.
.augment/commands/check-pr-review-comments.md (2)
46-46: Consider the phrasing on line 46.The phrase "from that branch - Use:" may benefit from a preposition or rephrasing for clarity. Consider: "from that branch. Use:" or "from that branch — to do so, use:".
189-189: Hyphenate compound modifier.The phrase "one page" should be hyphenated when used as a compound modifier before a noun: "one-page".
🔎 Proposed fix
-- don't assume all comments fit in one page +- don't assume all comments fit in one-pagesrc/mcp/skills.rs (1)
60-80: Consider logging when skills fail to parse individually.The
load_skillsmethod silently ignores parse errors at line 72 (if let Ok(skill) = ...). Whilst this prevents one malformed skill from blocking others, it would be helpful to log which specific skill files failed to parse for debugging purposes.🔎 Proposed enhancement
let skill_file = path.join("SKILL.md"); if skill_file.exists() { - if let Ok(skill) = self.parse_skill(&skill_file).await { - self.skills.insert(skill.id.clone(), skill); + match self.parse_skill(&skill_file).await { + Ok(skill) => { + self.skills.insert(skill.id.clone(), skill); + } + Err(e) => { + tracing::warn!("Failed to parse skill at {:?}: {}", skill_file, e); + } } }src/mcp/prompts.rs (1)
251-331: Template logic is correct; consider optimising regex compilation.The template rendering correctly handles conditionals and substitutions, including mid-line blocks as addressed in previous reviews. However, the regex pattern is compiled on every call to
get(), which is inefficient.🔎 Refactor: Cache regex compilation using once_cell
Add
once_cellto dependencies inCargo.toml:[dependencies] once_cell = "1.19"Then refactor the placeholder cleanup:
+use once_cell::sync::Lazy; + +static PLACEHOLDER_RE: Lazy<regex::Regex> = + Lazy::new(|| regex::Regex::new(r"\{\{[a-zA-Z_][a-zA-Z0-9_]*\}\}").unwrap()); + pub fn get(&self, name: &str, arguments: &HashMap<String, String>) -> Option<GetPromptResult> { self.prompts.get(name).map(|(prompt, template)| { // ... conditional and substitution logic ... - // Replace any remaining unsubstituted placeholders with empty string - let placeholder_re = regex::Regex::new(r"\{\{[a-zA-Z_][a-zA-Z0-9_]*\}\}").ok(); - if let Some(re) = placeholder_re { - text = re.replace_all(&text, "").to_string(); - } + // Replace any remaining unsubstituted placeholders with empty string + text = PLACEHOLDER_RE.replace_all(&text, "").to_string(); GetPromptResult { // ...This compiles the regex once at first use and caches it for subsequent calls.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (10)
.augment/commands/check-pr-build.md.augment/commands/check-pr-review-comments.md.augment/commands/create-pr.md.gitignoresrc/main.rssrc/mcp/prompts.rssrc/mcp/skills.rssrc/tools/mod.rssrc/tools/skills.rstests/mcp_integration_test.rs
💤 Files with no reviewable changes (1)
- .gitignore
✅ Files skipped from review due to trivial changes (1)
- .augment/commands/create-pr.md
🚧 Files skipped from review as they are similar to previous changes (1)
- tests/mcp_integration_test.rs
🧰 Additional context used
🧬 Code graph analysis (3)
src/tools/skills.rs (5)
src/mcp/handler.rs (4)
error_result(100-105)get_optional_string_arg(121-123)get_string_arg(108-118)success_result(92-97)src/mcp/prompts.rs (1)
new(84-88)src/mcp/skills.rs (1)
new(52-57)src/config/mod.rs (1)
from(96-112)src/mcp/server.rs (1)
as_str(97-108)
src/mcp/prompts.rs (1)
src/mcp/skills.rs (3)
new(52-57)list(142-144)get(147-149)
src/main.rs (5)
src/mcp/prompts.rs (2)
new(84-88)list(247-249)src/mcp/skills.rs (2)
new(52-57)list(142-144)src/tools/skills.rs (3)
new(26-28)new(87-89)new(152-154)src/mcp/handler.rs (2)
new(28-32)tool_count(62-64)src/mcp/server.rs (1)
with_features(172-190)
🪛 LanguageTool
.augment/commands/check-pr-review-comments.md
[uncategorized] ~46-~46: Possible missing preposition found.
Context: ... GitHub for open PRs from that branch - Use: `GET /repos/rishitank/context-engine/p...
(AI_HYDRA_LEO_MISSING_TO)
[grammar] ~189-~189: ‘one page’ is missing a hyphen.
Context: ...ders - don't assume all comments fit in one page - Sort comments by created date desce...
(STARS_AND_STEPS)
🔇 Additional comments (12)
.augment/commands/check-pr-build.md (1)
1-166: Overall: Well-structured specification with practical gaps.The command specification is comprehensive in scope and aligns well with the PR's workflow improvements. However, the hardcoded repository paths and incomplete specification for suggestion generation need to be addressed before implementation. Once parameterised and clarified, this will be a valuable automated workflow tool.
src/main.rs (2)
67-78: Skills initialisation logic looks correct.The skills directory setup, registry initialisation, and asynchronous loading with proper error handling (logging warnings on failure) are well implemented. The code correctly reports the count of loaded skills.
91-97: Prompt registry integration is well structured.The prompt registry creation and skills registration flow is clean and follows the expected pattern. The logging of registered skills count provides useful feedback during startup.
src/mcp/skills.rs (1)
104-139: Frontmatter parsing bounds checking is well implemented.The current implementation properly addresses the previous concern about string slicing. The bounds check at line 125 (
if instructions_start <= after_opening.len()) prevents panics, and the slicing operations are safe becauseend_posis derived fromfind()onafter_opening.src/tools/skills.rs (1)
194-200: Error handling correctly updated.The skill not found case now properly uses
error_resultwithis_error: true, addressing the previous critical review concern. The test coverage at lines 444-446 verifies this behaviour.src/tools/mod.rs (2)
165-174: Skills tools registration is well implemented.The new
register_skills_toolsfunction correctly registers the three skills tools (list, search, load) with the sharedSkillRegistry. The pattern of passingArc<RwLock<SkillRegistry>>allows proper concurrent access across tools.
16-23: Skills architecture documentation is clear and helpful.The documentation explaining the "Tool Search Tool" pattern and the ~75% token overhead reduction provides valuable context for understanding the design decision.
src/mcp/prompts.rs (5)
10-74: LGTM!The data structures are well-designed with appropriate serde annotations and derive macros. The use of a tagged enum for
PromptContentis idiomatic, and the separation betweenPromptmetadata andPromptTemplateimplementation is clean.
214-249: LGTM!Both methods are straightforward and well-documented. The
register()method correctly documents that existing prompts are replaced, andlist()appropriately returns owned values for API safety.
333-371: LGTM!The skills-to-prompts integration is well-designed. The use of escaped braces (
{{{{#if task}}}}) correctly produces literal template syntax in the format string, which will be processed during rendering. This enables skills to be accessed through the prompt API whilst maintaining consistent conditional behaviour.
90-212: LGTM!The built-in prompts are well-structured with clear metadata and templates. The mid-line conditionals (e.g., line 133, 166, 199) now work correctly with the fixed template rendering logic verified by the tests.
374-642: LGTM!The test suite provides excellent coverage, specifically verifying the corrected template substitution behaviour including placeholder cleanup (lines 421-422) and conditional rendering with and without values (lines 437-472). The skills integration tests (lines 474-642) thoroughly validate the prompt-from-skill generation functionality.
- Add rust-version = "1.82" to Cargo.toml for MSRV documentation (is_none_or requires 1.82+)
- Fix check-pr-build.md: remove unsupported 'details: true' parameter, parameterize repo paths with {owner}/{repo}, fix malformed code fence, add error analysis algorithm section
- Fix regex escaping in search_specialized.rs: use regex::escape() for user input, pre-compile regexes outside loops for better performance
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
src/tools/search_specialized.rs (1)
783-819: Wire theinclude_relatedflag into search logic or clarify it is informational only.
ContextSearchToolparses theinclude_relatedflag (line 789) and echoes it back in the response (line 812), but never passes it toContextService::search()(lines 804-807), so it has no effect on search results. This misaligns the schema with the actual behaviour—callers expect settinginclude_related = falseto narrow results. Either extendContextService::search()to accept and use this parameter, or update the schema description to clarify that it is currently informational only.
🧹 Nitpick comments (1)
src/tools/search_specialized.rs (1)
553-638: Clarify themax_resultsparameter semantics.The schema defines a
max_resultsparameter (line 583) but multiplies it by 100 when passing toContextService::search()asmax_tokens(line 612). This is potentially confusing—users would expectmax_resultsto limit the number of results returned, not to be a token budget multiplier. Consider either renaming the parameter tomax_tokensin the schema or documenting the conversion heuristic clearly in the description.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
.augment/commands/check-pr-build.mdCargo.tomlsrc/tools/search_specialized.rs
🚧 Files skipped from review as they are similar to previous changes (1)
- Cargo.toml
🧰 Additional context used
🧬 Code graph analysis (1)
src/tools/search_specialized.rs (3)
src/mcp/handler.rs (3)
get_optional_string_arg(121-123)get_string_arg(108-118)new(28-32)src/service/context.rs (1)
workspace(124-126)src/mcp/protocol.rs (1)
new(248-261)
🪛 markdownlint-cli2 (0.18.1)
.augment/commands/check-pr-build.md
150-150: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
🔇 Additional comments (10)
.augment/commands/check-pr-build.md (1)
1-184: Past issues successfully resolved — file ready for integration.This file comprehensively addresses all flagged concerns from prior reviews:
- ✅ Parameterised repository paths (lines 46, 53, 65, 78, 91): Now uses
{owner}/{repo}format instead of hardcoded paths, enabling reuse across forks and contexts.- ✅ GitHub API compliance (line 80): Removed unsupported
details: trueparameter; only standard pagination (per_page,page) is used.- ✅ Error analysis algorithm (lines 159–177): Added comprehensive algorithm with pattern matching, categorisation rules, and actionable fix suggestions in tabular form.
- ✅ Markdown structure: Code fences are properly balanced; no stray markers.
The specification is well-structured, provides clear step-by-step guidance, and includes concrete examples and error categorisation patterns. The file integrates well with the broader MCP tooling ecosystem described in the PR.
Note on static analysis hint: The MD040 warning on line 150 is a false positive (it flags the closing fence marker, which correctly inherits the language from the opening fence at line 115).
src/tools/search_specialized.rs (9)
1-24: LGTM! Clear documentation and appropriate imports.The module documentation clearly describes all seven specialized search tools, and the imports are well-suited for the file system operations, pattern matching, and async execution required.
26-122: LGTM! Well-designed constants and efficient helper function.The glob patterns provide excellent coverage for test and config files. The helper function correctly pre-compiles patterns, uses case-insensitive matching, and implements sensible limits.
124-250: LGTM! Both tools are correctly implemented.
SearchTestsForToolandSearchConfigForToolproperly implement theToolHandlertrait with clear schemas, appropriate validation, and correct delegation to the shared helper function.
321-399: LGTM! Previous regex issues have been properly addressed.The
search_callersfunction now correctly escapes user input withregex::escape()(line 331) and pre-compiles all regex patterns before the file iteration loop (lines 334-342). This fixes both the security concern and the performance issue identified in the previous review.
401-551: LGTM! Regex handling correctly implemented.The
search_importersfunction properly escapes the module name (line 411) and pre-compiles all language-specific import patterns (lines 414-424), addressing the same concerns that were fixed insearch_callers. TheSearchImportersForToolimplementation is consistent and correct.
640-733: LGTM! Flexible pattern matching with sensible defaults.The
PatternSearchToolcleanly supports both custom regex patterns and language-specific preset patterns, with a reasonable fallback to\w+when neither is specified. The implementation is well-structured.
821-871: LGTM! Comprehensive language-specific pattern generators.The pattern generation functions provide appropriate regex patterns for multiple languages with sensible defaults. The patterns correctly match common syntactic structures for functions, classes, imports, and variables across different programming languages.
873-935: LGTM! Efficient pattern-based search implementation.The
search_with_patternhelper correctly pre-compiles the regex once (line 881) and returns early with an empty result set if the pattern is invalid (line 883), avoiding repeated compilation overhead.
937-1095: LGTM! Comprehensive test coverage.The test suite thoroughly validates pattern generation across all supported languages, verifies glob coverage for test and config patterns, and systematically ensures all generated regex patterns are valid. This provides strong confidence in the correctness of the pattern generators.
Fixes clippy::bool_assert_comparison warning that was causing CI failure with -D warnings flag.
- Add comment explaining that include_related is parsed for API compatibility - When include_related is false, reduce max_tokens by half to limit result scope - This provides a meaningful behavioral difference while the underlying ContextService::search() doesn't support native related-result filtering
Addressing CodeRabbit Comment on
|
|
Tip For best results, initiate chat on the files or code changes. @rishitank, thank you for addressing the Your pragmatic approach is solid—reducing This is a reasonable and well-documented workaround. Nice work! 🎉 If you found this review helpful, would you consider giving us a shout-out on X? Thank you for using CodeRabbit! |
PR Review Status SummaryAll Critical/Major Issues Addressed ✅
CI Status
Remaining CommentsThe remaining CodeRabbit comments are either:
The PR is ready for final review and approval. |
|
@coderabbitai review All issues have been addressed:
CI is passing. Please review and approve if all issues are resolved. |
|
@rishitank, thank you for the comprehensive update and detailed checklist! I can see you've systematically addressed all the issues raised during the review: ✅ Error handling and safety - The thoroughness of your fixes across commits aa64cef, 1cde7d5, 776dbd8, and e38ce36, along with passing CI, demonstrates excellent attention to detail. I'll now trigger a full review to verify all changes and provide final approval. ✅ Actions performedReview triggered.
|
|
@coderabbitai review All code review comments have been addressed and replied to. Please perform a fresh review to verify all issues are resolved. |
|
@rishitank, thank you for the thorough follow-up! Your systematic approach to addressing all review feedback is exemplary—tracking each issue with specific commits and verification status makes the review process much smoother. I can see you've resolved:
With CI passing and all 244 tests green, I'll trigger a fresh review to verify the fixes and provide final approval. ✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
src/tools/search_specialized.rs (1)
57-122: Consider excluding common build and dependency directories.The WalkDir traversal doesn't filter out directories like
node_modules,target,.git,dist, orbuild. Including these can significantly slow down searches and return irrelevant results.Consider adding directory filtering similar to
.gitignoreconventions.🔎 Example implementation
+// Directories to skip during traversal +const SKIP_DIRS: &[&str] = &[ + "node_modules", + "target", + ".git", + "dist", + "build", + "vendor", + ".next", + ".cache", +]; + async fn search_files_with_patterns( workspace: &Path, query: &str, patterns: &[&str], limit: usize, ) -> Vec<serde_json::Value> { let query_lower = query.to_lowercase(); let mut results = Vec::new(); // Compile glob patterns let compiled_patterns: Vec<Pattern> = patterns .iter() .filter_map(|p| Pattern::new(p).ok()) .collect(); for entry in WalkDir::new(workspace) .max_depth(10) .into_iter() - .filter_map(|e| e.ok()) + .filter_entry(|e| { + !SKIP_DIRS.iter().any(|skip| { + e.file_name().to_string_lossy().starts_with(skip) + }) + }) + .filter_map(|e| e.ok()) {
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/tools/search_specialized.rs
🧰 Additional context used
🧬 Code graph analysis (1)
src/tools/search_specialized.rs (3)
src/mcp/handler.rs (4)
get_optional_string_arg(121-123)get_string_arg(108-118)new(28-32)default(68-70)src/service/context.rs (1)
workspace(124-126)src/mcp/protocol.rs (3)
new(248-261)read_only(165-171)read_only(282-285)
🔇 Additional comments (10)
src/tools/search_specialized.rs (10)
1-24: LGTM! Clear module documentation and appropriate imports.The module documentation provides a good overview of the seven specialized search tools, and all necessary imports are present.
26-55: LGTM! Comprehensive glob patterns for test and config files.The patterns cover common naming conventions across multiple languages and frameworks.
124-186: LGTM! SearchTestsForTool is well-implemented.The tool properly uses preset test patterns, includes appropriate input validation, and has a sensible result limit.
188-250: LGTM! SearchConfigForTool follows the same solid pattern.Consistent implementation with appropriate configuration file patterns and validation.
252-399: LGTM! SearchCallersForTool properly implements the fixes from previous reviews.The implementation correctly uses
regex::escape()to sanitise user input (line 331) and pre-compiles patterns before the loop (lines 334-342). The call patterns cover multiple languages appropriately.
401-551: LGTM! SearchImportersForTool correctly implements regex escaping and pre-compilation.The implementation properly sanitises user input and pre-compiles patterns. The import patterns comprehensively cover multiple programming languages.
640-733: LGTM! PatternSearchTool provides flexible pattern matching.The tool elegantly handles both custom patterns and preset pattern types with language-specific variants. The fallback to a word-matching pattern is sensible.
735-829: LGTM! ContextSearchTool correctly implements theinclude_relatedworkaround.The implementation includes clear documentation about the limitation (lines 786-788) and provides meaningful behavioural differences based on the flag. This matches the pragmatic approach accepted in previous reviews.
831-881: LGTM! Language-specific pattern generators are well-designed.The patterns appropriately capture syntax for multiple languages, with sensible defaults for unrecognised languages. The test coverage at lines 1060-1104 validates their correctness.
947-1105: LGTM! Comprehensive test coverage for pattern generators and constants.The tests validate pattern generation for all supported languages, verify glob coverage, and ensure all generated regex patterns compile successfully. This provides good confidence in the pattern generation logic.
- Changed search_with_pattern to return Result<Vec<Value>> instead of Vec<Value> - Invalid regex patterns now return a proper error message instead of silently returning empty results - Callers can now distinguish between 'no matches found' and 'invalid pattern' Addresses CodeRabbit review comment about silent regex compilation failures.
rishitank
changed the title
|
@coderabbitai review All code review comments have been addressed through code changes:
CI Status: ✅ All checks passing Please perform a final review and approve if all issues are resolved. |
|
@rishitank, outstanding work on this comprehensive PR! 🎉 Your systematic approach to addressing all review feedback has been exemplary. The evidence you've provided across 15 distinct fixes—spanning safety improvements (bounds checking, error handling), performance optimizations (regex pre-compilation), security enhancements (percent-decoding, canonical paths), and API completeness—demonstrates exceptional attention to detail. Key highlights:
With all 244 tests passing, CI green, and pre-merge checks complete, this PR is ready for merge. Excellent job tracking each issue through specific commits and providing clear status updates! ✅ Actions performedComments resolved. Review triggered.
|
Summary
This PR implements the complete MCP specification (2025-11-25), adds 70+ tools across multiple categories, introduces an Agent Skills framework for progressive disclosure, and improves development workflows.
MCP Spec Features
Resources Support
resources/list- List all indexed files as browsable resourcesresources/read- Read file content by URI (file://scheme) with percent-decodingresources/subscribe/resources/unsubscribe- Subscribe to file changesProgress Notifications
notifications/progresssupport for long-running operationsProgressManagerandProgressReporterfor tracking indexing progressCompletions API
completion/completehandler for autocompleteRequest Cancellation
HashSet<RequestId>cancelled_requestsset for explicit cancellation trackingnotifications/cancelledto cancel in-progress operationsRoots Support
initializeparamsPrompt Templates
prompts/listandprompts/gethandlerscode_review,explain_code,write_testsPromptRegistrywith proper conditional template handling (mid-line{{#if}}blocks)Logging Support
logging/setLevelhandler per MCP specTool Annotations (MCP Spec 2025-11-25)
readOnlyHint,destructiveHint,idempotentHint,openWorldHinttitlefor all toolsTools (70+)
Navigation Tools (3)
find_referencesgo_to_definitiondiff_filesWorkspace Tools (7)
workspace_statsgit_statusextract_symbolsgit_loggit_diffdependency_graphfile_historySpecialized Search Tools (7)
search_tests_forsearch_config_forsearch_callers_forsearch_importers_forinfo_requestpattern_searchcontext_searchMemory Tools (4)
memory_storememory_retrievememory_listmemory_deleteSkills Tools (2)
search_skillsload_skillCore Tools (47+)
Agent Skills Framework
Implements Anthropic's Agent Skills architecture for progressive disclosure:
Built-in Skills (7)
planningcode_reviewsearch_patternsdebuggingrefactoringdocumentationtestingSkills as MCP Prompts
All skills are exposed via
prompts/listandprompts/getfor AI client integration.Multi-Language Symbol Detection
Supports symbol extraction for:
fn,struct,enum,trait,impldef,class,async deffunction,class,interface,typefunc,type struct,type interfaceSecurity Fixes
DependencyGraphToolandcomplete_file_pathregex::escape()Workflow Improvements
Release Workflow
Dependabot
SDK Sync Workflow
CI Improvements
rust-version = "1.82"in Cargo.toml-D warningsflagTesting
Test Coverage
Code Review Fixes
All CodeRabbit review comments addressed:
cancelled_requestssetpercent_decode_strfor URIsload_skillerror handlingerror_resulthelperparse_frontmatterboundsrust-version = "1.82"{owner}/{repo}placeholdersregex::escape()for user inputVec<Regex>before loopsassert!instead ofassert_eq!include_relatedflagmax_tokenswhen falseResultwith proper errorsFiles Changed
Next Steps After Merge
main