EPMRPP-108223 || Refactoring. Divide application into separate parts #63
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 CodeRabbitRelease Notes
WalkthroughConsolidates entrypoint to Changes
Sequence Diagram(s)sequenceDiagram
participant OS as Process (cmd/main.go)
participant RunApp as config.RunApp
participant CLI as CLI/Mode selector
participant Runner as Mode runner (Stdio/HTTP)
participant NewServer as NewMCPServer / NewServer
participant Analytics as analytics.NewAnalytics
participant MCP as MCP Server
OS->>OS: create signal-aware context
OS->>RunApp: RunApp(ctx, AppVersion)
RunApp->>CLI: determine MCP mode
CLI-->>Runner: call RunStdioServer or RunStreamingServer
Runner->>NewServer: NewMCPServer(cmd, appVersion)
NewServer->>Analytics: analytics.NewAnalytics(...) (conditional)
Analytics-->>NewServer: analytics client or log failure
NewServer->>MCP: register tools via RegisterLaunchTools / RegisterTestItemTools
Runner->>MCP: start server (stdio stream or HTTP listener)
Note right of MCP: utils and middleware used for context, token, pagination, and response helpers
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Areas to focus during review:
Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
Pull Request Overview
This PR refactors the application into separate packages to improve modularity and maintainability. The main changes involve moving code from a monolithic mcpreportportal package into dedicated packages: utils, middleware, mcp_handlers, analytics, http, and config.
Key changes:
- Package name changes from
mcpreportportalto specialized packages (utils,middleware,mcp_handlers,analytics,http,config) - Exported previously private functions/constants by capitalizing their names to enable cross-package access
- Introduced registration functions (
RegisterLaunchTools,RegisterTestItemTools) to encapsulate tool setup logic - Moved main application logic from
internal/config/cli.goto a newcmd/main.goentry point
Reviewed Changes
Copilot reviewed 24 out of 26 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| internal/utils/*.go | Changed package from mcpreportportal to utils and exported internal functions/constants |
| internal/middleware/*.go | Changed package to middleware and moved context utilities from http_token_middleware.go to ctx_utils.go |
| internal/mcp_handlers/*.go | Changed package to mcp_handlers, added registration functions, and updated imports |
| internal/http/*.go | Changed package to http, renamed middleware imports, and updated to use new package structure |
| internal/analytics/*.go | Changed package to analytics and updated imports to use middleware package |
| internal/config/cli.go | Refactored from main package to config package, extracted functions for reusability |
| cmd/main.go | New entry point that uses config package functions |
| Taskfile.yaml | Updated inspector tasks for HTTP mode to use Docker networking |
| .gitignore | Added *.exe pattern |
Comments suppressed due to low confidence (1)
internal/http/http_server_test.go:264
- Inconsistent indentation: this line and several others in this file use tabs followed by spaces. Should use consistent indentation (either tabs or spaces, not both).
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 9
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (10)
internal/middleware/middleware.go (1)
3-27: Middleware correctly delegates query param extraction to utilsMoving query param lookup to
utils.QueryParamsFromContextkeeps the context key logic centralized and avoids duplication; the merge logic intorq.URL.Query()remains unchanged and safe. You can optionally simplify the auth branch to rely solely onGetTokenFromContext’s boolean (it already filters out empty tokens), but that’s a readability nit rather than a functional issue.internal/analytics/analytics.go (2)
3-22: Fix gci import-formatting to satisfy the build checkThe build is currently failing with a
gciformatting error on this file. Please run your standard formatter (e.g.gci+goimports) overinternal/analytics/analytics.goso the import groups/order match the project’s configured rules. This should clear the CI failure without changing behavior.
100-115: Make Analytics.Stop idempotent to avoid panics on repeated callsRight now
(*Analytics).Stopwill panic if it’s ever called more than once becauseclose(a.stopChan)is not guarded against multiple invocations. That’s brittle in a long-lived process where shutdown may be triggered from more than one place.You can make
Stopsafely idempotent by adding a smallstoppedflag to the struct and guarding the shutdown with an atomic CAS:type Analytics struct { config *AnalyticsConfig httpClient *http.Client @@ - // Metrics system with atomic counters - // Map structure: userID -> toolName -> counter - metrics map[string]map[string]*int64 // userID -> (tool name -> counter) - metricsLock sync.RWMutex // protects metrics map - - // Background processing - stopChan chan struct{} - wg sync.WaitGroup - tickerDone chan struct{} + // Metrics system with atomic counters + // Map structure: userID -> toolName -> counter + metrics map[string]map[string]*int64 // userID -> (tool name -> counter) + metricsLock sync.RWMutex // protects metrics map + + // Background processing + stopChan chan struct{} + tickerDone chan struct{} + wg sync.WaitGroup + + // Ensures Stop is idempotent (safe to call multiple times) + stopped uint32 } @@ func (a *Analytics) Stop() { - if a == nil || a.stopChan == nil { + if a == nil { + return + } + + // Make Stop idempotent: only the first caller proceeds + if !atomic.CompareAndSwapUint32(&a.stopped, 0, 1) { + return + } + + if a.stopChan == nil { return }This keeps the public API the same but removes a potential panic path if shutdown is invoked more than once.
Also applies to: 367-385
internal/mcp_handlers/integration_project_test.go (1)
95-100: Rename variable to avoid shadowing the imported package.The variable
middlewareshadows the importedinternal/middlewarepackage, which reduces code clarity.Apply this diff:
// Apply middleware to get context with project var ctx context.Context - middleware := middleware.HTTPTokenMiddleware( + handler := middleware.HTTPTokenMiddleware( http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { ctx = r.Context() w.WriteHeader(http.StatusOK) }), ) rr := httptest.NewRecorder() - middleware.ServeHTTP(rr, req) + handler.ServeHTTP(rr, req)internal/mcp_handlers/items.go (1)
29-42: Consider documenting analytics nil handling.The
NewTestItemResourcesconstructor accepts ananalyticsClient *analytics.Analyticsthat could be nil (when analytics is disabled). While the code likely handles this through theWithAnalyticswrapper method, consider adding a comment documenting this behavior or adding a nil check if analytics is required for proper operation.Based on learnings from the HTTP server code, analytics can be nil when disabled, and this should be properly handled.
internal/config/cli.go (3)
118-148: Harden logger initialization against badlog-levelvaluesRight now, an invalid
--log-level(orLOG_LEVEL) causesInitLoggerto return an error and abort the CLI. Consider defaulting toINFOand logging a warning instead of failing the whole command; this is more user-friendly while still surfacing the misconfiguration.For example:
func InitLogger() func(ctx context.Context, command *cli.Command) (context.Context, error) { return func(ctx context.Context, command *cli.Command) (context.Context, error) { // Set up default logging configuration var logLevel slog.Level - if err := logLevel.UnmarshalText([]byte(command.String("log-level"))); err != nil { - return nil, err - } + if err := logLevel.UnmarshalText([]byte(command.String("log-level"))); err != nil { + slog.Warn("invalid log level, falling back to INFO", "value", command.String("log-level"), "error", err) + logLevel = slog.LevelInfo + }
150-226: Validaterp-hostmore strictly and consider consistent version formattingTwo small robustness improvements here:
rp-hostvalidation:
url.Parse(host)happily accepts values likereportportal.example.comwithout a scheme, which will produce a URL with emptyScheme/Hostand a non-emptyPath. That will likely break downstream consumers (e.g. HTTP clients expecting a proper base URL) in surprising ways.- Consider enforcing a scheme and host, and returning a clearer error when they’re missing:
hostUrl, err := url.Parse(host) if err != nil { return httpserver.HTTPServerConfig{}, fmt.Errorf("invalid host URL: %w", err) } +if hostUrl.Scheme == "" || hostUrl.Host == "" { + return httpserver.HTTPServerConfig{}, fmt.Errorf("invalid host URL %q: expected full URL like https://reportportal.example.com", host) +}
- Version string consistency:
BuildHTTPServerConfiguses the full formatted version ("%s (%s) %s"), whileNewMCPServerpasses onlyappVersion.Versionintomcp_handlers.NewServer.- If the commit and date are useful in stdio mode too, consider reusing the same formatted version string there for consistency across modes.
293-350: Usenet.JoinHostPortto support IPv6/host formatting in HTTP mode
addr := fmt.Sprintf("%s:%d", host, port)will fail for IPv6 literals like::(explicitly suggested in thehostflag usage) and can mis-handle hosts containing colons. The idiomatic and safe approach isnet.JoinHostPort, which correctly wraps IPv6 hosts in[]and formats empty hosts as:port.Suggested change:
-import ( - "context" - "errors" - "fmt" - "io" - "log/slog" - "net/http" - "net/url" - "os" - "runtime" - "strings" - "time" +import ( + "context" + "errors" + "fmt" + "io" + "log/slog" + "net" + "net/http" + "net/url" + "os" + "runtime" + "strings" + "time" @@ - // Build address from --port and --host - port := cmd.Int("port") - host := cmd.String("host") - addr := fmt.Sprintf("%s:%d", host, port) + // Build address from --port and --host (IPv4/IPv6 safe) + port := cmd.Int("port") + host := cmd.String("host") + addr := net.JoinHostPort(host, fmt.Sprintf("%d", port))This fixes IPv6 binding and keeps the
hostflag examples valid.internal/utils/utils.go (2)
101-127: Defensively handle nilerrinExtractResponseError
ExtractResponseErrorassumeserris non-nil and immediately callserr.Error(). If a nil slips through from a caller, this will panic. A small guard makes the helper safer without changing its intended behavior:-func ExtractResponseError(err error, rs *http.Response) (errText string) { - errText = err.Error() +func ExtractResponseError(err error, rs *http.Response) (errText string) { + if err == nil { + errText = "unknown error" + } else { + errText = err.Error() + }You can still rely on the existing semantics while avoiding crashes from unexpected nils.
219-240: FixIsTextContentso JSON/XML/etc. are recognized as textThe current
switchonly returnstruefor"application/csv". For values like"application/json"or"application/xml", the case bodies are empty, so control breaks out of theswitchand the function returnsfalse. That means most text-based application types are misclassified.You can fix this by using a single case with a comma-separated list:
func IsTextContent(mediaType string) bool { lowerType := strings.ToLower(mediaType) @@ - // 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": - return true - } + // Popular application types that are text-based + switch lowerType { + case "application/json", + "application/xml", + "application/javascript", + "application/xhtml+xml", + "application/yaml", + "application/csv": + return true + }This ensures all of the intended media types are correctly treated as text.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
⛔ Files ignored due to path filters (1)
.gitignoreis excluded by none and included by none
📒 Files selected for processing (24)
Taskfile.yaml(2 hunks)cmd/main.go(1 hunks)internal/analytics/analytics.go(4 hunks)internal/analytics/analytics_test.go(13 hunks)internal/config/cli.go(10 hunks)internal/http/http_server.go(11 hunks)internal/http/http_server_test.go(7 hunks)internal/mcp_handlers/integration_project_test.go(4 hunks)internal/mcp_handlers/items.go(27 hunks)internal/mcp_handlers/items_test.go(1 hunks)internal/mcp_handlers/launches.go(16 hunks)internal/mcp_handlers/launches_test.go(1 hunks)internal/mcp_handlers/mock_test.go(1 hunks)internal/mcp_handlers/server.go(6 hunks)internal/middleware/http_token_middleware.go(3 hunks)internal/middleware/http_token_middleware_test.go(7 hunks)internal/middleware/middleware.go(3 hunks)internal/middleware/middleware_test.go(2 hunks)internal/utils/ctx_utils.go(4 hunks)internal/utils/ctx_utils_test.go(1 hunks)internal/utils/mock_test.go(2 hunks)internal/utils/utils.go(15 hunks)internal/utils/utils_project_test.go(2 hunks)internal/utils/utils_test.go(3 hunks)
🧰 Additional context used
📓 Path-based instructions (3)
**/*.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/mock_test.gointernal/utils/utils_project_test.gointernal/analytics/analytics_test.gointernal/middleware/middleware_test.gointernal/middleware/http_token_middleware_test.gointernal/utils/utils_test.gointernal/utils/ctx_utils.gointernal/http/http_server_test.gointernal/mcp_handlers/mock_test.gointernal/mcp_handlers/server.gointernal/http/http_server.gointernal/config/cli.gointernal/mcp_handlers/launches.gointernal/mcp_handlers/launches_test.gointernal/mcp_handlers/items.gointernal/middleware/http_token_middleware.gointernal/mcp_handlers/items_test.gocmd/main.gointernal/analytics/analytics.gointernal/mcp_handlers/integration_project_test.gointernal/utils/ctx_utils_test.gointernal/utils/utils.gointernal/middleware/middleware.go
**/*_test.go
⚙️ CodeRabbit configuration file
Ensure comprehensive test coverage, proper test isolation, meaningful assertions, and good test naming conventions.
Files:
internal/utils/mock_test.gointernal/utils/utils_project_test.gointernal/analytics/analytics_test.gointernal/middleware/middleware_test.gointernal/middleware/http_token_middleware_test.gointernal/utils/utils_test.gointernal/http/http_server_test.gointernal/mcp_handlers/mock_test.gointernal/mcp_handlers/launches_test.gointernal/mcp_handlers/items_test.gointernal/mcp_handlers/integration_project_test.gointernal/utils/ctx_utils_test.go
cmd/**
⚙️ CodeRabbit configuration file
Review CLI interface design, configuration handling, and application startup logic for robustness and usability.
Files:
cmd/main.go
🧬 Code graph analysis (16)
internal/analytics/analytics_test.go (1)
internal/middleware/http_token_middleware.go (2)
WithTokenInContext(92-94)HTTPTokenMiddleware(22-64)
internal/middleware/middleware_test.go (1)
internal/utils/ctx_utils.go (1)
WithQueryParams(23-26)
internal/middleware/http_token_middleware_test.go (1)
internal/utils/ctx_utils.go (3)
WithProjectInContext(35-39)GetProjectFromContext(42-46)RPProjectContextKey(16-16)
internal/utils/utils_test.go (1)
internal/utils/utils.go (1)
ProcessAttributeKeys(189-217)
internal/http/http_server_test.go (2)
internal/http/http_server.go (2)
HTTPServerConfig(48-60)NewHTTPServer(84-142)internal/analytics/analytics.go (2)
Analytics(101-114)NewAnalytics(123-172)
internal/mcp_handlers/mock_test.go (2)
internal/utils/mock_test.go (2)
NewMockCallToolRequest(16-18)MockCallToolRequest(11-13)internal/utils/ctx_utils.go (1)
GetProjectFromContext(42-46)
internal/mcp_handlers/server.go (4)
internal/analytics/analytics.go (2)
Analytics(101-114)NewAnalytics(123-172)internal/middleware/middleware.go (1)
QueryParamsMiddleware(9-28)internal/mcp_handlers/launches.go (1)
RegisterLaunchTools(487-502)internal/mcp_handlers/items.go (1)
RegisterTestItemTools(785-800)
internal/http/http_server.go (5)
internal/analytics/analytics.go (2)
Analytics(101-114)NewAnalytics(123-172)internal/middleware/middleware.go (1)
QueryParamsMiddleware(9-28)internal/mcp_handlers/launches.go (1)
RegisterLaunchTools(487-502)internal/mcp_handlers/items.go (1)
RegisterTestItemTools(785-800)internal/middleware/http_token_middleware.go (1)
HTTPTokenMiddleware(22-64)
internal/config/cli.go (4)
internal/http/http_server.go (2)
HTTPServerConfig(48-60)CreateHTTPServerWithMiddleware(209-235)internal/analytics/analytics.go (3)
GetAnalyticArg(328-342)Analytics(101-114)StopAnalytics(560-569)internal/mcp_handlers/server.go (1)
NewServer(21-69)internal/utils/ctx_utils.go (1)
WithProjectInContext(35-39)
internal/mcp_handlers/launches.go (3)
internal/analytics/analytics.go (1)
Analytics(101-114)internal/utils/utils.go (8)
SetPaginationOptions(41-56)DefaultSortingForLaunches(21-21)ExtractProject(87-99)ProcessStartTimeFilter(164-186)ApplyPaginationOptions(59-84)ProcessAttributeKeys(189-217)ExtractResponseError(102-127)ReadResponseBody(286-295)internal/utils/ctx_utils.go (1)
WithQueryParams(23-26)
internal/mcp_handlers/items.go (3)
internal/analytics/analytics.go (1)
Analytics(101-114)internal/utils/utils.go (20)
SetPaginationOptions(41-56)DefaultSortingForItems(22-22)ExtractProject(87-99)DefaultProviderType(25-25)DefaultFilterEqHasStats(27-27)DefaultFilterEqHasChildren(26-26)DefaultFilterInType(28-28)ProcessStartTimeFilter(164-186)ApplyPaginationOptions(59-84)ProcessAttributeKeys(189-217)ExtractResponseError(102-127)ReadResponseBody(286-295)ReadResponseBodyRaw(259-281)IsTextContent(220-240)FirstPage(18-18)DefaultPageSize(20-20)DefaultSortingForLogs(24-24)DefaultItemLogLevel(30-30)DefaultSortingForSuites(23-23)DefaultFilterInTypeSuites(29-29)internal/utils/ctx_utils.go (1)
WithQueryParams(23-26)
internal/middleware/http_token_middleware.go (1)
internal/utils/ctx_utils.go (2)
WithProjectInContext(35-39)ValidateRPToken(50-63)
cmd/main.go (1)
internal/config/cli.go (2)
AppVersion(26-30)RunApp(356-423)
internal/analytics/analytics.go (1)
internal/middleware/http_token_middleware.go (1)
GetTokenFromContext(97-100)
internal/mcp_handlers/integration_project_test.go (1)
internal/middleware/http_token_middleware.go (1)
HTTPTokenMiddleware(22-64)
internal/middleware/middleware.go (1)
internal/utils/ctx_utils.go (1)
QueryParamsFromContext(28-32)
🪛 GitHub Actions: Build
cmd/main.go
[error] 1-1: File is not properly formatted (gci).
🪛 GitHub Check: build
internal/config/cli.go
[failure] 200-200:
File is not properly formatted (gci)
cmd/main.go
[failure] 1-1:
File is not properly formatted (gci)
internal/analytics/analytics.go
[failure] 570-570:
File is not properly formatted (gci)
🪛 YAMLlint (1.37.1)
Taskfile.yaml
[error] 93-93: too many spaces inside brackets
(brackets)
[error] 93-93: too many spaces inside brackets
(brackets)
[error] 105-105: too many spaces inside brackets
(brackets)
[error] 105-105: too many spaces inside brackets
(brackets)
[error] 115-115: too many spaces inside brackets
(brackets)
[error] 115-115: too many spaces inside brackets
(brackets)
🔇 Additional comments (55)
Taskfile.yaml (2)
96-96: Verify PowerShell cleanup commands are syntactically and functionally correct.The cleanup logic attempts to stop containers by port and image name before running new instances. However, the PowerShell pipeline on line 96 is complex:
docker ps --no-trunc | Select-String ":{{.MCP_SERVER_PORT}}->" | ForEach-Object { ($_ -split "\s+")[0] } | ForEach-Object { docker stop $_ }A few concerns:
- The regex pattern
:{{.MCP_SERVER_PORT}}->assumes port mapping output format; consider whetherdocker psoutput is guaranteed to match this pattern consistently.- If the pattern doesn't match, the first pipeline command silently returns no results, causing no containers to be stopped. Error handling is implicit but not explicit.
- If
docker stopfails (e.g., permission issues), the error propagates but may not halt the task execution depending on PowerShell's error handling mode.Consider adding error handling or test these commands manually to ensure they work reliably across different Docker configurations. Optionally, add
-ErrorAction Stopto fail fast if cleanup fails.Also applies to: 99-99
18-19: Introduce new public variables for containerized inspector support.The addition of
INSPECTOR_IMAGEandINSPECTOR_CMDvariables cleanly abstracts the Docker image and inspector command, making the setup easy to override or customize. This supports the HTTP-mode refactoring where inspector runs inside a container.internal/middleware/middleware_test.go (1)
1-29: Query params test correctly switched to shared utils helperUsing
utils.WithQueryParamshere keeps the middleware test aligned with the shared context helpers ininternal/utils, and the assertions still cover both preservation of existing params and addition of new ones. No changes needed.internal/utils/ctx_utils_test.go (1)
1-111: UUID and token validation tests look solid under the utils packageRenaming the package to
utilskeeps the tests colocated with the production helpers, and the table-driven coverage forIsUUIDFormatandValidateRPTokenspans the right edge cases. No changes needed.internal/utils/mock_test.go (1)
1-74: MockCallToolRequest helper cleanly models project extraction behaviorThe mock request type and
extractProjectWithMockhelper accurately mirror the documented priority (request param, then context, then error), and moving this under theutilspackage keeps test scaffolding local to the utils behaviors it exercises. Looks good as-is.internal/mcp_handlers/launches_test.go (1)
1-227: Launch MCP handler tests remain robust after package renameThe tests under
mcp_handlersstill exercise URI templates, list launches, andrun_auto_analysis(including schemaitems/enum and the outgoingAnalyzeLaunchRQ) thoroughly, with correct use ofhttptestandmcptestlifecycles. No changes needed.internal/utils/utils_project_test.go (1)
1-179: ExtractProject behavior is well specified and tested under utilsThe move to the
utilspackage is consistent with the underlying helpers, and the table-drivenTestExtractProjectthoroughly documents precedence, whitespace trimming, and error cases for HTTP vs stdio modes using the shared context APIs and mock request. No changes needed.internal/mcp_handlers/items_test.go (1)
88-161: Make error-prefix and substring checks safer and simpler in testsThe tests cover the important JSON shapes, but two minor improvements would make them more robust and readable:
- Avoid slicing
err.Error()by length
Usingerr.Error()[:len(tt.errorMsg)]will panic if the actual message is ever shorter thantt.errorMsg. Preferstrings.HasPrefixinstead:-import ( - "testing" -) +import ( + "strings" + "testing" +) @@ - if tt.errorMsg != "" && err.Error()[:len(tt.errorMsg)] != tt.errorMsg { + if tt.errorMsg != "" && !strings.HasPrefix(err.Error(), tt.errorMsg) {
- Simplify
containshelper (optional)
The manual substring search incontainsduplicatesstrings.Contains. You could replace its body with a direct call:func contains(s, substr string) bool { - return len(s) >= len(substr) && (s == substr || len(substr) == 0 || - func() bool { - for i := 0; i <= len(s)-len(substr); i++ { - if s[i:i+len(substr)] == substr { - return true - } - } - return false - }()) + return strings.Contains(s, substr) }Both changes keep behavior the same while making the tests more resilient and easier to maintain.
[ suggest_optional_refactor ]internal/analytics/analytics.go (1)
18-22: Using middleware.GetTokenFromContext keeps analytics aligned with HTTP token handlingSwitching
getUserIDFromContextto usemiddleware.GetTokenFromContextcentralizes token extraction logic in one place and keeps analytics consistent with the HTTP middleware’s context contract. The precedence (env/userID hash, then bearer token, then anonymous) remains intact, and the extratoken != ""check is redundant but harmless given the helper’s current behavior.Also applies to: 192-208
internal/mcp_handlers/integration_project_test.go (1)
1-12: LGTM! Package structure aligns with refactoring goals.The package rename to
mcp_handlersand the import ofinternal/middlewarecorrectly reflect the modular architecture introduced in this PR.internal/middleware/http_token_middleware_test.go (1)
1-274: LGTM! Excellent test refactoring.The tests have been properly updated to use the new
utilspackage for context handling. The test coverage is comprehensive, including:
- Project extraction from headers
- Context handling with whitespace
- Middleware integration
- Combined token and project extraction
The test structure follows Go best practices with clear test names and good use of table-driven tests.
cmd/main.go (1)
19-38: LGTM! Clean entry point with proper signal handling.The main function correctly:
- Sets up signal handling for graceful shutdown
- Constructs version information
- Delegates to
config.RunAppfor application logic- Handles errors with proper logging and exit codes
The explicit
stop()call beforeos.Exit(1)ensures the signal context is cancelled even on error paths.internal/http/http_server_test.go (1)
14-184: LGTM! Comprehensive test coverage.The test suite thoroughly covers:
- Server initialization with various token configurations
- Analytics initialization logic
- Default configuration values
- Server lifecycle (start/stop)
- Error cases
The test structure is clean and uses table-driven tests effectively.
internal/utils/utils_test.go (1)
1-172: LGTM! Excellent test coverage for exported utility function.The tests comprehensively cover:
- Basic attribute key processing
- Multiple keys with various formats
- Whitespace handling
- Edge cases (empty strings, colons, etc.)
- Performance testing with 1000+ keys
The function rename to
ProcessAttributeKeys(exported) aligns with the modular refactoring, making this utility available to other packages.internal/analytics/analytics_test.go (1)
1-1071: LGTM! Outstanding test coverage for analytics integration.This test suite is exceptionally thorough, covering:
- Token validation and hashing
- Analytics initialization with various configurations
- Event tracking with context integration
- User ID precedence (env var → bearer token → custom ID → anonymous)
- Concurrent metric increments (thread safety)
- HTTP middleware integration
- Edge cases and error handling
Notable strengths:
- Proper cleanup with
defer analytics.Stop()- Clear test names and documentation
- Table-driven tests for maintainability
- Verification of hash-based user identification
- No variable shadowing issues (uses descriptive names like
handler)internal/utils/ctx_utils.go (3)
14-17: LGTM! Well-defined context key constant.The
RPProjectContextKeyis properly typed ascontextKeyto avoid collisions, exported for cross-package use, and includes an appropriate nolint directive for the security tool.
34-46: LGTM! Robust context utilities with whitespace handling.The context helper functions properly:
- Trim whitespace on both storage and retrieval
- Return
falsefor empty strings inGetProjectFromContext- Follow Go conventions for context value handling
The consistent trimming prevents issues with user-provided values containing leading/trailing spaces.
48-63: LGTM! Solid token validation with UUID parsing.The validation logic correctly:
- Trims whitespace before validation
- Uses proper UUID parsing via
uuid.Validate- Falls back to length check for non-UUID tokens
- Returns
falsefor empty tokensThis approach handles both UUID-format tokens (preferred) and legacy token formats (16+ characters).
internal/mcp_handlers/server.go (3)
1-16: LGTM! Clean package structure with proper imports.The package correctly imports the new modular packages (
internal/analyticsandinternal/middleware), aligning with the PR's refactoring goals.
21-69: LGTM! Well-structured server initialization.The refactored
NewServerfunction demonstrates several improvements:
- Clear separation of concerns with dedicated packages
- Factory pattern for analytics initialization (
analytics.NewAnalytics)- Consolidated tool registration via
RegisterLaunchToolsandRegisterTestItemTools- Proper error handling with contextual messages
- Middleware assignment for query parameter handling
The renamed
analyticsClientvariable (fromanalytics) avoids any potential confusion with the imported package.
71-94: LGTM! Clean prompt loading with proper error handling.The
readPromptsfunction correctly:
- Uses
embed.FSfor embedded resources- Handles filesystem errors
- Uses
filepath.Cleanfor path safety- Returns descriptive errors
internal/middleware/http_token_middleware.go (5)
1-10: LGTM: Clean package structure.The package rename to
middlewareand the import ofinternal/utilsalign well with the modularization objectives of this refactoring.
12-19: LGTM: Proper context key typing.Using a custom
contextKeytype is a Go best practice to prevent context key collisions across packages. Thegosecexception is appropriately justified.
22-64: LGTM: Well-structured middleware.The middleware correctly extracts and stores both the RP API token and project parameter in the request context, with appropriate debug logging. The use of
utils.WithProjectInContextensures consistency across the codebase.
66-89: LGTM: Centralized token validation.Token extraction and validation is properly delegated to
utils.ValidateRPToken, ensuring consistent validation logic across the application.
91-113: LGTM: Clean context helpers.The token context helpers (
WithTokenInContext,GetTokenFromContext) and project extraction function are well-implemented. The separation where token helpers remain in middleware while project helpers moved to utils reflects a logical division of responsibilities.internal/mcp_handlers/items.go (6)
44-236: LGTM: Comprehensive utils integration.The tool implementation correctly migrates to utils functions (
ExtractProject,ProcessStartTimeFilter,WithQueryParams,ApplyPaginationOptions,ProcessAttributeKeys,ExtractResponseError,ReadResponseBody), ensuring consistency across the codebase. The analytics wrapping pattern is also properly applied.
238-268: LGTM: Clean tool implementation.The
toolGetTestItemByIdfunction follows the established pattern with proper utils integration and error handling.
270-308: LGTM: Proper resource template handling.The resource template handler correctly validates URI parameters and handles errors appropriately. Resource handlers don't need analytics wrapping, which is correct.
310-372: LGTM: Proper attachment handling.The attachment tool correctly handles both text and binary content types using
utils.IsTextContentand appropriate encoding (base64 for binary). Error handling and resource cleanup are properly implemented.
374-782: LGTM: Consistent tool implementations.All remaining tools (
toolGetTestItemLogsByFilter,toolGetTestSuitesByFilter,toolGetProjectDefectTypes,toolUpdateDefectTypeForTestItems) follow consistent patterns with proper utils integration, error handling, and analytics wrapping. The defect type update logic correctly constructs issue definitions.
784-800: LGTM: Clean tool registration pattern.The
RegisterTestItemToolsfunction provides a clean, centralized way to register all test item-related tools and resources. This pattern improves maintainability and makes the server initialization code cleaner.internal/http/http_server.go (7)
1-29: LGTM: Clear import organization.The middleware import alias (
chimiddleware) effectively prevents naming conflicts with the internal middleware package. ThebatchSendIntervalconstant provides clear configuration for analytics batching.
84-142: LGTM: Proper analytics initialization.The analytics client is correctly initialized conditionally based on configuration, with appropriate nil handling throughout. The informative logging about analytics status is helpful for debugging.
144-162: LGTM: Clean tool initialization.The use of
RegisterLaunchToolsandRegisterTestItemToolsprovides clean, centralized tool registration. The comment about prompts being registered by the mcp_handlers package is helpful documentation. Using an empty project parameter for HTTP mode is correct since projects come from headers per-request.
172-206: LGTM: Proper lifecycle management.The
StartandStopmethods correctly manage server lifecycle with proper synchronization and nil-safe analytics handling.
208-372: LGTM: Consistent analytics client handling.The updated function signatures (
CreateHTTPServerWithMiddlewarereturning the analytics client,GetHTTPServerInfoaccepting it as a parameter) provide better encapsulation. Nil checks for analytics are consistently applied throughout.
272-351: LGTM: Consistent middleware configuration.All chi middleware references use the
chimiddlewarealias consistently. The separation between chi's built-in middleware and the internalmiddleware.HTTPTokenMiddlewareis clear and well-structured.
374-587: LGTM: Robust request handling.All HTTP handlers properly handle optional analytics with nil checks. The request validation logic (
isMCPRequest,validateMCPPayload) correctly handles both single and batch JSON-RPC requests according to the spec. The SSE stream detection logic properly handles content negotiation per RFC 7231.internal/mcp_handlers/launches.go (4)
1-41: LGTM: Consistent package structure.The package structure, imports, and constructor signature are consistent with
items.go, maintaining a uniform pattern across tool handlers.
43-435: LGTM: Comprehensive utils migration.All launch tool functions (
toolGetLaunches,toolRunQualityGate,toolGetLastLaunchByName,toolDeleteLaunch,toolRunAutoAnalysis,toolUniqueErrorAnalysis,toolForceFinishLaunch) correctly use utils functions for consistent project extraction, error handling, and response processing. The analytics wrapping pattern is uniformly applied.
437-484: LGTM: Proper resource template implementation.The
resourceLaunchfunction correctly implements the resource template pattern with proper URI parameter validation and error handling.
486-502: LGTM: Consistent tool registration pattern.The
RegisterLaunchToolsfunction follows the same clean, centralized registration pattern asRegisterTestItemTools, improving maintainability and consistency across the codebase.internal/config/cli.go (5)
25-116: Nice separation of common vs mode-specific flagsThe split between
GetCommonFlags,GetHTTPFlags, andGetStdioFlagsis clear and keeps mode concerns local to their respective helpers. Flag naming, env bindings, and required/optional semantics all look consistent and will be easy to extend.
228-248: Error classification and analytics shutdown look sound
HandleServerErrorcorrectly treatsnil,http.ErrServerClosed, and context cancellations/deadlines as graceful shutdowns, and centralizes analytics shutdown with an appropriate reason string. This keeps the calling code simple and avoids double‑logging normal termination paths.
250-291: Stdio runner correctly validates token and wires context
RunStdioServerdoes the right things: enforces thatRP_API_TOKEN/--tokenis present, threads the optionalprojectviautils.WithProjectInContext, and uses a buffered error channel so the listener goroutine can report failures without blocking. The shutdown path onctx.Done()with analytics stop is also clean.
335-350: Graceful HTTP shutdown and analytics cleanup on context cancelThe shutdown path for streaming mode is well-structured: you stop analytics, then perform a bounded
server.Shutdownwith its own timeout, and finally stop the MCP HTTP server itself. That sequence should give in‑flight requests time to drain while keeping the process responsive to termination.
355-423: RunApp wiring is clear and mode-aware
RunAppcleanly derivesmcpModefrom the environment, composes the flag set based on that mode, and exposes a singlecli.Commandwith a detailed description and usage examples. TheActionswitch dispatching toRunStreamingServervsRunStdioServeris straightforward, and the default branch logging for unknownMCP_MODEis helpful for debugging misconfiguration.internal/utils/utils.go (7)
17-56: Pagination defaults and tool options are well-structuredThe exported pagination constants and
SetPaginationOptionshelper look good: they centralize page/page-size/sort defaults and make the MCP tool options easy to reuse across handlers. UsingFirstPageandDefaultPageSizehere will help keep behavior consistent between tools.
58-84: ApplyPaginationOptions clamps large values but trusts negative input
ApplyPaginationOptionscorrectly capspageandpage-sizeatmath.MaxInt32before casting, which avoids overflow into the underlying API types. If you ever want to be stricter, you could also clamp or normalize non-positive values back toFirstPage/DefaultPageSize, but the current behavior (passing user-provided negatives through) is at least explicit and safe from panics.
86-99: Project extraction behavior is clear and consistent
ExtractProjectprioritizes the explicitprojecttool argument and then falls back to the context-derived project (from headers/env viaGetProjectFromContext), returning a clear error if neither is present. This is a good, predictable resolution order for multi-mode usage.
129-186: Timestamp parsing and interval filter logic look robust
ParseTimestampToEpochsensibly tries integer Unix timestamps, RFC3339, and a few common date-time formats, andProcessStartTimeFiltercomposes a validated[from,to]interval, rejecting inverted ranges or half-open intervals. The behavior is strict and should surface bad filters early.
188-217: Attribute key processing matches documented behavior
ProcessAttributeKeyscorrectly normalizes incoming keys by trimming whitespace, extracting postfix portions when akey:postfixform is used, and appending a:suffix otherwise before merging with existingfilterAttributes. The guard rails on empty inputs keep the output predictable.
242-255: Already-closed-body detection is a useful, low-friction guard
IsAlreadyClosedErrorcentralizes detection of common “already closed” error phrases, which keeps both logging and error-wrapping helpers from being noisy in expected shutdown paths. This is a nice bit of defensive ergonomics around HTTP body handling.
257-295: Response body helpers correctly centralize close/error handling
ReadResponseBodyRawandReadResponseBodyenforce that the body is non-nil, always close it with a guarded defer, and provide a clean way to surface read failures as MCP tool errors. This avoids scattereddefer resp.Body.Close()+ error handling throughout the codebase and is a solid pattern for HTTP integration.
AleksandrParamonoff
force-pushed
the
refactoring-detach-to-modules
branch
3 times, most recently
from
4d63fc1 to
7ccc5fa
Compare
There was a problem hiding this comment.
Pull Request Overview
Copilot reviewed 28 out of 30 changed files in this pull request and generated 2 comments.
Comments suppressed due to low confidence (2)
internal/http/http_server.go:160
- The comment states "Prompts are registered by mcp_handlers package" but prompts are not actually being registered for HTTP mode. The
mcp_handlers.NewServer()function registers prompts in stdio mode, but in HTTP mode,initializeTools()only callsRegisterLaunchTools()andRegisterTestItemTools()without any prompt registration. This means prompts will not be available when running the server in HTTP mode. Consider either:
- Creating a public
RegisterPrompts()function in the mcp_handlers package and calling it here - Directly calling prompt registration logic in this function similar to how it's done in
mcp_handlers.NewServer()
internal/middleware/http_token_middleware.go:13
- The
contextKeytype is defined in bothinternal/middleware/http_token_middleware.goandinternal/utils/ctx_utils.go. While Go's package scoping prevents actual collisions, having two separatecontextKeytype definitions in different packages can be confusing and makes the code less maintainable. Consider consolidating both type definitions into a single shared location (e.g., ininternal/utils/ctx_utils.go) and importing it where needed, or use a more specific type name likemiddlewareContextKeyto clarify the intended scope.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
AleksandrParamonoff
force-pushed
the
refactoring-detach-to-modules
branch
from
7ccc5fa to
a1ccc88
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_test.go (1)
335-361: Consider explicit analytics cleanup and avoidingpanicin the test helperInitializing analytics via
analytics.NewAnalyticsexercises the real setup, which is good. IfNewAnalyticsstarts background goroutines (as its fields suggest), it’s safer for tests to explicitly stop/close the instance (e.g., via aStop/Closemethod) to avoid leaking goroutines across tests. Also, instead of panicking on construction failure inside the helper, you might move the initialization into the test body and userequire.NoError(t, err)so failures are reported as normal test assertions.internal/analytics/analytics.go (1)
191-214: Minor simplification opportunity ingetUserIDFromContext
middleware.GetTokenFromContextalready guarantees the boolean istrueonly for non‑empty tokens, so thetoken != ""check is redundant. You can simplify without changing behavior:- if token, ok := middleware.GetTokenFromContext(ctx); ok && token != "" { + if token, ok := middleware.GetTokenFromContext(ctx); ok {internal/mcp_handlers/server.go (1)
71-94: FixreadPromptsslice preallocation to avoid leading empty handlers
handlers := make([]promptreader.PromptHandlerPair, len(entries))combined withhandlers = append(handlers, prompts...)leaves the firstlen(entries)elements as zero values, soNewServerwill attempt to register empty prompts before the real ones. Preallocate capacity instead of length:- handlers := make([]promptreader.PromptHandlerPair, len(entries)) + handlers := make([]promptreader.PromptHandlerPair, 0, len(entries))This keeps the returned slice free of bogus empty entries while still avoiding repeated allocations.
♻️ Duplicate comments (1)
Taskfile.yaml (1)
115-115: Fix YAML bracket spacing per linting rules.The
deps:declaration has extra spaces inside brackets, which violates YAML linting rules (as indicated by the static analysis hint). Remove the spaces for consistency with the rest of the file.Apply this diff:
- deps: [ docker:build-debug ] + deps: [docker:build-debug]
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
⛔ Files ignored due to path filters (1)
.gitignoreis excluded by none and included by none
📒 Files selected for processing (28)
Dockerfile(1 hunks)README.md(1 hunks)Taskfile.yaml(3 hunks)cmd/main.go(1 hunks)debug.dockerfile(1 hunks)internal/analytics/analytics.go(5 hunks)internal/analytics/analytics_test.go(13 hunks)internal/config/cli.go(9 hunks)internal/http/http_server.go(11 hunks)internal/http/http_server_test.go(4 hunks)internal/mcp_handlers/integration_project_test.go(5 hunks)internal/mcp_handlers/items.go(27 hunks)internal/mcp_handlers/items_test.go(1 hunks)internal/mcp_handlers/launches.go(16 hunks)internal/mcp_handlers/launches_test.go(1 hunks)internal/mcp_handlers/mock_test.go(1 hunks)internal/mcp_handlers/server.go(5 hunks)internal/middleware/http_token_middleware.go(3 hunks)internal/middleware/http_token_middleware_test.go(7 hunks)internal/middleware/middleware.go(2 hunks)internal/middleware/middleware_test.go(2 hunks)internal/testutil/mock.go(1 hunks)internal/utils/ctx_utils.go(3 hunks)internal/utils/ctx_utils_test.go(1 hunks)internal/utils/mock_test.go(1 hunks)internal/utils/utils.go(14 hunks)internal/utils/utils_project_test.go(2 hunks)internal/utils/utils_test.go(3 hunks)
🧰 Additional context used
📓 Path-based instructions (4)
**/*.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:
cmd/main.gointernal/mcp_handlers/items_test.gointernal/utils/mock_test.gointernal/testutil/mock.gointernal/middleware/http_token_middleware_test.gointernal/utils/utils_project_test.gointernal/middleware/middleware.gointernal/analytics/analytics.gointernal/middleware/middleware_test.gointernal/analytics/analytics_test.gointernal/utils/ctx_utils_test.gointernal/utils/utils_test.gointernal/mcp_handlers/server.gointernal/mcp_handlers/items.gointernal/mcp_handlers/launches.gointernal/middleware/http_token_middleware.gointernal/http/http_server_test.gointernal/config/cli.gointernal/mcp_handlers/mock_test.gointernal/mcp_handlers/launches_test.gointernal/utils/ctx_utils.gointernal/http/http_server.gointernal/mcp_handlers/integration_project_test.gointernal/utils/utils.go
cmd/**
⚙️ CodeRabbit configuration file
Review CLI interface design, configuration handling, and application startup logic for robustness and usability.
Files:
cmd/main.go
**/*_test.go
⚙️ CodeRabbit configuration file
Ensure comprehensive test coverage, proper test isolation, meaningful assertions, and good test naming conventions.
Files:
internal/mcp_handlers/items_test.gointernal/utils/mock_test.gointernal/middleware/http_token_middleware_test.gointernal/utils/utils_project_test.gointernal/middleware/middleware_test.gointernal/analytics/analytics_test.gointernal/utils/ctx_utils_test.gointernal/utils/utils_test.gointernal/http/http_server_test.gointernal/mcp_handlers/mock_test.gointernal/mcp_handlers/launches_test.gointernal/mcp_handlers/integration_project_test.go
**/Dockerfile*
⚙️ CodeRabbit configuration file
Focus on security best practices, image size optimization, proper layering, and multi-stage builds where appropriate.
Files:
Dockerfile
🧬 Code graph analysis (18)
cmd/main.go (1)
internal/config/cli.go (2)
AppVersion(26-30)RunApp(356-423)
internal/utils/mock_test.go (2)
internal/testutil/mock.go (1)
MockCallToolRequest(8-10)internal/utils/ctx_utils.go (1)
GetProjectFromContext(42-46)
internal/middleware/http_token_middleware_test.go (1)
internal/utils/ctx_utils.go (3)
WithProjectInContext(35-39)GetProjectFromContext(42-46)RPProjectContextKey(16-16)
internal/utils/utils_project_test.go (1)
internal/testutil/mock.go (1)
NewMockCallToolRequest(13-15)
internal/middleware/middleware.go (1)
internal/utils/ctx_utils.go (1)
QueryParamsFromContext(28-32)
internal/analytics/analytics.go (1)
internal/middleware/http_token_middleware.go (1)
GetTokenFromContext(97-100)
internal/middleware/middleware_test.go (1)
internal/utils/ctx_utils.go (1)
WithQueryParams(23-26)
internal/analytics/analytics_test.go (1)
internal/middleware/http_token_middleware.go (2)
WithTokenInContext(92-94)HTTPTokenMiddleware(22-64)
internal/utils/utils_test.go (1)
internal/utils/utils.go (1)
ProcessAttributeKeys(189-217)
internal/mcp_handlers/server.go (4)
internal/analytics/analytics.go (2)
Analytics(101-115)NewAnalytics(124-173)internal/middleware/middleware.go (1)
QueryParamsMiddleware(9-28)internal/mcp_handlers/launches.go (1)
RegisterLaunchTools(498-513)internal/mcp_handlers/items.go (1)
RegisterTestItemTools(803-818)
internal/mcp_handlers/items.go (3)
internal/analytics/analytics.go (1)
Analytics(101-115)internal/utils/utils.go (20)
SetPaginationOptions(41-56)DefaultSortingForItems(22-22)ExtractProject(87-99)DefaultProviderType(25-25)DefaultFilterEqHasStats(27-27)DefaultFilterEqHasChildren(26-26)DefaultFilterInType(28-28)ProcessStartTimeFilter(164-186)ApplyPaginationOptions(59-84)ProcessAttributeKeys(189-217)ExtractResponseError(102-127)ReadResponseBody(286-295)ReadResponseBodyRaw(259-281)IsTextContent(220-240)FirstPage(18-18)DefaultPageSize(20-20)DefaultSortingForLogs(24-24)DefaultItemLogLevel(30-30)DefaultSortingForSuites(23-23)DefaultFilterInTypeSuites(29-29)internal/utils/ctx_utils.go (1)
WithQueryParams(23-26)
internal/mcp_handlers/launches.go (3)
internal/analytics/analytics.go (1)
Analytics(101-115)internal/utils/utils.go (8)
SetPaginationOptions(41-56)DefaultSortingForLaunches(21-21)ExtractProject(87-99)ProcessStartTimeFilter(164-186)ApplyPaginationOptions(59-84)ProcessAttributeKeys(189-217)ExtractResponseError(102-127)ReadResponseBody(286-295)internal/utils/ctx_utils.go (1)
WithQueryParams(23-26)
internal/middleware/http_token_middleware.go (1)
internal/utils/ctx_utils.go (2)
WithProjectInContext(35-39)ValidateRPToken(50-63)
internal/http/http_server_test.go (1)
internal/analytics/analytics.go (2)
Analytics(101-115)NewAnalytics(124-173)
internal/config/cli.go (4)
internal/http/http_server.go (2)
HTTPServerConfig(48-60)CreateHTTPServerWithMiddleware(209-235)internal/analytics/analytics.go (3)
GetAnalyticArg(329-343)Analytics(101-115)StopAnalytics(569-578)internal/mcp_handlers/server.go (1)
NewServer(21-69)internal/utils/ctx_utils.go (1)
WithProjectInContext(35-39)
internal/mcp_handlers/mock_test.go (2)
internal/testutil/mock.go (1)
MockCallToolRequest(8-10)internal/utils/ctx_utils.go (1)
GetProjectFromContext(42-46)
internal/http/http_server.go (5)
internal/analytics/analytics.go (2)
Analytics(101-115)NewAnalytics(124-173)internal/middleware/middleware.go (1)
QueryParamsMiddleware(9-28)internal/mcp_handlers/launches.go (1)
RegisterLaunchTools(498-513)internal/mcp_handlers/items.go (1)
RegisterTestItemTools(803-818)internal/middleware/http_token_middleware.go (1)
HTTPTokenMiddleware(22-64)
internal/mcp_handlers/integration_project_test.go (2)
internal/testutil/mock.go (1)
NewMockCallToolRequest(13-15)internal/middleware/http_token_middleware.go (1)
HTTPTokenMiddleware(22-64)
🪛 YAMLlint (1.37.1)
Taskfile.yaml
[error] 115-115: too many spaces inside brackets
(brackets)
[error] 115-115: too many spaces inside brackets
(brackets)
🔇 Additional comments (42)
internal/utils/utils_project_test.go (1)
7-9: Centralizing mock construction viatestutil.NewMockCallToolRequestlooks goodUsing the shared helper keeps tests consistent with other packages and avoids duplicating mock setup; the new call site and import are correct and type-safe.
Also applies to: 165-169
internal/utils/ctx_utils_test.go (1)
1-111: Tests correctly validate UUID and token handling under the newutilspackagePackage alignment is correct, and the existing cases give good coverage of happy paths and edge conditions for both
IsUUIDFormatandValidateRPToken.internal/testutil/mock.go (1)
7-32:MockCallToolRequesttest helper is cohesive and matches its useThe dedicated constructor and minimal method set give a clear, reusable test double for call tool requests without leaking production concerns into tests.
internal/utils/ctx_utils.go (1)
14-46: Context helpers and RP token validation follow solid, predictable semanticsUsing a dedicated
contextKeytype andRPProjectContextKeyavoids collisions, and trimming in bothWithProjectInContextandGetProjectFromContextensures whitespace-only values are treated as “no project”.ValidateRPToken’s combination of UUID check plus minimum-length fallback is simple, efficient, and well covered by the accompanying tests.Also applies to: 48-63
Dockerfile (1)
17-19: Docker build now correctly targets the newcmd/main.goentrypointSwitching the build target to
cmd/main.gowhile preserving the existing binary name and ldflags keeps the container behavior intact and consistent with the new application layout.README.md (1)
383-385: Build instructions now match the newcmd/main.goentrypointUpdating the example to
go build -o reportportal-mcp-server ./cmd/main.gokeeps the README aligned with the Dockerfile and the refactored command layout.debug.dockerfile (1)
20-20: LGTM - Build path updated correctly.The build output path has been properly updated to use the new
cmd/main.goentrypoint, aligning with the modular refactoring objectives of this PR.internal/mcp_handlers/launches_test.go (2)
1-1: LGTM - Package rename aligns with refactoring.The package rename from
mcpreportportaltomcp_handlersis consistent with the PR's objective to modularize the codebase.
46-46: No action needed—nil analytics handling is already safely implemented.The
WithAnalyticsmethod ininternal/analytics/analytics.goincludes an explicit nil receiver check (if a != nil { a.TrackMCPEvent(...) }) that safely handles nil analytics clients. The codebase has a dedicated testTestWithAnalyticsNilAnalyticsthat verifies this behavior, and both test cases inlaunches_test.gointentionally pass nil analytics, then execute the tools to confirm nil handling works end-to-end. This is intentional Go design—no nil pointer dereference risk exists.Likely an incorrect or invalid review comment.
internal/mcp_handlers/items_test.go (1)
1-1: LGTM - Package rename and test coverage.The package rename to
mcp_handlersis consistent with the refactoring effort. The test suite provides comprehensive coverage for thegetDefectTypesFromJsonfunction, including edge cases and error scenarios.internal/middleware/middleware_test.go (2)
1-1: LGTM - Package rename consistent.The package rename to
middlewarealigns with the modular restructuring objectives.
10-10: LGTM - Proper use of utils package.The import of
internal/utilsand subsequent use ofutils.WithQueryParamscorrectly reflects the refactored utility location.cmd/main.go (2)
13-17: LGTM - Build-time version variables.The version variables are properly declared for ldflags injection during the build process, aligning with the build configuration in the Dockerfiles and Taskfile.
19-38: LGTM - Robust signal-aware main implementation.The main function demonstrates Go best practices:
- Proper signal handling with
signal.NotifyContextfor graceful shutdown on OS interrupts and SIGTERM- Deferred
stop()ensures cleanup under normal flow- Explicit
stop()call beforeos.Exit(1)prevents context leak on error paths- Structured logging with
slogfor error reportinginternal/middleware/http_token_middleware_test.go (3)
1-1: LGTM - Package rename consistent.The package rename to
middlewareis consistent with the broader refactoring effort.
11-11: LGTM - Proper migration to utils package.The import of
internal/utilsand subsequent migration of all context helper calls (WithProjectInContext,GetProjectFromContext,RPProjectContextKey) to the utils package properly centralizes these utilities and eliminates duplication.
75-90: LGTM - Comprehensive context helper tests.The tests thoroughly verify context operations with proper assertions for both positive and negative cases.
internal/mcp_handlers/mock_test.go (1)
13-25: LGTM - Test helper implementation.The
extractProjectWithMockfunction correctly implements the priority order for project extraction (request parameter → context → error). The past review comments regarding duplication withinternal/utils/mock_test.gowere already addressed in commit eae13d0.internal/utils/utils_test.go (2)
1-1: LGTM - Package rename and API export.The package rename to
utilsand the corresponding update to test the now-exportedProcessAttributeKeysfunction align with Go conventions for public APIs and the broader refactoring effort.
8-147: LGTM - Comprehensive test coverage.The test suite for
ProcessAttributeKeysdemonstrates excellent coverage:
- Basic cases and edge cases
- Whitespace handling
- Multiple keys and mixed scenarios
- Complex real-world scenarios
- Performance testing with large inputs
internal/middleware/middleware.go (1)
3-7: Centralizing query param extraction viautils.QueryParamsFromContextlooks goodUsing
utils.QueryParamsFromContextkeeps context handling in one place and the middleware logic remains straightforward and side‑effect free beyond mutating the request URL, which is appropriate here.Also applies to: 17-27
internal/analytics/analytics_test.go (2)
525-615: Good alignment between analytics tests and middleware token context behaviorSwitching to
middleware.WithTokenInContextin analytics tests keeps token propagation consistent with production behavior and avoids duplicated helpers. The various scenarios around env token, bearer token, and anonymous mode are clearly expressed and remain isolated per test instance.Also applies to: 617-722, 724-887, 889-958
960-1071: HTTP middleware integration tests correctly exercise real middlewareUsing
middleware.HTTPTokenMiddlewarewith thehttpHandlervariable inTestHTTPTokenMiddlewareIntegrationWithAnalyticsavoids package shadowing and ensures the tests run through the real HTTP middleware stack, which is exactly what you want to validate end‑to‑end behavior.internal/analytics/analytics.go (1)
100-115: IdempotentStopimplementation with atomic flag and channels is solidAdding the
stopped int32flag and guardingStopwithatomic.CompareAndSwapInt32ensures the shutdown sequence (closingstopChan, waiting ontickerDone, thenwg.Wait) runs exactly once, even if multiple goroutines callStopconcurrently or tests call it repeatedly. This removes the risk of double‑close panics while keeping the background processor shutdown clean.Also applies to: 368-395
internal/mcp_handlers/server.go (1)
21-69: Server wiring for analytics and middleware is coherentReturning
(*server.MCPServer, *analytics.Analytics, error)fromNewServerand wiringrpClient.APIClient.GetConfig().Middleware = middleware.QueryParamsMiddlewaremakes the dependencies explicit and allows the caller to control analytics lifecycle (e.g., deferred stop). Tool registration viaRegisterLaunchToolsandRegisterTestItemToolskeeps this constructor focused on composition rather than per‑tool details.internal/mcp_handlers/integration_project_test.go (2)
81-115: Integration test cleanly composes HTTP middleware with mock MCP request
TestIntegration_ProjectExtractionFlownow usestestutil.NewMockCallToolRequestplusmiddleware.HTTPTokenMiddlewareto build the context, which keeps the test aligned with the real request path and removes duplicate mock construction. Capturingctxfrom the wrapped handler and feeding it intoextractProjectWithMockaccurately exercises the header vs. request‑project precedence logic.
117-155: End‑to‑end HTTP flow test uses middleware without package shadowingIn
TestIntegration_CompleteHTTPFlow, introducing thehttpHandler := middleware.HTTPTokenMiddleware(handler)variable avoids shadowing themiddlewarepackage and makes the request execution (httpHandler.ServeHTTP) straightforward to read. Usingtestutil.NewMockCallToolRequestkeeps the project parameter setup centralized and ensures the test reflects actual tool request shape.internal/middleware/http_token_middleware.go (1)
12-19: LGTM! Proper context key type safety.The introduction of a local
contextKeytype is a Go best practice that prevents context key collisions. The delegation toutils.WithProjectInContextandutils.ValidateRPTokenis consistent with the refactoring goals and improves code organization.internal/http/http_server.go (2)
109-124: LGTM! Clean analytics initialization with proper error handling.The analytics client initialization properly handles the HTTP mode constraint (empty FallbackRPToken) and includes appropriate error handling and logging. The use of
*analytics.Analyticsas a parameter aligns well with the refactoring goals.
155-158: LGTM! Proper delegation of tool registration.The tool registration is now cleanly delegated to
mcp_handlers.RegisterLaunchToolsandmcp_handlers.RegisterTestItemTools, which improves modularity and separation of concerns. The empty project parameter is appropriate for HTTP mode where projects come from request headers.internal/mcp_handlers/launches.go (2)
497-513: LGTM! Clean tool registration pattern.The new
RegisterLaunchToolsfunction provides a clean, centralized way to register all launch-related tools and resources. This improves maintainability and makes the server initialization code more readable.
52-52: LGTM! Consistent use of utils package constants.The migration to
utils.SetPaginationOptions(utils.DefaultSortingForLaunches)is consistent across all tool definitions and improves code organization by centralizing pagination logic.internal/mcp_handlers/items.go (2)
802-818: LGTM! Consistent tool registration pattern.The
RegisterTestItemToolsfunction follows the same clean pattern asRegisterLaunchTools, providing centralized registration of all test item-related tools and resources. This consistency improves maintainability across the codebase.
156-160: LGTM! Proper use of exported utils constants.The use of exported constants like
utils.DefaultProviderType,utils.DefaultFilterEqHasStats, etc., centralizes default values and makes them easily discoverable and maintainable.internal/config/cli.go (4)
160-160: LGTM! Analytics secret properly retrieved.The call to
analytics.GetAnalyticArg()properly retrieves the analytics API secret. This is consistent with the analytics initialization pattern used throughout the codebase.
228-248: LGTM! Robust error handling with proper analytics cleanup.The
HandleServerErrorfunction properly distinguishes between graceful shutdowns and actual errors, ensuring analytics is stopped in both cases. The use oferrors.Isfor error checking is correct and follows Go best practices.
340-347: LGTM! Proper graceful shutdown with timeout.The shutdown logic properly uses a separate context with a 5-second timeout to ensure the server stops gracefully. The defer cancel pattern and sequential shutdown of both the HTTP server and MCP components follow best practices.
355-423: LGTM! Clean mode-based server dispatch.The
RunAppfunction provides a clean entry point that dynamically builds flags based on MCP_MODE and dispatches to the appropriate server function. The help text is comprehensive and clearly documents the differences between modes.internal/utils/utils.go (4)
17-31: LGTM! Proper export of public constants.The renaming of constants from camelCase to PascalCase correctly exposes them as part of the public API. The naming is descriptive and follows Go conventions.
59-84: LGTM! Type-safe generic pagination function.The use of generics for
ApplyPaginationOptionsprovides type safety while avoiding code duplication. The overflow checks formath.MaxInt32are appropriate for the int32 API constraints.
244-255: LGTM! Defensive error checking prevents log spam.The
IsAlreadyClosedErrorhelper is a good defensive pattern that prevents unnecessary error logging when response bodies are already closed. This is a common issue in HTTP client code and the checks cover the main error message patterns.
259-281: LGTM! Proper resource cleanup with defensive error handling.The
ReadResponseBodyRawfunction properly handles response body cleanup with defer, and usesIsAlreadyClosedErrorto avoid spurious error logs. This is a solid implementation of the cleanup pattern.
AleksandrParamonoff
force-pushed
the
refactoring-detach-to-modules
branch
from
a1ccc88 to
dcde972
Compare
There was a problem hiding this comment.
Pull Request Overview
Copilot reviewed 27 out of 29 changed files in this pull request and generated no new comments.
Comments suppressed due to low confidence (1)
internal/middleware/http_token_middleware.go:13
- The
contextKeytype is duplicated in bothinternal/middleware/http_token_middleware.go(line 13) andinternal/utils/ctx_utils.go(line 12). While this doesn't cause a compilation error since they're in different packages, it's redundant and could lead to confusion.
Consider consolidating this type definition into one location (e.g., in internal/utils/ctx_utils.go) and removing it from the middleware package. The middleware package can then simply use the string type directly for its context keys, or import the type from utils if needed.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (5)
internal/mcp_handlers/items_test.go (1)
152-162: Usestrings.Containsfrom the standard library.This custom implementation unnecessarily reimplements the standard library's
strings.Containsfunction with added complexity. The standard library version is clearer, more efficient, and well-tested.Apply this diff:
+import ( + "strings" + "testing" +) + func TestGetDefectTypesFromJson_VerifyContent(t *testing.T) { // ... expectedStrings := []string{"NO_DEFECT", "locator", "nd001"} for _, expected := range expectedStrings { - if !contains(result, expected) { + if !strings.Contains(result, expected) { t.Errorf("result should contain %q, got: %s", expected, result) } } } - -// Helper function to check if a string contains a substring -func contains(s, substr string) bool { - return len(s) >= len(substr) && (s == substr || len(substr) == 0 || - func() bool { - for i := 0; i <= len(s)-len(substr); i++ { - if s[i:i+len(substr)] == substr { - return true - } - } - return false - }()) -}internal/http/http_server_test.go (1)
335-361: Tighten analytics test setup and cleanupUsing
analytics.NewAnalyticshere is fine, but a couple of small improvements would make the test more robust:
- Prefer
require.NoError(t, err)overpanic(err)so failures are reported as proper test failures rather than panics.- Since
NewAnalyticsstarts background processing, consider stopping it viadefer analyticsInstance.Stop()ort.Cleanup(analyticsInstance.Stop)to avoid leaking goroutines across tests.These keep the test behavior clear and isolated while still exercising the real analytics client.
internal/mcp_handlers/server.go (1)
71-93: Fix slice preallocation bug inreadPromptsto avoid empty prompt registrations
readPromptscurrently does:handlers := make([]promptreader.PromptHandlerPair, len(entries)) for _, entry := range entries { ... prompts, err := promptreader.LoadPromptsFromYAML(data) ... handlers = append(handlers, prompts...) }This creates a slice with
len(entries)zero-valued elements, then appends all loaded prompts. When you later iterate overpromptsinNewServer, the firstlen(entries)entries will have empty names/handlers, which can lead to invalid prompt registrations.Adjust to start with length 0 and only append real prompts:
-func readPrompts(files embed.FS, dir string) ([]promptreader.PromptHandlerPair, error) { +func readPrompts(files embed.FS, dir string) ([]promptreader.PromptHandlerPair, error) { @@ - handlers := make([]promptreader.PromptHandlerPair, len(entries)) + // Start with an empty slice; preallocate capacity if desired + handlers := make([]promptreader.PromptHandlerPair, 0, len(entries)) @@ - handlers = append(handlers, prompts...) + handlers = append(handlers, prompts...) @@ - return handlers, nil + return handlers, nil }This ensures only valid prompt definitions are returned and registered.
internal/http/http_server.go (1)
237-248: Clarify or populateAnalyticsEnabledvs nestedAnalytics.Enabled
HTTPServerInfoexposes both a top-levelAnalyticsEnabledand a nestedAnalytics.Enabled, but only the nested field is actively set:type HTTPServerInfo struct { ... AnalyticsEnabled bool `json:"analytics_enabled"` ... Analytics AnalyticsInfo `json:"analytics"` } ... func GetHTTPServerInfo(analyticsClient *analytics.Analytics) HTTPServerInfo { ... if analyticsClient != nil { info.Analytics = AnalyticsInfo{Enabled: true, Type: "batch", Interval: batchSendInterval.String()} } else { info.Analytics = AnalyticsInfo{Enabled: false} } }and later:
info.Analytics.Enabled = hs.analytics != nilThe top-level
AnalyticsEnabledremains its zero value, which can be misleading for clients reading the JSON.Consider either:
- Populating
AnalyticsEnabled(e.g.,info.AnalyticsEnabled = hs.analytics != nil), or- Removing it and relying solely on
info.Analytics.Enabledto avoid redundant/confusing fields.This will make the
/infoand/metricsresponses more self-consistent.Also applies to: 353-372, 413-425
internal/mcp_handlers/launches.go (1)
21-41: Guardlr.analyticsto avoid panics when analytics is disabled
LaunchResources.analyticsis a*analytics.Analyticsthat can legitimately benilwhen analytics is turned off or initialization fails. All tools currently wrap handlers with:return mcp.NewTool("...", options...), lr.analytics.WithAnalytics("tool_name", func(ctx context.Context, request mcp.CallToolRequest) (*mcp.CallToolResult, error) { ... })If
analyticsClientisnil(as passed fromNewServer/ HTTP server), calling a method on a nil receiver will panic unlessWithAnalyticsis explicitly written to handle a nil receiver internally.To make this robust regardless of
WithAnalytics’s implementation, consider a small helper onLaunchResources:func (lr *LaunchResources) wrapWithAnalytics( toolName string, handler server.ToolHandlerFunc, ) server.ToolHandlerFunc { if lr.analytics == nil { return handler } return lr.analytics.WithAnalytics(toolName, handler) }and then use it across tools:
-), lr.analytics.WithAnalytics("get_launches", func(ctx context.Context, request mcp.CallToolRequest) (*mcp.CallToolResult, error) { +), lr.wrapWithAnalytics("get_launches", func(ctx context.Context, request mcp.CallToolRequest) (*mcp.CallToolResult, error) {Apply the same pattern to the other tool constructors (
run_quality_gate,get_last_launch_by_name,launch_delete,run_auto_analysis,run_unique_error_analysis,launch_force_finish).This keeps analytics purely optional without risking runtime panics when it’s disabled.
Also applies to: 43-45, 97-100, 167-176, 200-213, 259-268, 290-328, 371-387, 415-425
♻️ Duplicate comments (1)
Taskfile.yaml (1)
113-116: Fix remaining YAML bracket spacing for linter compliance
depsat Line 115 still has extra spaces inside the brackets, which YAMLlint flags.Apply this minimal fix:
- deps: [ docker:build-debug ] + deps: [docker:build-debug]This aligns with the previous lint feedback and removes the remaining brackets warning.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
⛔ Files ignored due to path filters (1)
.gitignoreis excluded by none and included by none
📒 Files selected for processing (27)
Dockerfile(1 hunks)README.md(1 hunks)Taskfile.yaml(3 hunks)cmd/main.go(1 hunks)debug.dockerfile(1 hunks)internal/analytics/analytics.go(5 hunks)internal/analytics/analytics_test.go(13 hunks)internal/config/cli.go(9 hunks)internal/http/http_server.go(11 hunks)internal/http/http_server_test.go(4 hunks)internal/mcp_handlers/integration_project_test.go(4 hunks)internal/mcp_handlers/items.go(27 hunks)internal/mcp_handlers/items_test.go(1 hunks)internal/mcp_handlers/launches.go(16 hunks)internal/mcp_handlers/launches_test.go(1 hunks)internal/mcp_handlers/server.go(5 hunks)internal/middleware/http_token_middleware.go(3 hunks)internal/middleware/http_token_middleware_test.go(7 hunks)internal/middleware/middleware.go(2 hunks)internal/middleware/middleware_test.go(2 hunks)internal/reportportal/mock_test.go(0 hunks)internal/testutil/mock.go(1 hunks)internal/utils/ctx_utils.go(3 hunks)internal/utils/ctx_utils_test.go(1 hunks)internal/utils/utils.go(14 hunks)internal/utils/utils_project_test.go(2 hunks)internal/utils/utils_test.go(4 hunks)
💤 Files with no reviewable changes (1)
- internal/reportportal/mock_test.go
🧰 Additional context used
📓 Path-based instructions (4)
**/*.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/ctx_utils_test.gointernal/mcp_handlers/launches_test.gointernal/utils/ctx_utils.gointernal/analytics/analytics_test.gointernal/middleware/middleware.gointernal/utils/utils_project_test.gointernal/http/http_server_test.gointernal/mcp_handlers/launches.gointernal/mcp_handlers/server.gointernal/middleware/http_token_middleware.gointernal/utils/utils_test.gointernal/http/http_server.gocmd/main.gointernal/middleware/http_token_middleware_test.gointernal/mcp_handlers/items_test.gointernal/config/cli.gointernal/mcp_handlers/integration_project_test.gointernal/utils/utils.gointernal/testutil/mock.gointernal/analytics/analytics.gointernal/mcp_handlers/items.gointernal/middleware/middleware_test.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/mcp_handlers/launches_test.gointernal/analytics/analytics_test.gointernal/utils/utils_project_test.gointernal/http/http_server_test.gointernal/utils/utils_test.gointernal/middleware/http_token_middleware_test.gointernal/mcp_handlers/items_test.gointernal/mcp_handlers/integration_project_test.gointernal/middleware/middleware_test.go
**/Dockerfile*
⚙️ CodeRabbit configuration file
Focus on security best practices, image size optimization, proper layering, and multi-stage builds where appropriate.
Files:
Dockerfile
cmd/**
⚙️ CodeRabbit configuration file
Review CLI interface design, configuration handling, and application startup logic for robustness and usability.
Files:
cmd/main.go
🧬 Code graph analysis (16)
internal/analytics/analytics_test.go (1)
internal/middleware/http_token_middleware.go (2)
WithTokenInContext(92-94)HTTPTokenMiddleware(22-64)
internal/middleware/middleware.go (1)
internal/utils/ctx_utils.go (1)
QueryParamsFromContext(28-32)
internal/utils/utils_project_test.go (2)
internal/testutil/mock.go (1)
NewMockCallToolRequest(13-15)internal/utils/utils_test.go (1)
ExtractProjectWithMock(179-194)
internal/http/http_server_test.go (1)
internal/analytics/analytics.go (2)
Analytics(101-115)NewAnalytics(124-173)
internal/mcp_handlers/launches.go (3)
internal/analytics/analytics.go (1)
Analytics(101-115)internal/utils/utils.go (8)
SetPaginationOptions(41-56)DefaultSortingForLaunches(21-21)ExtractProject(87-99)ProcessStartTimeFilter(164-186)ApplyPaginationOptions(59-84)ProcessAttributeKeys(189-217)ExtractResponseError(102-127)ReadResponseBody(286-295)internal/utils/ctx_utils.go (1)
WithQueryParams(23-26)
internal/mcp_handlers/server.go (4)
internal/analytics/analytics.go (2)
Analytics(101-115)NewAnalytics(124-173)internal/middleware/middleware.go (1)
QueryParamsMiddleware(9-28)internal/mcp_handlers/launches.go (1)
RegisterLaunchTools(498-513)internal/mcp_handlers/items.go (1)
RegisterTestItemTools(803-818)
internal/middleware/http_token_middleware.go (1)
internal/utils/ctx_utils.go (2)
WithProjectInContext(35-39)ValidateRPToken(50-63)
internal/utils/utils_test.go (2)
internal/utils/utils.go (1)
ProcessAttributeKeys(189-217)internal/utils/ctx_utils.go (1)
GetProjectFromContext(42-46)
internal/http/http_server.go (5)
internal/analytics/analytics.go (2)
Analytics(101-115)NewAnalytics(124-173)internal/middleware/middleware.go (1)
QueryParamsMiddleware(9-28)internal/mcp_handlers/launches.go (1)
RegisterLaunchTools(498-513)internal/mcp_handlers/items.go (1)
RegisterTestItemTools(803-818)internal/middleware/http_token_middleware.go (1)
HTTPTokenMiddleware(22-64)
cmd/main.go (1)
internal/config/cli.go (2)
AppVersion(26-30)RunApp(356-423)
internal/middleware/http_token_middleware_test.go (1)
internal/utils/ctx_utils.go (3)
WithProjectInContext(35-39)GetProjectFromContext(42-46)RPProjectContextKey(16-16)
internal/config/cli.go (4)
internal/http/http_server.go (2)
HTTPServerConfig(48-60)CreateHTTPServerWithMiddleware(209-235)internal/analytics/analytics.go (3)
GetAnalyticArg(329-343)Analytics(101-115)StopAnalytics(569-578)internal/mcp_handlers/server.go (1)
NewServer(21-69)internal/utils/ctx_utils.go (1)
WithProjectInContext(35-39)
internal/mcp_handlers/integration_project_test.go (2)
internal/middleware/http_token_middleware.go (1)
HTTPTokenMiddleware(22-64)internal/utils/utils.go (1)
ExtractProject(87-99)
internal/analytics/analytics.go (1)
internal/middleware/http_token_middleware.go (1)
GetTokenFromContext(97-100)
internal/mcp_handlers/items.go (3)
internal/analytics/analytics.go (1)
Analytics(101-115)internal/utils/utils.go (20)
SetPaginationOptions(41-56)DefaultSortingForItems(22-22)ExtractProject(87-99)DefaultProviderType(25-25)DefaultFilterEqHasStats(27-27)DefaultFilterEqHasChildren(26-26)DefaultFilterInType(28-28)ProcessStartTimeFilter(164-186)ApplyPaginationOptions(59-84)ProcessAttributeKeys(189-217)ExtractResponseError(102-127)ReadResponseBody(286-295)ReadResponseBodyRaw(259-281)IsTextContent(220-240)FirstPage(18-18)DefaultPageSize(20-20)DefaultSortingForLogs(24-24)DefaultItemLogLevel(30-30)DefaultSortingForSuites(23-23)DefaultFilterInTypeSuites(29-29)internal/utils/ctx_utils.go (1)
WithQueryParams(23-26)
internal/middleware/middleware_test.go (1)
internal/utils/ctx_utils.go (1)
WithQueryParams(23-26)
🪛 YAMLlint (1.37.1)
Taskfile.yaml
[error] 115-115: too many spaces inside brackets
(brackets)
[error] 115-115: too many spaces inside brackets
(brackets)
🔇 Additional comments (35)
internal/utils/utils.go (1)
1-295: LGTM! Clean refactoring to exported API.The refactoring from
mcpreportportalpackage toutilswith properly exported CamelCase identifiers follows Go best practices. The implementation logic remains intact, and all functions are well-structured with appropriate error handling.Key strengths:
- Consistent naming conventions for exported functions and constants
- Proper error handling with context-aware messages
- Defensive programming in
IsAlreadyClosedErrorandReadResponseBodyRawto handle edge cases gracefully- Generic pagination interface
PaginatedRequest[T]provides good type safetyDockerfile (1)
17-18: LGTM! Build path updated correctly.The Dockerfile correctly references the new entrypoint at
cmd/main.gowhile maintaining the same binary output name and build flags. The change aligns with the PR's modular restructuring.debug.dockerfile (1)
19-20: LGTM! Debug build path updated correctly.The debug Dockerfile correctly references the new entrypoint at
cmd/main.gowhile preserving debug flags (-gcflags "all=-N -l") and Delve integration. Consistent with the main Dockerfile changes.README.md (1)
384-384: LGTM! Documentation updated correctly.The build command now correctly references the new entrypoint at
./cmd/main.go, aligning with the codebase restructuring.internal/mcp_handlers/launches_test.go (1)
1-1: LGTM! Package rename aligns with refactoring.The package rename from
mcpreportportaltomcp_handlersis consistent with the broader modular restructuring in this PR.internal/utils/ctx_utils_test.go (1)
1-1: LGTM! Package rename aligns with refactoring.The package rename from
mcpreportportaltoutilsis consistent with the broader modular restructuring in this PR.internal/mcp_handlers/items_test.go (1)
1-1: LGTM! Package rename is consistent with the refactoring.The package name change aligns with the PR's goal of organizing code into separate modules.
internal/middleware/middleware_test.go (1)
1-29: LGTM! Clean refactoring to use the utils package.The package rename and migration to
utils.WithQueryParamsis consistent with the PR's modularization objectives. The test maintains its functionality while properly using the extracted utility.internal/utils/utils_project_test.go (1)
1-179: LGTM! Well-structured test refactoring.The migration to use
testutil.NewMockCallToolRequestandExtractProjectWithMockcentralizes test utilities and improves maintainability. The test cases comprehensively cover various project extraction scenarios including precedence, whitespace handling, and error cases.internal/middleware/http_token_middleware_test.go (1)
1-274: LGTM! Comprehensive test coverage with proper utility usage.The migration to use
utilspackage functions for project context handling is well-executed. The test suite thoroughly covers various scenarios including header handling, whitespace trimming, combined token and project extraction, and edge cases.internal/utils/utils_test.go (1)
1-194: LGTM! Excellent test coverage and useful test helper.The test suite for
ProcessAttributeKeysis comprehensive, covering basic cases, multiple keys, mixed scenarios, whitespace handling, and edge cases. The addition ofExtractProjectWithMockas an exported test helper is a good practice for enabling cross-package test usage with consistent behavior.internal/middleware/middleware.go (1)
1-28: LGTM! Clean extraction of utility function.The refactoring to use
utils.QueryParamsFromContextproperly separates concerns and makes the code more maintainable. The middleware logic remains intact while leveraging the centralized utility package.internal/analytics/analytics_test.go (1)
1-1071: LGTM! Comprehensive analytics test suite with proper middleware integration.The test suite is exceptionally thorough, covering:
- Token handling (env var vs. Bearer header)
- User ID generation and hashing
- Concurrent metric incrementing
- Batch sending per user
- Analytics lifecycle management
- Middleware integration
The migration to use
middleware.WithTokenInContextandmiddleware.HTTPTokenMiddlewareproperly reflects the new package structure.internal/analytics/analytics.go (4)
1-22: LGTM! Proper package organization with middleware integration.The addition of the
internal/middlewareimport and package structure aligns with the refactoring objectives to separate concerns across packages.
114-114: LGTM! Proper use of atomic flag for idempotent Stop().The
stoppedfield usingint32is the correct type for atomic compare-and-swap operations to ensure thread-safe, idempotent shutdown behavior.
191-214: LGTM! Clean separation of concerns with middleware package.The use of
middleware.GetTokenFromContextproperly delegates token extraction to the middleware layer. The priority logic (env var → Bearer header → anonymous) is clear and well-documented.
369-394: LGTM! Excellent idempotent Stop() implementation.The Stop() method demonstrates proper Go concurrency patterns:
- Atomic compare-and-swap (CAS) ensures idempotency
- Safe channel closure (only once)
- Graceful shutdown with timeout
- Proper goroutine synchronization with WaitGroup
This implementation is thread-safe and handles multiple Stop() calls correctly.
internal/mcp_handlers/integration_project_test.go (1)
16-119: Good end-to-end coverage of project resolution pathThe updated tests exercise the full flow—from HTTP headers through
HTTPTokenMiddlewareintoutils.ExtractProject—including precedence rules and whitespace handling. This gives solid confidence that the new context helpers and extraction logic behave as intended.Also applies to: 121-162
internal/utils/ctx_utils.go (1)
14-17: Context and token helpers look solidThe new
RPProjectContextKeyplusWithProjectInContext/GetProjectFromContextcorrectly encapsulate project handling in context, including trimming and treating whitespace-only values as absent. MovingValidateRPTokento useuuid.Validatewith a length-based fallback is also a good balance between strictness and compatibility.Also applies to: 34-46, 48-63
internal/middleware/http_token_middleware.go (1)
12-19: Middleware correctly delegates to shared utils and narrows context scopeUsing a local
contextKey+RPTokenContextKeykeeps token context concerns encapsulated in this package, while delegating project handling and token validation toutils.WithProjectInContextandutils.ValidateRPTokencentralizes the shared logic. The overall flow (extract → validate → enrich context → log) looks clean and security-conscious.Also applies to: 21-63, 66-89
internal/http/http_server.go (1)
145-162: HTTP server tool + middleware wiring looks coherent
initializeToolsandsetupRoutescombine nicely:
gorp.NewClientuses the sharedhttpClient.middleware.QueryParamsMiddlewareinjects token and query params per request.- Launch and test-item tools are registered via
mcp_handlers.RegisterLaunchTools/RegisterTestItemTools.- HTTP routing groups MCP endpoints under chi with
HTTPTokenMiddlewareand yourmcpMiddleware.This keeps HTTP concerns (routing, throttling, CORS, timeout) separate from MCP/tool wiring while still allowing per-request auth and project context.
Also applies to: 320-351
internal/mcp_handlers/launches.go (1)
43-53: Nice consolidation of launch tooling via utils andRegisterLaunchToolsThe launch tools now lean heavily on shared helpers in
utils:
- Pagination defaults and application (
SetPaginationOptions,ApplyPaginationOptions,DefaultSortingForLaunches).- Project resolution via
utils.ExtractProject.- Time-window, attribute, and response handling via
ProcessStartTimeFilter,ProcessAttributeKeys,ExtractResponseError, andReadResponseBody.
RegisterLaunchToolsalso centralizes the registration of all launch-related tools and the resource template, keepingserver.goand the HTTP server free of low-level details.This structure should make it much easier to maintain launch behavior and to reuse the same tooling in both HTTP and stdio modes.
Also applies to: 115-164, 221-237, 497-513
internal/mcp_handlers/items.go (4)
1-20: LGTM: Clean package structure and importsThe package rename to
mcp_handlersand the introduction of separateinternal/analyticsandinternal/utilspackages improve modularity and separation of concerns.
22-42: LGTM: Clean refactoring of analytics integrationThe constructor correctly uses the extracted
analytics.Analyticstype and clearly names the parameteranalyticsClientfor readability.
45-800: LGTM: Consistent refactoring to utils packageAll tool handlers have been successfully refactored to use the centralized
utilspackage functions. The error handling, pagination, filtering, and response reading logic is now consistent across all handlers.
802-818: LGTM: Clean tool registration patternThe
RegisterTestItemToolsfunction provides a clean, modular way to wire up all test item-related tools and resources with the MCP server. This pattern improves maintainability and makes the registration process explicit.internal/config/cli.go (9)
25-30: LGTM: Clean version information structureThe
AppVersionstruct provides a clear, type-safe way to pass build-time version information through the application. This is a good practice for versioned applications.
32-116: LGTM: Well-organized flag definitionsThe separation of common, HTTP-specific, and stdio-specific flags is clean and maintainable. The usage strings with mode prefixes (
[GLOBAL/REQUIRED],[HTTP-ONLY], etc.) provide excellent clarity for users.
129-148: LGTM: Proper logger initializationThe logger initialization follows Go best practices by writing to stderr and allowing configurable log levels. The use of a CLI
Beforefunction ensures logging is set up before command execution.
150-192: LGTM: Well-designed configuration builderThe function correctly:
- Auto-detects
maxWorkersbased on CPU count when not specified- Validates the host URL
- Properly constructs the version string from
AppVersion- Ensures
FallbackRPTokenis empty for HTTP mode (as documented)
194-226: LGTM: Clean server creation logicThe function properly:
- Extracts all required parameters from CLI flags
- Validates the host URL
- Delegates to
mcp_handlers.NewServerwith correct parameters- Handles errors appropriately
228-248: LGTM: Robust error handlingThe function correctly distinguishes between graceful shutdowns and actual errors by checking for expected shutdown signals (
http.ErrServerClosed,context.Canceled,context.DeadlineExceeded). Analytics cleanup is properly handled in all cases.
250-291: LGTM: Well-structured stdio server runnerThe function:
- Validates required token for stdio mode
- Properly uses context for project information
- Handles server lifecycle with goroutine and error channel
- Correctly manages analytics cleanup on shutdown
293-353: LGTM: Secure and robust HTTP server implementationThe function demonstrates good practices:
- Configures appropriate HTTP timeouts (including
ReadHeaderTimeoutto prevent Slowloris attacks)- Uses the Chi router directly with middleware
- Implements graceful shutdown with a 5-second timeout
- Properly stops analytics on shutdown
355-423: LGTM: Comprehensive CLI application runnerThe function provides:
- Mode-aware flag composition (common + mode-specific)
- Extensive, well-organized help documentation covering environment variables, flag categories, authentication, and analytics
- Clear mode dispatching with sensible default to stdio
- Proper integration with the version information
This is a well-designed entry point for the refactored application.
AleksandrParamonoff
force-pushed
the
refactoring-detach-to-modules
branch
from
dcde972 to
b0f2865
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (7)
internal/mcp_handlers/items_test.go (1)
151-162: Simplify withstrings.Contains.The custom
containshelper unnecessarily reimplementsstrings.Containswith a convoluted IIFE pattern. The standard library function is clearer, more maintainable, and equally performant.Apply this diff:
+import ( + "strings" + "testing" +) + // Helper function to check if a string contains a substring func contains(s, substr string) bool { - return len(s) >= len(substr) && (s == substr || len(substr) == 0 || - func() bool { - for i := 0; i <= len(s)-len(substr); i++ { - if s[i:i+len(substr)] == substr { - return true - } - } - return false - }()) + return strings.Contains(s, substr) }Or simply inline
strings.Containsat the call sites (line 145) and remove the helper entirely.internal/http/http_server_test.go (1)
335-361: Add defer cleanup for analytics instance to prevent goroutine leaks in testsTwo concerns in the test:
- Goroutine cleanup – The inline
analytics.NewAnalytics()creates a background ticker goroutine (started in the factory). Without callingStop(), this goroutine persists after the test completes, leaking across your test suite. TheAnalytics.Stop()method is already implemented with proper idempotent semantics (atomic flag). Usedefer tt.analytics.Stop()as suggested:for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { + if tt.analytics != nil { + defer tt.analytics.Stop() + }
- Interval constant fragility –
batchSendIntervalis defined separately in two places:internal/http/http_server.goandinternal/analytics/analytics.go. The test currently uses thehttppackage's version (line 28), while production analytics code uses its own (line 36 in analytics.go). Both are10 * time.Secondnow, but this coupling is fragile. If someone updates the interval ininternal/analytics, the test won't catch the drift. Consider exporting a helper from the analytics package or wiring the test to use the production constant directly.internal/config/cli.go (2)
150-192: HTTP server config construction is correct; consider stricter URL validation
BuildHTTPServerConfigcleanly applies defaults for workers/timeouts and computes a version string fromAppVersion. One small robustness improvement would be to rejectrp-hostvalues that parse without a scheme/host (e.g.,url.Parse("example.com")succeeds but gives only aPath):hostURL, err := url.Parse(host) if err != nil || hostURL.Scheme == "" || hostURL.Host == "" { return httpserver.HTTPServerConfig{}, fmt.Errorf("invalid host URL (expected scheme and host): %w", err) }This prevents subtle misconfiguration where the URL parses but is unusable downstream.
293-353: HTTP streaming runner is well-structured; mirror MCP stop on error for symmetryThe streaming runner correctly:
- Builds
HTTPServerConfigviaBuildHTTPServerConfig.- Creates the HTTP/MCP server with middleware.
- Starts the MCP server before
ListenAndServe.- Uses timeouts on
http.Serverand a buffered error channel.- Performs graceful shutdown (analytics stop,
server.Shutdown,httpServer.MCP.Stop) onctx.Done().In the error path (
case err := <-errC), you delegate toHandleServerError, which stops analytics but does not attempt an MCP shutdown. Consider adding a best-efforthttpServer.MCP.Stop()in that path as well for symmetry, unlessCreateHTTPServerWithMiddlewarealready guarantees no resources remain afterListenAndServefails.internal/utils/utils.go (3)
40-84: Pagination helpers look correct; consider clamping to sane minimums
SetPaginationOptions/ApplyPaginationOptionscorrectly wire MCP parameters and guard against values overmath.MaxInt32before casting toint32. One additional safety improvement would be to clamp to minimum sensible values (e.g., page ≥ 1, page-size ≥ 1) to avoid negative or zero values from unexpected client input:pageInt := request.GetInt("page", FirstPage) if pageInt < FirstPage { pageInt = FirstPage } if pageInt > math.MaxInt32 { pageInt = math.MaxInt32 } pageSizeInt := request.GetInt("page-size", DefaultPageSize) if pageSizeInt < 1 { pageSizeInt = 1 } if pageSizeInt > math.MaxInt32 { pageSizeInt = math.MaxInt32 }This keeps downstream APIs from seeing nonsense pagination values.
129-161: Clarify timestamp units and accepted formats inParseTimestampToEpochThe function sensibly tries integer epoch, then RFC3339, then a few common layouts, and always returns milliseconds. Given the multiple accepted formats and the “seconds vs milliseconds” heuristic, consider tightening the doc comment to explicitly state:
- The accepted string formats (Unix seconds, RFC3339,
YYYY-MM-DD, etc.).- That the return value is always Unix epoch in milliseconds.
This will help callers avoid double-converting or misinterpreting values.
101-127: Add nil guard toExtractResponseErrorto prevent panic on nil errorThe review is valid. While all current call sites in the codebase check
if err != nilbefore callingExtractResponseError, the function is exported and could be called with a nil error by external code or future callers. Callingerr.Error()on a nilerrorinterface will panic, making this a realistic robustness issue for a public utility function.Add the nil check as suggested:
func ExtractResponseError(err error, rs *http.Response) (errText string) { - errText = err.Error() + if err == nil { + errText = "request failed" + } else { + errText = err.Error() + }This follows Go best practices for exported error-handling helpers—defensive guards prevent panics and make the function more usable across different code paths.
♻️ Duplicate comments (4)
internal/mcp_handlers/server.go (1)
42-57: Analytics initialization properly implements best-effort approach.The analytics initialization now follows a best-effort pattern consistent with the HTTP server behavior. When initialization fails, it logs a warning and continues server startup rather than failing completely. This addresses the previous review feedback about alignment with HTTP server behavior.
internal/testutil/mock.go (1)
1-56: Mock implementation is functional for current needs.The
MockCallToolRequestprovides basic functionality for testing project extraction. The implementation is straightforward and serves its current purpose.As noted in the previous review, if broader test coverage is needed in the future, consider making the mock more flexible with a map-based approach to support arbitrary key-value pairs and configurable error scenarios.
Taskfile.yaml (1)
115-115: Fix YAML lint bracket spacing error.The
depsdeclaration has extra spaces inside brackets, violating YAML lint rules.Apply this fix:
- deps: [ docker:build-debug ] + deps: [docker:build-debug]internal/config/cli.go (1)
118-127: SimplifyGetMCPModeand avoid doubleToLowerYou currently lowercase twice (
rawMcpModeandmcpMode). Behavior is correct but can be simplified and made a bit clearer with an early return:-func GetMCPMode() string { - rawMcpMode := strings.ToLower(os.Getenv("MCP_MODE")) - slog.Debug("MCP_MODE env variable is set to: " + rawMcpMode) - mcpMode := strings.ToLower(rawMcpMode) - if mcpMode == "" { - mcpMode = "stdio" - } - return mcpMode -} +func GetMCPMode() string { + mcpMode := strings.ToLower(os.Getenv("MCP_MODE")) + slog.Debug("MCP_MODE env variable is set to: " + mcpMode) + if mcpMode == "" { + return "stdio" + } + return mcpMode +}
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
⛔ Files ignored due to path filters (1)
.gitignoreis excluded by none and included by none
📒 Files selected for processing (27)
Dockerfile(1 hunks)README.md(1 hunks)Taskfile.yaml(3 hunks)cmd/main.go(1 hunks)debug.dockerfile(1 hunks)internal/analytics/analytics.go(5 hunks)internal/analytics/analytics_test.go(13 hunks)internal/config/cli.go(9 hunks)internal/http/http_server.go(11 hunks)internal/http/http_server_test.go(4 hunks)internal/mcp_handlers/integration_project_test.go(4 hunks)internal/mcp_handlers/items.go(27 hunks)internal/mcp_handlers/items_test.go(1 hunks)internal/mcp_handlers/launches.go(16 hunks)internal/mcp_handlers/launches_test.go(1 hunks)internal/mcp_handlers/server.go(4 hunks)internal/middleware/http_token_middleware.go(3 hunks)internal/middleware/http_token_middleware_test.go(7 hunks)internal/middleware/middleware.go(2 hunks)internal/middleware/middleware_test.go(2 hunks)internal/reportportal/mock_test.go(0 hunks)internal/testutil/mock.go(1 hunks)internal/utils/ctx_utils.go(3 hunks)internal/utils/ctx_utils_test.go(1 hunks)internal/utils/utils.go(14 hunks)internal/utils/utils_project_test.go(2 hunks)internal/utils/utils_test.go(4 hunks)
💤 Files with no reviewable changes (1)
- internal/reportportal/mock_test.go
🧰 Additional context used
📓 Path-based instructions (4)
**/Dockerfile*
⚙️ CodeRabbit configuration file
Focus on security best practices, image size optimization, proper layering, and multi-stage builds where appropriate.
Files:
Dockerfile
**/*.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_test.gointernal/middleware/middleware.gointernal/analytics/analytics.gointernal/mcp_handlers/items_test.gocmd/main.gointernal/http/http_server_test.gointernal/http/http_server.gointernal/utils/utils_test.gointernal/analytics/analytics_test.gointernal/middleware/middleware_test.gointernal/mcp_handlers/server.gointernal/testutil/mock.gointernal/config/cli.gointernal/mcp_handlers/integration_project_test.gointernal/middleware/http_token_middleware.gointernal/utils/ctx_utils.gointernal/mcp_handlers/launches.gointernal/utils/utils_project_test.gointernal/utils/utils.gointernal/utils/ctx_utils_test.gointernal/mcp_handlers/items.gointernal/middleware/http_token_middleware_test.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/mcp_handlers/items_test.gointernal/http/http_server_test.gointernal/utils/utils_test.gointernal/analytics/analytics_test.gointernal/middleware/middleware_test.gointernal/mcp_handlers/integration_project_test.gointernal/utils/utils_project_test.gointernal/utils/ctx_utils_test.gointernal/middleware/http_token_middleware_test.go
cmd/**
⚙️ CodeRabbit configuration file
Review CLI interface design, configuration handling, and application startup logic for robustness and usability.
Files:
cmd/main.go
🧬 Code graph analysis (16)
internal/middleware/middleware.go (1)
internal/utils/ctx_utils.go (1)
QueryParamsFromContext(28-32)
internal/analytics/analytics.go (1)
internal/middleware/http_token_middleware.go (1)
GetTokenFromContext(97-100)
cmd/main.go (1)
internal/config/cli.go (2)
AppVersion(26-30)RunApp(356-423)
internal/http/http_server_test.go (1)
internal/analytics/analytics.go (2)
Analytics(101-115)NewAnalytics(124-173)
internal/http/http_server.go (5)
internal/analytics/analytics.go (2)
Analytics(101-115)NewAnalytics(124-173)internal/middleware/middleware.go (1)
QueryParamsMiddleware(9-28)internal/mcp_handlers/launches.go (1)
RegisterLaunchTools(498-513)internal/mcp_handlers/items.go (1)
RegisterTestItemTools(803-818)internal/middleware/http_token_middleware.go (1)
HTTPTokenMiddleware(22-64)
internal/utils/utils_test.go (2)
internal/utils/utils.go (1)
ProcessAttributeKeys(189-217)internal/utils/ctx_utils.go (1)
GetProjectFromContext(42-46)
internal/analytics/analytics_test.go (1)
internal/middleware/http_token_middleware.go (2)
WithTokenInContext(92-94)HTTPTokenMiddleware(22-64)
internal/middleware/middleware_test.go (1)
internal/utils/ctx_utils.go (1)
WithQueryParams(23-26)
internal/mcp_handlers/server.go (4)
internal/analytics/analytics.go (2)
Analytics(101-115)NewAnalytics(124-173)internal/middleware/middleware.go (1)
QueryParamsMiddleware(9-28)internal/mcp_handlers/launches.go (1)
RegisterLaunchTools(498-513)internal/mcp_handlers/items.go (1)
RegisterTestItemTools(803-818)
internal/config/cli.go (4)
internal/http/http_server.go (2)
HTTPServerConfig(48-60)CreateHTTPServerWithMiddleware(209-235)internal/analytics/analytics.go (3)
GetAnalyticArg(329-343)Analytics(101-115)StopAnalytics(569-578)internal/mcp_handlers/server.go (1)
NewServer(22-76)internal/utils/ctx_utils.go (1)
WithProjectInContext(35-39)
internal/mcp_handlers/integration_project_test.go (2)
internal/middleware/http_token_middleware.go (1)
HTTPTokenMiddleware(22-64)internal/utils/utils.go (1)
ExtractProject(87-99)
internal/middleware/http_token_middleware.go (1)
internal/utils/ctx_utils.go (2)
WithProjectInContext(35-39)ValidateRPToken(50-63)
internal/mcp_handlers/launches.go (3)
internal/analytics/analytics.go (1)
Analytics(101-115)internal/utils/utils.go (8)
SetPaginationOptions(41-56)DefaultSortingForLaunches(21-21)ExtractProject(87-99)ProcessStartTimeFilter(164-186)ApplyPaginationOptions(59-84)ProcessAttributeKeys(189-217)ExtractResponseError(102-127)ReadResponseBody(286-295)internal/utils/ctx_utils.go (1)
WithQueryParams(23-26)
internal/utils/utils_project_test.go (2)
internal/testutil/mock.go (1)
NewMockCallToolRequest(13-15)internal/utils/utils_test.go (1)
ExtractProjectWithMock(179-194)
internal/mcp_handlers/items.go (3)
internal/analytics/analytics.go (1)
Analytics(101-115)internal/utils/utils.go (20)
SetPaginationOptions(41-56)DefaultSortingForItems(22-22)ExtractProject(87-99)DefaultProviderType(25-25)DefaultFilterEqHasStats(27-27)DefaultFilterEqHasChildren(26-26)DefaultFilterInType(28-28)ProcessStartTimeFilter(164-186)ApplyPaginationOptions(59-84)ProcessAttributeKeys(189-217)ExtractResponseError(102-127)ReadResponseBody(286-295)ReadResponseBodyRaw(259-281)IsTextContent(220-240)FirstPage(18-18)DefaultPageSize(20-20)DefaultSortingForLogs(24-24)DefaultItemLogLevel(30-30)DefaultSortingForSuites(23-23)DefaultFilterInTypeSuites(29-29)internal/utils/ctx_utils.go (1)
WithQueryParams(23-26)
internal/middleware/http_token_middleware_test.go (1)
internal/utils/ctx_utils.go (3)
WithProjectInContext(35-39)GetProjectFromContext(42-46)RPProjectContextKey(16-16)
🪛 YAMLlint (1.37.1)
Taskfile.yaml
[error] 115-115: too many spaces inside brackets
(brackets)
[error] 115-115: too many spaces inside brackets
(brackets)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Agent
🔇 Additional comments (54)
internal/middleware/middleware_test.go (1)
1-29: LGTM! Clean refactoring to use the new utils package.The test correctly adopts the new package structure and uses the public
utils.WithQueryParamsAPI. The test logic remains unchanged and appropriate.internal/utils/ctx_utils.go (1)
14-46: LGTM! Well-designed context utilities with defensive checks.The new project context helpers are well-implemented:
RPProjectContextKeyappropriately usesnolint:gosecfor a context key constantWithProjectInContextdefensively trims whitespace before storingGetProjectFromContextcorrectly returnsfalsenot only when the value is missing but also when it's empty after trimming, preventing empty-string pollution in downstream codeThe defensive trimming in both directions ensures consistent handling of whitespace.
Dockerfile (1)
17-18: LGTM! Entrypoint consolidation is correct.The build command now targets the consolidated
cmd/main.goentrypoint while preserving all build flags and the output binary name. This aligns with the PR's refactoring objectives.README.md (1)
384-384: LGTM! Documentation updated to match the new entrypoint.The build instruction correctly references
./cmd/main.go, consistent with the Dockerfile changes and the PR's consolidation of the application entrypoint.internal/mcp_handlers/items_test.go (1)
1-1: LGTM! Package rename aligns with refactoring.The package declaration correctly updates to
mcp_handlersas part of the broader reorganization.internal/mcp_handlers/launches_test.go (1)
1-1: LGTM! Package rename aligns with refactoring.The package declaration correctly updates to
mcp_handlersas part of the broader codebase reorganization.internal/utils/ctx_utils_test.go (1)
1-1: LGTM! Package rename aligns with refactoring.The package declaration correctly updates to
utilsas part of the broader reorganization. The comprehensive test coverage for UUID validation and token validation remains intact.debug.dockerfile (1)
19-20: LGTM! Debug build updated for new entrypoint.The debug build correctly targets
cmd/main.gowhile preserving all debug compiler flags (-gcflags "all=-N -l"). This is consistent with the regular Dockerfile changes.cmd/main.go (1)
19-37: Signal-aware main entrypoint looks solidUsing
signal.NotifyContextand passing the derived context intoconfig.RunAppis idiomatic and gives you clean cancellation on SIGINT/SIGTERM; the error path logging + explicit exit code are also appropriate here. No changes needed from a correctness or context-handling standpoint.internal/analytics/analytics_test.go (1)
525-653: Good reuse of shared middleware for token-in-context behaviorSwitching to
middleware.WithTokenInContextandmiddleware.HTTPTokenMiddlewarein these tests keeps the analytics behavior aligned with the real HTTP/middleware stack and exercises the exact token→context→analytics flow (env var vs bearer vs anonymous) very thoroughly. This is a solid improvement in realism and coverage.internal/middleware/http_token_middleware_test.go (1)
75-144: Project context tests correctly target the shared utils helpersUpdating these tests to use
utils.WithProjectInContext,utils.GetProjectFromContext, andutils.RPProjectContextKeyensures they exercise the same context machinery as production code. The table-driven cases around empty/whitespace and missing keys give good confidence in project extraction behavior.internal/utils/utils_project_test.go (1)
165-179: Mock request wiring for project extraction is appropriateUsing
testutil.NewMockCallToolRequestand passing&requestintoExtractProjectWithMockfits the interface-based helper and keeps the tests decoupled from the concrete mock type. The table covers all key precedence paths (request vs header vs env) and whitespace handling well.internal/middleware/middleware.go (1)
9-27: QueryParamsMiddleware correctly reuses shared query-param context helperReading query params via
utils.QueryParamsFromContextand merging intorq.URL.Query()is a clean way to keep HTTP behavior in sync with the shared context utilities while preserving any existing query string. Token propagation viaGetTokenFromContextremains straightforward and idiomatic.internal/analytics/analytics.go (1)
191-214: Idempotent Stop and shared token-context helper improve analytics robustnessRouting token extraction through
middleware.GetTokenFromContextkeeps analytics aligned with the HTTP middleware’s context contract, and the updatedgetUserIDFromContextlogic still enforces the intended precedence (env/custom ID → bearer token → anonymous).The new
stoppedflag withatomic.CompareAndSwapInt32inStopmakes shutdown idempotent and safe even under concurrent or repeated calls, while the existingstopChan/tickerDone/wgusage still ensures the background ticker goroutine exits cleanly. This is a solid upgrade for lifecycle and concurrency safety.Also applies to: 368-394
internal/mcp_handlers/integration_project_test.go (1)
82-117: ---Integration tests now mirror the real MCP + HTTP flow with verified API usage
Switching to
mcp.CallToolRequestwithParams.Arguments["project"]and running requests throughmiddleware.HTTPTokenMiddlewarebefore callingutils.ExtractProjectgives these tests much closer fidelity to the actual end-to-end flow. The mcp-go library safely supports assigning a map toParams.Argumentson zero-valued structs, so this pattern is sound. The cases around precedence (request vs header) and trimming are clear and match the ExtractProject contract well.internal/utils/utils_test.go (1)
1-174: LGTM! Comprehensive test coverage.The test suite for
ProcessAttributeKeysis well-structured with thorough edge case coverage, including whitespace handling, empty values, and complex scenarios. The addition of a performance test is a good practice.internal/middleware/http_token_middleware.go (4)
12-19: LGTM! Proper context key pattern.The introduction of a dedicated
contextKeytype follows Go best practices for avoiding context key collisions. Thenolint:goseccomment is appropriately justified.
21-64: LGTM! Clean middleware implementation.The middleware properly extracts tokens and project parameters from HTTP headers, with appropriate delegation to the utils package for context management.
66-89: LGTM! Robust token extraction with validation.The Bearer token extraction properly validates tokens using
utils.ValidateRPTokenbefore processing, with appropriate debug logging for troubleshooting.
91-113: LGTM! Clean context management helpers.The context management functions follow standard Go patterns for storing and retrieving values from context, with proper validation.
internal/mcp_handlers/server.go (2)
22-41: LGTM! Clean server initialization.The server initialization properly sets up the MCP server with the required capabilities and configures the ReportPortal client with the appropriate middleware.
59-76: LGTM! Clean tool registration.The tool registration is properly consolidated using
RegisterLaunchToolsandRegisterTestItemTools, with the analytics client correctly passed through.Taskfile.yaml (5)
18-19: LGTM! Inspector containerization variables added.The new variables properly configure the containerized inspector with a reasonable Node.js base image and the correct npx command.
53-65: LGTM! Build paths updated to new entrypoint.The build tasks correctly reference the new consolidated entrypoint at
cmd/main.go, consistent with the PR's refactoring objectives.
91-101: LGTM! Container cleanup improves development workflow.The pre-run container cleanup commands prevent port conflicts and stale containers, improving the development experience.
103-112: LGTM! Inspector containerization for HTTP mode.The inspector tasks properly run in containers for HTTP mode with correct host.docker.internal mapping for Linux compatibility.
113-121: LGTM! Consistent inspector-debug configuration.The debug inspector task follows the same containerization pattern as the regular inspector, with proper configuration for HTTP mode.
internal/http/http_server.go (6)
1-29: LGTM! Clean package structure and constants.The package declaration and constants are well-organized. The
chimiddlewarealias clearly distinguishes Chi middleware from the application's middleware package.
47-74: LGTM! Struct definitions properly updated.The server configuration and struct definitions correctly integrate the analytics package type.
106-142: LGTM! Consistent best-effort analytics initialization.The analytics initialization follows the same best-effort pattern as the stdio mode server, with proper error handling that logs warnings without preventing server startup.
144-162: LGTM! Clean tool initialization.The tool initialization properly delegates to the mcp_handlers package functions and configures middleware correctly. The empty project string is appropriate for HTTP mode where projects come from request headers.
208-235: LGTM! Consistent server creation pattern.The server creation function properly returns the analytics client for lifecycle management, consistent with the stdio mode implementation.
287-351: LGTM! Well-structured middleware stack.The Chi router setup properly layers standard middleware with custom middleware, maintaining clean separation between framework and application concerns.
internal/mcp_handlers/launches.go (4)
1-41: LGTM! Clean package structure and constructor.The
LaunchResourcesstruct and constructor properly integrate the analytics package and accept the necessary parameters for initialization.
43-165: LGTM! Clean utils integration.The tool properly uses utility functions from the utils package, maintaining clean separation of concerns and consistent error handling patterns.
167-495: LGTM! Consistent refactoring across all launch tools.All tool functions properly use utility functions from the utils package and integrate analytics consistently. The refactoring maintains proper error handling throughout.
497-513: LGTM! Well-designed tool registration function.The
RegisterLaunchToolsfunction properly consolidates all launch-related tool and resource registration, making the codebase more maintainable and consistent with theRegisterTestItemToolspattern.internal/mcp_handlers/items.go (5)
1-42: LGTM! Consistent structure with launches.The
TestItemResourcesstruct and constructor follow the same pattern asLaunchResources, maintaining consistency across the codebase.
44-243: LGTM! Thorough utils integration.The tool properly uses utility functions and constants from the utils package throughout, maintaining clean separation of concerns and consistent patterns.
245-379: LGTM! Proper handling of text and binary content.The attachment tool correctly uses
utils.IsTextContentto determine content type and handles both text and binary responses appropriately.
381-800: LGTM! Consistent refactoring across all test item tools.All tool functions properly integrate with the utils package and maintain consistent error handling and analytics patterns throughout.
802-818: LGTM! Well-designed registration function.The
RegisterTestItemToolsfunction follows the same pattern asRegisterLaunchTools, providing a clean and maintainable way to register all test item-related tools and resources.internal/config/cli.go (6)
32-104: Flag grouping and env-driven configuration look solidSplitting common vs mode-specific flags and wiring them to env vars (
RP_HOST,RP_PROJECT,LOG_LEVEL, etc.) is clear and keeps CLI ergonomics good. The “HTTP-ONLY” vs “STDIO-ONLY” usage text aligns with the behavior enforced later inRunApp/runners.
129-148: Logger init is fine; consider slightly richer error for bad log-levels
InitLoggercorrectly parses thelog-levelflag and configures a globalsloghandler. If you want to improve UX further, you could wrap the error fromUnmarshalTextwith the offending value to make CLI failures more self-explanatory, but the current behavior is already acceptable.
194-226:NewMCPServerwiring matches handler expectationsFlag extraction, URL parsing, and the call into
mcp_handlers.NewServerare consistent with the referenced server constructor (version, host URL, token, userID, project, analytics secret, analyticsOn). Error wrapping is appropriate and preserves context.
228-248: Server error handling and analytics shutdown behavior
HandleServerErrorcorrectly treatsnil,http.ErrServerClosed, and context cancellations/deadlines as graceful shutdowns and ensuresanalytics.StopAnalyticsis called in both graceful and error cases. This centralization should keep the runners simpler and avoids analytics clients leaking.
250-291: Stdio runner: goroutine + context handling are reasonable
RunStdioServervalidatestoken, injectsprojectinto the context when present, then startsListenin a goroutine and selects onctx.Done()vs an error channel. As long asserver.NewStdioServer(...).Listenrespects the passed context, this pattern gives a clean shutdown path and avoids blocking the caller. No obvious race or leak stands out here.
355-423:RunApporchestrator cleanly separates modes and flagsUsing
MCP_MODEto decide which flag set to expose, while still defaulting to stdio for unknown/empty values, gives predictable behavior and clear help text. The long description documenting env vars, categories, and auth/analytics semantics is very helpful for users.internal/utils/utils.go (6)
86-99: Project extraction priority is sensible and robust
ExtractProjectcorrectly prefers the explicitprojecttool argument, then falls back toGetProjectFromContext, and finally returns a clear error when nothing is available. This ordering matches typical expectations (explicit > ambient) and should work well with both HTTP headers and env-based defaults.
163-187: Start time filter construction is consistent and validates ranges
ProcessStartTimeFiltercorrectly:
- Requires both from/to or neither.
- Uses
ParseTimestampToEpochfor both sides.- Enforces
from < to.- Returns the comma-separated form expected by the ReportPortal API.
This is a nice, self-contained helper for building the filter string.
188-217: Attribute key post-processing logic is reasonable
ProcessAttributeKeyshandles trimming, optional:suffixing, and combining with existing attributes. The branch that strips the prefix up to:(key[colonIndex+1:]) is a little domain-specific; as long as this matches the backend’s expectations (using postfix-only keys in some modes), the implementation looks fine.
242-255:IsAlreadyClosedErrorheuristic is reasonableThe string-based checks for common “already closed” error forms are pragmatic and should effectively suppress noisy logs in both
ExtractResponseErrorandReadResponseBodyRaw. Good balance between robustness and simplicity.
257-295: HTTP response body helpers handle cleanup and logging well
ReadResponseBodyRawproperly guards against nil responses, always closes the body with a defer, and filters out “already closed” errors usingIsAlreadyClosedErrorbefore logging.ReadResponseBodythen wraps this into an MCPCallToolResult, converting I/O failures intoNewToolResultError, which is appropriate for tool-level error reporting.This centralization should reduce duplicated defer/close logic in callers.
219-240: I'll verify the actual implementation in the repository to confirm the review comment's claim about Go switch semantics.The review comment is based on a misunderstanding of Go switch semantics and should be disregarded.
Go switch statements break implicitly, and when you want to use multiple values in the same case, you use a comma-separated list. However, the code structure shown—with multiple empty case labels followed by a final case with the return statement—is also correct Go. In this pattern, the empty cases don't require explicit fallthrough; they naturally execute the code associated with the next case that contains a statement.
Go allows combining duplicated logic by separating each case value with a comma in the case clause declaration. While both approaches (comma-separated or stacked empty cases) work correctly, your current code already functions as intended: all listed media types (
application/json,application/xml, etc.) returntrue. The function is correct and requires no changes.Likely an incorrect or invalid review comment.
There was a problem hiding this comment.
Pull Request Overview
Copilot reviewed 27 out of 29 changed files in this pull request and generated 1 comment.
Comments suppressed due to low confidence (5)
internal/utils/utils_test.go:194
- The
ExtractProjectWithMockfunction is placed in a production test file (utils_test.go) and is exported, which is unusual. Test helpers should typically be in test-only files or test utility packages. Consider moving this function to theinternal/testutilpackage alongsideMockCallToolRequestsince:
- It's specifically designed to work with test mocks (
testutil.MockCallToolRequest) - It's a test helper function, not production code
- It would be more discoverable and maintainable in the testutil package
This would improve separation of concerns and make it clearer that this is a testing utility.
internal/config/cli.go:30
- The
AppVersionstruct should have documentation comments for its exported fields to explain what each field represents. For example:
// AppVersion holds build-time version information
type AppVersion struct {
Version string // Semantic version (e.g., "1.0.0")
Commit string // Git commit hash
Date string // Build date in ISO format
}This improves code documentation and makes it clear what format is expected for each field.
internal/mcp_handlers/server.go:57
- The analytics initialization logic has changed from returning an error to just logging a warning when analytics initialization fails. However, this inconsistency should be documented:
The comment at line 43-44 mentions "consistent with HTTP server behavior", but it would be helpful to explicitly document that:
- Analytics failures are non-fatal and won't prevent server startup
- The server will continue to function without analytics if initialization fails
- This is by design to ensure the server remains operational even if analytics is misconfigured
Consider updating the comment to explicitly state: "Analytics initialization failures are non-fatal - the server will continue to operate without analytics rather than failing to start."
internal/http/http_server.go:161
- The comment states "Prompts are registered by mcp_handlers package", but this is misleading. Looking at the
mcp_handlers.NewServerfunction, prompts ARE registered there for stdio mode, but in HTTP mode (this code path), the prompts are not being registered at all since we're only callingRegisterLaunchToolsandRegisterTestItemTools.
Either:
- Update the comment to clarify that prompts are not registered in HTTP mode (if that's intentional), OR
- Actually register the prompts by calling the appropriate function from mcp_handlers
This appears to be a missing feature rather than just a documentation issue - prompts should likely be available in HTTP mode as well.
internal/analytics/analytics.go:114
- The new
stoppedatomic field is properly used withatomic.CompareAndSwapInt32for idempotent shutdown, which is a good improvement. However, the field should be explicitly documented to indicate it requires atomic access. Consider adding a comment like:
stopped int32 // atomic flag: 0 = running, 1 = stopped (must use atomic operations)This makes it clear to future maintainers that this field requires atomic operations and should not be accessed directly.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
1 participant
No description provided.