feat: MCP phase 1 read-only operations

[kfelternv]

No description provided.

[copy-pr-bot]

Auto-sync is disabled for draft pull requests in this repository. Workflows must be run manually.

Contributors can view more details about this message here.

[coderabbitai]

Important

Review skipped

Draft detected.

Please check the settings in the CodeRabbit UI or the .coderabbit.yaml file in this repository. To trigger a single review, invoke the @coderabbitai review command.

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Enterprise

Run ID: d9949502-3ae7-4e66-9af3-ac4a2cde6682

You can disable this status message by setting the reviews.review_status to false in the CodeRabbit configuration file.

Use the checkbox below for a quick retry:

• 🔍 Trigger review

✨ Finishing Touches

🧪 Generate unit tests (beta)

• Create PR with unit tests

---

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

[coderabbitai]

Actionable comments posted: 6

🧹 Nitpick comments (3)

cli/mcp/schema_test.go (1)

84-123: ⚡ Quick win

Add a regression test for {in,name} override + unique Required entries.

Current tests are strong, but they do not yet assert operation-level override behavior against path-item parameters with the same key, or enforce uniqueness in schema.Required.

Suggested test addition

+func TestBuildInputSchema_OperationParamOverridesPathItemParam(t *testing.T) {
+	item := appcli.PathItem{
+		Parameters: []appcli.Parameter{
+			{Name: "status", In: "query", Required: true, Description: "from item", Schema: &appcli.Schema{Type: "string"}},
+		},
+	}
+	op := &appcli.Operation{
+		OperationID: "get-override-case",
+		Parameters: []appcli.Parameter{
+			{Name: "status", In: "query", Required: false, Description: "from op", Schema: &appcli.Schema{Type: "string"}},
+		},
+	}
+
+	schema := buildInputSchema(item, op)
+	require.Equal(t, "from op", schema.Properties["status"].Description)
+	require.NotContains(t, schema.Required, "status")
+	require.Len(t, schema.Required, len(uniqueStrings(schema.Required)))
+}
+
+func uniqueStrings(in []string) map[string]struct{} {
+	out := make(map[string]struct{}, len(in))
+	for _, s := range in {
+		out[s] = struct{}{}
+	}
+	return out
+}

🤖 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 `@cli/mcp/schema_test.go` around lines 84 - 123, Add a regression test that
verifies operation-level parameters override path-item parameters when they
share the same (In, Name) key and that schema.Required entries are deduplicated:
construct an appcli.PathItem with a parameter (e.g., In: "path", Name: "id",
Description: "path desc") and an appcli.Operation that also declares a parameter
with the same In/Name but a different Description and that includes the
parameter in its Required list (and ensure the path-item also marks it
required), call buildInputSchema(pathItem, operation), then assert the resulting
schema.Properties["id"].Description equals the operation-level description
(ensuring override) and assert the schema.Required slice contains "id" only once
(use require.Contains/require.Equal/require.Len as appropriate). Reference
buildInputSchema, appcli.PathItem, appcli.Operation, schema.Required, and
schema.Properties to locate where to add the test.

cli/mcp/config.go (1)

128-128: 💤 Low value

Prefer idiomatic slice declaration.

Use var missing []string rather than []string{} when initializing an empty slice that will be appended to. The former is the idiomatic Go pattern and avoids an unnecessary allocation when no elements are appended.

