Tm/add mcp server specification/2025 04 04 #247
Conversation
📝 WalkthroughWalkthroughA new specification document was added, detailing the design and requirements for the Arbigent MCP Server. The document describes the server's architecture, command-line interface, error handling, logging, non-functional requirements, and includes a TypeScript example for implementation. It specifies dynamic tool registration and subprocess management for test execution. Changes
Sequence Diagram(s)sequenceDiagram
participant MCP Client
participant MCP Server
participant arbigent-cli (Subprocess)
participant Project/Config YAML
MCP Client->>MCP Server: Invoke run-arbigent-test-[tag]
MCP Server->>Project/Config YAML: Read project/config YAML
MCP Server->>arbigent-cli (Subprocess): Spawn arbigent-cli with args
arbigent-cli (Subprocess)-->>MCP Server: Return stdout, stderr, exit code
MCP Server-->>MCP Client: Return test results or error
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
Documentation and Community
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 9
♻️ Duplicate comments (1)
arbigent-mcp-server-specification.md (1)
345-353: Argument parsing is still a placeholder here; see comment on lines 228–238 for implementing a robust parser.
🧹 Nitpick comments (16)
arbigent-mcp-server-specification.md (16)
1-4: Consider adding document metadata
Including author, contact details, and a changelog or version history will help track future revisions and contributors.
5-8: Clarify the Purpose statement
You could explicitly mention the target users (e.g., CI pipelines, LLM-integrated testing) to provide clearer context.
9-23: Use consistent list formatting for Scope
Transform the scope items (lines 12–18) into a markdown bullet list with parallel structure and consistent capitalization for readability.🧰 Tools
🪛 LanguageTool
[style] ~22-~22: Using “back” with the verb “return” may be redundant.
Context: ...cess. Returning test results or errors back to the MCP client. 1.3. Goals Provide ...(RETURN_BACK)
24-31: Define measurable success criteria
Consider adding concrete metrics or SLAs (e.g., “respond within 500 ms” or “<1% error rate”) to make Goals actionable and testable.
60-65: Add schema definitions for tools
Define input/output JSON schemas (e.g., via Zod) forrun-arbigent-test-alland tag-specific tools to enable parameter validation and self-documentation.
74-79: Enhance result reporting structure
Recommend specifying a structured response (fields likeexitCode,stdout,stderr) rather than freeform text, so clients can parse results programmatically.
95-100: Clarify sensitive logging policies
Explicitly state which fields (like API keys) must never be logged and ensure logs are redacted accordingly to meet security requirements.
108-116: Recommend structured logging
Rather than usingconsole.error, consider integrating a logger (e.g.,pino,winston) to support log levels, structured output, and better diagnostics in production.
133-137: Include tag extraction validation in the plan
Add a sub-step in Phase 2 to define and test the tag extraction logic, including edge cases like missing tags or malformed YAML.
169-191: Detail tool schemas and examples
Include JSON schema definitions and example MCP request/response payloads forrun-arbigent-test-allandrun-arbigent-test-[tag]to guide implementers.
207-215: Add environment validation step
Mention runtime checks for Node.js version and presence ofarbigent-clibefore startup to fail fast with clear messages.🧰 Tools
🪛 LanguageTool
[uncategorized] ~210-~210: Possible missing comma found.
Context: ...cutable is either present in the system PATH or its path is provided via the --arbig...(AI_HYDRA_LEO_MISSING_COMMA)
258-263: Handle Windows executable suffix
When scanningPATH, account for platform-specific extensions (e.g.,.exe,.bat) soarbigent-cli.exeon Windows is discovered.🧰 Tools
🪛 LanguageTool
[formatting] ~259-~259: Did you mean “?”
Context: ...mand name const paths = process.env.PATH?.split(path.delimiter) || []; for (const ...(MULTIPLE_PUNCTATION_MARKS_1)
320-327: Use consistent logging methods
Remove or standardize theconsole.login thestdouthandler; use the chosen logger orconsole.errorto keep log levels consistent.
377-385: Decouple server metadata
Consider sourcingnameandversionfrompackage.jsonto keep code DRY and avoid manual sync issues.
425-433: Consider retry logic on connection failure
Implement backoff or alerting ifserver.connectfails intermittently, to improve resilience in unstable environments.
436-439: Handle fatal errors explicitly
Optionally return a structured MCP error before exiting to give clients clear failure semantics, rather than only logging to stderr.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
arbigent-mcp-server-specification.md(1 hunks)
🧰 Additional context used
🪛 LanguageTool
arbigent-mcp-server-specification.md
[style] ~22-~22: Using “back” with the verb “return” may be redundant.
Context: ...cess. Returning test results or errors back to the MCP client. 1.3. Goals Provide ...
(RETURN_BACK)
[uncategorized] ~40-~40: Possible missing comma found.
Context: ...e> [options]. It will operate as an MCP server communicating via standard input/output...
(AI_HYDRA_LEO_MISSING_COMMA)
[style] ~55-~55: Three successive sentences begin with the same word. Consider rewording the sentence or use a thesaurus to find a synonym.
Context: ...he system's PATH environment variable. If arbigent-cli cannot be found using eith...
(ENGLISH_WORD_REPEAT_BEGINNING_RULE)
[uncategorized] ~210-~210: Possible missing comma found.
Context: ...cutable is either present in the system PATH or its path is provided via the --arbig...
(AI_HYDRA_LEO_MISSING_COMMA)
[formatting] ~259-~259: Did you mean “?”
Context: ...mand name const paths = process.env.PATH?.split(path.delimiter) || []; for (const ...
(MULTIPLE_PUNCTATION_MARKS_1)
[formatting] ~286-~286: Did you mean “?”
Context: ...L structure // Example: data.scenarios?.forEach(s => s.tags?.forEach(t => tags.a...
(MULTIPLE_PUNCTATION_MARKS_1)
[formatting] ~286-~286: Did you mean “?”
Context: ...ple: data.scenarios?.forEach(s => s.tags?.forEach(t => tags.add(t))); console.er...
(MULTIPLE_PUNCTATION_MARKS_1)
[style] ~299-~299: Three successive sentences begin with the same word. Consider rewording the sentence or use a thesaurus to find a synonym.
Context: ...projectYaml Path to the project YAML. * @param configYaml Path to the config YAML. * @...
(ENGLISH_WORD_REPEAT_BEGINNING_RULE)
[style] ~300-~300: Three successive sentences begin with the same word. Consider rewording the sentence or use a thesaurus to find a synonym.
Context: ...m configYaml Path to the config YAML. * @param tag Optional tag to execute. * @returns...
(ENGLISH_WORD_REPEAT_BEGINNING_RULE)
🪛 markdownlint-cli2 (0.17.2)
arbigent-mcp-server-specification.md
219-219: No space after hash on atx style heading
null
(MD018, no-missing-space-atx)
225-225: Spaces inside emphasis markers
null
(MD037, no-space-in-emphasis)
226-226: Spaces inside emphasis markers
null
(MD037, no-space-in-emphasis)
⏰ Context from checks skipped due to timeout of 90000ms (11)
- GitHub Check: cli-e2e-ios
- GitHub Check: cli-e2e-android (2, 2)
- GitHub Check: cli-e2e-android (1, 2)
- GitHub Check: build
- GitHub Check: build (windows-latest)
- GitHub Check: build (macos-latest)
- GitHub Check: store-screenshot-test
- GitHub Check: build (macos-13)
- GitHub Check: build (ubuntu-latest)
- GitHub Check: test
- GitHub Check: compare-screenshot-test
🔇 Additional comments (4)
arbigent-mcp-server-specification.md (4)
192-207: The Error Handling Summary reiterates FR-08 clearly and doesn’t introduce new information.
219-219: LGTM
Shebang line is correct for a Node.js CLI entrypoint.🧰 Tools
🪛 markdownlint-cli2 (0.17.2)
219-219: No space after hash on atx style heading
null(MD018, no-missing-space-atx)
361-367: LGTM
The CLI resolution flow matches the spec once the helper and checks are fully implemented.
390-402: LGTM
Therun-arbigent-test-alltool registration correctly follows the spec and wraps execution results.
| const process = spawn(cliPath, args); | ||
| let stdout = ''; | ||
| let stderr = ''; | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Avoid shadowing the process object
Rename the local process constant (spawn result) to something like childProcess to avoid confusion with the global process.
🤖 Prompt for AI Agents
In arbigent-mcp-server-specification.md around lines 316 to 319, the local
constant named `process` shadows the global `process` object, which can cause
confusion and potential bugs. Rename the local `process` variable to
`childProcess` or another distinct name to clearly differentiate it from the
global `process` object.
| CR-02: Node.js: Requires Node.js version [Specify Minimum Version, e.g., 18.x or higher]. | ||
|
|
||
| CR-03: Arbigent CLI: Designed to work with arbigent-cli version [Specify Target Version Range, if known, e.g., 1.x]. Compatibility with future versions depends on the stability of the CLI's arguments and output. | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Replace Node.js version placeholder
Specify the actual minimum supported Node.js version (e.g., “Node.js >= 18.x”) to avoid ambiguity during implementation.
🤖 Prompt for AI Agents
In arbigent-mcp-server-specification.md around lines 104 to 107, replace the
placeholder text for the Node.js version with the actual minimum supported
version, such as "Node.js >= 18.x". This will clarify the required Node.js
version and remove any ambiguity for implementers.
| FR-08: Error Handling: The server MUST handle and report errors, including: | ||
|
|
||
| Failure to find arbigent-cli (FR-03). | ||
|
|
||
| Failure to parse project.yaml. | ||
|
|
||
| Errors during the execution of arbigent-cli (e.g., invalid config, runtime errors during tests). | ||
|
|
||
| Errors should be reported back to the MCP client as informative text content. | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Standardize error response format
Define a uniform error object (e.g., { code: string; message: string; details?: string }) to ensure consistent MCP client handling across failure modes.
🤖 Prompt for AI Agents
In arbigent-mcp-server-specification.md around lines 80 to 89, the error
handling section lacks a standardized error response format. Define a uniform
error object structure with fields such as code (string), message (string), and
an optional details (string) to ensure consistent and clear error reporting back
to the MCP client across all failure scenarios.
| // --- Extract Tags --- | ||
| const tags = extractTagsFromYaml(args.project); | ||
| if (!tags) { | ||
| console.error("Error: Could not read or parse project YAML."); | ||
| // Decide whether to continue without tag-specific tools or exit | ||
| // process.exit(1); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Correct tags existence check
Since extractTagsFromYaml returns an array, if (!tags) will never fire. Use if (tags.length === 0) or validate for null explicitly.
🤖 Prompt for AI Agents
In arbigent-mcp-server-specification.md around lines 368 to 374, the check for
tags existence uses if (!tags), but since extractTagsFromYaml returns an array,
this condition will never be true. Update the condition to check if the array is
empty by using if (tags.length === 0) or explicitly check for null if the
function can return null. This ensures proper handling when no tags are
extracted.
| tags.forEach(tag => { | ||
| const toolName = `run-arbigent-test-${tag.replace(/[^a-zA-Z0-9_]/g, '_')}`; // Sanitize tag for tool name | ||
| server.tool( | ||
| toolName, | ||
| `Runs Arbigent tests with the tag '${tag}'.`, | ||
| {}, // No input parameters | ||
| async (params: z.infer<typeof z.object({})>): Promise<McpResponse> => { | ||
| console.error(`Executing tool: ${toolName} (tag: ${tag})`); | ||
| const result = await runArbigentCli(cliPath, args.project, args.config, tag); | ||
| const outputText = `Exit Code: ${result.exitCode}\n\nSTDOUT:\n${result.stdout}\n\nSTDERR:\n${result.stderr}`; | ||
| return { | ||
| content: [{ type: "text", text: outputText }], | ||
| }; | ||
| } | ||
| ); | ||
| console.error(`Registered tool: ${toolName}`); | ||
| }); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Sanitize and dedupe tool names
Ensure sanitized tag-based tool names are unique (e.g., handle collisions like a-b vs. a_b) to prevent registration conflicts.
🤖 Prompt for AI Agents
In arbigent-mcp-server-specification.md around lines 405 to 421, the tool names
generated by sanitizing tags may collide if different tags produce the same
sanitized string (e.g., 'a-b' and 'a_b'). To fix this, implement a mechanism to
track generated tool names and append a unique suffix or index to duplicates to
ensure all tool names are unique before registering them with server.tool.
| function extractTagsFromYaml(projectYamlPath: string): string[] { | ||
| try { | ||
| const fileContents = fs.readFileSync(projectYamlPath, 'utf8'); | ||
| const data = yaml.load(fileContents) as any; // Adjust type based on YAML structure | ||
| const tags = new Set<string>(); | ||
| // TODO: Implement logic to extract tags based on the actual YAML structure | ||
| // Example: data.scenarios?.forEach(s => s.tags?.forEach(t => tags.add(t))); | ||
| console.error("Extracted tags:", Array.from(tags)); | ||
| return Array.from(tags); | ||
| } catch (error) { |
There was a problem hiding this comment.
Fix placeholder tag extraction and syntax error
The implementation lacks real parsing logic and has an extra closing }. Replace the TODO with actual scenario/tag traversal (e.g., data.scenarios?.forEach(s => s.tags?.forEach(t => tags.add(t)))) and remove the stray brace at line 293.
🧰 Tools
🪛 LanguageTool
[formatting] ~286-~286: Did you mean “?”
Context: ...L structure // Example: data.scenarios?.forEach(s => s.tags?.forEach(t => tags.a...
(MULTIPLE_PUNCTATION_MARKS_1)
[formatting] ~286-~286: Did you mean “?”
Context: ...ple: data.scenarios?.forEach(s => s.tags?.forEach(t => tags.add(t))); console.er...
(MULTIPLE_PUNCTATION_MARKS_1)
🤖 Prompt for AI Agents
In arbigent-mcp-server-specification.md around lines 280 to 289, replace the
TODO comment with actual logic to traverse the YAML data structure and extract
tags, for example by iterating over data.scenarios and their tags to add them to
the tags set. Also, remove the extra closing brace at line 293 to fix the syntax
error.
| // import { ArgumentParser } from 'argparse'; // Consider using an argument parser like yargs or commander | ||
|
|
||
| // --- Constants --- | ||
| // (Define constants if needed) | ||
|
|
||
| // --- Argument Parsing (Example) --- | ||
| // TODO: Implement robust argument parsing using yargs, commander, or similar | ||
| // Example: | ||
| // const args = { | ||
| // project: process.argv[2] || 'arbigent-project.yaml', // Temporary default | ||
| // config: process.argv[3] || 'arbigent-config.yaml', // Temporary default |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Implement argument parsing
Replace the placeholder comments and defaults with a robust CLI parser (e.g., yargs or commander) as outlined in Phase 1 to ensure reliable option handling.
🤖 Prompt for AI Agents
In arbigent-mcp-server-specification.md around lines 228 to 238, the current
code uses placeholder comments and manual defaults for argument parsing. Replace
these with a robust CLI argument parser like yargs or commander by importing the
chosen library, defining the expected options and their defaults, and parsing
process.argv accordingly to handle options reliably and cleanly.
| function resolveArbigentCliPath(explicitPath: string | null): string | null { | ||
| if (explicitPath) { | ||
| // TODO: Verify if explicitPath actually exists and is executable | ||
| console.error(`Using explicit path: ${explicitPath}`); | ||
| return explicitPath; | ||
| } | ||
|
|
||
| // Search in PATH environment variable (may need cross-platform handling) | ||
| const command = "arbigent-cli"; // Or the actual command name |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Validate explicit CLI path
Check that explicitPath exists and is executable (e.g., using fs.statSync or fs.accessSync) before returning, otherwise fallback detection may be skipped incorrectly.
🤖 Prompt for AI Agents
In arbigent-mcp-server-specification.md around lines 250 to 258, the function
resolveArbigentCliPath returns the explicitPath without verifying if the path
exists and is executable. Update the function to check if explicitPath exists
and has execute permissions using appropriate filesystem methods like
fs.statSync or fs.accessSync. Only return explicitPath if these checks pass;
otherwise, continue with the fallback detection logic.
| If --arbigent-bin-path is provided, the server MUST use that path to locate arbigent-cli. | ||
|
|
||
| If --arbigent-bin-path is not provided, the server MUST attempt to find arbigent-cli in the system's PATH environment variable. | ||
|
|
||
| If arbigent-cli cannot be found using either method, the server MUST log a fatal error and exit immediately upon startup or connection attempt. | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Specify exit behavior and error format
When the CLI isn’t found, clarify whether the server should return an MCP error payload (with code/message) or simply exit. Define the expected MCP‐level error response structure.
🧰 Tools
🪛 LanguageTool
[style] ~55-~55: Three successive sentences begin with the same word. Consider rewording the sentence or use a thesaurus to find a synonym.
Context: ...he system's PATH environment variable. If arbigent-cli cannot be found using eith...
(ENGLISH_WORD_REPEAT_BEGINNING_RULE)
🤖 Prompt for AI Agents
In arbigent-mcp-server-specification.md around lines 52 to 57, clarify the
server's behavior when arbigent-cli is not found by specifying whether it should
return an MCP error payload or just exit. Define the exact MCP-level error
response structure including the error code and message format that the server
must use before exiting or terminating the connection.
No description provided.