Conversation
…nt tools - Added `server.go` to define the MCP server and its tools for managing projects, coverage runs, and integration tests. - Implemented various handlers for listing projects, getting project details, and managing coverage and integration runs. - Introduced prompts for summarizing project health and investigating regressions and integration failures. - Created `server_test.go` to test the registration of tools and the functionality of the list projects handler. - Added `bootstrap.go` to initialize application components and manage database connections. - Updated `config.go` to include MCP server configuration options and validation logic.
|
Warning Review limit reached
More reviews will be available in 40 minutes and 59 seconds. Learn how PR review limits work. Your organization has run out of usage credits. Purchase more in the billing tab. ⌛ How to resolve this issue?After more reviews become available, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans include higher PR review limits than trial, open-source, and free plans. In all cases, reviews become available again over time. During sustained high-volume PR review activity, CodeRabbit may temporarily slow when the next review becomes available. Please see our Fair Usage Limits Policy for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (12)
📝 WalkthroughWalkthroughThis PR adds an MCP server: new MCP stdio entrypoint, bootstrap container for app wiring, extended config with MCP settings/validation, an mcp-go adapter registering tools/resources/prompts and handlers (including optional ingest), tool/schema helpers, comprehensive tests, docs, and go.mod dependency updates. ChangesMCP Server Implementation and Integration
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (3)
internal/adapters/mcp/server.go (1)
605-605: 💤 Low valueDrop the unnecessary
fmt.Sprintf(no format args).This call has no format arguments, so
Sprintfis redundant; staticcheck flags it (S1039).♻️ Proposed change
- text += fmt.Sprintf(". Then call list_coverage_runs for recent history and list_branches if branch context is unclear. Focus on package deltas with direction down, compare against default-branch baseline behavior, and summarize likely regression hotspots.") + text += ". Then call list_coverage_runs for recent history and list_branches if branch context is unclear. Focus on package deltas with direction down, compare against default-branch baseline behavior, and summarize likely regression hotspots."🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/adapters/mcp/server.go` at line 605, The appended string uses fmt.Sprintf with no format verbs (triggering staticcheck S1039); replace the call to fmt.Sprintf in the code that updates the variable text with a plain string literal append (i.e., use text += "Then call list_coverage_runs..." or concatenate directly) so you remove the unnecessary fmt.Sprintf invocation and its import if no longer used; locate the offending call to fmt.Sprintf in internal/adapters/mcp/server.go where text is updated.internal/platform/bootstrap/bootstrap.go (1)
34-48: ⚡ Quick winWrap errors with context.
The three failure paths return the raw error, which loses the originating stage (pool creation vs. migration vs. ping) and makes startup failures harder to diagnose.
As per coding guidelines: "Wrap errors with context using
fmt.Errorf(\"...: %w\", err)".♻️ Proposed wrapping
pool, err := pgxpool.New(ctx, cfg.DatabaseURL) if err != nil { - return nil, err + return nil, fmt.Errorf("create pgx pool: %w", err) } if err := migrations.Up(ctx, cfg.DatabaseURL, cfg.MigrationsDir); err != nil { pool.Close() - return nil, err + return nil, fmt.Errorf("run migrations: %w", err) } if err := pool.Ping(ctx); err != nil { pool.Close() - return nil, err + return nil, fmt.Errorf("ping database: %w", err) }Add
"fmt"to the imports.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/platform/bootstrap/bootstrap.go` around lines 34 - 48, The error returns in New must be wrapped with contextual messages: when pgxpool.New(ctx, cfg.DatabaseURL) fails, return fmt.Errorf("create pgx pool: %w", err); when migrations.Up(ctx, cfg.DatabaseURL, cfg.MigrationsDir) fails, keep pool.Close() and return fmt.Errorf("run migrations: %w", err); when pool.Ping(ctx) fails, keep pool.Close() and return fmt.Errorf("ping database: %w", err). Add "fmt" to imports and update the error returns in New accordingly, referencing New, pgxpool.New, migrations.Up, pool.Ping, cfg.DatabaseURL and cfg.MigrationsDir.internal/platform/config/config.go (1)
70-92: ⚡ Quick winInvalid env values are silently ignored instead of failing fast.
getEnvIntfalls back to the default when the value is non-numeric or<= 0, andgetEnvBoolfalls back on any parse error. A typo likeMCP_MAX_PAGE_SIZE=abcorMCP_ENABLE_WRITE_TOOLS=yezwill be silently masked rather than surfaced at startup. Consider returning an error so misconfiguration fails fast.As per coding guidelines: "Use environment-driven config with explicit struct validation at startup; fail fast on invalid config."
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/platform/config/config.go` around lines 70 - 92, getEnvInt and getEnvBool currently swallow invalid environment values by returning defaults; change them to return (int, error) and (bool, error) respectively, validate that the env var is present if required, attempt to parse (use strconv.Atoi/ParseBool), for getEnvInt additionally enforce value > 0, and return a descriptive error containing the key and invalid value on failure; update all callers of getEnvInt/getEnvBool (the config construction/validation path) to propagate and surface these errors so startup fails fast and configuration validation can report the exact misconfigured env var.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@go.mod`:
- Line 17: The go.mod entry for github.com/mark3labs/mcp-go is incorrectly
marked as "// indirect" despite being directly imported by
internal/adapters/mcp/server.go, internal/adapters/mcp/server_test.go and
cmd/mcp/main.go; update go.mod to list github.com/mark3labs/mcp-go v0.54.1 in
the direct require block (remove the "// indirect" marker or run go mod tidy to
promote it) so the dependency is recorded as direct rather than indirect.
In `@internal/adapters/mcp/server.go`:
- Around line 459-487: The handlers handleIngestCoverageRun and
handleIngestIntegrationRun currently gate only on cfg.MCPEnableWriteTools but
never verify the configured APIKeySecret; update the MCP adapter to accept and
store an Authenticator (add an authenticator field to Adapter and change the
constructor used by mcpadapter.NewServer to pass it through) and then, at the
start of both handleIngestCoverageRun and handleIngestIntegrationRun, invoke the
authenticator to validate the incoming request's API key (reject with
application.NewUnauthenticated if authentication fails) before binding arguments
or calling the IngestCoverageRun / IngestIntegrationRun services; alternatively
remove the APIKeySecret requirement from ValidateMCP if you intend no auth for
stdio, but do not leave the current mismatch.
In `@README.md`:
- Around line 91-97: Add documentation for the MCP_LOG_LEVEL optional setting
and note the API_KEY_SECRET prerequisite for enabling write tools: update the
"Optional MCP settings" block to include `MCP_LOG_LEVEL` (describe default and
purpose) and augment the `MCP_ENABLE_WRITE_TOOLS` entry to explicitly state that
write mode requires `API_KEY_SECRET` to be configured (and mention expected
format/where to set it), referencing the exact config names `MCP_LOG_LEVEL`,
`MCP_ENABLE_WRITE_TOOLS`, and `API_KEY_SECRET` so users see the dependency and
avoid startup failures.
---
Nitpick comments:
In `@internal/adapters/mcp/server.go`:
- Line 605: The appended string uses fmt.Sprintf with no format verbs
(triggering staticcheck S1039); replace the call to fmt.Sprintf in the code that
updates the variable text with a plain string literal append (i.e., use text +=
"Then call list_coverage_runs..." or concatenate directly) so you remove the
unnecessary fmt.Sprintf invocation and its import if no longer used; locate the
offending call to fmt.Sprintf in internal/adapters/mcp/server.go where text is
updated.
In `@internal/platform/bootstrap/bootstrap.go`:
- Around line 34-48: The error returns in New must be wrapped with contextual
messages: when pgxpool.New(ctx, cfg.DatabaseURL) fails, return
fmt.Errorf("create pgx pool: %w", err); when migrations.Up(ctx, cfg.DatabaseURL,
cfg.MigrationsDir) fails, keep pool.Close() and return fmt.Errorf("run
migrations: %w", err); when pool.Ping(ctx) fails, keep pool.Close() and return
fmt.Errorf("ping database: %w", err). Add "fmt" to imports and update the error
returns in New accordingly, referencing New, pgxpool.New, migrations.Up,
pool.Ping, cfg.DatabaseURL and cfg.MigrationsDir.
In `@internal/platform/config/config.go`:
- Around line 70-92: getEnvInt and getEnvBool currently swallow invalid
environment values by returning defaults; change them to return (int, error) and
(bool, error) respectively, validate that the env var is present if required,
attempt to parse (use strconv.Atoi/ParseBool), for getEnvInt additionally
enforce value > 0, and return a descriptive error containing the key and invalid
value on failure; update all callers of getEnvInt/getEnvBool (the config
construction/validation path) to propagate and surface these errors so startup
fails fast and configuration validation can report the exact misconfigured env
var.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 3e7a5978-a02a-478b-84e7-2f0d7900ddfb
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
📒 Files selected for processing (9)
README.mdcmd/api/main.gocmd/mcp/main.gogo.modinternal/adapters/mcp/server.gointernal/adapters/mcp/server_test.gointernal/platform/bootstrap/bootstrap.gointernal/platform/config/config.gospecs/MCP_SERVER.md
There was a problem hiding this comment.
Pull request overview
Adds a new MCP server adapter that exposes OpenCoverage project, coverage, contributor, and integration-test data through MCP tools/resources/prompts, while refactoring shared application wiring into a reusable bootstrap package.
Changes:
- Added MCP server entrypoint, adapter, tests, specification, and README startup instructions.
- Introduced MCP-related configuration and validation.
- Refactored API startup wiring to use shared bootstrap initialization.
Reviewed changes
Copilot reviewed 9 out of 10 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
specs/MCP_SERVER.md |
Defines the intended MCP server architecture, tools, resources, prompts, config, and rollout plan. |
README.md |
Documents local MCP server startup and MCP environment variables. |
internal/platform/config/config.go |
Adds MCP config fields, parsing helpers, and MCP validation. |
internal/platform/bootstrap/bootstrap.go |
Centralizes DB, migrations, repositories, auth, and use-case wiring. |
internal/adapters/mcp/server.go |
Implements MCP tools, resources, prompts, error mapping, and write-tool auth. |
internal/adapters/mcp/server_test.go |
Adds unit tests for tool registration, list projects, error mapping, and write auth. |
cmd/mcp/main.go |
Adds the MCP server executable using stdio transport. |
cmd/api/main.go |
Refactors API startup to use shared bootstrap wiring. |
go.mod |
Adds the MCP SDK dependency and indirect dependencies. |
go.sum |
Updates dependency checksums for the new MCP dependency set. |
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 (1)
internal/platform/config/config.go (1)
72-93:⚠️ Potential issue | 🟠 Major | ⚡ Quick winFail fast on malformed env vars instead of defaulting them.
These helpers currently turn invalid values into defaults, so bad startup config is silently accepted. For example, an invalid
MCP_MAX_PAGE_SIZEorMCP_ENABLE_WRITE_TOOLSvalue will boot with fallback settings instead of surfacing a configuration error.As per coding guidelines, "Use environment-driven config with explicit struct validation at startup; fail fast on invalid config".
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/platform/config/config.go` around lines 72 - 93, getEnvInt and getEnvBool silently fall back to defaults on malformed values; change them to fail fast by returning an error when an env var is present but cannot be parsed (e.g., non-integer for getEnvInt or non-bool for getEnvBool) instead of swallowing the error. Update signatures (getEnvInt -> (int, error), getEnvBool -> (bool, error)), have them return defaultValue with nil error only when the env var is absent, and return a descriptive parsing error when the env var is present but invalid; then propagate these errors up to the config construction/validation code so startup can log the error and exit (update all callers of getEnvInt/getEnvBool accordingly).
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@internal/adapters/mcp/handlers.go`:
- Around line 19-22: Validate and sanitize numeric query params at the handler
boundary before calling service methods: ensure values from request.GetInt for
"page", "pageSize", "limit", and "runsPerProject" are clamped to acceptable
ranges (e.g., page >= 1, pageSize between 1 and your max, limit and
runsPerProject >= 1 and <= sensible max) and reject or default invalid inputs;
update the call sites that pass these raw ints into application methods (e.g.,
ListProjects.Execute) to use the sanitized/clamped values and reuse existing
helpers like normalizePageSize or add small validators in the same handler file
to centralize checks.
- Around line 232-236: After decoding with bindPayloadOrArguments for
application.IngestCoverageRunInput, explicitly validate that all required fields
are present and sane before calling a.services.IngestCoverageRun.Execute; add a
validation step (e.g., call an existing validate function or implement
Validate/struct-tag based checks) immediately after bindPayloadOrArguments and
return a toolErrorResult(application.NewInvalidArgument(...)) on validation
failure. Apply the same pattern to the other handler block around lines 251-259
so that both bindPayloadOrArguments -> validation -> Execute flows enforce the
advertised ingest contract. Ensure the validation references the concrete type
application.IngestCoverageRunInput (and any other input structs used in the
second block) and returns clear error details on missing/invalid fields.
---
Outside diff comments:
In `@internal/platform/config/config.go`:
- Around line 72-93: getEnvInt and getEnvBool silently fall back to defaults on
malformed values; change them to fail fast by returning an error when an env var
is present but cannot be parsed (e.g., non-integer for getEnvInt or non-bool for
getEnvBool) instead of swallowing the error. Update signatures (getEnvInt ->
(int, error), getEnvBool -> (bool, error)), have them return defaultValue with
nil error only when the env var is absent, and return a descriptive parsing
error when the env var is present but invalid; then propagate these errors up to
the config construction/validation code so startup can log the error and exit
(update all callers of getEnvInt/getEnvBool accordingly).
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 793b712c-74a5-4240-b345-8d64ce907a0f
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
📒 Files selected for processing (9)
README.mdcmd/mcp/main.gogo.modinternal/adapters/mcp/handlers.gointernal/adapters/mcp/handlers_test.gointernal/adapters/mcp/server.gointernal/adapters/mcp/server_test.gointernal/adapters/mcp/tool_schemas.gointernal/platform/config/config.go
🚧 Files skipped from review as they are similar to previous changes (2)
- README.md
- cmd/mcp/main.go
…ies for ginkgo report and failure details
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
internal/adapters/mcp/ingest_validation.go (1)
11-42: ⚡ Quick winExtract shared ingest field checks to a helper.
The repeated top-level validation block in both functions is already long and can drift. A small shared validator for
projectKey/branch/commitSha/triggerType/runTimestampwill keep behavior aligned.As per coding guidelines, "Keep functions short and intention-revealing".
Also applies to: 44-86
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/adapters/mcp/ingest_validation.go` around lines 11 - 42, Extract the repeated top-level checks (ProjectKey, Branch, CommitSHA, TriggerType, RunTimestamp RFC3339 parse) into a helper function (e.g., validateCommonIngestFields) and call it from validateCoverageIngestInput and the other ingest validator (validateTraceIngestInput) to remove duplication; the helper should perform the same application.NewInvalidArgument returns and time.Parse(time.RFC3339, strings.TrimSpace(in.RunTimestamp)) check, and keep the same error messages/field maps so behavior is unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@internal/adapters/mcp/ingest_validation.go`:
- Around line 69-84: When state is "failed" validate nested failure.location
fields at runtime: inside the loop over in.GinkgoReport.SpecReports (the block
referencing spec.Failure), ensure spec.Failure.Location is non-nil,
spec.Failure.Location.FileName is non-empty after TrimSpace, and
spec.Failure.Location.LineNumber is a sensible integer (e.g., >= 0); return
application.NewInvalidArgument with a clear field path like
"ginkgoReport.specReports[%d].failure.location.fileName" or "...lineNumber" if
those checks fail so malformed nested failure.location data can't slip through
schema-only validation.
---
Nitpick comments:
In `@internal/adapters/mcp/ingest_validation.go`:
- Around line 11-42: Extract the repeated top-level checks (ProjectKey, Branch,
CommitSHA, TriggerType, RunTimestamp RFC3339 parse) into a helper function
(e.g., validateCommonIngestFields) and call it from validateCoverageIngestInput
and the other ingest validator (validateTraceIngestInput) to remove duplication;
the helper should perform the same application.NewInvalidArgument returns and
time.Parse(time.RFC3339, strings.TrimSpace(in.RunTimestamp)) check, and keep the
same error messages/field maps so behavior is unchanged.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 47b80429-a738-4790-a8f3-71e4a9b9d219
📒 Files selected for processing (6)
internal/adapters/mcp/handlers.gointernal/adapters/mcp/handlers_test.gointernal/adapters/mcp/ingest_validation.gointernal/adapters/mcp/server.gointernal/adapters/mcp/server_test.gointernal/adapters/mcp/tool_schemas.go
🚧 Files skipped from review as they are similar to previous changes (4)
- internal/adapters/mcp/handlers_test.go
- internal/adapters/mcp/server_test.go
- internal/adapters/mcp/server.go
- internal/adapters/mcp/handlers.go
… use new validation method
…level validation and add unit tests
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary by CodeRabbit
New Features
Documentation
Tests