Conversation
|
Warning Review limit reached
Your plan includes 1 review of capacity. Refill in 21 minutes and 45 seconds. Your organization has run out of usage credits. Purchase more in the billing tab. ⌛ How to resolve this issue?After more review capacity refills, 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 have higher rate limits than trial, open-source, and free plans. In all cases, review capacity refills continuously over time. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (38)
📝 WalkthroughWalkthroughAdds a Model Context Protocol server to https-wrench with embedded schema/assets, MCP resources/prompts, tools for validation/template/CLI building and execution (requests, certinfo, jwtinfo, JWKS), a new ChangesMCP Server Feature
🎯 4 (Complex) | ⏱️ ~60 minutes Possibly Related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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: 8
🧹 Nitpick comments (1)
internal/mcp/embed.go (1)
29-30: ⚡ Quick winAvoid pointing
$schemato movingmainbranch.Using
refs/heads/mainmakes editor validation drift over time relative to the running binary. Prefer a versioned/tagged schema URL (or serve the MCP schema URI directly in generated examples) so validation remains stable.🤖 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/mcp/embed.go` around lines 29 - 30, schemaCommentHeader currently points the $schema URL to a moving ref ("refs/heads/main"); change it to a stable, versioned or tagged schema URL (or to the MCP schema URI you serve) so editor validation won't drift. Update the const schemaCommentHeader to use a released tag or versioned path (or the generated MCP schema endpoint) instead of "refs/heads/main" so the schema URL is immutable and matches the binary's expected schema.
🤖 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 `@https-wrench.schema.json`:
- Around line 58-60: The JSON schema currently declares "clientTimeout" as
"type": "number" but runtime expects an integer (RequestConfig.ClientTimeout in
internal/requests/requests.go); change the schema for the "clientTimeout"
property to use "type": "integer" (and optionally add a "minimum": 0 if
non-negative is required) so validation matches the runtime contract and
prevents float values from being accepted.
In `@internal/mcp/assets/examples/https-wrench-proxyProtocolV2.yaml`:
- Line 14: Typo in the example request name: rename the example's name value
from "RequestOverProxyPtorocolV2" to "RequestOverProxyProtocolV2" (fixing
"Ptorocol" -> "Protocol") and update any references to this symbol in the same
YAML/example outputs so they remain consistent (look for the name key and any
example identifiers that reference RequestOverProxyPtorocolV2).
In `@internal/mcp/server_test.go`:
- Line 277: The current code does an unchecked type assertion on
res.Messages[0].Content to *sdkmcp.TextContent which can panic; change it to a
two-value assertion (e.g., content, ok :=
res.Messages[0].Content.(*sdkmcp.TextContent)) and if !ok call the test helper
(t.Fatalf or t.Fatalf-like) with a clear message including the actual type,
otherwise extract text := content.Text and continue; update any nearby uses of
the variable `text` accordingly.
In `@internal/mcp/tools_exec.go`:
- Around line 411-443: captureWithStdout is unsafe because it reassigns the
global os.Stdout (process-wide) and drains the read end only after fn returns,
which can deadlock; to fix, serialize stdout capture by introducing a
package-level mutex (e.g., stdoutMu) and protect the section that sets os.Stdout
and restores it, run fn while concurrently draining pipeR into stdoutBuf (start
a goroutine that io.Copy into stdoutBuf before calling fn), ensure pipeW is
closed after fn returns, wait for the copy goroutine to finish (e.g.,
sync.WaitGroup or channel), then restore os.Stdout and merge stdoutBuf and
writerBuf into combined as before; reference captureWithStdout, fn, pipeR/pipeW,
os.Stdout, stdoutBuf, writerBuf, and combined to locate the changes.
- Around line 167-200: The functions executeRunRequests and executeCertinfo
ignore the passed context (they use _), so timeouts/cancellations aren’t
enforced; change each function to use the provided ctx and run the long-running
work under a context-aware wrapper (e.g., implement or use the suggested
runWithContext generic helper) so captureWithStdout and requests.HandleRequests
(or any long-running cert ops) execute inside runWithContext(ctx, func() (T,
error) { ... }); update the calls where you currently do
captureWithStdout(func(w io.Writer) error { ... }) to run that body inside the
runWithContext wrapper and return ctx.Err() on cancellation, and ensure
requests.HandleRequests or the cert handlers receive a context if they support
it (or are cancelled by the wrapper) so timeouts are properly enforced.
In `@internal/mcp/tools.go`:
- Around line 242-258: The template emits raw string scalars (variables name,
method, transport, hostname, and each path p) directly into YAML which can break
parsing for values containing special characters; update the writers in
internal/mcp/tools.go that call fmt.Fprintf/Fprintln for "name",
"requestMethod", "transportOverrideUrl", "hosts"->"name", and each "uriList"
entry to emit quoted, escaped YAML-safe scalars (e.g. use strconv.Quote or a
YAML marshaler to produce a quoted string) instead of raw %s so all interpolated
values are safely quoted/escaped; leave the boolean insecure emission unchanged.
In `@internal/requests/requests_handlers_print_test.go`:
- Around line 17-18: TestPrintResponseData (and the other test at 60-76) is
unsafely mutating global os.Stdout while running t.Parallel(), causing flaky
tests; fix it by removing the global stdout swap or by refactoring to avoid
global mutation: either (A) remove t.Parallel() from TestPrintResponseData and
the other stdout-swapping test so they run serially, or (B, preferred) change
the code under test (PrintResponseData) to accept an io.Writer (or add an
overload/helper that accepts io.Writer) and update the tests to pass a
bytes.Buffer (or io.Pipe) to capture output instead of reassigning os.Stdout;
ensure any temporary stdout replacement is eliminated so no global state is
mutated during parallel runs.
---
Nitpick comments:
In `@internal/mcp/embed.go`:
- Around line 29-30: schemaCommentHeader currently points the $schema URL to a
moving ref ("refs/heads/main"); change it to a stable, versioned or tagged
schema URL (or to the MCP schema URI you serve) so editor validation won't
drift. Update the const schemaCommentHeader to use a released tag or versioned
path (or the generated MCP schema endpoint) instead of "refs/heads/main" so the
schema URL is immutable and matches the binary's expected schema.
🪄 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
Run ID: 3ccb890c-f292-4a0b-bc69-eccf7474a6fe
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
📒 Files selected for processing (26)
README.mdassets/examples/https-wrench-k3s-anchor-and-aliases.yamlgo.modhttps-wrench.schema.jsoninternal/certinfo/certinfo_handlers.gointernal/cmd/mcp.gointernal/cmd/mcp_test.gointernal/cmd/root.gointernal/cmd/root_test.gointernal/jwtinfo/jwtinfo_test.gointernal/mcp/assets/examples/https-wrench-k3s.yamlinternal/mcp/assets/examples/https-wrench-proxyProtocolV2.yamlinternal/mcp/assets/examples/https-wrench-response-certificates-filter.yamlinternal/mcp/assets/sample-config.yamlinternal/mcp/assets/schema.jsoninternal/mcp/coverage_test.gointernal/mcp/embed.gointernal/mcp/prompts.gointernal/mcp/resources.gointernal/mcp/server.gointernal/mcp/server_test.gointernal/mcp/tools.gointernal/mcp/tools_exec.gointernal/mcp/tools_exec_test.gointernal/requests/requests_handlers_print_test.gointernal/requests/requests_test.go
| "fmt" | ||
| "io" | ||
| "maps" | ||
| "net/http" |
There was a problem hiding this comment.
Avoid external network dependency in the nil-reader test.
This test should be hermetic; using example.com makes it flaky if validation order changes and a request is attempted.
Suggested fix
import (
"bytes"
"context"
"encoding/base64"
"fmt"
"io"
"maps"
"net/http"
+ "net/http/httptest"
"os"
"strings"
"testing"
"time"
@@
func TestRequestToken_nilReadAll(t *testing.T) {
t.Parallel()
+ srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) {
+ w.WriteHeader(http.StatusOK)
+ _, _ = w.Write([]byte(`{"access_token":"x"}`))
+ }))
+ defer srv.Close()
+
_, err := RequestToken(
context.Background(),
- "http://example.com/token",
+ srv.URL,
map[string]string{"grant_type": "client_credentials"},
- &http.Client{},
+ srv.Client(),
nil,
)
require.Error(t, err)
require.ErrorContains(t, err, "nil body reader")
}Also applies to: 260-272
it is the probable cause of random test failure in Github CI
Route requests verbose output through the HandleRequests writer instead of replacing os.Stdout, and skip terminal passphrase prompts for readers that declare NoPasswordPrompt (MCP and test mocks). Co-authored-by: Cursor <cursoragent@cursor.com>
Enforce handler timeouts via runWithContext, quote generated YAML scalars, tighten clientTimeout schema to integer, fix example typo, stabilize the embedded $schema URI, and harden tests (safe type assertion, local jwt URL). Co-authored-by: Cursor <cursoragent@cursor.com>
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 (1)
internal/cmd/mcp_test.go (1)
11-33:⚠️ Potential issue | 🟠 Major | ⚡ Quick winParallel subtests still race on global
os.Args.Line 26 runs subtests in parallel while Lines 28-33 mutate
os.Args. That can cause cross-subtest interference and flaky outcomes.Suggested fix
for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { - t.Parallel() - oldArgs := os.Args t.Cleanup(func() { os.Args = oldArgs }) os.Args = tt.args require.Equal(t, tt.want, isMCPCommand()) }) }🤖 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/cmd/mcp_test.go` around lines 11 - 33, The subtests call t.Parallel() while mutating the global os.Args which causes races; fix by making the subtests run serially (remove the t.Parallel() call inside the anonymous subtest) or, if you need parallelism, serialize access to os.Args by guarding modifications/restores with a mutex around the os.Args assignment and require.Equal call; locate the anonymous subtest where t.Parallel() is invoked, the os.Args assignments and t.Cleanup, and the isMCPCommand() invocation to apply the change.
🤖 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/mcp/tools_exec.go`:
- Around line 169-176: The wrapper returns on ctx.Done() but the underlying
executors keep running because runRequestsExec and certinfoExec don't accept a
context and HTTP requests are created without one; fix by adding a context
parameter and propagating it: change runRequestsExec(...) and certinfoExec(...)
to runRequestsExec(ctx context.Context, ...) and certinfoExec(ctx
context.Context, ...), update executeRunRequests and executeCertinfo to call
runWithContext with a function that forwards the ctx into those executors, and
update internal/requests to create requests with http.NewRequestWithContext(ctx,
...) (and ensure reqClient.client.Do(req) uses that request) so in-flight HTTP
work is cancelled when the tool context is done.
---
Outside diff comments:
In `@internal/cmd/mcp_test.go`:
- Around line 11-33: The subtests call t.Parallel() while mutating the global
os.Args which causes races; fix by making the subtests run serially (remove the
t.Parallel() call inside the anonymous subtest) or, if you need parallelism,
serialize access to os.Args by guarding modifications/restores with a mutex
around the os.Args assignment and require.Equal call; locate the anonymous
subtest where t.Parallel() is invoked, the os.Args assignments and t.Cleanup,
and the isMCPCommand() invocation to apply the change.
🪄 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
Run ID: 5bc3404c-0b5f-45de-91d5-9787e4124cc8
📒 Files selected for processing (20)
.testcoverage.ymlassets/examples/https-wrench-proxyProtocolV2.yamlhttps-wrench.schema.jsoninternal/certinfo/certinfo.gointernal/certinfo/common_handlers.gointernal/certinfo/main_test.gointernal/cmd/mcp_test.gointernal/jwtinfo/jwtinfo_test.gointernal/mcp/assets/examples/https-wrench-proxyProtocolV2.yamlinternal/mcp/assets/schema.jsoninternal/mcp/coverage_test.gointernal/mcp/embed.gointernal/mcp/server_test.gointernal/mcp/tools.gointernal/mcp/tools_exec.gointernal/requests/requests.gointernal/requests/requests_handlers.gointernal/requests/requests_handlers_print_test.gointernal/requests/requests_handlers_test.gointernal/requests/requests_test.go
✅ Files skipped from review due to trivial changes (2)
- .testcoverage.yml
- assets/examples/https-wrench-proxyProtocolV2.yaml
Thread cancellation from MCP tool timeouts into HandleRequests, http.NewRequestWithContext, and certinfo DialContext handshakes. Remove parallel os.Args mutation in TestIsMCPCommand subtests. Co-authored-by: Cursor <cursoragent@cursor.com>
Owner
Author
|
@coderabbitai full review |
✅ Actions performedFull review triggered. |
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
1 participant
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
mcpCLI subcommand and shown in CLI help.Documentation
Bug Fixes