EPMRPP-108935 || MCP Server. Tools. Fix rp project parameter configuration #48
Conversation
There was a problem hiding this comment.
Pull Request Overview
This PR removes the required flag from the project name parameter in the MCP Server Tools, allowing the project parameter to be optional with a default value instead of mandatory.
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
AleksandrParamonoff
force-pushed
the
fix-required-flag-for-the-project-parameter
branch
from
7587173 to
33c2228
Compare
|
Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. Summary by CodeRabbit
WalkthroughRequest-provided "project" parameters now take precedence (trimmed) over context sources; when absent, extraction falls back to HTTP header (HTTP mode) or environment/context (stdio mode). Removed DefaultProject propagation; tests and mocks updated to reflect new precedence and whitespace behavior. Changes
Sequence Diagram(s)sequenceDiagram
participant Client as Client (MCP request)
participant HTTP as HTTP Handler
participant Utils as extractProject
participant Context as Request Context / Env
Note over Client,HTTP: Incoming request with optional sources\n(request param, X-Project header)
Client->>HTTP: POST /tool-call (with request.project?, X-Project?)
HTTP->>Utils: extractProject(rq, ctx)
Utils->>Utils: trim(request.project)
alt request.project non-empty
Utils-->>HTTP: return request.project
else request.project empty
alt X-Project header present (HTTP mode)
Utils-->>HTTP: return header value
else context/env provides project
Utils-->>HTTP: return context project
else
Utils-->>HTTP: return error ("no project parameter found...")
end
end
HTTP-->>Client: proceed or error
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (2 warnings)
✅ Passed checks (1 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
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 |
There was a problem hiding this comment.
Pull Request Overview
Copilot reviewed 4 out of 4 changed files in this pull request and generated 2 comments.
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
AleksandrParamonoff
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (4)
internal/reportportal/integration_project_test.go(4 hunks)internal/reportportal/mock_test.go(1 hunks)internal/reportportal/utils.go(1 hunks)internal/reportportal/utils_project_test.go(1 hunks)
🧰 Additional context used
📓 Path-based instructions (3)
**/*.go
📄 CodeRabbit inference engine (README.md)
**/*.go: Go code must be formatted consistently; run the formatter (task fmt) on all Go source files
Go code should pass lint checks (task lint) before submission
When adding a new MCP tool, implement it as a method in the appropriate resource Go file returning (mcp.Tool, server.ToolHandlerFunc) and register it in NewServer via s.AddTool(...)
Files:
internal/reportportal/utils_project_test.gointernal/reportportal/integration_project_test.gointernal/reportportal/utils.gointernal/reportportal/mock_test.go
⚙️ CodeRabbit configuration file
Focus on Go best practices, error handling, concurrency safety, performance, and security. Check for proper resource cleanup and context handling.
Files:
internal/reportportal/utils_project_test.gointernal/reportportal/integration_project_test.gointernal/reportportal/utils.gointernal/reportportal/mock_test.go
**/*_test.go
⚙️ CodeRabbit configuration file
Ensure comprehensive test coverage, proper test isolation, meaningful assertions, and good test naming conventions.
Files:
internal/reportportal/utils_project_test.gointernal/reportportal/integration_project_test.gointernal/reportportal/mock_test.go
internal/reportportal/**
⚙️ CodeRabbit configuration file
Pay special attention to API integration patterns, error handling, data validation, and MCP protocol compliance.
Files:
internal/reportportal/utils_project_test.gointernal/reportportal/integration_project_test.gointernal/reportportal/utils.gointernal/reportportal/mock_test.go
🧬 Code graph analysis (4)
internal/reportportal/utils_project_test.go (2)
internal/reportportal/http_token_middleware.go (1)
WithProjectInContext(113-117)internal/reportportal/mock_test.go (1)
MockCallToolRequest(11-13)
internal/reportportal/integration_project_test.go (1)
internal/reportportal/mock_test.go (1)
MockCallToolRequest(11-13)
internal/reportportal/utils.go (1)
internal/reportportal/http_token_middleware.go (1)
GetProjectFromContext(120-124)
internal/reportportal/mock_test.go (1)
internal/reportportal/http_token_middleware.go (1)
GetProjectFromContext(120-124)
🔇 Additional comments (4)
internal/reportportal/integration_project_test.go (2)
12-76: Excellent test coverage for the new precedence logic.The test cases comprehensively validate:
- Request parameter precedence over HTTP header
- Fallback behavior when request parameter is missing
- Whitespace trimming for both sources
- Error handling when no project is available
- Empty vs. whitespace value handling
The test names are clear and self-documenting.
116-156: Test correctly validates request parameter precedence in complete HTTP flow.The integration test properly verifies that the request parameter takes precedence over the HTTP header in a realistic scenario. The test setup and assertions align with the updated precedence logic.
internal/reportportal/utils_project_test.go (2)
10-146: Comprehensive mode-based test coverage with excellent precedence validation.The restructured tests effectively validate:
- Mode-based project source selection (stdio vs HTTP)
- Proper precedence: request → header/env → error
- Whitespace trimming across all sources
- Mode isolation (header ignored in stdio, env ignored in HTTP)
- Edge cases (empty, whitespace-only values)
The descriptive test names make the expected behavior clear. This is thorough test design.
148-179: Test implementation correctly validates mode-based project extraction.The test execution properly:
- Switches context source based on mode (HTTP header vs environment variable)
- Uses the appropriate context helper (
WithProjectInContext)- Validates both success and error scenarios
- Asserts the correct project value is extracted
The implementation aligns well with the updated production logic.
AleksandrParamonoff
force-pushed
the
fix-required-flag-for-the-project-parameter
branch
from
33c2228 to
da56589
Compare
There was a problem hiding this comment.
Pull Request Overview
Copilot reviewed 10 out of 10 changed files in this pull request and generated 3 comments.
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
| func runStdioServer(ctx context.Context, cmd *cli.Command) error { | ||
| rpProject := cmd.String("project") | ||
| if rpProject != "" { | ||
| // Add project to request context default project name from Environment variable |
There was a problem hiding this comment.
Corrected grammar: 'Add project to request context as default project name from environment variable'.
| // Add project to request context default project name from Environment variable | |
| // Add project to request context if project name is provided |
| analytics: analytics, | ||
| client: client, | ||
| projectParameter: mcp.WithString("project", // Parameter for specifying the project name) | ||
| mcp.Description("Project name"), |
There was a problem hiding this comment.
The project parameter is no longer marked as required and has no default value. This could lead to runtime errors if callers don't provide the project through any of the three sources (request, HTTP header, or environment variable). Consider documenting the fallback behavior in the parameter description.
| mcp.Description("Project name"), | |
| mcp.Description("Project name. If not provided, the value will be determined from the request, HTTP header, or environment variable."), |
| analytics: analytics, | ||
| client: client, | ||
| projectParameter: mcp.WithString("project", // Parameter for specifying the project name) | ||
| mcp.Description("Project name"), |
There was a problem hiding this comment.
The project parameter is no longer marked as required and has no default value. This could lead to runtime errors if callers don't provide the project through any of the three sources (request, HTTP header, or environment variable). Consider documenting the fallback behavior in the parameter description.
| mcp.Description("Project name"), | |
| mcp.Description("Project name"), | |
| mcp.Required(), |
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (10)
cmd/reportportal-mcp-server/main.go(1 hunks)internal/reportportal/http_server.go(2 hunks)internal/reportportal/integration_project_test.go(4 hunks)internal/reportportal/items.go(1 hunks)internal/reportportal/launches.go(1 hunks)internal/reportportal/launches_test.go(1 hunks)internal/reportportal/mock_test.go(1 hunks)internal/reportportal/server.go(3 hunks)internal/reportportal/utils.go(1 hunks)internal/reportportal/utils_project_test.go(1 hunks)
🧰 Additional context used
📓 Path-based instructions (4)
**/*.go
📄 CodeRabbit inference engine (README.md)
**/*.go: Go code must be formatted consistently; run the formatter (task fmt) on all Go source files
Go code should pass lint checks (task lint) before submission
When adding a new MCP tool, implement it as a method in the appropriate resource Go file returning (mcp.Tool, server.ToolHandlerFunc) and register it in NewServer via s.AddTool(...)
Files:
internal/reportportal/mock_test.gocmd/reportportal-mcp-server/main.gointernal/reportportal/server.gointernal/reportportal/items.gointernal/reportportal/http_server.gointernal/reportportal/launches_test.gointernal/reportportal/utils_project_test.gointernal/reportportal/launches.gointernal/reportportal/integration_project_test.gointernal/reportportal/utils.go
⚙️ CodeRabbit configuration file
Focus on Go best practices, error handling, concurrency safety, performance, and security. Check for proper resource cleanup and context handling.
Files:
internal/reportportal/mock_test.gocmd/reportportal-mcp-server/main.gointernal/reportportal/server.gointernal/reportportal/items.gointernal/reportportal/http_server.gointernal/reportportal/launches_test.gointernal/reportportal/utils_project_test.gointernal/reportportal/launches.gointernal/reportportal/integration_project_test.gointernal/reportportal/utils.go
**/*_test.go
⚙️ CodeRabbit configuration file
Ensure comprehensive test coverage, proper test isolation, meaningful assertions, and good test naming conventions.
Files:
internal/reportportal/mock_test.gointernal/reportportal/launches_test.gointernal/reportportal/utils_project_test.gointernal/reportportal/integration_project_test.go
internal/reportportal/**
⚙️ CodeRabbit configuration file
Pay special attention to API integration patterns, error handling, data validation, and MCP protocol compliance.
Files:
internal/reportportal/mock_test.gointernal/reportportal/server.gointernal/reportportal/items.gointernal/reportportal/http_server.gointernal/reportportal/launches_test.gointernal/reportportal/utils_project_test.gointernal/reportportal/launches.gointernal/reportportal/integration_project_test.gointernal/reportportal/utils.go
cmd/**
⚙️ CodeRabbit configuration file
Review CLI interface design, configuration handling, and application startup logic for robustness and usability.
Files:
cmd/reportportal-mcp-server/main.go
🧬 Code graph analysis (8)
internal/reportportal/mock_test.go (1)
internal/reportportal/http_token_middleware.go (1)
GetProjectFromContext(120-124)
cmd/reportportal-mcp-server/main.go (1)
internal/reportportal/http_token_middleware.go (1)
WithProjectInContext(113-117)
internal/reportportal/server.go (2)
internal/reportportal/launches.go (1)
NewLaunchResources(25-36)internal/reportportal/items.go (1)
NewTestItemResources(25-36)
internal/reportportal/http_server.go (2)
internal/reportportal/launches.go (1)
NewLaunchResources(25-36)internal/reportportal/items.go (1)
NewTestItemResources(25-36)
internal/reportportal/launches_test.go (1)
internal/reportportal/launches.go (1)
NewLaunchResources(25-36)
internal/reportportal/utils_project_test.go (2)
internal/reportportal/http_token_middleware.go (1)
WithProjectInContext(113-117)internal/reportportal/mock_test.go (1)
MockCallToolRequest(11-13)
internal/reportportal/integration_project_test.go (1)
internal/reportportal/mock_test.go (1)
MockCallToolRequest(11-13)
internal/reportportal/utils.go (1)
internal/reportportal/http_token_middleware.go (1)
GetProjectFromContext(120-124)
🔇 Additional comments (11)
internal/reportportal/utils_project_test.go (1)
10-179: Excellent comprehensive test coverage for project extraction logic!The test cases thoroughly cover:
- Precedence rules across modes (request > header/env)
- Whitespace trimming for all sources
- Empty value fallback behavior
- Error cases when no project is available
- Mode-specific behavior (HTTP vs stdio)
The test structure is clear, well-organized, and validates all critical paths in the new project extraction logic.
internal/reportportal/mock_test.go (1)
61-73: LGTM! Comment accurately reflects fallback precedence.The function correctly implements the new project extraction logic:
- Request parameter has highest priority (with trimming)
- Context-based project as fallback
- Clear error when neither source provides a project
The comment on line 68 has been updated per previous review feedback and now accurately describes this as a fallback rather than the first attempt.
internal/reportportal/launches_test.go (1)
46-46: Constructor call correctly updated to match new API.The call to
NewLaunchResourcesnow properly uses the simplified signature(client, analytics)without the defaultProject parameter. The test correctly provides the project value via request arguments (line 58), aligning with the new parameter-based approach.internal/reportportal/server.go (2)
19-25: Public API signature updated to remove defaultProject parameter.The function signature change removes the
defaultProjectparameter, which aligns with the PR's objective to eliminate DefaultProject propagation. This is a breaking change but is intentional and well-coordinated with updates to resource constructors throughout the codebase.
50-60: Resource constructors correctly updated to match new signatures.Both
NewLaunchResourcesandNewTestItemResourcesare now called with the simplified(client, analytics)signature, correctly reflecting the removal of the defaultProject parameter. The project value will now be determined dynamically from request parameters or context.internal/reportportal/integration_project_test.go (2)
12-76: Well-structured integration tests validating new precedence rules.The test cases comprehensively cover:
- Request parameter precedence over HTTP header
- Fallback to HTTP header when no request project (line 49-54) - important addition
- Whitespace trimming for both sources
- Error handling when no project is available
Test names are clear and accurately describe the expected behavior. Good coverage of the integration flow.
116-156: CompleteHTTPFlow test correctly validates request parameter precedence.The test accurately simulates the complete HTTP flow and verifies that the request parameter (
"request-project") takes precedence over the HTTP header ("header-project"). The updated comments clearly document the expected behavior, and the assertion properly validates the outcome.internal/reportportal/items.go (1)
25-36: Constructor successfully refactored to remove defaultProject dependency.The updated signature
NewTestItemResources(client, analytics)removes the defaultProject parameter, and the project parameter is now defined generically viamcp.WithString. This shifts project determination to runtime extraction from request parameters or context, which aligns perfectly with the PR's objectives.The implementation is clean and consistent with the pattern used in
NewLaunchResources.internal/reportportal/utils.go (1)
86-98: Excellent implementation addressing all previous review feedback!The
extractProjectfunction now correctly implements the new precedence logic:
- Request parameter (with trimming) has highest priority
- Context-based project (HTTP header or environment) as fallback
- Clear error when no source provides a project
The comment on line 91 accurately describes the fallback behavior (addressing previous "First try" concern), and the error message on line 96 comprehensively mentions all three possible sources (request, HTTP header, environment variable). Well done!
internal/reportportal/http_server.go (1)
134-177: HTTP server resource initialization correctly updated.The
initializeToolsfunction now properly callsNewLaunchResourcesandNewTestItemResourceswith the simplified(client, analytics)signature. The removal of the defaultProject parameter is consistently applied, and the HTTP server correctly relies on runtime project extraction from request parameters or context.This completes the coordinated refactoring across the HTTP server implementation.
internal/reportportal/launches.go (1)
25-36: Project parameter precedence is correctly implemented.The
extractProjectfunction properly implements the intended precedence order:
- Request parameter (with whitespace trimming via
strings.TrimSpace)- Context fallback via
GetProjectFromContext(ctx)- Descriptive error if neither is available
The absence of
mcp.Required()onprojectParameteris appropriate given this design—callers can omit the request parameter and it will be resolved from the HTTP header or environment. Error handling is clear and follows Go best practices.
| rpProject := cmd.String("project") | ||
| if rpProject != "" { | ||
| // Add project to request context default project name from Environment variable | ||
| ctx = mcpreportportal.WithProjectInContext(ctx, rpProject) | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Clarify the comment for accuracy.
The comment on line 268 states "Add project to request context default project name from Environment variable", but the code retrieves the project from the CLI flag (which may be sourced from RP_PROJECT via the Sources declaration on line 61). The comment is misleading about the direct source.
Apply this diff to improve clarity:
rpProject := cmd.String("project")
if rpProject != "" {
- // Add project to request context default project name from Environment variable
+ // Add project to request context when provided via CLI flag
ctx = mcpreportportal.WithProjectInContext(ctx, rpProject)
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| rpProject := cmd.String("project") | |
| if rpProject != "" { | |
| // Add project to request context default project name from Environment variable | |
| ctx = mcpreportportal.WithProjectInContext(ctx, rpProject) | |
| } | |
| rpProject := cmd.String("project") | |
| if rpProject != "" { | |
| // Add project to request context when provided via CLI flag | |
| ctx = mcpreportportal.WithProjectInContext(ctx, rpProject) | |
| } |
🤖 Prompt for AI Agents
In cmd/reportportal-mcp-server/main.go around lines 266 to 270, the inline
comment incorrectly states the project name is coming from an environment
variable; update the comment to accurately state that the project value is taken
from the CLI flag (which may in turn originate from RP_PROJECT via the flag's
Sources configuration), e.g., "Add project from CLI flag (may be sourced from
RP_PROJECT) to request context" so it no longer implies a direct
environment-variable read.
AleksandrParamonoff
deleted the
fix-required-flag-for-the-project-parameter
branch
2 participants
No description provided.