♻️ Suggested change

 func (c resolvedConfig) requireNonEmpty() error {
-	missing := []string{}
+	var missing []string
 	if c.Org == "" {

🤖 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 `@cli/mcp/config.go` at line 128, Replace the non-idiomatic empty-slice literal
initialization for the variable named "missing" with the idiomatic zero-value
declaration; locate the declaration of missing in the config.go snippet and
change the initialization from []string{} to using the zero value (var missing
[]string) so it avoids an unnecessary allocation and follows Go conventions.

cli/mcp/transport_test.go (1)

286-292: ⚡ Quick win

Assert per-call success in the refresh loop.

The loop currently closes the body without validating result status/payload, so the test can pass on counter effects even when a tool call fails. Add StatusOK and isError=false checks per iteration.

Suggested test hardening

 for i := 0; i < 2; i++ {
 	resp := mcpPost(t, ts.URL, "", jsonrpcRequest(i+1, "tools/call", map[string]any{
 		"name":      "nico_get_foo",
 		"arguments": map[string]any{"fooId": "foo-" + itoa(i)},
 	}))
-	_ = resp.Body.Close()
+	require.Equal(t, http.StatusOK, resp.StatusCode)
+	body, err := io.ReadAll(resp.Body)
+	require.NoError(t, err)
+	_ = resp.Body.Close()
+	result := decodeToolCallResult(t, body)
+	require.False(t, result.IsError, "tool call should succeed: %s", body)
 }

🤖 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 `@cli/mcp/transport_test.go` around lines 286 - 292, The test loop sends two
tool calls using mcpPost with jsonrpcRequest but currently closes resp.Body
without verifying success; update the loop that calls mcpPost (in the for i :=
0; i < 2; i++) to assert the HTTP response is StatusOK and the JSON-RPC payload
indicates isError == false for each iteration (use the same response parsing
used elsewhere in this test), then close resp.Body after those assertions so
each call is validated individually; reference mcpPost, jsonrpcRequest, and the
per-response StatusOK/isError checks when making the changes.

🤖 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 `@cli/mcp/cmd.go`:
- Around line 146-147: LoadConfig's returned error is being ignored; change the
call to capture the error (e.g., cfg, err := appcli.LoadConfig()) and handle it
instead of discarding it — validate err and bail out (return or log/exit) with a
clear message so malformed configs don't continue, then only call
appcli.ApplyEnvOverrides(cfg) when err is nil.
- Around line 122-124: The code registers sigCh with signal.Notify but never
unregisters it; add a call to signal.Stop(sigCh) to unregister the channel when
the server/goroutine exits (either via a defer immediately after
signal.Notify(sigCh, ...) or in the existing shutdown/cleanup path) so that
repeated in-process runs don't accumulate stale signal registrations; reference
the sigCh variable and the existing signal.Notify usage and place
signal.Stop(sigCh) in the same scope where the server shutdown is handled.
- Around line 103-107: Validate the user-supplied path before registering it
with ServeMux to avoid panics: in the function creating the mux (where variables
path, shutdownTimeout, mux and call mux.Handle(path, NewHandler(server)) live),
check that path is non-empty and starts with '/' (and optionally reject patterns
containing spaces or invalid characters), and if invalid return an error or exit
with a clear message instead of calling mux.Handle; only call mux.Handle(path,
NewHandler(server)) when the validation passes.

In `@cli/mcp/schema.go`:
- Around line 50-66: The code builds allParams by appending item.Parameters then
op.Parameters which can leave path-level params duplicated when an
operation-level param overrides the same (in,name), causing incorrect required
entries; change the logic to deduplicate by the tuple (p.In, p.Name) preferring
operation-level values: build a map keyed by fmt.Sprintf("%s|%s", p.In, p.Name)
(or equivalent) and first insert path-item parameters then overwrite with
operation parameters (or insert op params last to override), then iterate the
deduplicated slice/map to populate props and required using paramToJSONSchema
and the existing checks (p.In, p.Name, p.Required), ensuring required only
reflects the final parameter set.

In `@cli/mcp/server.go`:
- Around line 161-184: The splitArgs function currently drops unsupported shapes
silently (coerceToString returning ""), which can hide bad requests; change
splitArgs to return an error (e.g., func splitArgs(...) (pathParams, queryParams
map[string]string, err error)) and when coerceToString(raw) yields an empty
string due to an unsupported type (arrays/maps/objects) return a descriptive
error mentioning the parameter name and type instead of skipping it; add a TODO
in the function noting that full OpenAPI style/explode serialization is
intentionally deferred and should be implemented later (reference splitArgs,
coerceToString, and appcli.Parameter), and apply the same fail-fast behavior to
the other analogous block mentioned (lines ~186-213).

In `@go.mod`:
- Line 56: Update the dependency version for module
github.com/modelcontextprotocol/go-sdk from v0.8.0 to v1.4.1 in go.mod and
ensure the repository's Go toolchain constraint (go directive in go.mod) is set
to at least 1.25 to satisfy the new module's requirement; run go mod tidy
afterwards to refresh go.sum and resolve transitive changes.

---

Nitpick comments:
In `@cli/mcp/config.go`:
- Line 128: Replace the non-idiomatic empty-slice literal initialization for the
variable named "missing" with the idiomatic zero-value declaration; locate the
declaration of missing in the config.go snippet and change the initialization
from []string{} to using the zero value (var missing []string) so it avoids an
unnecessary allocation and follows Go conventions.

In `@cli/mcp/schema_test.go`:
- Around line 84-123: Add a regression test that verifies operation-level
parameters override path-item parameters when they share the same (In, Name) key
and that schema.Required entries are deduplicated: construct an appcli.PathItem
with a parameter (e.g., In: "path", Name: "id", Description: "path desc") and an
appcli.Operation that also declares a parameter with the same In/Name but a
different Description and that includes the parameter in its Required list (and
ensure the path-item also marks it required), call buildInputSchema(pathItem,
operation), then assert the resulting schema.Properties["id"].Description equals
the operation-level description (ensuring override) and assert the
schema.Required slice contains "id" only once (use
require.Contains/require.Equal/require.Len as appropriate). Reference
buildInputSchema, appcli.PathItem, appcli.Operation, schema.Required, and
schema.Properties to locate where to add the test.

In `@cli/mcp/transport_test.go`:
- Around line 286-292: The test loop sends two tool calls using mcpPost with
jsonrpcRequest but currently closes resp.Body without verifying success; update
the loop that calls mcpPost (in the for i := 0; i < 2; i++) to assert the HTTP
response is StatusOK and the JSON-RPC payload indicates isError == false for
each iteration (use the same response parsing used elsewhere in this test), then
close resp.Body after those assertions so each call is validated individually;
reference mcpPost, jsonrpcRequest, and the per-response StatusOK/isError checks
when making the changes.

🪄 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: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Enterprise

Run ID: 5948a2fe-d9b7-48d9-94e8-70bbf4b3cd77

📥 Commits

Reviewing files that changed from the base of the PR and between 2232094 and 7e8f02a.

⛔ Files ignored due to path filters (1)

• go.sum is excluded by !**/*.sum

📒 Files selected for processing (12)

• AGENTS.md
• cli/README.md
• cli/cmd/cli/main.go
• cli/mcp/cmd.go
• cli/mcp/config.go
• cli/mcp/config_test.go
• cli/mcp/schema.go
• cli/mcp/schema_test.go
• cli/mcp/server.go
• cli/mcp/server_test.go
• cli/mcp/transport_test.go
• go.mod