MCP Server: refactoring: all application part is separated now #45
Conversation
|
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
WalkthroughStartup moved to a config-driven RunApp(ctx, AppVersion) flow; analytics and MCP helpers were centralized into internal/utils and internal/analytics; HTTP server, middleware, security, and mcp handlers were refactored to use exported utils/analytics surfaces and new Register* tool APIs; batch/JSON‑RPC tests added. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor User
participant Main as cmd/main
participant Config as internal/config.RunApp
participant Logger as InitLogger
participant Builder as NewMCPServer / BuildHTTPServerConfig
participant MCP as MCP Server
participant HTTP as HTTP Server
participant Analytics as analytics.NewAnalytics
User->>Main: start binary (env/flags)
Main->>Config: RunApp(ctx, AppVersion)
Config->>Logger: InitLogger()
Config->>Config: resolve mode (stdio / http)
alt stdio
Config->>Builder: NewMCPServer(...)
Builder->>Analytics: NewAnalytics(...)
Builder-->>Config: *MCP, *Analytics
Config->>MCP: run stdio server
else http
Config->>Builder: BuildHTTPServerConfig(...)
Builder->>HTTP: CreateHTTPServerWithMiddleware(...)
HTTP->>Analytics: NewAnalytics(...)
HTTP-->>Config: server, *Analytics
Config->>HTTP: ListenAndServe
end
Config->>Config: HandleServerError(err, analytics, type)
sequenceDiagram
autonumber
participant Router as HTTP Router
participant Sec as security.HTTPTokenMiddleware
participant Mid as middleware.QueryParamsMiddleware
participant Handlers as mcp_handlers
participant Utils as utils
participant RP as ReportPortal API
Router->>Sec: incoming request
Sec->>Router: validate & inject token (utils.ValidateRPToken)
Router->>Mid: populate query params in ctx (utils.QueryParamsFromContext)
Router->>Handlers: invoke tool handler (wrapped via utils.WithAnalytics)
Handlers->>Utils: ExtractProject / ApplyPaginationOptions / ReadResponseBody
Handlers->>RP: call ReportPortal API
RP-->>Handlers: response
Handlers-->>Router: MCP result
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Poem
Pre-merge checks and finishing touches❌ Failed checks (2 warnings, 1 inconclusive)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro 📒 Files selected for processing (18)
🧰 Additional context used📓 Path-based instructions (4)**/*.go📄 CodeRabbit inference engine (README.md)
Files:
⚙️ CodeRabbit configuration file
Files:
**/*_test.go⚙️ CodeRabbit configuration file
Files:
cmd/reportportal-mcp-server/**/*.go📄 CodeRabbit inference engine (README.md)
Files:
cmd/**⚙️ CodeRabbit configuration file
Files:
🧠 Learnings (1)📚 Learning: 2025-09-17T11:00:40.261ZApplied to files:
🧬 Code graph analysis (13)internal/mcp_handlers/items.go (4)
internal/mcp_handlers/launches.go (4)
internal/mcp_handlers/server.go (5)
internal/http/http_server.go (7)
internal/middleware/middleware.go (2)
internal/security/http_token_middleware.go (1)
internal/http/http_server_batch_test.go (1)
internal/middleware/middleware_test.go (1)
cmd/reportportal-mcp-server/main.go (1)
internal/analytics/analytics_test.go (1)
internal/utils/utils_test.go (1)
internal/utils/mcp_utils.go (1)
internal/config/cli.go (3)
🔇 Additional comments (15)
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
This PR refactors the MCP server codebase by separating the application logic into appropriate packages. The refactoring moves from a single mcpreportportal package to a more modular structure with distinct packages for different concerns.
- Separates utilities, analytics, security, middleware, HTTP handling, and MCP handlers into their own packages
- Creates proper separation of concerns with dedicated packages for config, security, analytics, and HTTP handling
- Maintains all existing functionality while improving code organization and maintainability
Reviewed Changes
Copilot reviewed 19 out of 20 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| internal/utils/ | Utility functions moved from main package with proper exports and imports |
| internal/analytics/ | Analytics functionality separated into its own package |
| internal/security/ | HTTP token middleware moved to dedicated security package |
| internal/middleware/ | Query parameter middleware separated into middleware package |
| internal/mcp_handlers/ | MCP handlers moved with registration functions for better organization |
| internal/http/ | HTTP server functionality moved to dedicated package |
| internal/config/ | CLI configuration and command creation moved to config package |
| cmd/reportportal-mcp-server/main.go | Main function simplified to use the new config package |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
AleksandrParamonoff
force-pushed
the
refactoring-detaching-app-to-modules
branch
from
7a1a071 to
a456b0b
Compare
There was a problem hiding this comment.
Pull Request Overview
Copilot reviewed 19 out of 20 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.
AleksandrParamonoff
force-pushed
the
add-remote-local-deploy-tasks
branch
2 times, most recently
from
9a10381 to
18dce09
Compare
AleksandrParamonoff
force-pushed
the
refactoring-detaching-app-to-modules
branch
from
a456b0b to
8d438fb
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 21
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (22)
internal/mcp_handlers/launches_test.go (3)
22-28: Fix argument order in assertions for clearer failuresTestify expects expected, actual. Swap args to avoid confusing diffs.
- require.Equal(t, vals.Get("filter").String(), "xxx") - require.Equal(t, vals.Get("launch").String(), "123") + require.Equal(t, "xxx", vals.Get("filter").String()) + require.Equal(t, "123", vals.Get("launch").String())
33-41: Handle marshal error and use the marshaled bytes consistentlyAvoid ignoring errors; use a single var for the JSON and assert against it.
- launches, _ := json.Marshal(testLaunches()) + launchesJSON, err := json.Marshal(testLaunches()) + require.NoError(t, err) @@ - _, _ = w.Write(launches) + _, _ = w.Write(launchesJSON) @@ - assert.Equal(t, string(launches), text) + assert.Equal(t, string(launchesJSON), text)Also applies to: 68-69
64-66: Guard against empty content before indexingAdd a length check to make the failure message actionable.
- var textContent mcp.TextContent - require.IsType(t, textContent, result.Content[0]) + require.NotEmpty(t, result.Content, "tool response should contain at least one content item") + var textContent mcp.TextContent + require.IsType(t, textContent, result.Content[0])internal/utils/ctx_utils_test.go (2)
10-12: Make the test helper unexported (or add a doc comment) to satisfy lintersExported symbol in tests without doc triggers lint warnings. Unexport for clarity.
-func IsUUIDFormat(token string) bool { +func isUUIDFormat(token string) bool { @@ - result := IsUUIDFormat(tt.token) + result := isUUIDFormat(tt.token)Also applies to: 67-72
76-102: Cover trim and threshold edges in ValidateRPToken testsAdd cases for surrounding spaces (should pass) and 15-char token (should fail with current >=16 rule).
{ name: "long token without prefix should pass", token: "this_is_a_very_long_token_that_should_pass", expected: true, }, + { + name: "valid UUID with surrounding spaces should pass", + token: " 014b329b-a882-4c2d-9988-c2f6179a421b ", + expected: true, + }, + { + name: "15-char token should fail threshold", + token: "aaaaaaaaaaaaaaa", + expected: false, + },If RP tokens have an officially documented minimum length, confirm it and adjust the threshold accordingly.
internal/middleware/middleware.go (1)
17-27: Nit: Skip encode on empty params to avoid needless allocationsMicro-optimization: bail out if no paramsFromContext entries.
- paramsFromContext, ok := utils.QueryParamsFromContext(rq.Context()) - if ok && paramsFromContext != nil { + paramsFromContext, ok := utils.QueryParamsFromContext(rq.Context()) + if ok && paramsFromContext != nil && len(paramsFromContext) > 0 {internal/utils/utils_test.go (1)
154-158: Use strconv.Itoa for clearer, printable perf test datastring(rune(i)) yields control chars for small i. Prefer decimal strings to keep test data readable and consistent.
- for i := 0; i < 1000; i++ { - keys = append(keys, "key"+string(rune(i))+":") - } + for i := 0; i < 1000; i++ { + keys = append(keys, "key"+strconv.Itoa(i)+":") + }Add: import "strconv".
As per coding guidelines.internal/analytics/analytics.go (5)
274-291: Stop can block longer than the advertised timeout; make shutdown boundedIf a send is in-flight (httpClient.Timeout = 10s), Stop may wait past the 5s timeout due to the unconditional a.wg.Wait(). Bound the wait and proceed with shutdown.
func (a *Analytics) Stop() { if a == nil || a.stopChan == nil { return } slog.Debug("Stopping analytics metrics processor") close(a.stopChan) - // Wait for ticker goroutine to finish - select { - case <-a.tickerDone: - slog.Debug("Analytics metrics processor stopped gracefully") - case <-time.After(5 * time.Second): - slog.Warn("Analytics metrics processor stop timeout") - } - - a.wg.Wait() + // Wait for ticker goroutine to finish, but don’t block indefinitely + select { + case <-a.tickerDone: + slog.Debug("Analytics metrics processor stopped gracefully") + case <-time.After(5 * time.Second): + slog.Warn("Analytics metrics processor stop timeout") + } + done := make(chan struct{}) + go func() { a.wg.Wait(); close(done) }() + select { + case <-done: + slog.Debug("Analytics worker goroutine joined") + case <-time.After(5 * time.Second): + slog.Warn("Analytics worker join timeout; proceeding with shutdown") + } }Additionally consider lowering httpClient.Timeout or making it configurable to align with shutdown expectations.
As per coding guidelines.
62-68: Correct the field name to match units and JSON: TimestampMicrosThe struct uses TimestampMS for a JSON key timestamp_micros, and you populate it with UnixMicro(). Rename for clarity.
-type GAPayload struct { - ClientID string `json:"client_id"` - UserID string `json:"user_id,omitempty"` - Events []GAEvent `json:"events"` - TimestampMS int64 `json:"timestamp_micros"` -} +type GAPayload struct { + ClientID string `json:"client_id"` + UserID string `json:"user_id,omitempty"` + Events []GAEvent `json:"events"` + TimestampMicros int64 `json:"timestamp_micros"` +}And update the assignment site accordingly.
As per coding guidelines.
152-158: Fix comments to reflect actual identifiersBoth ClientID and UserID are set to the SHA256 hex hash, not “number with '.' postfix”. Update the comments to avoid confusion.
- ClientID: a.config.UserID, // Contains number with "." postfix - UserID: a.config.UserID, // Contains number with "." postfix + ClientID: a.config.UserID, // Stable pseudonymous ID (SHA256 hex of RP token) + UserID: a.config.UserID, // Same as ClientID for GA4 correlation
26-31: HashAlgorithm label is misleadingHashAlgorithm = "SHA256-128bit" contradicts the full SHA256 hex produced by HashToken. Either drop the constant or rename to reflect 256-bit.
- HashAlgorithm = "SHA256-128bit" + HashAlgorithm = "SHA256-256bit"
164-177: Consider minimizing payload/response logging to reduce PII footprint and log volumeYou pretty-print entire request/response bodies. Even with hashed IDs, this can be high-volume and potentially sensitive. Recommend gating under a more verbose level or redacting IDs.
- Log counts and status only at debug.
- Move full payload/response to trace or behind an env flag.
As per coding guidelines.internal/mcp_handlers/server.go (1)
76-92: Slice preallocation bug: handlers has len(entries) zero-values then appends prompts.This returns extra empty elements. Initialize with length 0 and capacity len(entries) (or no prealloc).
- handlers := make([]promptreader.PromptHandlerPair, len(entries)) + handlers := make([]promptreader.PromptHandlerPair, 0, len(entries))Optionally guard directories:
- for _, entry := range entries { + for _, entry := range entries { + if entry.IsDir() { + continue + }Add a unit test to assert no zero-value entries are present. As per coding guidelines.
internal/http/http_server.go (3)
455-468: Unbounded body read: add max payload limit to prevent memory abuse.Reading arbitrary-size bodies risks OOM/DoS. Limit to a sane size before json.Unmarshal.
func (hs *HTTPServer) validateMCPPayload(r *http.Request) bool { - // Read the request body - body, err := io.ReadAll(r.Body) + // Read the request body with a limit (e.g., 2 MiB) + const maxMCPPayload = 2 << 20 // 2 MiB + body, err := io.ReadAll(io.LimitReader(r.Body, maxMCPPayload)) if err != nil { return false }Optionally add an explicit size check and reject if exactly at limit with trailing data. As per best practices.
444-447: Content-Type check: parse media type instead of substring match.Substring checks can be brittle. Use mime.ParseMediaType to handle parameters and case correctly.
- contentType := r.Header.Get("Content-Type") - if !strings.Contains(contentType, "application/json") { + ct, _, _ := mime.ParseMediaType(r.Header.Get("Content-Type")) + if ct != "application/json" { return false }Add:
+ "mime"to imports.
31-41: Transport tuning nit: IdleConnTimeout tied to request timeout.IdleConnTimeout typically benefits from a higher value (e.g., 90s) independent of per-request timeout, improving connection reuse.
- IdleConnTimeout: timeout, + IdleConnTimeout: 90 * time.Second,This is a minor performance tweak; keep as-is if constrained by environment.
internal/utils/mcp_utils.go (1)
92-105: ReadResponseBody: good cleanup wrapper; consider propagating HTTP status on errors.Optional: include response.StatusCode in the error text to aid debugging.
- return mcp.NewToolResultError( - fmt.Sprintf("failed to read response body: %v", err), - ), nil + status := 0 + if response != nil { + status = response.StatusCode + } + return mcp.NewToolResultError( + fmt.Sprintf("failed to read response body (status %d): %v", status, err), + ), nilinternal/utils/utils.go (3)
154-174: Fix IsTextContent switch: only 'application/csv' currently returns trueEach case is empty except the last; Go doesn't fall through implicitly. As written, JSON/XML/etc. return false.
Apply:
- switch lowerType { - case "application/json": - case "application/xml": - case "application/javascript": - case "application/xhtml+xml": - case "application/yaml": - case "application/csv": - return true - } + switch lowerType { + case "application/json", + "application/xml", + "application/javascript", + "application/xhtml+xml", + "application/yaml", + "application/csv": + return true + }
123-151: Attribute key processing drops the key and keeps the value (likely wrong)For inputs like "env:prod", the code keeps "prod" instead of ensuring "env:". This corrupts filters.
Use the key prefix before ':' and normalize with a trailing colon; then merge:
- if colonIndex := strings.Index(key, ":"); colonIndex > 0 && colonIndex < len(key)-1 { - processed = append(processed, key[colonIndex+1:]) // Extract postfix - } else if !strings.HasSuffix(key, ":") { - processed = append(processed, key+":") // Add suffix - } else { - processed = append(processed, key) // Keep as is - } + // Keep only the key prefix and ensure trailing colon + if idx := strings.IndexByte(key, ':'); idx >= 0 { + key = key[:idx] + } + key = strings.TrimSpace(key) + if key == "" { + continue + } + processed = append(processed, key+":")Follow-up: add table-driven tests covering:
- "key1,key2" -> "key1:,key2:"
- "key1:,key2:" -> "key1:,key2:"
- "key1:value,key2:val2" -> "key1:,key2:"
- Merge with existing filterAttributes.
37-62: Bound response-body reads to avoid unbounded memory on large/error responsesBoth ExtractResponseError and ReadResponseBodyRaw call io.ReadAll without limits. A hostile upstream could return very large bodies.
Cap reads (e.g., 1MiB):
+ const maxBodyBytes = 1 << 20 // 1 MiB ... - if errContent, rErr := io.ReadAll(rs.Body); rErr == nil { + if errContent, rErr := io.ReadAll(io.LimitReader(rs.Body, maxBodyBytes)); rErr == nil { errText = errText + ": " + string(errContent) }And:
- rawBody, err := io.ReadAll(response.Body) + rawBody, err := io.ReadAll(io.LimitReader(response.Body, maxBodyBytes))Optionally log when truncated; keep it simple for now.
Also applies to: 193-215
internal/mcp_handlers/launches.go (1)
253-256: Improve error detail by extracting response body on failureConsistent with other tools, include server-provided error text.
- launches, _, err := apiRequest.Execute() + launches, rs, err := apiRequest.Execute() if err != nil { - return mcp.NewToolResultError(err.Error()), nil + return mcp.NewToolResultError(utils.ExtractResponseError(err, rs)), nil }internal/mcp_handlers/items.go (1)
584-591: Restore hyphenated MCP request keysThe tool options register parameters like
filter-cnt-name,filter-has-compositeAttribute, etc., but the handler now reads dotted variants (filter.cnt.name,filter.has.compositeAttribute, …). That mismatch means every optional filter is silently dropped, so suites are always fetched without user-supplied filters—a regression. Please revert to the hyphenated keys when callingrequest.GetString.- filterName := request.GetString("filter.cnt.name", "") - filterAttributes := request.GetString("filter.has.compositeAttribute", "") - filterAttributeKeys := request.GetString("filter.has.attributeKey", "") - filterDescription := request.GetString("filter.cnt.description", "") - filterStartTimeFrom := request.GetString("filter.btw.startTime.from", "") - filterStartTimeTo := request.GetString("filter.btw.startTime.to", "") - filterParentId := request.GetString("filter.eq.parentId", "") + filterName := request.GetString("filter-cnt-name", "") + filterAttributes := request.GetString("filter-has-compositeAttribute", "") + filterAttributeKeys := request.GetString("filter-has-attributeKey", "") + filterDescription := request.GetString("filter-cnt-description", "") + filterStartTimeFrom := request.GetString("filter-btw-startTime-from", "") + filterStartTimeTo := request.GetString("filter-btw-startTime-to", "") + filterParentId := request.GetString("filter-eq-parentId", "")
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (18)
cmd/reportportal-mcp-server/main.go(2 hunks)internal/analytics/analytics.go(3 hunks)internal/analytics/analytics_test.go(5 hunks)internal/config/cli.go(1 hunks)internal/http/http_server.go(12 hunks)internal/http/http_server_batch_test.go(1 hunks)internal/mcp_handlers/items.go(21 hunks)internal/mcp_handlers/launches.go(14 hunks)internal/mcp_handlers/launches_test.go(2 hunks)internal/mcp_handlers/server.go(4 hunks)internal/middleware/middleware.go(1 hunks)internal/middleware/middleware_test.go(2 hunks)internal/security/http_token_middleware.go(2 hunks)internal/utils/ctx_utils.go(2 hunks)internal/utils/ctx_utils_test.go(1 hunks)internal/utils/mcp_utils.go(1 hunks)internal/utils/utils.go(7 hunks)internal/utils/utils_test.go(3 hunks)
🧰 Additional context used
📓 Path-based instructions (4)
**/*.go
📄 CodeRabbit inference engine (README.md)
**/*.go: Run formatter (task fmt / go fmt) to keep all Go source files consistently formatted
Run linter (task lint) to maintain code quality across Go files
Files:
internal/middleware/middleware_test.gointernal/analytics/analytics.gointernal/security/http_token_middleware.gointernal/http/http_server_batch_test.gointernal/mcp_handlers/launches_test.gocmd/reportportal-mcp-server/main.gointernal/utils/ctx_utils.gointernal/http/http_server.gointernal/config/cli.gointernal/utils/ctx_utils_test.gointernal/utils/utils_test.gointernal/utils/mcp_utils.gointernal/mcp_handlers/items.gointernal/middleware/middleware.gointernal/analytics/analytics_test.gointernal/utils/utils.gointernal/mcp_handlers/launches.gointernal/mcp_handlers/server.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/middleware/middleware_test.gointernal/analytics/analytics.gointernal/security/http_token_middleware.gointernal/http/http_server_batch_test.gointernal/mcp_handlers/launches_test.gocmd/reportportal-mcp-server/main.gointernal/utils/ctx_utils.gointernal/http/http_server.gointernal/config/cli.gointernal/utils/ctx_utils_test.gointernal/utils/utils_test.gointernal/utils/mcp_utils.gointernal/mcp_handlers/items.gointernal/middleware/middleware.gointernal/analytics/analytics_test.gointernal/utils/utils.gointernal/mcp_handlers/launches.gointernal/mcp_handlers/server.go
**/*_test.go
⚙️ CodeRabbit configuration file
Ensure comprehensive test coverage, proper test isolation, meaningful assertions, and good test naming conventions.
Files:
internal/middleware/middleware_test.gointernal/http/http_server_batch_test.gointernal/mcp_handlers/launches_test.gointernal/utils/ctx_utils_test.gointernal/utils/utils_test.gointernal/analytics/analytics_test.go
cmd/reportportal-mcp-server/**/*.go
📄 CodeRabbit inference engine (README.md)
Keep the main server entrypoint under cmd/reportportal-mcp-server and build from that path
Files:
cmd/reportportal-mcp-server/main.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
🧠 Learnings (1)
📚 Learning: 2025-09-17T11:00:40.261Z
Learnt from: CR
PR: reportportal/reportportal-mcp-server#0
File: README.md:0-0
Timestamp: 2025-09-17T11:00:40.261Z
Learning: Applies to cmd/reportportal-mcp-server/**/*.go : Keep the main server entrypoint under cmd/reportportal-mcp-server and build from that path
Applied to files:
internal/mcp_handlers/launches_test.gocmd/reportportal-mcp-server/main.gointernal/config/cli.gointernal/mcp_handlers/items.gointernal/mcp_handlers/launches.gointernal/mcp_handlers/server.go
🧬 Code graph analysis (13)
internal/middleware/middleware_test.go (1)
internal/utils/ctx_utils.go (1)
WithQueryParams(18-21)
internal/security/http_token_middleware.go (1)
internal/utils/ctx_utils.go (2)
ContextKey(12-12)ValidateRPToken(31-44)
internal/http/http_server_batch_test.go (1)
internal/http/http_server.go (3)
HTTPServer(62-73)HTTPServerConfig(46-59)NewHTTPServer(83-140)
cmd/reportportal-mcp-server/main.go (1)
internal/config/cli.go (2)
AppVersion(26-30)RunApp(336-400)
internal/http/http_server.go (7)
internal/analytics/analytics.go (3)
Analytics(71-83)NewAnalytics(92-133)BatchSendInterval(31-31)internal/utils/ctx_utils.go (1)
ValidateRPToken(31-44)internal/middleware/middleware.go (1)
QueryParamsMiddleware(10-28)internal/mcp_handlers/launches.go (1)
RegisterLaunchTools(41-56)internal/mcp_handlers/items.go (1)
RegisterTestItemTools(41-54)internal/mcp_handlers/server.go (2)
ReadPrompts(71-93)PromptFiles(19-19)internal/security/http_token_middleware.go (1)
HTTPTokenMiddleware(19-42)
internal/config/cli.go (3)
internal/http/http_server.go (2)
HTTPServerConfig(46-59)CreateHTTPServerWithMiddleware(220-246)internal/analytics/analytics.go (3)
GetAnalyticArg(234-248)Analytics(71-83)StopAnalytics(405-414)internal/mcp_handlers/server.go (1)
NewServer(21-68)
internal/utils/utils_test.go (1)
internal/utils/utils.go (1)
ProcessAttributeKeys(124-152)
internal/utils/mcp_utils.go (1)
internal/utils/utils.go (4)
FirstPage(15-15)DefaultPageSize(16-16)PaginatedRequest(31-35)ReadResponseBodyRaw(193-215)
internal/mcp_handlers/items.go (4)
internal/analytics/analytics.go (1)
Analytics(71-83)internal/utils/mcp_utils.go (6)
NewProjectParameter(58-64)SetPaginationOptions(13-28)WithAnalytics(76-90)ExtractProject(66-68)ApplyPaginationOptions(31-56)ReadResponseBody(95-104)internal/utils/utils.go (16)
DefaultSortingForItems(18-18)DefaultProviderType(21-21)DefaultFilterEqHasStats(23-23)DefaultFilterEqHasChildren(22-22)DefaultFilterInType(24-24)ProcessStartTimeFilter(99-121)ProcessAttributeKeys(124-152)ExtractResponseError(37-62)ReadResponseBodyRaw(193-215)IsTextContent(154-174)FirstPage(15-15)DefaultPageSize(16-16)DefaultSortingForLogs(20-20)DefaultItemLogLevel(26-26)DefaultSortingForSuites(19-19)DefaultFilterInTypeSuites(25-25)internal/utils/ctx_utils.go (1)
WithQueryParams(18-21)
internal/middleware/middleware.go (2)
internal/security/http_token_middleware.go (1)
GetTokenFromContext(75-78)internal/utils/ctx_utils.go (1)
QueryParamsFromContext(23-27)
internal/analytics/analytics_test.go (1)
internal/utils/mcp_utils.go (1)
WithAnalytics(76-90)
internal/mcp_handlers/launches.go (4)
internal/analytics/analytics.go (1)
Analytics(71-83)internal/utils/mcp_utils.go (6)
NewProjectParameter(58-64)SetPaginationOptions(13-28)WithAnalytics(76-90)ExtractProject(66-68)ApplyPaginationOptions(31-56)ReadResponseBody(95-104)internal/utils/utils.go (4)
DefaultSortingForLaunches(17-17)ProcessStartTimeFilter(99-121)ProcessAttributeKeys(124-152)ExtractResponseError(37-62)internal/utils/ctx_utils.go (1)
WithQueryParams(18-21)
internal/mcp_handlers/server.go (5)
internal/analytics/analytics.go (2)
Analytics(71-83)NewAnalytics(92-133)internal/middleware/middleware.go (1)
QueryParamsMiddleware(10-28)internal/mcp_handlers/launches.go (1)
RegisterLaunchTools(41-56)internal/mcp_handlers/items.go (1)
RegisterTestItemTools(41-54)internal/promptreader/promptreader.go (1)
PromptHandlerPair(16-19)
🪛 GitHub Check: build
internal/http/http_server.go
[failure] 19-19:
File is not properly formatted (goimports)
[failure] 20-20:
File is not properly formatted (gci)
🪛 GitHub Actions: Build
internal/http/http_server.go
[error] 20-20: golangci-lint: File is not properly formatted (gci).
🔇 Additional comments (10)
internal/middleware/middleware_test.go (1)
13-29: LGTM on query params merge behaviorAdds values without clobbering existing query param, as intended.
internal/utils/ctx_utils.go (1)
31-44: Confirm minimum length for non-UUID tokensThe >=16 heuristic may accept weak inputs. If RP non-UUID tokens are longer, tighten this check.
Would you like me to look up ReportPortal token format/length and adjust the implementation and tests accordingly?
internal/mcp_handlers/launches_test.go (1)
43-49: No stale references to legacy toolGetLaunches found
Search fortoolGetLaunches(returned zero matches; no further action required.internal/security/http_token_middleware.go (2)
15-16: Good switch to a typed context keyUsing utils.ContextKey for RPTokenContextKey avoids collisions with other context values and keeps the API explicit. Looks good.
54-59: Token validation routed through utils — confirm tests cover edge casesThe move to utils.ValidateRPToken centralizes validation. Please ensure we have tests for:
- Lower/upper-case “Bearer” with extra spaces
- Invalid UUIDs and short tokens rejected
- Very long tokens accepted
This also addresses a prior note about exporting ValidateRPToken.
internal/utils/utils_test.go (1)
138-145: API usage updated to exported ProcessAttributeKeys — LGTMCall sites correctly reference utils.ProcessAttributeKeys and assertions remain meaningful.
internal/http/http_server.go (1)
395-396: Use the unified analytics import for interval string.After deduping imports, ensure this still references analytics.BatchSendInterval.
No code change needed if you applied the import fix; re-run tests to confirm.
internal/utils/utils.go (1)
14-28: Constants export + math.MaxInt32 usage look goodUsing math.MaxInt32 via an alias constant is fine and improves clarity.
internal/config/cli.go (2)
32-75: Sources API usage is correct cli.EnvVars returns a ValueSourceChain, which StringFlag.Sources accepts in urfave/cli v3.
243-271: RemovemcpServer.Stop()suggestion (method unavailable)
server.MCPServerdoes not expose aStop()method; the stdio server cleanly shuts down when the passed context is cancelled inListen().Likely an incorrect or invalid review comment.
AleksandrParamonoff
force-pushed
the
refactoring-detaching-app-to-modules
branch
from
8d438fb to
d2483c9
Compare
There was a problem hiding this comment.
Actionable comments posted: 10
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (16)
internal/utils/ctx_utils_test.go (4)
70-71: Use assert.Equalf to format the failure messageassert.Equal doesn’t format msg placeholders. Use the “f” variant so %s/%v are rendered.
- assert.Equal(t, tt.expected, result, - "IsUUIDFormat(%s) = %v, expected %v", tt.token, result, tt.expected) + assert.Equalf(t, tt.expected, result, + "IsUUIDFormat(%s) = %v, expected %v", tt.token, result, tt.expected)As per coding guidelines.
106-109: Format the assertion message for ValidateRPToken testsSame nit as above: switch to Equalf so the token/result are interpolated.
- assert.Equal(t, tt.expected, result, - "ValidateRPToken(%s) = %v, expected %v", tt.token, result, tt.expected) + assert.Equalf(t, tt.expected, result, + "ValidateRPToken(%s) = %v, expected %v", tt.token, result, tt.expected)As per coding guidelines.
14-14: Run subtests in parallelThis table test is independent; add t.Parallel() to speed up CI and surface data races.
func TestIsUUIDFormat(t *testing.T) { + t.Parallel()As per coding guidelines.
76-76: Parallelize the second table test tooSame reasoning as above.
func TestValidateRPToken(t *testing.T) { + t.Parallel()As per coding guidelines.
internal/utils/ctx_utils.go (1)
37-44: Optionally reject the all‑zero UUID as a valid RP tokenFormat‑valid but nil UUIDs are trivially guessable; treat them as invalid while keeping non‑UUID tokens supported.
- // Check for valid UUID format using proper parsing - if uuid.Validate(token) == nil { - return true - } + // Accept proper UUIDs, but reject the nil UUID + if u, err := uuid.Parse(token); err == nil { + return u != uuid.Nil + }As per coding guidelines.
internal/analytics/analytics_test.go (1)
66-73: Clean up analytics instances to avoid goroutine leaksWhen NewAnalytics succeeds, register a cleanup to stop background workers.
} else { assert.NoError(t, err) assert.NotNil(t, analytics) + t.Cleanup(analytics.Stop) // Analytics should be created when API secret is provided assert.NotNil(t, analytics.config) assert.NotNil(t, analytics.httpClient)As per coding guidelines.
internal/utils/utils_test.go (2)
155-157: Avoid control characters in generated keysUsing
string(rune(i))injects control chars/newlines/commas into keys, making assertions noisy and platform-sensitive. Prefer decimal indices.-import ( - "strings" - "testing" -) +import ( + "strconv" + "strings" + "testing" +) @@ - for i := 0; i < 1000; i++ { - keys = append(keys, "key"+string(rune(i))+":") - } + for i := 0; i < 1000; i++ { + keys = append(keys, "key"+strconv.Itoa(i)+":") + }
8-8: Run tests in parallelThese tests are pure and can run in parallel to reduce CI time.
func TestProcessAttributeKeys(t *testing.T) { + t.Parallel() @@ func TestProcessAttributeKeys_Performance(t *testing.T) { + t.Parallel()Also applies to: 149-149
internal/security/http_token_middleware.go (2)
47-66: Reject control characters in tokens to harden header injection defensesAdd a fast-path guard for CR/LF and other control chars before calling
utils.ValidateRPToken. This prevents tainted tokens from ever reaching context and later header propagation.func extractRPTokenFromRequest(r *http.Request) string { auth := r.Header.Get("Authorization") if auth != "" { - parts := strings.SplitN(auth, " ", 2) + parts := strings.SplitN(auth, " ", 2) if len(parts) == 2 && strings.EqualFold(parts[0], "Bearer") { - token := strings.TrimSpace(parts[1]) + token := strings.TrimSpace(parts[1]) + // Reject CR/LF and control characters + if strings.ContainsAny(token, "\r\n") { + slog.Debug("Rejected RP API token containing CR/LF") + return "" + } // Validate the extracted token before processing if !utils.ValidateRPToken(token) { slog.Debug("Invalid RP API token rejected", "source", "Authorization Bearer", "validation", "failed") return "" }
49-52: Tolerate multiple/irregular whitespace in Authorization
strings.Fieldsis more robust thanSplitN(" ", 2)for headers likeBearer <token>.- parts := strings.SplitN(auth, " ", 2) - if len(parts) == 2 && strings.EqualFold(parts[0], "Bearer") { - token := strings.TrimSpace(parts[1]) + fields := strings.Fields(auth) + if len(fields) == 2 && strings.EqualFold(fields[0], "Bearer") { + token := fields[1]internal/analytics/analytics.go (2)
274-291: Make Stop idempotent to avoid double-close panics.Stop closes a.stopChan without guarding multiple calls. Use sync.Once.
type Analytics struct { @@ - stopChan chan struct{} + stopChan chan struct{} wg sync.WaitGroup tickerDone chan struct{} + stopOnce sync.Once } @@ func (a *Analytics) Stop() { if a == nil || a.stopChan == nil { return } slog.Debug("Stopping analytics metrics processor") - close(a.stopChan) + a.stopOnce.Do(func() { close(a.stopChan) })Also applies to: 70-83
64-68: Field name doesn’t match units.TimestampMS is populated with UnixMicro and marshaled as "timestamp_micros". Rename for clarity.
-type GAPayload struct { - ClientID string `json:"client_id"` - UserID string `json:"user_id,omitempty"` - Events []GAEvent `json:"events"` - TimestampMS int64 `json:"timestamp_micros"` -} +type GAPayload struct { + ClientID string `json:"client_id"` + UserID string `json:"user_id,omitempty"` + Events []GAEvent `json:"events"` + TimestampMicros int64 `json:"timestamp_micros"` +} @@ - TimestampMS: time.Now().UnixMicro(), + TimestampMicros: time.Now().UnixMicro(),Also applies to: 152-157
internal/mcp_handlers/server.go (1)
70-93: handlers slice preallocation bug causes leading zero entries.You allocate len(entries) and then append, yielding a slice with len(entries) zero-values plus appended prompts. Initialize with capacity, not length.
- handlers := make([]promptreader.PromptHandlerPair, len(entries)) + handlers := make([]promptreader.PromptHandlerPair, 0, len(entries)) @@ - handlers = append(handlers, prompts...) + handlers = append(handlers, prompts...)Consider skipping non-file entries:
if entry.IsDir() { continue }internal/mcp_handlers/launches.go (1)
265-267: Return MCP error result instead of a raw error for consistency.- if err != nil { - return nil, fmt.Errorf("failed to marshal response: %w", err) - } + if err != nil { + return mcp.NewToolResultError(fmt.Sprintf("failed to marshal response: %v", err)), nil + }internal/mcp_handlers/items.go (2)
584-590: Broken filter readers: option names use hyphens, but readers use dots (filters will be ignored).Align parameter names to the declared options.
- filterName := request.GetString("filter.cnt.name", "") - filterAttributes := request.GetString("filter.has.compositeAttribute", "") - filterAttributeKeys := request.GetString("filter.has.attributeKey", "") - filterDescription := request.GetString("filter.cnt.description", "") - filterStartTimeFrom := request.GetString("filter.btw.startTime.from", "") - filterStartTimeTo := request.GetString("filter.btw.startTime.to", "") - filterParentId := request.GetString("filter.eq.parentId", "") + filterName := request.GetString("filter-cnt-name", "") + filterAttributes := request.GetString("filter-has-compositeAttribute", "") + filterAttributeKeys := request.GetString("filter-has-attributeKey", "") + filterDescription := request.GetString("filter-cnt-description", "") + filterStartTimeFrom := request.GetString("filter-btw-startTime-from", "") + filterStartTimeTo := request.GetString("filter-btw-startTime-to", "") + filterParentId := request.GetString("filter-eq-parentId", "")
401-413: Prefer numeric schema for IDs to avoid manual parsing; simplify parent-item-id handling.Define parent-item-id as a number and use RequireInt. This removes fragile string parsing and invalid-negative checks remain intact.
@@ - mcp.WithString("parent-item-id", // ID of the parent item - mcp.Description("Items with specific Parent Item ID, this is a required parameter"), - ), + mcp.WithNumber("parent-item-id", // ID of the parent item + mcp.Description("Items with specific Parent Item ID, this is a required parameter"), + ), @@ - parentIdStr, err := request.RequireString("parent-item-id") + parentIdInt, err := request.RequireInt("parent-item-id") if err != nil { return mcp.NewToolResultError(err.Error()), nil } @@ - // Validate parentIdStr and convert it to int64 - var parentIdValue int64 - if parentIdStr != "" { - var err error - parentIdValue, err = strconv.ParseInt(parentIdStr, 10, 64) - if err != nil || parentIdValue < 0 { - return mcp.NewToolResultError( - fmt.Sprintf("invalid parent filter ID value: %s", parentIdStr), - ), nil - } - } + // Validate parentId and convert to int64 + if parentIdInt < 0 { + return mcp.NewToolResultError( + fmt.Sprintf("invalid parent filter ID value: %d", parentIdInt), + ), nil + } + parentIdValue := int64(parentIdInt) @@ - requiredUrlParams := map[string]string{ - "parentId": parentIdStr, - } + requiredUrlParams := map[string]string{ + "parentId": strconv.FormatInt(parentIdValue, 10), + }Also applies to: 448-453, 479-487
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (18)
cmd/reportportal-mcp-server/main.go(2 hunks)internal/analytics/analytics.go(3 hunks)internal/analytics/analytics_test.go(5 hunks)internal/config/cli.go(1 hunks)internal/http/http_server.go(11 hunks)internal/http/http_server_batch_test.go(1 hunks)internal/mcp_handlers/items.go(21 hunks)internal/mcp_handlers/launches.go(14 hunks)internal/mcp_handlers/launches_test.go(2 hunks)internal/mcp_handlers/server.go(4 hunks)internal/middleware/middleware.go(1 hunks)internal/middleware/middleware_test.go(2 hunks)internal/security/http_token_middleware.go(2 hunks)internal/utils/ctx_utils.go(2 hunks)internal/utils/ctx_utils_test.go(1 hunks)internal/utils/mcp_utils.go(1 hunks)internal/utils/utils.go(7 hunks)internal/utils/utils_test.go(3 hunks)
🧰 Additional context used
📓 Path-based instructions (4)
**/*.go
📄 CodeRabbit inference engine (README.md)
**/*.go: Run formatter (task fmt / go fmt) to keep all Go source files consistently formatted
Run linter (task lint) to maintain code quality across Go files
Files:
internal/security/http_token_middleware.gointernal/utils/utils_test.gointernal/middleware/middleware_test.gointernal/mcp_handlers/launches_test.gointernal/analytics/analytics.gointernal/analytics/analytics_test.gointernal/http/http_server.gointernal/mcp_handlers/items.gocmd/reportportal-mcp-server/main.gointernal/utils/utils.gointernal/http/http_server_batch_test.gointernal/mcp_handlers/server.gointernal/middleware/middleware.gointernal/utils/ctx_utils_test.gointernal/utils/ctx_utils.gointernal/config/cli.gointernal/utils/mcp_utils.gointernal/mcp_handlers/launches.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/security/http_token_middleware.gointernal/utils/utils_test.gointernal/middleware/middleware_test.gointernal/mcp_handlers/launches_test.gointernal/analytics/analytics.gointernal/analytics/analytics_test.gointernal/http/http_server.gointernal/mcp_handlers/items.gocmd/reportportal-mcp-server/main.gointernal/utils/utils.gointernal/http/http_server_batch_test.gointernal/mcp_handlers/server.gointernal/middleware/middleware.gointernal/utils/ctx_utils_test.gointernal/utils/ctx_utils.gointernal/config/cli.gointernal/utils/mcp_utils.gointernal/mcp_handlers/launches.go
**/*_test.go
⚙️ CodeRabbit configuration file
Ensure comprehensive test coverage, proper test isolation, meaningful assertions, and good test naming conventions.
Files:
internal/utils/utils_test.gointernal/middleware/middleware_test.gointernal/mcp_handlers/launches_test.gointernal/analytics/analytics_test.gointernal/http/http_server_batch_test.gointernal/utils/ctx_utils_test.go
cmd/reportportal-mcp-server/**/*.go
📄 CodeRabbit inference engine (README.md)
Keep the main server entrypoint under cmd/reportportal-mcp-server and build from that path
Files:
cmd/reportportal-mcp-server/main.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
🧠 Learnings (1)
📚 Learning: 2025-09-17T11:00:40.261Z
Learnt from: CR
PR: reportportal/reportportal-mcp-server#0
File: README.md:0-0
Timestamp: 2025-09-17T11:00:40.261Z
Learning: Applies to cmd/reportportal-mcp-server/**/*.go : Keep the main server entrypoint under cmd/reportportal-mcp-server and build from that path
Applied to files:
internal/mcp_handlers/launches_test.gointernal/mcp_handlers/items.gocmd/reportportal-mcp-server/main.gointernal/mcp_handlers/server.gointernal/config/cli.gointernal/mcp_handlers/launches.go
🧬 Code graph analysis (13)
internal/security/http_token_middleware.go (1)
internal/utils/ctx_utils.go (2)
ContextKey(12-12)ValidateRPToken(31-44)
internal/utils/utils_test.go (1)
internal/utils/utils.go (1)
ProcessAttributeKeys(124-152)
internal/middleware/middleware_test.go (1)
internal/utils/ctx_utils.go (1)
WithQueryParams(18-21)
internal/analytics/analytics_test.go (1)
internal/utils/mcp_utils.go (1)
WithAnalytics(76-90)
internal/http/http_server.go (7)
internal/analytics/analytics.go (3)
Analytics(71-83)NewAnalytics(92-133)BatchSendInterval(31-31)internal/utils/ctx_utils.go (1)
ValidateRPToken(31-44)internal/middleware/middleware.go (1)
QueryParamsMiddleware(10-28)internal/mcp_handlers/launches.go (1)
RegisterLaunchTools(41-56)internal/mcp_handlers/items.go (1)
RegisterTestItemTools(41-54)internal/mcp_handlers/server.go (2)
ReadPrompts(71-93)PromptFiles(19-19)internal/security/http_token_middleware.go (1)
HTTPTokenMiddleware(19-42)
internal/mcp_handlers/items.go (4)
internal/analytics/analytics.go (1)
Analytics(71-83)internal/utils/mcp_utils.go (6)
NewProjectParameter(58-64)SetPaginationOptions(13-28)WithAnalytics(76-90)ExtractProject(66-68)ApplyPaginationOptions(31-56)ReadResponseBody(95-104)internal/utils/utils.go (16)
DefaultSortingForItems(18-18)DefaultProviderType(21-21)DefaultFilterEqHasStats(23-23)DefaultFilterEqHasChildren(22-22)DefaultFilterInType(24-24)ProcessStartTimeFilter(99-121)ProcessAttributeKeys(124-152)ExtractResponseError(37-62)ReadResponseBodyRaw(193-215)IsTextContent(154-174)FirstPage(15-15)DefaultPageSize(16-16)DefaultSortingForLogs(20-20)DefaultItemLogLevel(26-26)DefaultSortingForSuites(19-19)DefaultFilterInTypeSuites(25-25)internal/utils/ctx_utils.go (1)
WithQueryParams(18-21)
cmd/reportportal-mcp-server/main.go (1)
internal/config/cli.go (2)
AppVersion(26-30)RunApp(336-400)
internal/http/http_server_batch_test.go (1)
internal/http/http_server.go (3)
HTTPServer(61-72)HTTPServerConfig(45-58)NewHTTPServer(82-139)
internal/mcp_handlers/server.go (5)
internal/analytics/analytics.go (2)
Analytics(71-83)NewAnalytics(92-133)internal/middleware/middleware.go (1)
QueryParamsMiddleware(10-28)internal/mcp_handlers/launches.go (1)
RegisterLaunchTools(41-56)internal/mcp_handlers/items.go (1)
RegisterTestItemTools(41-54)internal/promptreader/promptreader.go (1)
PromptHandlerPair(16-19)
internal/middleware/middleware.go (2)
internal/security/http_token_middleware.go (1)
GetTokenFromContext(75-78)internal/utils/ctx_utils.go (1)
QueryParamsFromContext(23-27)
internal/config/cli.go (3)
internal/http/http_server.go (2)
HTTPServerConfig(45-58)CreateHTTPServerWithMiddleware(219-245)internal/analytics/analytics.go (3)
GetAnalyticArg(234-248)Analytics(71-83)StopAnalytics(405-414)internal/mcp_handlers/server.go (1)
NewServer(21-68)
internal/utils/mcp_utils.go (1)
internal/utils/utils.go (4)
FirstPage(15-15)DefaultPageSize(16-16)PaginatedRequest(31-35)ReadResponseBodyRaw(193-215)
internal/mcp_handlers/launches.go (4)
internal/analytics/analytics.go (1)
Analytics(71-83)internal/utils/mcp_utils.go (6)
NewProjectParameter(58-64)SetPaginationOptions(13-28)WithAnalytics(76-90)ExtractProject(66-68)ApplyPaginationOptions(31-56)ReadResponseBody(95-104)internal/utils/utils.go (4)
DefaultSortingForLaunches(17-17)ProcessStartTimeFilter(99-121)ProcessAttributeKeys(124-152)ExtractResponseError(37-62)internal/utils/ctx_utils.go (1)
WithQueryParams(18-21)
🔇 Additional comments (24)
internal/middleware/middleware_test.go (1)
13-29: Add a test to assert Authorization header is sourced from contextVerifies middleware behavior when a token is present in context.
func TestQueryParamsMiddleware(t *testing.T) { @@ QueryParamsMiddleware(req) @@ assert.Equal(t, []string{"2", "3"}, got["b"]) } + +func TestQueryParamsMiddleware_AddsAuthorizationFromContext(t *testing.T) { + req, err := http.NewRequest("GET", "http://example.com/path", nil) + assert.NoError(t, err) + ctx := context.WithValue(req.Context(), security.RPTokenContextKey, "abc123") + req = req.WithContext(ctx) + + QueryParamsMiddleware(req) + + assert.Equal(t, "Bearer abc123", req.Header.Get("Authorization")) +}Don’t forget to import context and internal/security.
As per coding guidelines.internal/analytics/analytics_test.go (1)
250-285: Make the integration test deterministic; drop unused httptest server and sleepThe server is unused (GA URL is constant), and time.Sleep is flaky. Flush metrics synchronously.
-func TestAnalyticsIntegration(t *testing.T) { - // Create a test server to mock GA4 - testServer := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { - w.WriteHeader(http.StatusOK) - })) - defer testServer.Close() +func TestAnalyticsIntegration(t *testing.T) { @@ for i := 0; i < 3; i++ { _, err := wrappedHandler(context.Background(), mcp.CallToolRequest{}) assert.NoError(t, err) } - // Give some time for async processing - time.Sleep(100 * time.Millisecond) + // Process metrics synchronously to avoid time-based flakiness + analytics.processMetrics() // Stop analytics analytics.Stop() }Also remove now-unused imports (net/http, net/http/httptest). As per coding guidelines.
internal/utils/ctx_utils.go (1)
11-16: Context key should be an unexported struct value; keep backward‑compat during transitionUsing an exported string type increases collision risk across packages. Prefer a zero‑sized unexported struct key and expose a value var for cross‑package use. Keep lookup backward‑compatible by checking the legacy key too.
// ContextKey is a type for context keys to avoid collisions -type ContextKey string +type ContextKey string // Deprecated for new keys; kept for backward-compat -var contextKeyQueryParams = ContextKey( - "queryParams", -) // Key for storing query parameters in the context +// New, collision-safe key for query params +type queryParamsKey struct{} +var QueryParamsKey = queryParamsKey{} // exported value for cross-package useUpdate helpers to use the new key and read legacy values if present:
func WithQueryParams(ctx context.Context, queryParams url.Values) context.Context { - // Create a new context with the query parameters - return context.WithValue(ctx, contextKeyQueryParams, queryParams) + // Store query parameters under the struct{} key + return context.WithValue(ctx, QueryParamsKey, queryParams) } func QueryParamsFromContext(ctx context.Context) (url.Values, bool) { - // Retrieve the query parameters from the context - queryParams, ok := ctx.Value(contextKeyQueryParams).(url.Values) - return queryParams, ok + // Prefer the new key; fall back to legacy string-typed key for compatibility + if v, ok := ctx.Value(QueryParamsKey).(url.Values); ok { + return v, true + } + if v, ok := ctx.Value(ContextKey("queryParams")).(url.Values); ok { + return v, true + } + return nil, false }Run to find any direct usages of the legacy string key so we can plan a clean removal later:
#!/bin/bash rg -nP --type=go -C2 '\bContextKey\("queryParams"\)|context\.WithValue\([^,]+,\s*ContextKey\("queryParams"\)'As per coding guidelines.
internal/mcp_handlers/launches_test.go (1)
45-47: Check url.Parse error to prevent hidden failuresDon’t discard the parse error.
- serverURL, _ := url.Parse(mockServer.URL) + serverURL, urlErr := url.Parse(mockServer.URL) + require.NoError(t, urlErr)As per coding guidelines.
internal/utils/utils_test.go (1)
15-136: Good coverage and clear table casesThe table tests comprehensively exercise split/trim/colon behavior and mixed scenarios. Looks solid.
internal/middleware/middleware.go (3)
12-14: Confirm override semantics for Authorization
Header.Setwill overwrite any existing Authorization header. Is this intended when both context and request already carry tokens? If not, gate on absence of header.
17-27: Query param merge looks goodMerges context params into URL via
Encode(); safe and idiomatic. LGTM.
3-8: Sanitize Authorization and remove redundant non-empty checkPrevent header injection by trimming and rejecting CR/LF before setting the header. Also,
GetTokenFromContextalready guarantees non-empty whenokis true—drop the extratoken != "".import ( "net/http" "github.com/reportportal/reportportal-mcp-server/internal/security" "github.com/reportportal/reportportal-mcp-server/internal/utils" + "strings" ) @@ - if token, ok := security.GetTokenFromContext(rq.Context()); ok && token != "" { - rq.Header.Set("Authorization", "Bearer "+token) - } + if token, ok := security.GetTokenFromContext(rq.Context()); ok { + token = strings.TrimSpace(token) + if token != "" && !strings.ContainsAny(token, "\r\n") { + rq.Header.Set("Authorization", "Bearer "+token) + } + }Also applies to: 12-14
internal/security/http_token_middleware.go (2)
15-16: Typed context key usage is goodUsing a distinct key type avoids collisions with raw strings. Nice.
54-54: ValidateRPToken dependency verified:utils.ValidateRPToken(token string) boolis defined in internal/utils/ctx_utils.go and callsuuid.Validate(token)from github.com/google/uuid v1.6.0. All signatures match across modules.cmd/reportportal-mcp-server/main.go (1)
31-34: Main handles termination correctly; ensure RunApp doesn’t log.Fatal inside library code.main’s os.Exit(1) on error is fine. However, internal/config.RunApp still calls log.Fatal on failure, which prevents proper error propagation and testing. Return the error instead and let main exit.
Apply in internal/config/cli.go:
- if err := cmd.Run(ctx, os.Args); err != nil { - log.Fatal(err) - } - return nil + if err := cmd.Run(ctx, os.Args); err != nil { + return err + } + return nilRun to confirm no other log.Fatal/os.Exit in non-cmd packages:
#!/bin/bash rg -nP '(log\.Fatal(f)?\(|os\.Exit\()' -g '!cmd/**' -g '!**/*_test.go' -C2internal/http/http_server_batch_test.go (3)
206-214: Update call site for new helper signature.- req := createTestRequest(tt.payload) + req := createTestRequest(t, tt.payload)Run go fmt/lint after changes.
12-17: Make request helper a proper test helper; don’t ignore errors.Mark as t.Helper(), return *http.Request, and check http.NewRequest error. Also, pass a plain Reader.
-// Test helper to create HTTP requests with JSON payloads -func createTestRequest(payload string) *http.Request { - req, _ := http.NewRequest("POST", "/test", io.NopCloser(bytes.NewBufferString(payload))) - req.Header.Set("Content-Type", "application/json") - return req -} +// Test helper to create HTTP requests with JSON payloads +func createTestRequest(t *testing.T, payload string) *http.Request { + t.Helper() + req, err := http.NewRequest("POST", "/test", bytes.NewBufferString(payload)) + if err != nil { + t.Fatalf("failed to create request: %v", err) + } + req.Header.Set("Content-Type", "application/json") + return req +}
264-272: Don’t ignore http.NewRequest errors; add charset Content-Type case.- req, _ := http.NewRequest( + req, err := http.NewRequest( tt.method, "/test", - io.NopCloser(bytes.NewBufferString(tt.payload)), + bytes.NewBufferString(tt.payload), ) + if err != nil { + t.Fatalf("failed to create request: %v", err) + } req.Header.Set("Content-Type", tt.contentType)Optionally extend the table:
@@ { name: "valid POST with JSON content", @@ }, + { + name: "POST with JSON content and charset", + method: "POST", + contentType: "application/json; charset=utf-8", + payload: `{"jsonrpc":"2.0","method":"test","id":1}`, + expected: true, + },Also applies to: 241-262
internal/analytics/analytics.go (2)
31-31: Make BatchSendInterval overridable for tests.Declare BatchSendInterval as a var, not const, so tests can shorten it.
-const ( +const ( // Google Analytics 4 Measurement Protocol endpoint ga4EndpointURL = "https://www.google-analytics.com/mp/collect" @@ - // Batch send interval for analytics data - BatchSendInterval = 10 * time.Second + // Max events per GA4 request + maxPerRequest = 25 ) - maxPerRequest = 25 -) +// Batch send interval for analytics data (overridable in tests) +var BatchSendInterval = 10 * time.SecondNo code changes needed at usage sites (ticker/debug). As per coding guidelines.
Also applies to: 255-259
234-248: Don’t remove GetAnalyticArg —it’s actually used
This function is called in internal/config/cli.go (at lines 152 and 197) and covered by internal/analytics/analytics_test.go. Deleting it will break your CLI’s analytics setup and tests. If the obfuscation isn’t needed, refactor or rename it (e.g. GenerateAnalyticsSecret) and add clear documentation instead.Likely an incorrect or invalid review comment.
internal/mcp_handlers/server.go (1)
41-50: Don’t fail server init if analytics is misconfigured; log and continue.Align behavior with HTTP server path: analytics init failure should not abort server creation.
// Initialize analytics (disabled if analyticsOff is true) var analyticsClient *analytics.Analytics if analyticsOn { - var err error - // Pass RP API token for secure hashing as user identifier - analyticsClient, err = analytics.NewAnalytics(userID, analyticsAPISecret, token) - if err != nil { - return nil, nil, fmt.Errorf("failed to initialize analytics: %w", err) - } + ac, err := analytics.NewAnalytics(userID, analyticsAPISecret, token) + if err != nil { + slog.Warn("analytics initialization skipped", "error", err) + } else { + analyticsClient = ac + } }Add import:
+ "log/slog"internal/utils/mcp_utils.go (2)
3-10: Import math for integer bounds.import ( "context" "fmt" "net/http" + "math" "github.com/mark3labs/mcp-go/mcp" "github.com/mark3labs/mcp-go/server" )
36-56: Fix undefined maxInt32 and clamp pagination to valid ranges before int32 casts.Use math.MaxInt32 and enforce minimums to avoid overflow/negative values.
// Extract the "page" parameter from the request pageInt := request.GetInt("page", FirstPage) - if pageInt > maxInt32 { - pageInt = maxInt32 - } + if pageInt < FirstPage { + pageInt = FirstPage + } + if pageInt > int(math.MaxInt32) { + pageInt = int(math.MaxInt32) + } // Extract the "page-size" parameter from the request pageSizeInt := request.GetInt("page-size", DefaultPageSize) - if pageSizeInt > maxInt32 { - pageSizeInt = maxInt32 - } + if pageSizeInt < 1 { + pageSizeInt = 1 + } + if pageSizeInt > int(math.MaxInt32) { + pageSizeInt = int(math.MaxInt32) + }As per coding guidelines.
internal/config/cli.go (3)
290-299: Reuse the wrapper Handler instead of poking into MCP internals
CreateHTTPServerWithMiddlewarealready exposes the fully wired handler via itsHandlerfield. Reaching intohttpServer.MCP.Routerkeeps a tight coupling to private structure that we just abstracted away. Please use the wrapper’sHandlerso future router changes don’t break this call site.
337-344: Use GetMCPMode() to centralize mode parsingWe now have a dedicated helper that normalizes the
MCP_MODEenv var. Re-implementing the parsing here invites drift; callGetMCPMode()and reuse its result instead.
394-399: Return the CLI error instead of calling log.Fatal
RunAppis our library-style entry point. Callinglog.Fatalterminates the process immediately, bypassing deferred clean-up in callers and making tests impossible to recover from startup failures. Return the error and letmaindecide how to handle it.
Apply this diff:- if err := cmd.Run(ctx, os.Args); err != nil { - log.Fatal(err) - } - - return nil + return cmd.Run(ctx, os.Args)internal/mcp_handlers/launches.go (2)
279-281: Schema/handler mismatch: launch_id must be numeric (string+RequireInt => runtime error).Declare launch_id with WithNumber to match RequireInt in all tools listed below.
@@ - mcp.WithString("launch_id", // Parameter for specifying the launch name + mcp.WithNumber("launch_id", // Parameter for specifying the launch ID mcp.Description("Launch ID"), ), @@ - mcp.WithString("launch_id", // Parameter for specifying the launch name + mcp.WithNumber("launch_id", // Parameter for specifying the launch ID mcp.Description("Launch ID"), ), @@ - mcp.WithString("launch_id", // Parameter for specifying the launch name + mcp.WithNumber("launch_id", // Parameter for specifying the launch ID mcp.Description("Launch ID"), ), @@ - mcp.WithString("launch_id", // Parameter for specifying the launch name + mcp.WithNumber("launch_id", // Parameter for specifying the launch ID mcp.Description("Launch ID"), ),Tools affected:
- ToolDeleteLaunch
- ToolRunAutoAnalysis
- ToolUniqueErrorAnalysis
- ToolForceFinishLaunch
Also applies to: 310-313, 387-390, 431-434
429-430: Misleading description: this tool force-finishes, not deletes.Update the description to reflect the action.
- mcp.WithDescription("Delete ReportPortal launch"), + mcp.WithDescription("Force finish ReportPortal launch"),
AleksandrParamonoff
force-pushed
the
refactoring-detaching-app-to-modules
branch
from
d2483c9 to
c59c94b
Compare
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (6)
internal/mcp_handlers/server.go (1)
70-92: Fix prompt loader to avoid zero-value entries.
handlers := make(..., len(entries))pre-populateslen(entries)zero-value elements, and the subsequentappendpushes real prompt-handler pairs after them. When you later iterate and calls.AddPrompt, the firstlen(entries)items have empty prompts and nil handlers, which will panic or register bogus prompts. Start with length 0 (capacity only) before appending.- handlers := make([]promptreader.PromptHandlerPair, len(entries)) + handlers := make([]promptreader.PromptHandlerPair, 0, len(entries)) @@ - handlers = append(handlers, prompts...) + handlers = append(handlers, prompts...)internal/http/http_server.go (1)
163-170: Correct the MIME switch so JSON/XML over HTTP are treated as text.Go switch cases don’t implicitly fall through; only
application/csvcurrently reaches thereturn true. As a result, JSON/XML/etc. hit the default branch and get treated as binary blobs. Combine the cases in a single clause or return true in each case.- switch lowerType { - case "application/json": - case "application/xml": - case "application/javascript": - case "application/xhtml+xml": - case "application/yaml": - case "application/csv": - return true - } + switch lowerType { + case "application/json", + "application/xml", + "application/javascript", + "application/xhtml+xml", + "application/yaml", + "application/csv": + return true + }internal/mcp_handlers/items.go (1)
583-590: Use the actual parameter keys when reading request filters.The tool declares parameters like
"filter-cnt-name"/"filter-has-compositeAttribute", but here we read"filter.cnt.name"/"filter.has.compositeAttribute"(dots instead of hyphens). That mismatch forces everyGetString(...)to return the default value, so none of the suite filters ever make it into the API call.Update each lookup to use the hyphenated keys you registered, e.g.:
filterName := request.GetString("filter-cnt-name", "") filterAttributes := request.GetString("filter-has-compositeAttribute", "") filterAttributeKeys := request.GetString("filter-has-attributeKey", "") filterDescription := request.GetString("filter-cnt-description", "") filterStartTimeFrom := request.GetString("filter-btw-startTime-from", "") filterStartTimeTo := request.GetString("filter-btw-startTime-to", "") filterParentId := request.GetString("filter-eq-parentId", "")Without this correction, the handler ignores user input.
internal/utils/utils.go (3)
37-62: Consider removing the named return parameter.The function declares a named return
errTextbut never uses a nakedreturnstatement. Go convention suggests either using naked returns with named parameters or avoiding named returns altogether for clarity.Apply this diff for consistency:
-func ExtractResponseError(err error, rs *http.Response) (errText string) { - errText = err.Error() +func ExtractResponseError(err error, rs *http.Response) string { + errText := err.Error() if rs != nil && rs.Body != nil {
65-96: Replace magic numbers with named constants.Lines 72 and 74 contain magic numbers (
32503680000,10000000000) representing year boundaries. Extract these as package-level constants with descriptive names to improve readability and maintainability.Apply this diff:
const ( FirstPage = 1 // Default starting page for pagination DefaultPageSize = 50 // Default number of elements per page + maxEpochMillis = 32503680000 // Roughly year 3000 in milliseconds + maxEpochSeconds = 10000000000 // Roughly year 2286 in seconds DefaultSortingForLaunches = "startTime,number,DESC" // default sorting order for launchesThen update the function:
if epoch, err := strconv.ParseInt(timestampStr, 10, 64); err == nil { // If it's a reasonable Unix timestamp (after 1970 and before year 3000) - if epoch > 0 && epoch < 32503680000 { // roughly year 3000 + if epoch > 0 && epoch < maxEpochMillis { // If it looks like seconds, convert to milliseconds - if epoch < 10000000000 { // less than year 2286 in seconds + if epoch < maxEpochSeconds { return epoch * 1000, nil }
154-174: Simplify the switch statement structure.All cases in the switch fall through to
return true. This pattern is harder to maintain than a straightforward list of strings to check against.Apply this diff:
func IsTextContent(mediaType string) bool { lowerType := strings.ToLower(mediaType) // Text types (most common) if strings.HasPrefix(lowerType, "text/") { return true } - // Popular application types that are text-based - switch lowerType { - case "application/json": - case "application/xml": - case "application/javascript": - case "application/xhtml+xml": - case "application/yaml": - case "application/csv": + // Text-based application types + textAppTypes := []string{ + "application/json", + "application/xml", + "application/javascript", + "application/xhtml+xml", + "application/yaml", + "application/csv", + } + for _, textType := range textAppTypes { + if lowerType == textType { + return true + } + } - return true - } return false }
♻️ Duplicate comments (9)
internal/analytics/analytics_test.go (1)
180-181: Also assert metric increment after wrappingAfter calling wrappedHandler, assert the counter for "test_tool" incremented (under lock). This verifies the wrapper actually tracks events.
// Call the wrapped handler result, err := wrappedHandler(context.Background(), mcp.CallToolRequest{}) // Verify handler was called assert.NoError(t, err) assert.NotNil(t, result) assert.True(t, handlerCalled, "Original handler should be called") + + // Verify metric incremented + analytics.metricsLock.RLock() + if v, ok := analytics.metrics["test_tool"]; ok && v != nil { + assert.Equal(t, int64(1), *v) + } else { + assert.Fail(t, "metric for test_tool not recorded") + } + analytics.metricsLock.RUnlock()As per coding guidelines.
Also applies to: 200-206, 272-276
internal/config/cli.go (1)
291-300: Use the pre-wired Handler instead of reaching into MCP.RouterPrefer httpServer.Handler returned by CreateHTTPServerWithMiddleware to avoid tight coupling and keep middleware intact.
- Handler: httpServer.MCP.Router, // Use Chi router directly with throttling/health/info/metrics + Handler: httpServer.Handler, // Use pre‑wired Chi router with middlewareinternal/mcp_handlers/launches.go (2)
274-303: Critical: Type mismatch between launch_id parameter and RequireInt callTools declare
launch_idas string (mcp.WithString) but the handler callsrequest.RequireInt("launch_id"), causing runtime errors when the tool is invoked.Apply this diff to fix the type mismatch:
- mcp.WithString("launch_id", // Parameter for specifying the launch name + mcp.WithNumber("launch_id", // Parameter for specifying the launch ID mcp.Description("Launch ID"), ),Apply the same fix to
ToolRunAutoAnalysis(line 310),ToolUniqueErrorAnalysis(line 387), andToolForceFinishLaunch(line 431).
215-272: Minor: Tool description should clarify it returns a single launch by nameThe description says "Get list of last ReportPortal launches by name" but the tool returns only the most recent launch matching the given name.
Apply this diff to clarify:
- mcp.WithDescription("Get list of last ReportPortal launches by name"), + mcp.WithDescription("Get last ReportPortal launch by name"),internal/middleware/middleware_test.go (1)
19-21: Check NewRequest error in testThe error from
http.NewRequestis discarded, which could mask test setup failures.Apply this diff:
- req, _ := http.NewRequest("GET", "http://example.com/path?x=9", nil) + req, err := http.NewRequest("GET", "http://example.com/path?x=9", nil) + require.NoError(t, err) ctx := utils.WithQueryParams(req.Context(), params)As per coding guidelines.
internal/mcp_handlers/launches_test.go (1)
45-46: Check url.Parse error to prevent test failuresThe error from
url.Parseis discarded, which could lead to nil pointer panics if the URL is invalid.Apply this diff:
- serverURL, _ := url.Parse(mockServer.URL) + serverURL, err := url.Parse(mockServer.URL) + require.NoError(t, err) launchTools := NewLaunchResources(gorp.NewClient(serverURL, ""), testProject, nil)internal/utils/ctx_utils.go (1)
11-16: Consider unexported struct type for context keys to prevent collisionsExporting
ContextKeyas a string type risks name collisions across packages. Go best practice recommends unexported, zero-sized struct types for context keys.Consider this alternative pattern:
-// ContextKey is a type for context keys to avoid collisions -type ContextKey string +// unexported struct type prevents collisions +type queryParamsKey struct{} -var contextKeyQueryParams = ContextKey( - "queryParams", -) // Key for storing query parameters in the context +// QueryParamsKey is the exported key value for query parameters +var QueryParamsKey = queryParamsKey{}Then update
WithQueryParamsandQueryParamsFromContextto useQueryParamsKeyinstead ofcontextKeyQueryParams. Apply the same pattern to token keys ininternal/security.internal/analytics/analytics.go (1)
28-32: Consider making BatchSendInterval a var for testabilityDeclaring
BatchSendIntervalas a const prevents tests from temporarily shortening the interval, forcing reliance on sleeps.Apply this diff:
const ( // Google Analytics 4 Measurement Protocol endpoint ga4EndpointURL = "https://www.google-analytics.com/mp/collect" // Configuration measurementID = "G-WJGRCEFLXF" userID = "692" - // Batch send interval for analytics data - BatchSendInterval = 10 * time.Second - maxPerRequest = 25 ) + +// BatchSendInterval is the batch send interval for analytics data (overridable in tests) +var BatchSendInterval = 10 * time.SecondAs per coding guidelines.
internal/http/http_server_batch_test.go (1)
266-271: Stop discardinghttp.NewRequesterrors in tests.We still ignore the error return here, so malformed URLs/payloads will silently pass. Capture and fail the test instead.
- req, _ := http.NewRequest( + req, err := http.NewRequest( tt.method, "/test", - io.NopCloser(bytes.NewBufferString(tt.payload)), + bytes.NewBufferString(tt.payload), ) + if err != nil { + t.Fatalf("failed to create request: %v", err) + }
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (18)
cmd/reportportal-mcp-server/main.go(2 hunks)internal/analytics/analytics.go(3 hunks)internal/analytics/analytics_test.go(5 hunks)internal/config/cli.go(1 hunks)internal/http/http_server.go(11 hunks)internal/http/http_server_batch_test.go(1 hunks)internal/mcp_handlers/items.go(21 hunks)internal/mcp_handlers/launches.go(14 hunks)internal/mcp_handlers/launches_test.go(2 hunks)internal/mcp_handlers/server.go(4 hunks)internal/middleware/middleware.go(1 hunks)internal/middleware/middleware_test.go(2 hunks)internal/security/http_token_middleware.go(2 hunks)internal/utils/ctx_utils.go(2 hunks)internal/utils/ctx_utils_test.go(1 hunks)internal/utils/mcp_utils.go(1 hunks)internal/utils/utils.go(8 hunks)internal/utils/utils_test.go(7 hunks)
🧰 Additional context used
📓 Path-based instructions (4)
**/*.go
📄 CodeRabbit inference engine (README.md)
**/*.go: Run formatter (task fmt / go fmt) to keep all Go source files consistently formatted
Run linter (task lint) to maintain code quality across Go files
Files:
internal/utils/mcp_utils.gointernal/mcp_handlers/launches.gointernal/mcp_handlers/launches_test.gointernal/utils/ctx_utils_test.gointernal/security/http_token_middleware.gointernal/middleware/middleware_test.gointernal/http/http_server_batch_test.gointernal/analytics/analytics_test.gointernal/http/http_server.gointernal/mcp_handlers/server.gointernal/utils/utils_test.gointernal/utils/ctx_utils.gointernal/middleware/middleware.gointernal/mcp_handlers/items.gocmd/reportportal-mcp-server/main.gointernal/utils/utils.gointernal/config/cli.gointernal/analytics/analytics.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/utils/mcp_utils.gointernal/mcp_handlers/launches.gointernal/mcp_handlers/launches_test.gointernal/utils/ctx_utils_test.gointernal/security/http_token_middleware.gointernal/middleware/middleware_test.gointernal/http/http_server_batch_test.gointernal/analytics/analytics_test.gointernal/http/http_server.gointernal/mcp_handlers/server.gointernal/utils/utils_test.gointernal/utils/ctx_utils.gointernal/middleware/middleware.gointernal/mcp_handlers/items.gocmd/reportportal-mcp-server/main.gointernal/utils/utils.gointernal/config/cli.gointernal/analytics/analytics.go
**/*_test.go
⚙️ CodeRabbit configuration file
Ensure comprehensive test coverage, proper test isolation, meaningful assertions, and good test naming conventions.
Files:
internal/mcp_handlers/launches_test.gointernal/utils/ctx_utils_test.gointernal/middleware/middleware_test.gointernal/http/http_server_batch_test.gointernal/analytics/analytics_test.gointernal/utils/utils_test.go
cmd/reportportal-mcp-server/**/*.go
📄 CodeRabbit inference engine (README.md)
Keep the main server entrypoint under cmd/reportportal-mcp-server and build from that path
Files:
cmd/reportportal-mcp-server/main.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
🧠 Learnings (1)
📚 Learning: 2025-09-17T11:00:40.261Z
Learnt from: CR
PR: reportportal/reportportal-mcp-server#0
File: README.md:0-0
Timestamp: 2025-09-17T11:00:40.261Z
Learning: Applies to cmd/reportportal-mcp-server/**/*.go : Keep the main server entrypoint under cmd/reportportal-mcp-server and build from that path
Applied to files:
cmd/reportportal-mcp-server/main.go
🧬 Code graph analysis (13)
internal/utils/mcp_utils.go (1)
internal/utils/utils.go (4)
FirstPage(15-15)DefaultPageSize(16-16)PaginatedRequest(31-35)ReadResponseBodyRaw(193-215)
internal/mcp_handlers/launches.go (4)
internal/analytics/analytics.go (1)
Analytics(69-81)internal/utils/mcp_utils.go (6)
NewProjectParameter(65-71)SetPaginationOptions(14-29)WithAnalytics(83-97)ExtractProject(73-75)ApplyPaginationOptions(32-63)ReadResponseBody(102-111)internal/utils/utils.go (4)
DefaultSortingForLaunches(17-17)ProcessStartTimeFilter(99-121)ProcessAttributeKeys(124-152)ExtractResponseError(37-62)internal/utils/ctx_utils.go (1)
WithQueryParams(18-21)
internal/security/http_token_middleware.go (1)
internal/utils/ctx_utils.go (2)
ContextKey(12-12)ValidateRPToken(31-44)
internal/middleware/middleware_test.go (1)
internal/utils/ctx_utils.go (1)
WithQueryParams(18-21)
internal/http/http_server_batch_test.go (1)
internal/http/http_server.go (3)
HTTPServer(61-72)HTTPServerConfig(45-58)NewHTTPServer(82-139)
internal/analytics/analytics_test.go (1)
internal/utils/mcp_utils.go (1)
WithAnalytics(83-97)
internal/http/http_server.go (7)
internal/analytics/analytics.go (3)
Analytics(69-81)NewAnalytics(90-131)BatchSendInterval(29-29)internal/utils/ctx_utils.go (1)
ValidateRPToken(31-44)internal/middleware/middleware.go (1)
QueryParamsMiddleware(11-32)internal/mcp_handlers/launches.go (1)
RegisterLaunchTools(41-56)internal/mcp_handlers/items.go (1)
RegisterTestItemTools(41-54)internal/mcp_handlers/server.go (2)
ReadPrompts(71-93)PromptFiles(19-19)internal/security/http_token_middleware.go (1)
HTTPTokenMiddleware(19-42)
internal/mcp_handlers/server.go (5)
internal/analytics/analytics.go (2)
Analytics(69-81)NewAnalytics(90-131)internal/middleware/middleware.go (1)
QueryParamsMiddleware(11-32)internal/mcp_handlers/launches.go (1)
RegisterLaunchTools(41-56)internal/mcp_handlers/items.go (1)
RegisterTestItemTools(41-54)internal/promptreader/promptreader.go (1)
PromptHandlerPair(16-19)
internal/utils/utils_test.go (1)
internal/utils/utils.go (1)
ProcessAttributeKeys(124-152)
internal/middleware/middleware.go (2)
internal/security/http_token_middleware.go (1)
GetTokenFromContext(75-78)internal/utils/ctx_utils.go (1)
QueryParamsFromContext(23-27)
internal/mcp_handlers/items.go (4)
internal/analytics/analytics.go (1)
Analytics(69-81)internal/utils/mcp_utils.go (6)
NewProjectParameter(65-71)SetPaginationOptions(14-29)WithAnalytics(83-97)ExtractProject(73-75)ApplyPaginationOptions(32-63)ReadResponseBody(102-111)internal/utils/utils.go (16)
DefaultSortingForItems(18-18)DefaultProviderType(21-21)DefaultFilterEqHasStats(23-23)DefaultFilterEqHasChildren(22-22)DefaultFilterInType(24-24)ProcessStartTimeFilter(99-121)ProcessAttributeKeys(124-152)ExtractResponseError(37-62)ReadResponseBodyRaw(193-215)IsTextContent(154-174)FirstPage(15-15)DefaultPageSize(16-16)DefaultSortingForLogs(20-20)DefaultItemLogLevel(26-26)DefaultSortingForSuites(19-19)DefaultFilterInTypeSuites(25-25)internal/utils/ctx_utils.go (1)
WithQueryParams(18-21)
cmd/reportportal-mcp-server/main.go (1)
internal/config/cli.go (2)
AppVersion(25-29)RunApp(337-390)
internal/config/cli.go (3)
internal/http/http_server.go (2)
HTTPServerConfig(45-58)CreateHTTPServerWithMiddleware(219-245)internal/analytics/analytics.go (3)
GetAnalyticArg(232-246)Analytics(69-81)StopAnalytics(403-412)internal/mcp_handlers/server.go (1)
NewServer(21-68)
🔇 Additional comments (12)
internal/utils/ctx_utils_test.go (1)
1-1: Package rename looks goodConsistent with utils package refactor. No issues spotted.
internal/analytics/analytics_test.go (1)
16-18: Switch to utils.WithAnalytics is correctDecouples analytics wrapper from analytics package; consistent with refactor.
cmd/reportportal-mcp-server/main.go (1)
24-35: Main delegating to config.RunApp with proper error handlingStraightforward, testable entrypoint. Looks good.
internal/utils/mcp_utils.go (3)
37-63: Pagination clamping is correct and safeLower/upper bounds prevent negatives and int32 overflow before casting. Good use of math.MaxInt32.
82-97: Nil-safe analytics wrapperWrapper cleanly tracks before invoking handler; nil tracker check prevents panics.
102-111: Tool-friendly response reader behaviorReturning a ToolResultError with nil Go error preserves MCP semantics while surfacing the failure to clients.
internal/security/http_token_middleware.go (1)
1-78: LGTM: Token validation properly integrated with utils packageThe migration to
utils.ContextKeyandutils.ValidateRPTokencorrectly centralizes validation logic and aligns with the broader refactoring.internal/middleware/middleware.go (1)
11-32: LGTM: Authorization header injection vulnerability addressedToken sanitization with
strings.TrimSpaceandstrings.ContainsAnycheck prevents header injection attacks. The implementation correctly validates the token before setting the Authorization header.internal/utils/utils_test.go (1)
8-172: LGTM: Comprehensive test coverage for ProcessAttributeKeysTest cases thoroughly validate the attribute key processing logic, including edge cases for whitespace handling, empty keys, multiple colons, and key:value pair preservation. The performance test ensures the function handles large inputs efficiently.
internal/utils/utils.go (3)
99-121: LGTM!The function correctly validates time intervals, ensures proper ordering, and handles edge cases with clear error messages.
177-189: LGTM!The string-based error detection is a pragmatic solution for handling various HTTP implementation error messages. The coverage of common error patterns is comprehensive.
193-215: LGTM!The function properly handles resource cleanup with defensive nil checks and avoids noisy logging for already-closed bodies. The error handling is thorough and follows Go best practices.
There was a problem hiding this comment.
Pull Request Overview
Copilot reviewed 18 out of 19 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.
AleksandrParamonoff
force-pushed
the
refactoring-detaching-app-to-modules
branch
from
c59c94b to
f7871a6
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
internal/http/http_server.go (3)
423-433: Cap request body size in MCP middleware to prevent memory DoSvalidateMCPPayload reads the entire body. Add a size limit before parsing.
func (hs *HTTPServer) mcpMiddleware(next http.Handler) http.Handler { return http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + // Cap request body to 4MB to mitigate memory DoS + r.Body = http.MaxBytesReader(w, r.Body, 4<<20) // Validate that this is a proper MCP request if !hs.isMCPRequest(r) { http.Error(w, "Invalid MCP request", http.StatusBadRequest) return } next.ServeHTTP(w, r) }) }
310-321: Restrict MCP endpoints to POST for correct semanticsCurrently all methods hit these handlers and non-POSTs return 400. Prefer method-limited routes to return 405 instead.
- mcpRouter.Handle("/mcp", hs.streamableServer) - mcpRouter.Handle("/api/mcp", hs.streamableServer) - mcpRouter.Handle("/mcp/*", hs.streamableServer) - mcpRouter.Handle("/api/mcp/*", hs.streamableServer) + mcpRouter.Method(http.MethodPost, "/mcp", hs.streamableServer) + mcpRouter.Method(http.MethodPost, "/api/mcp", hs.streamableServer) + mcpRouter.Method(http.MethodPost, "/mcp/*", hs.streamableServer) + mcpRouter.Method(http.MethodPost, "/api/mcp/*", hs.streamableServer)
161-169: Fix ReadPrompts slice allocation to avoid zero-value entries
Use a zero-length, capacity-preallocated slice so that only real prompts are included.Diff (internal/mcp_handlers/server.go):
- handlers := make([]promptreader.PromptHandlerPair, len(entries)) + handlers := make([]promptreader.PromptHandlerPair, 0, len(entries))
♻️ Duplicate comments (16)
internal/analytics/analytics_test.go (1)
180-189: Verify analytics metric was incremented in test.The test wraps the handler with
utils.WithAnalyticsbut doesn't assert that the metric counter was actually incremented. This reduces test coverage.Add metric verification after the wrapped handler call:
result, err := wrappedHandler(context.Background(), mcp.CallToolRequest{}) // Verify handler was called assert.NoError(t, err) assert.NotNil(t, result) assert.True(t, handlerCalled, "Original handler should be called") + + // Verify the metric was incremented + analytics.metricsLock.RLock() + defer analytics.metricsLock.RUnlock() + counter, exists := analytics.metrics["test_tool"] + assert.True(t, exists, "Metric for test_tool should exist") + assert.NotNil(t, counter, "Counter should not be nil") + assert.Equal(t, int64(1), *counter, "Counter should be incremented")Based on learnings (past review suggestion).
internal/mcp_handlers/launches.go (1)
279-291: Fix launch_id parameter type mismatch.The parameter is defined as a string with
mcp.WithString("launch_id", ...)(line 279) but the handler callsrequest.RequireInt("launch_id")(line 288), causing runtime errors.Apply this diff:
- mcp.WithString("launch_id", // Parameter for specifying the launch name + mcp.WithNumber("launch_id", // Parameter for specifying the launch ID mcp.Description("Launch ID"), ),Also apply to ToolRunAutoAnalysis (line 310), ToolUniqueErrorAnalysis (line 387), and ToolForceFinishLaunch (line 431).
Based on learnings (past review concern).
internal/mcp_handlers/items.go (2)
334-353: Use numeric parameter type for attachment-content-id.The parameter is defined as a string (line 334) then manually parsed as int64 (lines 344-353). Defining it as numeric eliminates the parsing code and provides better type safety.
Apply this diff:
- mcp.WithString("attachment-content-id", // Parameter for specifying the test item ID + mcp.WithNumber("attachment-content-id", // Parameter for specifying the attachment ID mcp.Description("Attachment binary content ID"), ),Then update the handler:
- // Extract the "attachment-content-id" parameter from the request - attachmentIdStr, err := request.RequireString("attachment-content-id") + attachmentId, err := request.RequireInt("attachment-content-id") if err != nil { return mcp.NewToolResultError(err.Error()), nil } - attachmentId, err := strconv.ParseInt(attachmentIdStr, 10, 64) - if err != nil { - return mcp.NewToolResultError( - fmt.Sprintf("invalid attachment ID value: %s", attachmentIdStr), - ), nil - } + if attachmentId < 0 { + return mcp.NewToolResultError("attachment ID must be non-negative"), nil + }Based on learnings (past review suggestion).
48-50: Consider exporting toolGetTestItemLogsByFilter for consistency.All other tool methods in this file are exported (e.g.,
ToolGetTestItemById,ToolGetTestItemsByFilter), buttoolGetTestItemLogsByFilterremains unexported. While not functionally wrong, the inconsistency may confuse API consumers.Apply this diff:
s.AddTool(testItems.ToolGetTestItemById()) s.AddTool(testItems.ToolGetTestItemsByFilter()) - s.AddTool(testItems.toolGetTestItemLogsByFilter()) + s.AddTool(testItems.ToolGetTestItemLogsByFilter())Then rename the function definition accordingly (around line 394).
Based on learnings (past review suggestion).
internal/utils/utils.go (2)
27-27: Remove redundant maxInt32 wrapper constant.The
maxInt32constant simply wrapsmath.MaxInt32without adding value. Code should referencemath.MaxInt32directly.Apply this diff:
DefaultFilterInTypeSuites = "SUITE,TEST" DefaultItemLogLevel = "TRACE" // Default log level for test item logs - maxInt32 = math.MaxInt32 // Standard library constant for maximum int32 value )Then search for any usages and replace with
math.MaxInt32.Based on learnings (past review suggestion).
136-137: Clarify comment for better understanding.The comment "Keep full key with colon" doesn't explain the condition. The check ensures the colon is not at the start or end of the string, preserving valid key:value pairs.
Apply this diff:
- if colonIndex := strings.Index(key, ":"); colonIndex > 0 && colonIndex < len(key)-1 { - processed = append(processed, key) // Keep full key with colon + if colonIndex := strings.Index(key, ":"); colonIndex > 0 && colonIndex < len(key)-1 { + processed = append(processed, key) // Keep key:value pair with content on both sidesBased on learnings (past review suggestion).
internal/http/http_server_batch_test.go (2)
266-271: Don't ignore http.NewRequest error.The error from
http.NewRequestis discarded with the blank identifier (line 266), which could hide malformed URLs or payload issues and cause test failures to be misattributed.Apply this diff:
for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { - req, _ := http.NewRequest( + req, err := http.NewRequest( tt.method, "/test", - io.NopCloser(bytes.NewBufferString(tt.payload)), + bytes.NewBufferString(tt.payload), ) + if err != nil { + t.Fatalf("failed to create request: %v", err) + } req.Header.Set("Content-Type", tt.contentType)As per coding guidelines (proper error handling).
234-262: Add test case for Content-Type with charset parameter.Real-world JSON requests often include charset parameters (e.g.,
application/json; charset=utf-8). Adding a test case ensures the parser handles this correctly.Add to the test table:
{ name: "POST with XML content", method: "POST", contentType: "application/xml", payload: `<xml></xml>`, expected: false, }, + { + name: "POST with JSON and charset", + method: "POST", + contentType: "application/json; charset=utf-8", + payload: `{"jsonrpc":"2.0","method":"test","id":1}`, + expected: true, + }, }Based on learnings (past review suggestion).
internal/analytics/analytics.go (1)
28-29: Consider making BatchSendInterval a var for test flexibility.While the current const works, tests that verify batch processing behavior would benefit from a shorter interval. Making this a var allows tests to temporarily override the duration.
Apply this diff:
- // Batch send interval for analytics data - BatchSendInterval = 10 * time.Second +) +// Batch send interval for analytics data (can be overridden in tests) +var BatchSendInterval = 10 * time.Second +const ( maxPerRequest = 25Based on learnings (past review suggestion).
internal/middleware/middleware_test.go (2)
13-29: Add test for Authorization header sourced from context.To ensure middleware properly retrieves and sets the Authorization header from context, add a test case.
import ( + "context" "net/http" "net/url" "testing" "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" "github.com/reportportal/reportportal-mcp-server/internal/utils" + "github.com/reportportal/reportportal-mcp-server/internal/security" ) @@ assert.Equal(t, []string{"1"}, got["a"]) assert.Equal(t, []string{"2", "3"}, got["b"]) } + +func TestQueryParamsMiddleware_AddsAuthorizationFromContext(t *testing.T) { + req, err := http.NewRequest("GET", "http://example.com/path", nil) + require.NoError(t, err) + ctx := context.WithValue(req.Context(), security.RPTokenContextKey, "abc123") + req = req.WithContext(ctx) + + QueryParamsMiddleware(req) + + assert.Equal(t, "Bearer abc123", req.Header.Get("Authorization")) +}
19-19: CheckNewRequesterror to prevent test failures.The error from
http.NewRequestis still being ignored. This should be checked to ensure test clarity if the request creation fails.- req, _ := http.NewRequest("GET", "http://example.com/path?x=9", nil) + req, err := http.NewRequest("GET", "http://example.com/path?x=9", nil) + require.NoError(t, err)As per coding guidelines.
internal/utils/ctx_utils.go (1)
11-16: Context key type remains an exported string; prefer unexported struct to avoid collisions.The
ContextKeytype is still an exported string, which can lead to collisions. Best practice is to use an unexported, zero-sized struct type with exported key variables.For example:
type queryParamsKey struct{} // QueryParamsKey is the context key for query parameters var QueryParamsKey = queryParamsKey{} var contextKeyQueryParams = QueryParamsKeyApply the same pattern to
RPTokenContextKeyininternal/securityfor consistency. Based on learnings.internal/mcp_handlers/server.go (1)
42-50: Analytics initialization returns hard error; consider non-fatal fallback.The current implementation treats analytics initialization failure as fatal and returns an error. The past review suggested aligning with HTTP server behavior by logging a warning and continuing with
analyticsClient = nilinstead.// Initialize analytics (disabled if analyticsOff is true) var analyticsClient *analytics.Analytics if analyticsOn { - var err error - // Pass RP API token for secure hashing as user identifier - analyticsClient, err = analytics.NewAnalytics(userID, analyticsAPISecret, token) - if err != nil { - return nil, nil, fmt.Errorf("failed to initialize analytics: %w", err) - } + ac, err := analytics.NewAnalytics(userID, analyticsAPISecret, token) + if err != nil { + // Fall back to no analytics and continue + slog.Warn("analytics initialization skipped", "error", err) + } else { + analyticsClient = ac + } }You'll need to add
"log/slog"to imports. Based on learnings.internal/mcp_handlers/launches_test.go (1)
45-46: Checkurl.Parseerror despite past review being marked as resolved.The error from
url.Parseis still being ignored on Line 45, even though the past review comment was marked as addressed in commit c59c94b. Ensure URL parsing errors are caught.- serverURL, _ := url.Parse(mockServer.URL) + serverURL, urlErr := url.Parse(mockServer.URL) + require.NoError(t, urlErr)internal/config/cli.go (2)
111-119: Dedup lowercasing and use structured logging in GetMCPModeYou lowercase twice and log via string concat. Keep it once and use structured slog.
func GetMCPMode() string { - rawMcpMode := strings.ToLower(os.Getenv("MCP_MODE")) - slog.Debug("MCP_MODE env variable is set to: " + rawMcpMode) - mcpMode := strings.ToLower(rawMcpMode) + rawMcpMode := os.Getenv("MCP_MODE") + mcpMode := strings.ToLower(rawMcpMode) + slog.Debug("MCP_MODE env variable", "value", mcpMode) if mcpMode == "" { mcpMode = "stdio" } return mcpMode }As per coding guidelines.
291-300: Use the wrapper Handler instead of reaching into MCP.RouterAvoid tight coupling; you already have a pre-wired handler from CreateHTTPServerWithMiddleware.
server := &http.Server{ Addr: addr, - Handler: httpServer.MCP.Router, // Use Chi router directly with throttling/health/info/metrics + Handler: httpServer.Handler, // Use pre‑wired Chi router with middleware ReadHeaderTimeout: 10 * time.Second, ReadTimeout: 30 * time.Second, WriteTimeout: 30 * time.Second, IdleTimeout: 120 * time.Second, }
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (18)
cmd/reportportal-mcp-server/main.go(2 hunks)internal/analytics/analytics.go(3 hunks)internal/analytics/analytics_test.go(5 hunks)internal/config/cli.go(1 hunks)internal/http/http_server.go(11 hunks)internal/http/http_server_batch_test.go(1 hunks)internal/mcp_handlers/items.go(21 hunks)internal/mcp_handlers/launches.go(14 hunks)internal/mcp_handlers/launches_test.go(2 hunks)internal/mcp_handlers/server.go(4 hunks)internal/middleware/middleware.go(1 hunks)internal/middleware/middleware_test.go(2 hunks)internal/security/http_token_middleware.go(2 hunks)internal/utils/ctx_utils.go(2 hunks)internal/utils/ctx_utils_test.go(1 hunks)internal/utils/mcp_utils.go(1 hunks)internal/utils/utils.go(8 hunks)internal/utils/utils_test.go(7 hunks)
🧰 Additional context used
📓 Path-based instructions (4)
**/*.go
📄 CodeRabbit inference engine (README.md)
**/*.go: Run formatter (task fmt / go fmt) to keep all Go source files consistently formatted
Run linter (task lint) to maintain code quality across Go files
Files:
internal/mcp_handlers/launches.gointernal/utils/mcp_utils.gointernal/middleware/middleware_test.gointernal/security/http_token_middleware.gointernal/middleware/middleware.gointernal/http/http_server_batch_test.gointernal/mcp_handlers/launches_test.gocmd/reportportal-mcp-server/main.gointernal/analytics/analytics_test.gointernal/mcp_handlers/server.gointernal/utils/utils.gointernal/analytics/analytics.gointernal/utils/ctx_utils_test.gointernal/config/cli.gointernal/utils/utils_test.gointernal/http/http_server.gointernal/utils/ctx_utils.gointernal/mcp_handlers/items.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/mcp_handlers/launches.gointernal/utils/mcp_utils.gointernal/middleware/middleware_test.gointernal/security/http_token_middleware.gointernal/middleware/middleware.gointernal/http/http_server_batch_test.gointernal/mcp_handlers/launches_test.gocmd/reportportal-mcp-server/main.gointernal/analytics/analytics_test.gointernal/mcp_handlers/server.gointernal/utils/utils.gointernal/analytics/analytics.gointernal/utils/ctx_utils_test.gointernal/config/cli.gointernal/utils/utils_test.gointernal/http/http_server.gointernal/utils/ctx_utils.gointernal/mcp_handlers/items.go
**/*_test.go
⚙️ CodeRabbit configuration file
Ensure comprehensive test coverage, proper test isolation, meaningful assertions, and good test naming conventions.
Files:
internal/middleware/middleware_test.gointernal/http/http_server_batch_test.gointernal/mcp_handlers/launches_test.gointernal/analytics/analytics_test.gointernal/utils/ctx_utils_test.gointernal/utils/utils_test.go
cmd/reportportal-mcp-server/**/*.go
📄 CodeRabbit inference engine (README.md)
Keep the main server entrypoint under cmd/reportportal-mcp-server and build from that path
Files:
cmd/reportportal-mcp-server/main.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
🧠 Learnings (1)
📚 Learning: 2025-09-17T11:00:40.261Z
Learnt from: CR
PR: reportportal/reportportal-mcp-server#0
File: README.md:0-0
Timestamp: 2025-09-17T11:00:40.261Z
Learning: Applies to cmd/reportportal-mcp-server/**/*.go : Keep the main server entrypoint under cmd/reportportal-mcp-server and build from that path
Applied to files:
cmd/reportportal-mcp-server/main.go
🧬 Code graph analysis (13)
internal/mcp_handlers/launches.go (4)
internal/analytics/analytics.go (1)
Analytics(69-81)internal/utils/mcp_utils.go (6)
NewProjectParameter(65-71)SetPaginationOptions(14-29)WithAnalytics(83-97)ExtractProject(73-75)ApplyPaginationOptions(32-63)ReadResponseBody(102-111)internal/utils/utils.go (4)
DefaultSortingForLaunches(17-17)ProcessStartTimeFilter(99-121)ProcessAttributeKeys(124-152)ExtractResponseError(37-62)internal/utils/ctx_utils.go (1)
WithQueryParams(18-21)
internal/utils/mcp_utils.go (1)
internal/utils/utils.go (4)
FirstPage(15-15)DefaultPageSize(16-16)PaginatedRequest(31-35)ReadResponseBodyRaw(193-215)
internal/middleware/middleware_test.go (1)
internal/utils/ctx_utils.go (1)
WithQueryParams(18-21)
internal/security/http_token_middleware.go (1)
internal/utils/ctx_utils.go (2)
ContextKey(12-12)ValidateRPToken(31-44)
internal/middleware/middleware.go (2)
internal/security/http_token_middleware.go (1)
GetTokenFromContext(75-78)internal/utils/ctx_utils.go (1)
QueryParamsFromContext(23-27)
internal/http/http_server_batch_test.go (1)
internal/http/http_server.go (3)
HTTPServer(61-72)HTTPServerConfig(45-58)NewHTTPServer(82-139)
cmd/reportportal-mcp-server/main.go (1)
internal/config/cli.go (2)
AppVersion(25-29)RunApp(337-390)
internal/analytics/analytics_test.go (1)
internal/utils/mcp_utils.go (1)
WithAnalytics(83-97)
internal/mcp_handlers/server.go (5)
internal/analytics/analytics.go (2)
Analytics(69-81)NewAnalytics(90-131)internal/middleware/middleware.go (1)
QueryParamsMiddleware(11-32)internal/mcp_handlers/launches.go (1)
RegisterLaunchTools(41-56)internal/mcp_handlers/items.go (1)
RegisterTestItemTools(41-54)internal/promptreader/promptreader.go (1)
PromptHandlerPair(16-19)
internal/config/cli.go (3)
internal/http/http_server.go (2)
HTTPServerConfig(45-58)CreateHTTPServerWithMiddleware(219-245)internal/analytics/analytics.go (3)
GetAnalyticArg(232-246)Analytics(69-81)StopAnalytics(403-412)internal/mcp_handlers/server.go (1)
NewServer(21-68)
internal/utils/utils_test.go (1)
internal/utils/utils.go (1)
ProcessAttributeKeys(124-152)
internal/http/http_server.go (7)
internal/analytics/analytics.go (3)
Analytics(69-81)NewAnalytics(90-131)BatchSendInterval(29-29)internal/utils/ctx_utils.go (1)
ValidateRPToken(31-44)internal/middleware/middleware.go (1)
QueryParamsMiddleware(11-32)internal/mcp_handlers/launches.go (1)
RegisterLaunchTools(41-56)internal/mcp_handlers/items.go (1)
RegisterTestItemTools(41-54)internal/mcp_handlers/server.go (2)
ReadPrompts(71-93)PromptFiles(19-19)internal/security/http_token_middleware.go (1)
HTTPTokenMiddleware(19-42)
internal/mcp_handlers/items.go (4)
internal/analytics/analytics.go (1)
Analytics(69-81)internal/utils/mcp_utils.go (5)
NewProjectParameter(65-71)SetPaginationOptions(14-29)WithAnalytics(83-97)ApplyPaginationOptions(32-63)ReadResponseBody(102-111)internal/utils/utils.go (16)
DefaultSortingForItems(18-18)DefaultProviderType(21-21)DefaultFilterEqHasStats(23-23)DefaultFilterEqHasChildren(22-22)DefaultFilterInType(24-24)ProcessStartTimeFilter(99-121)ProcessAttributeKeys(124-152)ExtractResponseError(37-62)ReadResponseBodyRaw(193-215)IsTextContent(154-174)FirstPage(15-15)DefaultPageSize(16-16)DefaultSortingForLogs(20-20)DefaultItemLogLevel(26-26)DefaultSortingForSuites(19-19)DefaultFilterInTypeSuites(25-25)internal/utils/ctx_utils.go (1)
WithQueryParams(18-21)
🔇 Additional comments (7)
internal/utils/mcp_utils.go (1)
1-111: LGTM! Pagination and MCP utility functions are well-structured.The file correctly addresses the past review concerns:
math.MaxInt32is now imported and used for bounds checking- Pagination parameters are properly clamped to valid ranges (minimum and maximum)
- The generic
ApplyPaginationOptionsfunction provides good type safetyAll utility functions follow Go best practices with appropriate error handling and nil checks.
internal/security/http_token_middleware.go (1)
1-78: LGTM! Token validation and context key usage aligned with utilities.The integration with
utils.ValidateRPTokenand the updatedRPTokenContextKeytype are correct. The middleware properly extracts, validates, and stores the token in context.internal/middleware/middleware.go (1)
11-32: LGTM! Token sanitization and query parameter handling implemented correctly.The middleware now properly sanitizes tokens (trimming and checking for CR/LF) before setting the Authorization header, addressing the previous security concern. Query parameter handling via
utils.QueryParamsFromContextis also correct.internal/utils/utils_test.go (1)
1-172: LGTM! Comprehensive test coverage forProcessAttributeKeys.The tests are thorough and cover basic cases, edge cases, mixed scenarios, whitespace handling, and performance. Test expectations correctly validate that
key:valuepairs are preserved intact.internal/mcp_handlers/server.go (2)
53-56: LGTM! Centralized tool registration.The use of
RegisterLaunchToolsandRegisterTestItemToolsproperly centralizes the registration logic and passes the analytics client through, aligning with the refactoring objectives.
70-93: LGTM!ReadPromptscorrectly exported and implemented.The function properly reads multiple YAML files from the embedded filesystem and returns prompt handler pairs. The filepath handling using forward slashes is correct for
embed.FS.cmd/reportportal-mcp-server/main.go (1)
24-35: Approve: Main delegates to config-driven startup Confirmed nolog.Fataloros.Exitcalls ininternal/config, soconfig.RunApphandles errors correctly.
AleksandrParamonoff
force-pushed
the
refactoring-detaching-app-to-modules
branch
from
f7871a6 to
c4b1035
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
internal/utils/utils.go (1)
154-174: Critical bug: switch cases don't return, causing incorrect false results.The
switchstatement has cases for JSON, XML, etc., but none of them executereturn true. All cases fall through to line 173, returningfalsefor content types that should be recognized as text.Apply this diff to fix the bug:
// Popular application types that are text-based switch lowerType { - case "application/json": - case "application/xml": - case "application/javascript": - case "application/xhtml+xml": - case "application/yaml": - case "application/csv": + case "application/json", + "application/xml", + "application/javascript", + "application/xhtml+xml", + "application/yaml", + "application/csv": return true }internal/mcp_handlers/launches.go (1)
274-302: Type mismatch: schema defineslaunch_idas string but handler callsRequireInt.The parameter is declared as
mcp.WithString("launch_id", ...)but the handler callsrequest.RequireInt("launch_id"), causing runtime errors when clients send string values.Apply this diff:
- mcp.WithString("launch_id", // Parameter for specifying the launch name + mcp.WithNumber("launch_id", mcp.Description("Launch ID"), ),Also apply the same fix to:
ToolRunAutoAnalysis(line 310)ToolUniqueErrorAnalysis(line 387)ToolForceFinishLaunch(line 431)internal/mcp_handlers/items.go (1)
329-390: Consider changingattachment-content-idto numeric type.The parameter is defined as
mcp.WithString(line 334) but immediately parsed asint64(lines 344-353). Declaring it asmcp.WithNumberwould eliminate parsing code and potential conversion errors.Apply this diff:
- mcp.WithString("attachment-content-id", // Parameter for specifying the test item ID + mcp.WithNumber("attachment-content-id", mcp.Description("Attachment binary content ID"), ),And update the handler:
- attachmentIdStr, err := request.RequireString("attachment-content-id") + attachmentId, err := request.RequireInt("attachment-content-id") if err != nil { return mcp.NewToolResultError(err.Error()), nil } - attachmentId, err := strconv.ParseInt(attachmentIdStr, 10, 64) - if err != nil { - return mcp.NewToolResultError( - fmt.Sprintf("invalid attachment ID value: %s", attachmentIdStr), - ), nil - }
♻️ Duplicate comments (6)
internal/mcp_handlers/launches.go (3)
305-379: Same type mismatch inToolRunAutoAnalysis.Apply this diff:
- mcp.WithString("launch_id", // Parameter for specifying the launch name + mcp.WithNumber("launch_id", mcp.Description("Launch ID"), ),
382-423: Same type mismatch inToolUniqueErrorAnalysis.Apply this diff:
- mcp.WithString("launch_id", // Parameter for specifying the launch name + mcp.WithNumber("launch_id", mcp.Description("Launch ID"), ),
426-457: Same type mismatch inToolForceFinishLaunch.Apply this diff:
- mcp.WithString("launch_id", // Parameter for specifying the launch name + mcp.WithNumber("launch_id", mcp.Description("Launch ID"), ),internal/utils/ctx_utils.go (1)
11-16: Use an unexported sentinel type for context keys to avoid collisions
Re-introducing an exportedContextKey(string-backed) lets any package mint its ownutils.ContextKey("queryParams"), so unrelated code can silently overwrite the same context slot. To keep the key unique, stick with an unexported zero-sized struct and expose only the exported helper or key variable. That preserves cross-package sharing without reopening the collision hazard we flagged earlier.-// ContextKey is a type for context keys to avoid collisions -type ContextKey string - -var contextKeyQueryParams = ContextKey( - "queryParams", -) // Key for storing query parameters in the context +type contextKeyQueryParams struct{} + +var contextKeyQueryParams = contextKeyQueryParams{} // unique sentinel for query paramsCallers can continue using
WithQueryParams/QueryParamsFromContext(and other packages should expose their own exported key values instead of constructing newContextKeys).internal/mcp_handlers/server.go (1)
41-49: Don’t fail server startup when analytics can’t initializeUnlike the HTTP path, this constructor still hard‑fails if
analytics.NewAnalyticsreturns an error. That makes the whole MCP server unusable whenever analytics is misconfigured (empty GA secret, missing token in tests, etc.), even though analytics is optional.Prefer to log a warning and continue without analytics:
var analyticsClient *analytics.Analytics if analyticsOn { - var err error - // Pass RP API token for secure hashing as user identifier - analyticsClient, err = analytics.NewAnalytics(userID, analyticsAPISecret, token) - if err != nil { - return nil, nil, fmt.Errorf("failed to initialize analytics: %w", err) - } + ac, err := analytics.NewAnalytics(userID, analyticsAPISecret, token) + if err != nil { + slog.Warn("analytics initialization skipped", "error", err) + } else { + analyticsClient = ac + } }(remember to import
"log/slog"). That keeps startup resilient while still surfacing misconfiguration.internal/http/http_server_batch_test.go (1)
265-271: Handlehttp.NewRequesterrors in testsHere we still discard
http.NewRequest’s error, which can silently hide malformed URLs or payload edge cases. Let the test fail fast so we know why it broke.A simple, explicit fix:
- req, _ := http.NewRequest( - tt.method, - "/test", - io.NopCloser(bytes.NewBufferString(tt.payload)), - ) + req, err := http.NewRequest(tt.method, "/test", bytes.NewBufferString(tt.payload)) + if err != nil { + t.Fatalf("failed to create request: %v", err) + }This keeps the test readable while making failures actionable.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (18)
cmd/reportportal-mcp-server/main.go(2 hunks)internal/analytics/analytics.go(3 hunks)internal/analytics/analytics_test.go(5 hunks)internal/config/cli.go(1 hunks)internal/http/http_server.go(12 hunks)internal/http/http_server_batch_test.go(1 hunks)internal/mcp_handlers/items.go(21 hunks)internal/mcp_handlers/launches.go(14 hunks)internal/mcp_handlers/launches_test.go(2 hunks)internal/mcp_handlers/server.go(4 hunks)internal/middleware/middleware.go(1 hunks)internal/middleware/middleware_test.go(2 hunks)internal/security/http_token_middleware.go(2 hunks)internal/utils/ctx_utils.go(2 hunks)internal/utils/ctx_utils_test.go(1 hunks)internal/utils/mcp_utils.go(1 hunks)internal/utils/utils.go(8 hunks)internal/utils/utils_test.go(7 hunks)
🧰 Additional context used
📓 Path-based instructions (4)
**/*.go
📄 CodeRabbit inference engine (README.md)
**/*.go: Run formatter (task fmt / go fmt) to keep all Go source files consistently formatted
Run linter (task lint) to maintain code quality across Go files
Files:
internal/mcp_handlers/launches.gointernal/http/http_server_batch_test.gointernal/utils/utils_test.gointernal/mcp_handlers/launches_test.gointernal/security/http_token_middleware.gointernal/utils/utils.gointernal/middleware/middleware.gointernal/utils/ctx_utils_test.gointernal/http/http_server.gointernal/utils/ctx_utils.gocmd/reportportal-mcp-server/main.gointernal/mcp_handlers/server.gointernal/config/cli.gointernal/middleware/middleware_test.gointernal/mcp_handlers/items.gointernal/analytics/analytics_test.gointernal/utils/mcp_utils.gointernal/analytics/analytics.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/mcp_handlers/launches.gointernal/http/http_server_batch_test.gointernal/utils/utils_test.gointernal/mcp_handlers/launches_test.gointernal/security/http_token_middleware.gointernal/utils/utils.gointernal/middleware/middleware.gointernal/utils/ctx_utils_test.gointernal/http/http_server.gointernal/utils/ctx_utils.gocmd/reportportal-mcp-server/main.gointernal/mcp_handlers/server.gointernal/config/cli.gointernal/middleware/middleware_test.gointernal/mcp_handlers/items.gointernal/analytics/analytics_test.gointernal/utils/mcp_utils.gointernal/analytics/analytics.go
**/*_test.go
⚙️ CodeRabbit configuration file
Ensure comprehensive test coverage, proper test isolation, meaningful assertions, and good test naming conventions.
Files:
internal/http/http_server_batch_test.gointernal/utils/utils_test.gointernal/mcp_handlers/launches_test.gointernal/utils/ctx_utils_test.gointernal/middleware/middleware_test.gointernal/analytics/analytics_test.go
cmd/reportportal-mcp-server/**/*.go
📄 CodeRabbit inference engine (README.md)
Keep the main server entrypoint under cmd/reportportal-mcp-server and build from that path
Files:
cmd/reportportal-mcp-server/main.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
🧠 Learnings (1)
📚 Learning: 2025-09-17T11:00:40.261Z
Learnt from: CR
PR: reportportal/reportportal-mcp-server#0
File: README.md:0-0
Timestamp: 2025-09-17T11:00:40.261Z
Learning: Applies to cmd/reportportal-mcp-server/**/*.go : Keep the main server entrypoint under cmd/reportportal-mcp-server and build from that path
Applied to files:
cmd/reportportal-mcp-server/main.go
🧬 Code graph analysis (13)
internal/mcp_handlers/launches.go (4)
internal/analytics/analytics.go (1)
Analytics(69-81)internal/utils/mcp_utils.go (6)
NewProjectParameter(65-71)SetPaginationOptions(14-29)WithAnalytics(83-97)ExtractProject(73-75)ApplyPaginationOptions(32-63)ReadResponseBody(102-111)internal/utils/utils.go (4)
DefaultSortingForLaunches(17-17)ProcessStartTimeFilter(99-121)ProcessAttributeKeys(124-152)ExtractResponseError(37-62)internal/utils/ctx_utils.go (1)
WithQueryParams(18-21)
internal/http/http_server_batch_test.go (1)
internal/http/http_server.go (3)
HTTPServer(66-77)HTTPServerConfig(50-63)NewHTTPServer(87-144)
internal/utils/utils_test.go (1)
internal/utils/utils.go (1)
ProcessAttributeKeys(124-152)
internal/security/http_token_middleware.go (1)
internal/utils/ctx_utils.go (2)
ContextKey(12-12)ValidateRPToken(31-44)
internal/middleware/middleware.go (2)
internal/security/http_token_middleware.go (1)
GetTokenFromContext(75-78)internal/utils/ctx_utils.go (1)
QueryParamsFromContext(23-27)
internal/http/http_server.go (7)
internal/analytics/analytics.go (3)
Analytics(69-81)NewAnalytics(90-131)BatchSendInterval(29-29)internal/utils/ctx_utils.go (1)
ValidateRPToken(31-44)internal/middleware/middleware.go (1)
QueryParamsMiddleware(11-32)internal/mcp_handlers/launches.go (1)
RegisterLaunchTools(41-56)internal/mcp_handlers/items.go (1)
RegisterTestItemTools(41-54)internal/mcp_handlers/server.go (2)
ReadPrompts(71-93)PromptFiles(19-19)internal/security/http_token_middleware.go (1)
HTTPTokenMiddleware(19-42)
cmd/reportportal-mcp-server/main.go (1)
internal/config/cli.go (2)
AppVersion(25-29)RunApp(336-389)
internal/mcp_handlers/server.go (5)
internal/analytics/analytics.go (2)
Analytics(69-81)NewAnalytics(90-131)internal/middleware/middleware.go (1)
QueryParamsMiddleware(11-32)internal/mcp_handlers/launches.go (1)
RegisterLaunchTools(41-56)internal/mcp_handlers/items.go (1)
RegisterTestItemTools(41-54)internal/promptreader/promptreader.go (1)
PromptHandlerPair(16-19)
internal/config/cli.go (3)
internal/http/http_server.go (2)
HTTPServerConfig(50-63)CreateHTTPServerWithMiddleware(224-250)internal/analytics/analytics.go (3)
GetAnalyticArg(232-246)Analytics(69-81)StopAnalytics(403-412)internal/mcp_handlers/server.go (1)
NewServer(21-68)
internal/middleware/middleware_test.go (1)
internal/utils/ctx_utils.go (1)
WithQueryParams(18-21)
internal/mcp_handlers/items.go (4)
internal/analytics/analytics.go (1)
Analytics(69-81)internal/utils/mcp_utils.go (6)
NewProjectParameter(65-71)SetPaginationOptions(14-29)WithAnalytics(83-97)ExtractProject(73-75)ApplyPaginationOptions(32-63)ReadResponseBody(102-111)internal/utils/utils.go (16)
DefaultSortingForItems(18-18)DefaultProviderType(21-21)DefaultFilterEqHasStats(23-23)DefaultFilterEqHasChildren(22-22)DefaultFilterInType(24-24)ProcessStartTimeFilter(99-121)ProcessAttributeKeys(124-152)ExtractResponseError(37-62)ReadResponseBodyRaw(193-215)IsTextContent(154-174)FirstPage(15-15)DefaultPageSize(16-16)DefaultSortingForLogs(20-20)DefaultItemLogLevel(26-26)DefaultSortingForSuites(19-19)DefaultFilterInTypeSuites(25-25)internal/utils/ctx_utils.go (1)
WithQueryParams(18-21)
internal/analytics/analytics_test.go (1)
internal/utils/mcp_utils.go (1)
WithAnalytics(83-97)
internal/utils/mcp_utils.go (1)
internal/utils/utils.go (4)
FirstPage(15-15)DefaultPageSize(16-16)PaginatedRequest(31-35)ReadResponseBodyRaw(193-215)
🔇 Additional comments (3)
internal/http/http_server.go (1)
1-537: LGTM! Clean refactoring with proper analytics integration.The duplicate import issue has been resolved, analytics is properly integrated throughout the HTTP server, and the MaxRequestBodySize constant provides good protection against DoS attacks. The batch request validation logic is comprehensive.
internal/utils/ctx_utils_test.go (1)
1-111: LGTM! Comprehensive test coverage.The tests cover valid UUIDs, edge cases (empty, too short/long, invalid format), and various token validation scenarios. Good use of table-driven tests.
internal/security/http_token_middleware.go (1)
1-78: LGTM! Clean refactoring to use centralized utilities.The migration to
utils.ContextKeyandutils.ValidateRPTokenmaintains consistency with the broader refactoring effort. Token extraction and validation logic is sound.
AleksandrParamonoff
force-pushed
the
refactoring-detaching-app-to-modules
branch
from
c4b1035 to
c987c3a
Compare
There was a problem hiding this comment.
Actionable comments posted: 7
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (11)
internal/analytics/analytics.go (4)
19-32: Make BatchSendInterval overridable for testsUse a var (not const) so tests can shorten it and avoid sleeps.
Apply:
-const ( +const ( // Google Analytics 4 Measurement Protocol endpoint ga4EndpointURL = "https://www.google-analytics.com/mp/collect" ... - // Batch send interval for analytics data - BatchSendInterval = 10 * time.Second ... maxPerRequest = 25 ) + +// Batch send interval for analytics data (overridable in tests) +var BatchSendInterval = 10 * time.Second
68-81: Make Analytics.Stop idempotent to prevent “close of closed channel” panicsStop can be called twice (shutdown paths, deferred calls). Guard with sync.Once.
Apply:
type Analytics struct { config *AnalyticsConfig httpClient *http.Client @@ stopChan chan struct{} wg sync.WaitGroup tickerDone chan struct{} + stopOnce sync.Once } @@ func (a *Analytics) Stop() { - if a == nil || a.stopChan == nil { - return - } - - slog.Debug("Stopping analytics metrics processor") - close(a.stopChan) - - // Wait for ticker goroutine to finish - select { - case <-a.tickerDone: - slog.Debug("Analytics metrics processor stopped gracefully") - case <-time.After(5 * time.Second): - slog.Warn("Analytics metrics processor stop timeout") - } - - a.wg.Wait() + if a == nil || a.stopChan == nil { + return + } + a.stopOnce.Do(func() { + slog.Debug("Stopping analytics metrics processor") + close(a.stopChan) + // Wait for ticker goroutine to finish + select { + case <-a.tickerDone: + slog.Debug("Analytics metrics processor stopped gracefully") + case <-time.After(5 * time.Second): + slog.Warn("Analytics metrics processor stop timeout") + } + a.wg.Wait() + }) }Also applies to: 272-289
61-66: Rename TimestampMS field to match “timestamp_micros” for clarityThe JSON tag is “timestamp_micros” while the field is named TimestampMS. Rename to avoid confusion.
Apply:
type GAPayload struct { ClientID string `json:"client_id"` UserID string `json:"user_id,omitempty"` Events []GAEvent `json:"events"` - TimestampMS int64 `json:"timestamp_micros"` + TimestampMicros int64 `json:"timestamp_micros"` } @@ payload := GAPayload{ ClientID: a.config.UserID, UserID: a.config.UserID, Events: events, - TimestampMS: time.Now().UnixMicro(), + TimestampMicros: time.Now().UnixMicro(), }Also applies to: 150-156
232-246: Document GetAnalyticArg’s purpose and obfuscation
- GetAnalyticArg is consumed in internal/config/cli.go (lines 153, 198) and covered by tests in internal/analytics/analytics_test.go.
- Add GoDoc above the function explaining why the string is built this way, what it represents (e.g., “Analytics API secret”), and any invariants consumers should know.
internal/utils/utils.go (1)
163-171: Bug: IsTextContent returns true only for “application/csv”Empty case blocks don’t fall through in Go. Combine cases so all listed types return true.
Apply:
switch lowerType { - case "application/json": - case "application/xml": - case "application/javascript": - case "application/xhtml+xml": - case "application/yaml": - case "application/csv": - return true + case "application/json", + "application/xml", + "application/javascript", + "application/xhtml+xml", + "application/yaml", + "application/csv": + return true }internal/http/http_server.go (1)
379-386: /info: AnalyticsEnabled field is never setYou populate Analytics.Enabled but leave AnalyticsEnabled stale (always false). Set it or remove the duplicate field.
Apply:
info.ServerRunning = hs.running - info.Analytics.Enabled = hs.analytics != nil + info.Analytics.Enabled = hs.analytics != nil + info.AnalyticsEnabled = hs.analytics != nil info.Timestamp = time.Now().UTC()internal/mcp_handlers/items.go (3)
334-354: Attachment ID: use numeric schema and validate non-negativeDefine parameter as number and drop string parsing. Also reject negative IDs.
- mcp.WithString("attachment-content-id", // Parameter for specifying the test item ID - mcp.Description("Attachment binary content ID"), - ), + mcp.WithNumber("attachment-content-id", + mcp.Description("Attachment binary content ID"), + ), @@ - attachmentIdStr, err := request.RequireString("attachment-content-id") + attachmentId, err := request.RequireInt("attachment-content-id") if err != nil { return mcp.NewToolResultError(err.Error()), nil } - attachmentId, err := strconv.ParseInt(attachmentIdStr, 10, 64) - if err != nil { - return mcp.NewToolResultError( - fmt.Sprintf("invalid attachment ID value: %s", attachmentIdStr), - ), nil - } + if attachmentId < 0 { + return mcp.NewToolResultError( + fmt.Sprintf("invalid attachment ID value: %d", attachmentId), + ), nil + } @@ - response, err := lr.client.FileStorageAPI.GetFile(ctx, attachmentId, project). + response, err := lr.client.FileStorageAPI.GetFile(ctx, int64(attachmentId), project). Execute() ```<!-- review_comment_end --> Also applies to: 355-360, 362-389 --- `371-389`: **Text-vs-binary detection likely wrong (utils.IsTextContent switch)** Current IsTextContent returns true only for the last case due to separate case labels. Fix to group cases; otherwise attachments like JSON/XML will be treated as binary. Based on relevant code in internal/utils/utils.go. ```diff - switch lowerType { - case "application/json": - case "application/xml": - case "application/javascript": - case "application/xhtml+xml": - case "application/yaml": - case "application/csv": - return true - } + switch lowerType { + case "application/json", "application/xml", "application/javascript", + "application/xhtml+xml", "application/yaml", "application/csv": + return true + }Alternatively, consider parsing with mime.ParseMediaType and matching vendor subtypes.
583-590: Critical: wrong request keys (dot vs hyphen) — filters never readTool options use hyphenated names, but handler reads dotted keys, so all filters default to empty values.
- filterName := request.GetString("filter.cnt.name", "") - filterAttributes := request.GetString("filter.has.compositeAttribute", "") - filterAttributeKeys := request.GetString("filter.has.attributeKey", "") - filterDescription := request.GetString("filter.cnt.description", "") - filterStartTimeFrom := request.GetString("filter.btw.startTime.from", "") - filterStartTimeTo := request.GetString("filter.btw.startTime.to", "") + filterName := request.GetString("filter-cnt-name", "") + filterAttributes := request.GetString("filter-has-compositeAttribute", "") + filterAttributeKeys := request.GetString("filter-has-attributeKey", "") + filterDescription := request.GetString("filter-cnt-description", "") + filterStartTimeFrom := request.GetString("filter-btw-startTime-from", "") + filterStartTimeTo := request.GetString("filter-btw-startTime-to", "") ```<!-- review_comment_end --> </blockquote></details> <details> <summary>internal/utils/utils_test.go (1)</summary><blockquote> `3-6`: **Fix perf test key generation; avoid control characters and preallocate** string(rune(i)) yields control characters; use strconv.Itoa and preallocate for performance. ```diff import ( + "strconv" "strings" "testing" ) @@ - var keys []string + keys := make([]string, 0, 1000) for i := 0; i < 1000; i++ { - keys = append(keys, "key"+string(rune(i))+":") + keys = append(keys, "key"+strconv.Itoa(i)+":") }Also applies to: 149-172
internal/utils/ctx_utils.go (1)
37-44: Use uuid.Parse instead of uuid.Validate in internal/utils/ctx_utils.goThe google/uuid v1.6.0 module doesn’t include Validate, so this code won’t compile. Replace with:
- if uuid.Validate(token) == nil { - return true - } + if _, err := uuid.Parse(token); err == nil { + return true + }
♻️ Duplicate comments (14)
internal/mcp_handlers/launches_test.go (1)
45-49: Check url.Parse error (don’t discard)
Ignoring url.Parse errors can hide failures and complicates debugging. Capture and assert it.Apply:
- serverURL, _ := url.Parse(mockServer.URL) + serverURL, urlErr := url.Parse(mockServer.URL) + require.NoError(t, urlErr)internal/utils/utils.go (2)
27-27: Remove redundant maxInt32 wrapper constantUse math.MaxInt32 directly; the wrapper adds no value and was previously flagged.
Apply:
- DefaultItemLogLevel = "TRACE" // Default log level for test item logs - maxInt32 = math.MaxInt32 // Standard library constant for maximum int32 value + DefaultItemLogLevel = "TRACE" // Default log level for test item logs
136-138: Clarify comment for key:value preservationMake the intent explicit.
Apply:
- if colonIndex := strings.Index(key, ":"); colonIndex > 0 && colonIndex < len(key)-1 { - processed = append(processed, key) // Keep full key with colon + if colonIndex := strings.Index(key, ":"); colonIndex > 0 && colonIndex < len(key)-1 { + processed = append(processed, key) // Keep key:value pair unchangedinternal/mcp_handlers/items.go (2)
393-416: Reuse SetPaginationOptions in logs tool to avoid driftInline page/page-size/page-sort duplicates logic. Switch to utils.SetPaginationOptions like other tools.
-func (lr *TestItemResources) toolGetTestItemLogsByFilter() (tool mcp.Tool, handler server.ToolHandlerFunc) { - return mcp.NewTool( - "get_test_item_logs_by_filter", - mcp.WithDescription("Get list of logs for test item with specific item ID with optional filters"), - lr.projectParameter, - mcp.WithString("parent-item-id", - mcp.Description("Items with specific Parent Item ID, this is a required parameter"), - ), - mcp.WithNumber("page", - mcp.DefaultNumber(utils.FirstPage), - mcp.Description("Page number"), - ), - mcp.WithNumber("page-size", - mcp.DefaultNumber(utils.DefaultPageSize), - mcp.Description("Page size"), - ), - mcp.WithString("page-sort", - mcp.DefaultString(utils.DefaultSortingForLogs), - mcp.Description("Sorting fields and direction"), - ), - // ... other options - ), utils.WithAnalytics( ... +func (lr *TestItemResources) toolGetTestItemLogsByFilter() (tool mcp.Tool, handler server.ToolHandlerFunc) { + opts := []mcp.ToolOption{ + mcp.WithDescription("Get list of logs for test item with specific item ID with optional filters"), + lr.projectParameter, + mcp.WithString("parent-item-id", + mcp.Description("Items with specific Parent Item ID, this is a required parameter"), + ), + } + opts = append(opts, utils.SetPaginationOptions(utils.DefaultSortingForLogs)...) + opts = append(opts, + // ... existing optional filters + ) + return mcp.NewTool("get_test_item_logs_by_filter", opts...), + utils.WithAnalytics( ... ```<!-- review_comment_end --> Also applies to: 498-504 --- `40-54`: **Export for API consistency: ToolGetTestItemLogsByFilter** All other tool constructors are exported; this one isn’t. Rename for symmetry. ```diff - s.AddTool(testItems.toolGetTestItemLogsByFilter()) + s.AddTool(testItems.ToolGetTestItemLogsByFilter())Apply corresponding rename on the receiver method and any callsites.
internal/middleware/middleware_test.go (2)
1-29: Add test: Authorization header from context is setCovers middleware behavior when token is placed in context.
import ( + "context" "net/http" "net/url" "testing" "github.com/stretchr/testify/assert" "github.com/reportportal/reportportal-mcp-server/internal/utils" + "github.com/reportportal/reportportal-mcp-server/internal/security" ) @@ func TestQueryParamsMiddleware(t *testing.T) { @@ } + +func TestQueryParamsMiddleware_AddsAuthorizationFromContext(t *testing.T) { + req, err := http.NewRequest("GET", "http://example.com/path", nil) + assert.NoError(t, err) + ctx := context.WithValue(req.Context(), security.RPTokenContextKey, "abc123") + req = req.WithContext(ctx) + + QueryParamsMiddleware(req) + + assert.Equal(t, "Bearer abc123", req.Header.Get("Authorization")) +} ```<!-- review_comment_end --> --- `19-21`: **Assert NewRequest error in tests** Don’t ignore request creation errors. ```diff - req, _ := http.NewRequest("GET", "http://example.com/path?x=9", nil) + req, err := http.NewRequest("GET", "http://example.com/path?x=9", nil) + assert.NoError(t, err) ```<!-- review_comment_end --> </blockquote></details> <details> <summary>internal/analytics/analytics_test.go (1)</summary><blockquote> `179-189`: **Also assert analytics counter incremented** Strengthens the test to verify tracking, not just delegation. ```diff result, err := wrappedHandler(context.Background(), mcp.CallToolRequest{}) // Verify handler was called assert.NoError(t, err) assert.NotNil(t, result) assert.True(t, handlerCalled, "Original handler should be called") + +// Verify metric incremented +analytics.metricsLock.RLock() +if v, ok := analytics.metrics["test_tool"]; ok && v != nil { + assert.Equal(t, int64(1), *v) +} else { + assert.Fail(t, "metric for test_tool not recorded") +} +analytics.metricsLock.RUnlock() ```<!-- review_comment_end --> </blockquote></details> <details> <summary>internal/utils/ctx_utils.go (1)</summary><blockquote> `11-16`: **Avoid string-based context keys; use an unexported struct key with an exported var** String keys can collide across packages. Prefer an unexported, zero-sized struct type and export only the key value. ```diff -// ContextKey is a type for context keys to avoid collisions -type ContextKey string - -var contextKeyQueryParams = ContextKey( - "queryParams", -) // Key for storing query parameters in the context +// QueryParamsKey is the context key for query parameters (exported var; unexported type) +type queryParamsKey struct{} +var QueryParamsKey = queryParamsKey{}And update usages below:
- return context.WithValue(ctx, contextKeyQueryParams, queryParams) + return context.WithValue(ctx, QueryParamsKey, queryParams)- queryParams, ok := ctx.Value(contextKeyQueryParams).(url.Values) + queryParams, ok := ctx.Value(QueryParamsKey).(url.Values)internal/mcp_handlers/server.go (1)
41-50: Make analytics initialization non-fatal and gate it (align with HTTP server)Mirror HTTP server behavior: only attempt when enabled and inputs are valid; warn on failure and continue.
- // Initialize analytics (disabled if analyticsOff is true) - var analyticsClient *analytics.Analytics - if analyticsOn { - var err error - // Pass RP API token for secure hashing as user identifier - analyticsClient, err = analytics.NewAnalytics(userID, analyticsAPISecret, token) - if err != nil { - return nil, nil, fmt.Errorf("failed to initialize analytics: %w", err) - } - } + // Initialize analytics (non-fatal on failure; gated like HTTP server) + var analyticsClient *analytics.Analytics + if analyticsOn && utils.ValidateRPToken(token) && analyticsAPISecret != "" { + if ac, err := analytics.NewAnalytics(userID, analyticsAPISecret, token); err != nil { + slog.Warn("analytics initialization skipped", "error", err) + } else { + analyticsClient = ac + } + }Add imports:
import ( "embed" "fmt" "io/fs" "net/url" "path/filepath" + "log/slog" @@ - "github.com/reportportal/reportportal-mcp-server/internal/middleware" + "github.com/reportportal/reportportal-mcp-server/internal/middleware" + "github.com/reportportal/reportportal-mcp-server/internal/utils"internal/http/http_server_batch_test.go (2)
266-271: Don’t ignore http.NewRequest error and avoid NopCloserMake failures explicit and simplify the body.
- req, _ := http.NewRequest( + req, err := http.NewRequest( tt.method, "/test", - io.NopCloser(bytes.NewBufferString(tt.payload)), + bytes.NewBufferString(tt.payload), ) + if err != nil { + t.Fatalf("failed to create request: %v", err) + }
241-262: Add charset-parameterized JSON caseEnsure requests with “application/json; charset=utf-8” are accepted.
{ name: "POST with XML content", method: "POST", contentType: "application/xml", payload: `<xml></xml>`, expected: false, }, + { + name: "POST with JSON content and charset", + method: "POST", + contentType: "application/json; charset=utf-8", + payload: `{"jsonrpc":"2.0","method":"test","id":1}`, + expected: true, + },internal/config/cli.go (2)
112-114: Remove redundant ToLower call.The code applies
strings.ToLowertwice: once when reading the environment variable and again on the already-lowercased value.Apply this diff:
- rawMcpMode := strings.ToLower(os.Getenv("MCP_MODE")) + rawMcpMode := os.Getenv("MCP_MODE") + mcpMode := strings.ToLower(rawMcpMode) slog.Debug("MCP_MODE env variable is set to: " + rawMcpMode) - mcpMode := strings.ToLower(rawMcpMode)
293-295: Use the Handler returned by CreateHTTPServerWithMiddleware.The server is tightly coupled to
httpServer.MCP.Router. Use theHandlerfield from theHTTPServerWithMiddlewarestruct instead to avoid reaching into internal structures.Apply this diff:
server := &http.Server{ Addr: addr, - Handler: httpServer.MCP.Router, // Use Chi router directly with throttling/health/info/metrics + Handler: httpServer.Handler, // Use pre‑wired Chi router with middleware ReadHeaderTimeout: 10 * time.Second, // Prevent Slowloris attacks
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (18)
cmd/reportportal-mcp-server/main.go(2 hunks)internal/analytics/analytics.go(3 hunks)internal/analytics/analytics_test.go(5 hunks)internal/config/cli.go(1 hunks)internal/http/http_server.go(12 hunks)internal/http/http_server_batch_test.go(1 hunks)internal/mcp_handlers/items.go(23 hunks)internal/mcp_handlers/launches.go(12 hunks)internal/mcp_handlers/launches_test.go(2 hunks)internal/mcp_handlers/server.go(4 hunks)internal/middleware/middleware.go(1 hunks)internal/middleware/middleware_test.go(2 hunks)internal/security/http_token_middleware.go(2 hunks)internal/utils/ctx_utils.go(2 hunks)internal/utils/ctx_utils_test.go(1 hunks)internal/utils/mcp_utils.go(1 hunks)internal/utils/utils.go(8 hunks)internal/utils/utils_test.go(7 hunks)
🧰 Additional context used
📓 Path-based instructions (4)
**/*.go
📄 CodeRabbit inference engine (README.md)
**/*.go: Run formatter (task fmt / go fmt) to keep all Go source files consistently formatted
Run linter (task lint) to maintain code quality across Go files
Files:
internal/http/http_server.gointernal/security/http_token_middleware.gointernal/mcp_handlers/items.gointernal/mcp_handlers/launches.gocmd/reportportal-mcp-server/main.gointernal/utils/ctx_utils_test.gointernal/utils/utils_test.gointernal/middleware/middleware.gointernal/mcp_handlers/launches_test.gointernal/utils/utils.gointernal/middleware/middleware_test.gointernal/utils/mcp_utils.gointernal/utils/ctx_utils.gointernal/analytics/analytics.gointernal/http/http_server_batch_test.gointernal/config/cli.gointernal/analytics/analytics_test.gointernal/mcp_handlers/server.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/http/http_server.gointernal/security/http_token_middleware.gointernal/mcp_handlers/items.gointernal/mcp_handlers/launches.gocmd/reportportal-mcp-server/main.gointernal/utils/ctx_utils_test.gointernal/utils/utils_test.gointernal/middleware/middleware.gointernal/mcp_handlers/launches_test.gointernal/utils/utils.gointernal/middleware/middleware_test.gointernal/utils/mcp_utils.gointernal/utils/ctx_utils.gointernal/analytics/analytics.gointernal/http/http_server_batch_test.gointernal/config/cli.gointernal/analytics/analytics_test.gointernal/mcp_handlers/server.go
cmd/reportportal-mcp-server/**/*.go
📄 CodeRabbit inference engine (README.md)
Keep the main server entrypoint under cmd/reportportal-mcp-server and build from that path
Files:
cmd/reportportal-mcp-server/main.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
**/*_test.go
⚙️ CodeRabbit configuration file
Ensure comprehensive test coverage, proper test isolation, meaningful assertions, and good test naming conventions.
Files:
internal/utils/ctx_utils_test.gointernal/utils/utils_test.gointernal/mcp_handlers/launches_test.gointernal/middleware/middleware_test.gointernal/http/http_server_batch_test.gointernal/analytics/analytics_test.go
🧠 Learnings (1)
📚 Learning: 2025-09-17T11:00:40.261Z
Learnt from: CR
PR: reportportal/reportportal-mcp-server#0
File: README.md:0-0
Timestamp: 2025-09-17T11:00:40.261Z
Learning: Applies to cmd/reportportal-mcp-server/**/*.go : Keep the main server entrypoint under cmd/reportportal-mcp-server and build from that path
Applied to files:
cmd/reportportal-mcp-server/main.gointernal/mcp_handlers/server.go
🧬 Code graph analysis (13)
internal/http/http_server.go (7)
internal/analytics/analytics.go (3)
Analytics(69-81)NewAnalytics(90-131)BatchSendInterval(29-29)internal/utils/ctx_utils.go (1)
ValidateRPToken(31-44)internal/middleware/middleware.go (1)
QueryParamsMiddleware(11-32)internal/mcp_handlers/launches.go (1)
RegisterLaunchTools(41-56)internal/mcp_handlers/items.go (1)
RegisterTestItemTools(41-54)internal/mcp_handlers/server.go (2)
ReadPrompts(71-93)PromptFiles(19-19)internal/security/http_token_middleware.go (1)
HTTPTokenMiddleware(19-42)
internal/security/http_token_middleware.go (1)
internal/utils/ctx_utils.go (2)
ContextKey(12-12)ValidateRPToken(31-44)
internal/mcp_handlers/items.go (4)
internal/analytics/analytics.go (1)
Analytics(69-81)internal/utils/mcp_utils.go (6)
NewProjectParameter(65-71)SetPaginationOptions(14-29)WithAnalytics(83-97)ExtractProject(73-75)ApplyPaginationOptions(32-63)ReadResponseBody(102-111)internal/utils/utils.go (16)
DefaultSortingForItems(18-18)DefaultProviderType(21-21)DefaultFilterEqHasStats(23-23)DefaultFilterEqHasChildren(22-22)DefaultFilterInType(24-24)ProcessStartTimeFilter(99-121)ProcessAttributeKeys(124-152)ExtractResponseError(37-62)ReadResponseBodyRaw(193-215)IsTextContent(154-174)FirstPage(15-15)DefaultPageSize(16-16)DefaultSortingForLogs(20-20)DefaultItemLogLevel(26-26)DefaultSortingForSuites(19-19)DefaultFilterInTypeSuites(25-25)internal/utils/ctx_utils.go (1)
WithQueryParams(18-21)
internal/mcp_handlers/launches.go (4)
internal/analytics/analytics.go (1)
Analytics(69-81)internal/utils/mcp_utils.go (6)
NewProjectParameter(65-71)SetPaginationOptions(14-29)WithAnalytics(83-97)ExtractProject(73-75)ApplyPaginationOptions(32-63)ReadResponseBody(102-111)internal/utils/utils.go (4)
DefaultSortingForLaunches(17-17)ProcessStartTimeFilter(99-121)ProcessAttributeKeys(124-152)ExtractResponseError(37-62)internal/utils/ctx_utils.go (1)
WithQueryParams(18-21)
cmd/reportportal-mcp-server/main.go (1)
internal/config/cli.go (2)
AppVersion(25-29)RunApp(336-389)
internal/utils/utils_test.go (1)
internal/utils/utils.go (1)
ProcessAttributeKeys(124-152)
internal/middleware/middleware.go (2)
internal/security/http_token_middleware.go (1)
GetTokenFromContext(75-78)internal/utils/ctx_utils.go (1)
QueryParamsFromContext(23-27)
internal/middleware/middleware_test.go (1)
internal/utils/ctx_utils.go (1)
WithQueryParams(18-21)
internal/utils/mcp_utils.go (1)
internal/utils/utils.go (4)
FirstPage(15-15)DefaultPageSize(16-16)PaginatedRequest(31-35)ReadResponseBodyRaw(193-215)
internal/http/http_server_batch_test.go (1)
internal/http/http_server.go (3)
HTTPServer(66-77)HTTPServerConfig(50-63)NewHTTPServer(87-144)
internal/config/cli.go (3)
internal/http/http_server.go (2)
HTTPServerConfig(50-63)CreateHTTPServerWithMiddleware(224-250)internal/analytics/analytics.go (3)
GetAnalyticArg(232-246)Analytics(69-81)StopAnalytics(403-412)internal/mcp_handlers/server.go (1)
NewServer(21-68)
internal/analytics/analytics_test.go (1)
internal/utils/mcp_utils.go (1)
WithAnalytics(83-97)
internal/mcp_handlers/server.go (5)
internal/analytics/analytics.go (2)
Analytics(69-81)NewAnalytics(90-131)internal/middleware/middleware.go (1)
QueryParamsMiddleware(11-32)internal/mcp_handlers/launches.go (1)
RegisterLaunchTools(41-56)internal/mcp_handlers/items.go (1)
RegisterTestItemTools(41-54)internal/promptreader/promptreader.go (1)
PromptHandlerPair(16-19)
🔇 Additional comments (9)
internal/utils/mcp_utils.go (2)
37-44: Pagination clamping is safe and avoids int32 overflowMin/max guards on page and page-size with int32 casts look correct and prevent overflow. Good use of constants.
Also applies to: 46-53, 58-63
103-111: Ignore ReadResponseBody “error-swallowing” warning
ReadResponseBody intentionally returns business errors as a CallToolResultError with a nil Go error, and all callers use httpLambdaWrapper which handles nil errors as expected; no change required.Likely an incorrect or invalid review comment.
internal/http/http_server.go (1)
431-433: Good hardening: cap request body sizeUsing MaxBytesReader prevents memory DoS on MCP endpoints. Nice.
internal/mcp_handlers/items.go (1)
69-71: Good adoption of shared utils and analyticsUsing utils.SetPaginationOptions, utils.ExtractProject, and utils.WithAnalytics reduces duplication and centralizes behavior. LGTM.
Also applies to: 137-145
internal/mcp_handlers/launches.go (1)
66-68: Shared pagination and analytics wiring looks goodConsistent use of utils.SetPaginationOptions, utils.WithAnalytics, and utils.ExtractProject. Solid refactor.
Also applies to: 112-118, 156-166
internal/security/http_token_middleware.go (1)
15-16: LGTM: centralized context key and validationGood move to reuse utils.ContextKey and utils.ValidateRPToken; header parsing and logging are safe and minimal.
Also applies to: 47-67, 69-78
internal/middleware/middleware.go (1)
13-17: LGTM: header sanitization and query param merge are soundToken is trimmed and CR/LF-guarded before setting Authorization. Query params are safely encoded into RawQuery.
Also applies to: 21-31
internal/config/cli.go (2)
318-327: Shutdown sequence is correct.The graceful shutdown properly uses a timeout context and calls both
server.ShutdownandhttpServer.MCP.Stopwithout explicitly stopping analytics (which would cause double-stop). The past issue has been addressed.
335-389: Well-structured application entry point.The
RunAppfunction properly orchestrates the CLI application by:
- Reading the MCP mode from environment before flag parsing
- Conditionally including HTTP-specific flags based on mode
- Delegating to mode-specific runner functions
- Returning errors to the caller for proper handling
The mode-based flag selection and error propagation follow Go best practices.
AleksandrParamonoff
force-pushed
the
refactoring-detaching-app-to-modules
branch
from
c987c3a to
182a251
Compare
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
internal/http/http_server.go (1)
463-472: Close the original request body before reusing it.After wrapping the body with
http.MaxBytesReader,io.ReadAlldrains but does not close the underlying reader. Reassigningr.Bodyto a new buffer without closing the original reader leaks the connection and defeats pooling. Store the old body (limitedBody := r.Body), read from it, close it (e.g.,defer limitedBody.Close()), and only then wrap the bytes withio.NopCloser.internal/utils/utils.go (1)
162-170: Returntruefor every listed media type.With empty case bodies, only
"application/csv"hits thereturn true;"application/json","application/xml", etc. fall through and incorrectly reportfalse. Collapse the cases into a single clause (e.g.,case "application/json", "application/xml", ...: return true) so all intended text types are recognised.
♻️ Duplicate comments (5)
internal/utils/ctx_utils.go (1)
11-12: Exported string context key risks collisions; prefer unexported struct type.Past review correctly identified this pattern. Exported string types for context keys can collide across packages. Best practice is an unexported zero-sized struct type with exported key variable.
internal/mcp_handlers/launches.go (1)
279-281: Critical: Schema/runtime type mismatch for launch_id parameter.The parameter is defined as string via
mcp.WithString("launch_id", ...)but line 288 callsrequest.RequireInt("launch_id"), causing runtime errors when the tool is invoked.Change to numeric schema:
- mcp.WithString("launch_id", + mcp.WithNumber("launch_id", mcp.Description("Launch ID"), ),Apply the same fix to:
toolRunAutoAnalysis(lines 310-313)toolUniqueErrorAnalysis(lines 387-389)toolForceFinishLaunch(lines 431-433)internal/analytics/analytics_test.go (1)
180-183: Optional: Assert thatWithAnalyticsincrements the metric counter.The test wraps the handler but doesn't verify that the analytics metric was actually incremented. Consider checking the metrics map after calling the wrapped handler to ensure event tracking occurred.
result, err := wrappedHandler(context.Background(), mcp.CallToolRequest{}) // Verify handler was called assert.NoError(t, err) assert.NotNil(t, result) assert.True(t, handlerCalled, "Original handler should be called") + + // Verify metric was incremented + analytics.metricsLock.RLock() + if counter, ok := analytics.metrics["test_tool"]; ok && counter != nil { + assert.Equal(t, int64(1), *counter) + } else { + assert.Fail(t, "metric for test_tool not recorded") + } + analytics.metricsLock.RUnlock()As per coding guidelines.
internal/middleware/middleware_test.go (1)
19-19: Optional: Checkhttp.NewRequesterror for test robustness.Capturing and asserting the error improves test clarity if the URL or request setup ever fails.
- req, _ := http.NewRequest("GET", "http://example.com/path?x=9", nil) + req, err := http.NewRequest("GET", "http://example.com/path?x=9", nil) + assert.NoError(t, err)As per coding guidelines.
internal/mcp_handlers/server.go (1)
42-50: Analytics init failure should be non-fatal for startup robustness.Currently, missing/invalid analytics secrets cause
NewServerto fail. The past review noted this diverges from HTTP server behavior, which logs and continues. Consider logging a warning and proceeding withanalyticsClient = nil.+ "log/slog"// Initialize analytics (disabled if analyticsOff is true) var analyticsClient *analytics.Analytics if analyticsOn { - var err error - // Pass RP API token for secure hashing as user identifier - analyticsClient, err = analytics.NewAnalytics(userID, analyticsAPISecret, token) - if err != nil { - return nil, nil, fmt.Errorf("failed to initialize analytics: %w", err) - } + ac, err := analytics.NewAnalytics(userID, analyticsAPISecret, token) + if err != nil { + slog.Warn("analytics initialization skipped", "error", err) + } else { + analyticsClient = ac + } }Based on learnings.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (18)
cmd/reportportal-mcp-server/main.go(2 hunks)internal/analytics/analytics.go(3 hunks)internal/analytics/analytics_test.go(5 hunks)internal/config/cli.go(1 hunks)internal/http/http_server.go(12 hunks)internal/http/http_server_batch_test.go(1 hunks)internal/mcp_handlers/items.go(23 hunks)internal/mcp_handlers/launches.go(12 hunks)internal/mcp_handlers/launches_test.go(2 hunks)internal/mcp_handlers/server.go(4 hunks)internal/middleware/middleware.go(1 hunks)internal/middleware/middleware_test.go(2 hunks)internal/security/http_token_middleware.go(2 hunks)internal/utils/ctx_utils.go(2 hunks)internal/utils/ctx_utils_test.go(1 hunks)internal/utils/mcp_utils.go(1 hunks)internal/utils/utils.go(8 hunks)internal/utils/utils_test.go(7 hunks)
🧰 Additional context used
📓 Path-based instructions (4)
**/*.go
📄 CodeRabbit inference engine (README.md)
**/*.go: Run formatter (task fmt / go fmt) to keep all Go source files consistently formatted
Run linter (task lint) to maintain code quality across Go files
Files:
internal/mcp_handlers/launches.gointernal/mcp_handlers/server.gointernal/mcp_handlers/launches_test.gointernal/utils/utils_test.gointernal/middleware/middleware.gocmd/reportportal-mcp-server/main.gointernal/utils/ctx_utils.gointernal/http/http_server_batch_test.gointernal/mcp_handlers/items.gointernal/analytics/analytics_test.gointernal/middleware/middleware_test.gointernal/utils/ctx_utils_test.gointernal/http/http_server.gointernal/security/http_token_middleware.gointernal/utils/utils.gointernal/utils/mcp_utils.gointernal/analytics/analytics.gointernal/config/cli.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/mcp_handlers/launches.gointernal/mcp_handlers/server.gointernal/mcp_handlers/launches_test.gointernal/utils/utils_test.gointernal/middleware/middleware.gocmd/reportportal-mcp-server/main.gointernal/utils/ctx_utils.gointernal/http/http_server_batch_test.gointernal/mcp_handlers/items.gointernal/analytics/analytics_test.gointernal/middleware/middleware_test.gointernal/utils/ctx_utils_test.gointernal/http/http_server.gointernal/security/http_token_middleware.gointernal/utils/utils.gointernal/utils/mcp_utils.gointernal/analytics/analytics.gointernal/config/cli.go
**/*_test.go
⚙️ CodeRabbit configuration file
Ensure comprehensive test coverage, proper test isolation, meaningful assertions, and good test naming conventions.
Files:
internal/mcp_handlers/launches_test.gointernal/utils/utils_test.gointernal/http/http_server_batch_test.gointernal/analytics/analytics_test.gointernal/middleware/middleware_test.gointernal/utils/ctx_utils_test.go
cmd/reportportal-mcp-server/**/*.go
📄 CodeRabbit inference engine (README.md)
Keep the main server entrypoint under cmd/reportportal-mcp-server and build from that path
Files:
cmd/reportportal-mcp-server/main.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
🧠 Learnings (1)
📚 Learning: 2025-09-17T11:00:40.261Z
Learnt from: CR
PR: reportportal/reportportal-mcp-server#0
File: README.md:0-0
Timestamp: 2025-09-17T11:00:40.261Z
Learning: Applies to cmd/reportportal-mcp-server/**/*.go : Keep the main server entrypoint under cmd/reportportal-mcp-server and build from that path
Applied to files:
cmd/reportportal-mcp-server/main.go
🧬 Code graph analysis (13)
internal/mcp_handlers/launches.go (3)
internal/utils/mcp_utils.go (6)
NewProjectParameter(65-71)SetPaginationOptions(14-29)WithAnalytics(83-97)ExtractProject(73-75)ApplyPaginationOptions(32-63)ReadResponseBody(102-111)internal/utils/utils.go (4)
DefaultSortingForLaunches(17-17)ProcessStartTimeFilter(99-121)ProcessAttributeKeys(124-152)ExtractResponseError(37-62)internal/utils/ctx_utils.go (1)
WithQueryParams(18-21)
internal/mcp_handlers/server.go (5)
internal/analytics/analytics.go (2)
Analytics(69-81)NewAnalytics(90-131)internal/middleware/middleware.go (1)
QueryParamsMiddleware(11-32)internal/mcp_handlers/launches.go (1)
RegisterLaunchTools(41-56)internal/mcp_handlers/items.go (1)
RegisterTestItemTools(41-54)internal/promptreader/promptreader.go (1)
PromptHandlerPair(16-19)
internal/utils/utils_test.go (1)
internal/utils/utils.go (1)
ProcessAttributeKeys(124-152)
internal/middleware/middleware.go (2)
internal/security/http_token_middleware.go (1)
GetTokenFromContext(75-78)internal/utils/ctx_utils.go (1)
QueryParamsFromContext(23-27)
cmd/reportportal-mcp-server/main.go (1)
internal/config/cli.go (2)
AppVersion(25-29)RunApp(336-389)
internal/http/http_server_batch_test.go (1)
internal/http/http_server.go (3)
HTTPServer(66-77)HTTPServerConfig(50-63)NewHTTPServer(87-144)
internal/mcp_handlers/items.go (4)
internal/analytics/analytics.go (1)
Analytics(69-81)internal/utils/mcp_utils.go (6)
NewProjectParameter(65-71)SetPaginationOptions(14-29)WithAnalytics(83-97)ExtractProject(73-75)ApplyPaginationOptions(32-63)ReadResponseBody(102-111)internal/utils/utils.go (16)
DefaultSortingForItems(18-18)DefaultProviderType(21-21)DefaultFilterEqHasStats(23-23)DefaultFilterEqHasChildren(22-22)DefaultFilterInType(24-24)ProcessStartTimeFilter(99-121)ProcessAttributeKeys(124-152)ExtractResponseError(37-62)ReadResponseBodyRaw(193-215)IsTextContent(154-174)FirstPage(15-15)DefaultPageSize(16-16)DefaultSortingForLogs(20-20)DefaultItemLogLevel(26-26)DefaultSortingForSuites(19-19)DefaultFilterInTypeSuites(25-25)internal/utils/ctx_utils.go (1)
WithQueryParams(18-21)
internal/analytics/analytics_test.go (1)
internal/utils/mcp_utils.go (1)
WithAnalytics(83-97)
internal/middleware/middleware_test.go (1)
internal/utils/ctx_utils.go (1)
WithQueryParams(18-21)
internal/http/http_server.go (7)
internal/analytics/analytics.go (3)
Analytics(69-81)NewAnalytics(90-131)BatchSendInterval(29-29)internal/utils/ctx_utils.go (1)
ValidateRPToken(31-44)internal/middleware/middleware.go (1)
QueryParamsMiddleware(11-32)internal/mcp_handlers/launches.go (1)
RegisterLaunchTools(41-56)internal/mcp_handlers/items.go (1)
RegisterTestItemTools(41-54)internal/mcp_handlers/server.go (2)
ReadPrompts(71-93)PromptFiles(19-19)internal/security/http_token_middleware.go (1)
HTTPTokenMiddleware(19-42)
internal/security/http_token_middleware.go (1)
internal/utils/ctx_utils.go (2)
ContextKey(12-12)ValidateRPToken(31-44)
internal/utils/mcp_utils.go (1)
internal/utils/utils.go (4)
FirstPage(15-15)DefaultPageSize(16-16)PaginatedRequest(31-35)ReadResponseBodyRaw(193-215)
internal/config/cli.go (3)
internal/http/http_server.go (2)
HTTPServerConfig(50-63)CreateHTTPServerWithMiddleware(224-250)internal/analytics/analytics.go (3)
GetAnalyticArg(232-246)Analytics(69-81)StopAnalytics(403-412)internal/mcp_handlers/server.go (1)
NewServer(21-68)
AleksandrParamonoff
force-pushed
the
refactoring-detaching-app-to-modules
branch
from
182a251 to
4b82b68
Compare
There was a problem hiding this comment.
Pull Request Overview
Copilot reviewed 18 out of 19 changed files in this pull request and generated 7 comments.
Comments suppressed due to low confidence (1)
internal/http/http_server.go:1
- [nitpick] This line appears to be inconsistent with the variable name context. The variable is being assigned but the surrounding code suggests this might be in the wrong location or the assignment might be incorrect.
package http
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
| return mcp.NewTool("get_last_launch_by_name", | ||
| // Tool metadata | ||
| mcp.WithDescription("Get list of last ReportPortal launches"), | ||
| mcp.WithDescription("Get last launch by name from ReportPortal"), |
There was a problem hiding this comment.
[nitpick] The description should clarify what 'last' means in this context. Consider changing to 'Get most recent launch by name from ReportPortal' for better clarity.
| mcp.WithDescription("Get last launch by name from ReportPortal"), | |
| mcp.WithDescription("Get most recent launch by name from ReportPortal"), |
| mcp.WithDescription("Delete ReportPortal launch"), | ||
| lr.projectParameter, | ||
| mcp.WithString("launch_id", // Parameter for specifying the launch name | ||
| mcp.WithNumber("launch_id", // Parameter for specifying the launch name |
There was a problem hiding this comment.
The comment says 'launch name' but the parameter is 'launch_id' which expects a number. The comment should say 'launch ID' to match the parameter type and name.
| mcp.WithNumber("launch_id", // Parameter for specifying the launch name | |
| mcp.WithNumber("launch_id", // Parameter for specifying the launch ID |
| mcp.WithDescription("Run auto analysis on ReportPortal launch"), | ||
| lr.projectParameter, | ||
| mcp.WithString("launch_id", // Parameter for specifying the launch name | ||
| mcp.WithNumber("launch_id", // Parameter for specifying the launch name |
There was a problem hiding this comment.
The comment says 'launch name' but the parameter is 'launch_id' which expects a number. The comment should say 'launch ID' to match the parameter type and name.
| mcp.WithNumber("launch_id", // Parameter for specifying the launch name | |
| mcp.WithNumber("launch_id", // Parameter for specifying the launch ID |
| mcp.WithDescription("Run unique error analysis on ReportPortal launch"), | ||
| lr.projectParameter, | ||
| mcp.WithString("launch_id", // Parameter for specifying the launch name | ||
| mcp.WithNumber("launch_id", // Parameter for specifying the launch name |
There was a problem hiding this comment.
The comment says 'launch name' but the parameter is 'launch_id' which expects a number. The comment should say 'launch ID' to match the parameter type and name.
| mcp.WithNumber("launch_id", // Parameter for specifying the launch name | |
| mcp.WithNumber("launch_id", // Parameter for specifying the launch ID |
| mcp.WithDescription("Force-finish ReportPortal launch"), | ||
| lr.projectParameter, | ||
| mcp.WithString("launch_id", // Parameter for specifying the launch name | ||
| mcp.WithNumber("launch_id", // Parameter for specifying the launch name |
There was a problem hiding this comment.
The comment says 'launch name' but the parameter is 'launch_id' which expects a number. The comment should say 'launch ID' to match the parameter type and name.
| mcp.WithNumber("launch_id", // Parameter for specifying the launch name | |
| mcp.WithNumber("launch_id", // Parameter for specifying the launch ID |
| } | ||
|
|
||
| // toolGetTestItemLogsByFilter creates a tool to get test items logs for a specific launch. | ||
| // ToolGetTestItemLogsByFilter creates a tool to get test items logs for a specific launch. |
There was a problem hiding this comment.
The function name in the comment should match the actual function name. Change 'ToolGetTestItemLogsByFilter' to 'toolGetTestItemLogsByFilter' to match the actual function name.
| // ToolGetTestItemLogsByFilter creates a tool to get test items logs for a specific launch. | |
| // toolGetTestItemLogsByFilter creates a tool to get test items logs for a specific launch. |
1 participant
No description provided.