DRAFT: Rch/scaffold and publish #130
Open
Conversation
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
Adds `cone connector` command group with: - `cone connector build [path]` - builds connector binaries - Supports cross-compilation via --os and --arch flags - Validates go.mod exists before building Part of Tier 1 implementation (MASTER_PLAN.md feature [1]). 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
Adds pkg/scaffold package and `cone connector init <name>` command for creating new connector projects from a standard template. Generated project includes: - main.go with CLI setup using baton-sdk - config/config.go with field definitions - pkg/connector/ with resource syncers (users, groups, roles) - pkg/client/ with API client stub - go.mod, Makefile, README.md, .gitignore Features: - --module flag for custom Go module path - --description flag for connector description - Auto-generates baton-<name> directory - Template variables for name, module path, description Part of Tier 2 implementation (MASTER_PLAN.md feature [6]). 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
Template was using obsolete SDK API that doesn't exist: - cli.NewApp, cli.WithConnector (never existed) - Wrong SDK version (v0.2.0 vs v0.4.7) Changed to current patterns: - configSdk.DefineConfiguration() for CLI setup - field.NewConfiguration() for config schema - field.Configurable interface implementation - connectorbuilder.NewConnector() wrapper Verified: generated connector compiles and runs.
analyze, consent, dev, validate commands with supporting packages for MCP client, consent storage, and prompts. The AI-related commands have only been tested against mocks until more C1 work occurs
WalkthroughThis PR introduces multiple new packages to build a complete MCP (Model Context Protocol) client ecosystem: a consent management system with versioning, interactive CLI prompting utilities, tool handling for MCP protocol workflows, code scaffolding generation, and comprehensive testing infrastructure including a mock MCP server for validation. Changes
Sequence DiagramssequenceDiagram
participant Client
participant Server
participant ToolHandler
Client->>Server: Initialize request (JSON-RPC)
Server-->>Client: Initialize response
Client->>Server: connector_analyze request
Server-->>Client: tool_call response (read_files)
Client->>ToolHandler: HandleToolCall(read_files)
ToolHandler-->>Client: ToolResult (files content)
Client->>Server: tool_result response
Server-->>Client: tool_call response (ask_user)
Client->>ToolHandler: HandleToolCall(ask_user)
ToolHandler-->>Client: ToolResult (user response)
Client->>Server: tool_result response
Server-->>Client: complete with summary
Client->>Client: parseAnalysisResult
sequenceDiagram
participant User
participant System
participant Disk
User->>System: Load consent
System->>Disk: Read ~/.cone/consent.json
alt Consent file missing
Disk-->>System: File not found
System-->>User: ErrNoConsent
else File exists
Disk-->>System: ConsentRecord JSON
alt Version matches
System-->>User: Valid ConsentRecord
else Version outdated
System-->>User: ErrConsentVersionMismatch
end
end
opt On consent
User->>System: Save consent
System->>Disk: Write ~/.cone/consent.json with version
Disk-->>System: Success
System-->>User: OK
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
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: 13
🤖 Fix all issues with AI agents
In `@pkg/consent/consent_test.go`:
- Around line 3-9: Update the tests in consent_test.go to be Windows-safe by
setting USERPROFILE in addition to HOME when overriding the home dir (use
os.Setenv for "USERPROFILE") and by guarding POSIX permission assertions (checks
against file modes like 0700/0600) with runtime.GOOS != "windows" so they are
skipped on Windows; specifically modify the test setup that overrides the home
directory (where os.UserHomeDir is relied on) and wrap any permission assertions
in an if runtime.GOOS != "windows" block to keep tests hermetic across
platforms.
In `@pkg/mcpclient/client_test.go`:
- Around line 14-34: The tests currently ignore errors from json.Decode,
json.Encode and json.Marshal which trips errcheck; update each handler in
pkg/mcpclient/client_test.go to check and handle those errors (e.g., after
json.NewDecoder(r.Body).Decode(&req) and after json.NewEncoder(w).Encode(resp)
return t.Fatalf or t.Errorf with the error), and update the jsonBody helper to
capture json.Marshal error and return it (or t.Fatalf from callers) instead of
discarding it; specifically modify the anonymous httptest handlers (where
Decode/Encode are called) and the jsonBody function to assign the error to a
variable, check it, and fail the test with a clear message including the error.
- Around line 96-112: In the mock server handler in pkg/mcpclient/client_test.go
(the "tools/call" case) replace the direct type assertion params :=
req["params"].(map[string]interface{}) with a safe ok-check (e.g., params, ok :=
req["params"].(map[string]interface{})) and if !ok return a JSON-RPC error
response (using the same resp structure with "error" containing code/message and
the incoming id) so the test mock fails gracefully instead of panicking; update
subsequent references (params["name"]) to use the guarded params variable.
In `@pkg/mcpclient/client.go`:
- Around line 141-212: In processAnalysisResponse, guard against a nil
ToolHandler before calling c.ToolHandler.HandleToolCall: if c.ToolHandler == nil
return a clear error (e.g., fmt.Errorf("no ToolHandler configured for tool_call
%s", result.ToolCall.Name)) instead of attempting to call HandleToolCall; apply
this check in the "tool_call" case (referencing processAnalysisResponse,
result.ToolCall, and ToolHandler.HandleToolCall) so the function returns a
helpful error instead of panicking.
- Around line 214-260: The sendRequest method dereferences c.HTTPClient directly
which can panic when nil; change it to use a local httpClient variable: if
c.HTTPClient is nil, set httpClient = &http.Client{Timeout: c.Timeout} (so the
documented default client behavior and the Client.Timeout field are honored),
otherwise use the provided c.HTTPClient unchanged; then call httpClient.Do(...)
and adjust any uses of c.HTTPClient to httpClient in sendRequest.
In `@pkg/mcpclient/mock/server_test.go`:
- Around line 33-35: The json.Marshal calls currently ignore errors (e.g., where
listBytes, _ := json.Marshal(resp["result"]) and the other two occurrences)
which can mask failures; update each Marshal usage to capture the error, check
it, and fail the test with t.Fatalf including the error (e.g., replace the blank
identifier with err, and call t.Fatalf("json.Marshal failed for resp[...] : %v",
err) before proceeding to json.Unmarshal). Apply this pattern to the three
instances around variables listBytes/listResult and the analogous variables at
lines 58 and 121 so Marshal failures are surfaced immediately.
In `@pkg/mcpclient/mock/server.go`:
- Around line 225-246: The response helpers sendResult and sendError currently
ignore json.NewEncoder(w).Encode errors; update both functions to capture the
returned error from Encoder.Encode and handle it (e.g., log the encoding error
via the server logger or fmt.Errorf and then close the connection or return)
instead of discarding it; ensure the Content-Type header is still set before
encoding, but do not attempt to call http.Error after headers are written —
simply log the error with context (include the id and whether it was result or
error) and terminate writing/close the response to avoid silent failures.
In `@pkg/mcpclient/tools.go`:
- Around line 264-317: In handleWriteFile, avoid calling prompt.Confirm when
h.DryRun is true: after detecting the file exists (the os.Stat(fullPath) == nil
branch) short-circuit and return the dry-run result if h.DryRun is set instead
of calling prompt.Confirm; move or add the h.DryRun check before invoking
prompt.Confirm so non-interactive dry runs never block.
- Around line 196-262: The handler handleEditFile currently allows missing "new"
to default to empty string which can delete content; validate the "new" argument
from args before proceeding (similar to the existing check for "path" and "old")
by ensuring args["new"] is present and is a non-empty string, and return a
ToolResult with Success:false and a clear error message (e.g., "missing required
argument: new") if it isn't; update the early-argument validation block (where
path, old, new are read) to perform this check and abort before showing the diff
or writing the file.
- Around line 64-127: handleReadFiles (and similarly
handleEditFile/handleWriteFile) is vulnerable to symlink TOCTOU because it uses
filepath.Join/Rel plus os.Stat/os.ReadFile; instead, wrap h.ConnectorDir with
os.Root and open target files via the root's OpenInRoot to atomically resolve
and read files beneath the directory. Concretely: create an os.Root for
h.ConnectorDir, use the relative path (relPath) computed from the glob match (or
pattern) and call root.OpenInRoot(relPath) to get an *os.File, replace os.Stat
with f.Stat() and os.ReadFile with io.ReadAll(f) (closing f), and remove
string-path-based security checks—apply the same replacement in handleEditFile
and handleWriteFile so all file access is symlink-safe and TOCTOU-resistant.
In `@pkg/prompt/prompt.go`:
- Around line 272-328: The DisplayBox function can panic when a long title makes
padding negative; update DisplayBox to truncate title to at most width-2
characters (or width-4 if you want spaces around), compute padding from the
truncated title, and clamp padding to zero (e.g., if
(width-2-len(truncatedTitle)) / 2 < 0 then set padding = 0) before calling
strings.Repeat; reference the DisplayBox function and the local variables title
and padding so you replace uses of title with the truncatedTitle and ensure
padding is non-negative to avoid strings.Repeat panics.
In `@pkg/scaffold/scaffold_test.go`:
- Around line 99-101: Handle errors from os.Getwd() and os.Chdir(tmpDir) and
ensure cleanup uses t.Cleanup: call oldWd, err := os.Getwd() and if err != nil
use t.Fatalf to stop the test; then if os.Chdir(tmpDir) returns an error use
t.Fatalf; register t.Cleanup(func(){ if err := os.Chdir(oldWd); err != nil {
t.Fatalf("restore cwd: %v", err) } }) instead of defer so the original working
directory is reliably restored even if the test fails.
In `@pkg/scaffold/scaffold.go`:
- Around line 24-31: Add a nil check for the incoming *Config at the top of
Generate and validate cfg.Name before using it: return an error if cfg is nil;
ensure cfg.Name matches a safe pattern (only lowercase letters, digits, and
hyphens, e.g. regex ^[a-z0-9-]+$), reject names containing "..",
leading/trailing slashes, or any path separators to prevent path traversal, and
return a clear error if invalid; only after validation, set cfg.ModulePath (when
empty) using the validated name to form "github.com/conductorone/baton-<name>";
ensure any later use of cfg.Name with filepath.Join or file writes relies on the
validated value to avoid writing outside the intended directory (references:
Generate, Config, cfg.Name, cfg.ModulePath, filepath.Join).
🧹 Nitpick comments (5)
pkg/scaffold/scaffold_test.go (1)
192-193: Useos.DevNullfor cross-platform build output.Line 193 hardcodes
/dev/null, which fails on Windows.os.DevNullkeeps this test portable.♻️ Suggested change
- runCommand(t, outputDir, 3*time.Minute, "go", "build", "-o", "/dev/null", ".") + runCommand(t, outputDir, 3*time.Minute, "go", "build", "-o", os.DevNull, ".")pkg/scaffold/scaffold.go (1)
64-66: Fail fast on missing template keys.Line 64-66 uses the default
text/templatemissing-key behavior, which silently renders empty strings. Enablingmissingkey=errorcatches template drift early (especially for docs that compile tests won’t catch).♻️ Suggested change
- tmpl, err := template.New(tf.Path).Parse(tf.Template) + tmpl, err := template.New(tf.Path). + Option("missingkey=error"). + Parse(tf.Template)pkg/consent/consent.go (1)
115-140: Optionally reapply 0600 when overwriting consent.json.
os.WriteFileonly applies permissions on create; if the file already exists with broader perms, they remain. If you want the 0600 guarantee to always hold, follow withos.Chmod.♻️ Proposed hardening
// Write with restrictive permissions (0600 = rw-------) if err := os.WriteFile(path, data, 0600); err != nil { return fmt.Errorf("failed to write consent file: %w", err) } + if err := os.Chmod(path, 0600); err != nil { + return fmt.Errorf("failed to set consent file permissions: %w", err) + }pkg/mcpclient/client_test.go (1)
143-150: Strengthen the assertion in the Analyze test.The test currently passes even if
Analyzeunexpectedly succeeds. Consider asserting the expected error (or expected success) to make the test deterministic.pkg/mcpclient/mock/server_test.go (1)
92-100: Consider asserting tool_result responses.To make the test deterministic, capture each
doRequestresponse and callassertNoErrorbefore moving on.
Comment on lines
+3
to
+9
| import ( | ||
| "encoding/json" | ||
| "os" | ||
| "path/filepath" | ||
| "testing" | ||
| "time" | ||
| ) |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
cat -n pkg/consent/consent_test.go | head -50Repository: ConductorOne/cone
Length of output: 1445
🏁 Script executed:
wc -l pkg/consent/consent_test.goRepository: ConductorOne/cone
Length of output: 92
🏁 Script executed:
sed -n '60,80p' pkg/consent/consent_test.goRepository: ConductorOne/cone
Length of output: 511
🏁 Script executed:
sed -n '230,250p' pkg/consent/consent_test.goRepository: ConductorOne/cone
Length of output: 554
🏁 Script executed:
sed -n '390,410p' pkg/consent/consent_test.goRepository: ConductorOne/cone
Length of output: 634
🏁 Script executed:
sed -n '1,50p' pkg/consent/consent.goRepository: ConductorOne/cone
Length of output: 1783
🏁 Script executed:
cat pkg/consent/consent.go | grep -A 5 "func ensureConsentDir"Repository: ConductorOne/cone
Length of output: 152
🏁 Script executed:
sed -n '50,100p' pkg/consent/consent.goRepository: ConductorOne/cone
Length of output: 1457
🌐 Web query:
os.UserHomeDir Windows HOME environment variable behavior
💡 Result:
In Go, os.UserHomeDir() does not use the HOME environment variable on Windows.
- Unix/macOS: returns
$HOME. - Windows: returns
%USERPROFILE%(the user profile directory, typicallyC:\Users\<name>). If the expected variable isn’t set, it may return a platform default or an error. [1] - Windows commonly exposes the user profile via
%USERPROFILE%(and related variables like%HOMEPATH%). [2]
So on Windows, setting HOME (e.g., via Git Bash/MSYS/Cygwin) generally won’t affect os.UserHomeDir(); if you want to honor HOME on Windows, you must check os.Getenv("HOME") yourself before falling back to os.UserHomeDir().
Sources: [1] Go os.UserHomeDir docs (pkg.go.dev) (pkg.go.dev), [2] Microsoft Learn: recognized environment variables (USERPROFILE, HOMEPATH) (learn.microsoft.com)
Citations:
- 1: https://pkg.go.dev/os%40go1.25.6?utm_source=openai
- 2: https://learn.microsoft.com/en-us/windows/deployment/usmt/usmt-recognized-environment-variables?utm_source=openai
🏁 Script executed:
grep -n "runtime.GOOS\|build.*windows\|+build" pkg/consent/consent_test.go pkg/consent/consent.goRepository: ConductorOne/cone
Length of output: 43
🏁 Script executed:
cat go.mod | head -20Repository: ConductorOne/cone
Length of output: 492
🏁 Script executed:
cat -n .github/workflows/*.yml 2>/dev/null | grep -i "windows\|os:" | head -20Repository: ConductorOne/cone
Length of output: 43
🏁 Script executed:
find .github -name "*.yml" -o -name "*.yaml" | xargs cat 2>/dev/nullRepository: ConductorOne/cone
Length of output: 6182
Make HOME override and permission checks Windows-safe.
os.UserHomeDir ignores HOME on Windows and reads USERPROFILE instead, so tests that only set HOME will fail there. Additionally, POSIX permission checks (0700, 0600) are unreliable on Windows. If Windows support is planned, set USERPROFILE in test setup and guard permission assertions with runtime.GOOS != "windows" to keep tests hermetic across platforms.
🛠️ Proposed fix
import (
"encoding/json"
"os"
"path/filepath"
+ "runtime"
"testing"
"time"
)
// setupTestDir creates a temp directory and sets HOME to point to it.
// Returns a cleanup function that restores the original HOME.
func setupTestDir(t *testing.T) func() {
t.Helper()
originalHome := os.Getenv("HOME")
+ originalUserProfile := os.Getenv("USERPROFILE")
tmpDir := t.TempDir()
if err := os.Setenv("HOME", tmpDir); err != nil {
t.Fatalf("failed to set HOME: %v", err)
}
+ _ = os.Setenv("USERPROFILE", tmpDir)
return func() {
- os.Setenv("HOME", originalHome)
+ _ = os.Setenv("HOME", originalHome)
+ _ = os.Setenv("USERPROFILE", originalUserProfile)
}
}
@@
- if info.Mode().Perm() != 0700 {
+ if runtime.GOOS != "windows" && info.Mode().Perm() != 0700 {
t.Errorf("expected permissions 0700, got %o", info.Mode().Perm())
}
@@
- if info.Mode().Perm() != 0600 {
+ if runtime.GOOS != "windows" && info.Mode().Perm() != 0600 {
t.Errorf("expected permissions 0600, got %o", info.Mode().Perm())
}
@@
- if info.Mode().Perm() != 0700 {
+ if runtime.GOOS != "windows" && info.Mode().Perm() != 0700 {
t.Errorf("expected permissions 0700, got %o", info.Mode().Perm())
}Also applies to: 16-24, 69-77, 239-245, 399-405
🤖 Prompt for AI Agents
In `@pkg/consent/consent_test.go` around lines 3 - 9, Update the tests in
consent_test.go to be Windows-safe by setting USERPROFILE in addition to HOME
when overriding the home dir (use os.Setenv for "USERPROFILE") and by guarding
POSIX permission assertions (checks against file modes like 0700/0600) with
runtime.GOOS != "windows" so they are skipped on Windows; specifically modify
the test setup that overrides the home directory (where os.UserHomeDir is relied
on) and wrap any permission assertions in an if runtime.GOOS != "windows" block
to keep tests hermetic across platforms.
Comment on lines
+14
to
+34
| server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { | ||
| var req map[string]interface{} | ||
| json.NewDecoder(r.Body).Decode(&req) | ||
|
|
||
| if req["method"] != "initialize" { | ||
| t.Errorf("expected initialize method, got %v", req["method"]) | ||
| } | ||
|
|
||
| resp := map[string]interface{}{ | ||
| "jsonrpc": "2.0", | ||
| "id": req["id"], | ||
| "result": map[string]interface{}{ | ||
| "protocolVersion": "2024-11-05", | ||
| "serverInfo": map[string]string{ | ||
| "name": "test-server", | ||
| "version": "1.0", | ||
| }, | ||
| }, | ||
| } | ||
| json.NewEncoder(w).Encode(resp) | ||
| })) |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
find . -name "client_test.go" -type f | head -20Repository: ConductorOne/cone
Length of output: 91
🏁 Script executed:
cat -n ./pkg/mcpclient/client_test.goRepository: ConductorOne/cone
Length of output: 7670
🏁 Script executed:
# Check for errcheck configuration in the repository
find . -name ".golangci.yml" -o -name ".golangci.yaml" -o -name "golangci.yml" | head -5
cat .golangci.yml 2>/dev/null || cat .golangci.yaml 2>/dev/null || echo "No golangci config found"Repository: ConductorOne/cone
Length of output: 5105
🏁 Script executed:
git log --oneline --all | head -20
git diff --stat HEAD~5..HEAD 2>/dev/null | head -20Repository: ConductorOne/cone
Length of output: 165
🏁 Script executed:
# Check for CI configuration files
find . -name "*.yml" -o -name "*.yaml" | grep -E "(github|gitlab|circle|travis|azure)" | head -10
ls -la .github/workflows/ 2>/dev/null || echo "No workflows directory"Repository: ConductorOne/cone
Length of output: 885
🏁 Script executed:
cat .github/workflows/ci.yamlRepository: ConductorOne/cone
Length of output: 1041
Handle JSON Encode/Decode errors to fix errcheck failures.
errcheck is failing on unhandled Decode, Encode, and Marshal errors across the test file. These must be fixed to keep CI green.
🐛 Suggested pattern (apply similarly in each handler and in jsonBody)
- json.NewDecoder(r.Body).Decode(&req)
+ if err := json.NewDecoder(r.Body).Decode(&req); err != nil {
+ t.Errorf("failed to decode request: %v", err)
+ http.Error(w, "bad request", http.StatusBadRequest)
+ return
+ }- json.NewEncoder(w).Encode(resp)
+ if err := json.NewEncoder(w).Encode(resp); err != nil {
+ t.Errorf("failed to encode response: %v", err)
+ }- body, _ := json.Marshal(req)
- return bytes.NewBuffer(body)
+ body, err := json.Marshal(req)
+ if err != nil {
+ panic(err)
+ }
+ return bytes.NewBuffer(body)Locations: Lines 16, 33, 51, 61, 82, 127 (Decode/Encode in handlers), and line 224 (Marshal in jsonBody helper).
🧰 Tools
🪛 GitHub Check: go-lint
[failure] 33-33:
Error return value of (*encoding/json.Encoder).Encode is not checked (errcheck)
[failure] 16-16:
Error return value of (*encoding/json.Decoder).Decode is not checked (errcheck)
🤖 Prompt for AI Agents
In `@pkg/mcpclient/client_test.go` around lines 14 - 34, The tests currently
ignore errors from json.Decode, json.Encode and json.Marshal which trips
errcheck; update each handler in pkg/mcpclient/client_test.go to check and
handle those errors (e.g., after json.NewDecoder(r.Body).Decode(&req) and after
json.NewEncoder(w).Encode(resp) return t.Fatalf or t.Errorf with the error), and
update the jsonBody helper to capture json.Marshal error and return it (or
t.Fatalf from callers) instead of discarding it; specifically modify the
anonymous httptest handlers (where Decode/Encode are called) and the jsonBody
function to assign the error to a variable, check it, and fail the test with a
clear message including the error.
Comment on lines
+96
to
+112
| case "tools/call": | ||
| params := req["params"].(map[string]interface{}) | ||
| if params["name"] == "connector_analyze" { | ||
| resp = map[string]interface{}{ | ||
| "jsonrpc": "2.0", | ||
| "id": req["id"], | ||
| "result": map[string]interface{}{ | ||
| "content": []map[string]interface{}{ | ||
| { | ||
| "type": "text", | ||
| "text": `{"status":"tool_call","session_id":"test-session","tool_call":{"name":"read_files","arguments":{"paths":["go.mod"]}}}`, | ||
| }, | ||
| }, | ||
| }, | ||
| } | ||
| } else if params["name"] == "tool_result" { | ||
| resp = map[string]interface{}{ |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, verify the file exists and check its size
ls -la pkg/mcpclient/client_test.go 2>/dev/null || echo "File not found at expected location"
wc -l pkg/mcpclient/client_test.go 2>/dev/null || echo "Cannot count lines"Repository: ConductorOne/cone
Length of output: 171
🏁 Script executed:
# Read the specific lines mentioned in the review (96-112)
sed -n '80,130p' pkg/mcpclient/client_test.goRepository: ConductorOne/cone
Length of output: 1466
🏁 Script executed:
# Search for other type assertions in the same test file to understand the pattern
rg '\.\(map\[string\]' pkg/mcpclient/client_test.go -n -A 2 -B 2Repository: ConductorOne/cone
Length of output: 239
🏁 Script executed:
# Look at the complete switch statement and surrounding code to understand context
sed -n '80,150p' pkg/mcpclient/client_test.goRepository: ConductorOne/cone
Length of output: 2111
🏁 Script executed:
# Check if there are other similar type assertions without ok-checks in the same pattern
rg 'req\[' pkg/mcpclient/client_test.go -n -B 1 -A 1 | head -40Repository: ConductorOne/cone
Length of output: 950
🏁 Script executed:
# Look for other test handlers or mock servers in this file or related files to see the pattern
rg 'httptest\.|switch.*method' pkg/mcpclient/ -n -A 5Repository: ConductorOne/cone
Length of output: 3353
🏁 Script executed:
# Check the integration_test.go mock server to see if it has similar patterns
sed -n '1,150p' pkg/mcpclient/integration_test.go | grep -A 30 "HandleMCP\|switch"Repository: ConductorOne/cone
Length of output: 1786
🏁 Script executed:
# Look at the mock/server.go to see how a more complete mock handles this
cat pkg/mcpclient/mock/server.go | head -100Repository: ConductorOne/cone
Length of output: 2486
🏁 Script executed:
# Search for type assertions with ok-checks in test files
rg '\.\(map\[string\].*\)\,\s*ok' pkg/mcpclient/ -nRepository: ConductorOne/cone
Length of output: 176
🏁 Script executed:
# Search for type assertions with ok-checks (proper escaping)
rg 'ok :=' pkg/mcpclient/ -n -B 2 -A 2 | head -50Repository: ConductorOne/cone
Length of output: 2216
🏁 Script executed:
# Look at the full test function context to understand what requests are being made
sed -n '75,135p' pkg/mcpclient/client_test.goRepository: ConductorOne/cone
Length of output: 1727
🏁 Script executed:
# Check what the client code sends when calling the server
rg -A 10 'tools/call\|Analyze' pkg/mcpclient/client.go | head -40Repository: ConductorOne/cone
Length of output: 43
🏁 Script executed:
# Look at the newRequest function to see how requests are constructed
rg -B 5 -A 15 'func.*newRequest\|func.*buildRequest' pkg/mcpclient/client.goRepository: ConductorOne/cone
Length of output: 43
🏁 Script executed:
# Find how the client calls tools/call
rg 'tools/call' pkg/mcpclient/ -n -B 5 -A 10Repository: ConductorOne/cone
Length of output: 7435
🏁 Script executed:
# Look at the Client struct and its methods
rg -A 20 'func.*\(c \*Client\)' pkg/mcpclient/client.go | head -60Repository: ConductorOne/cone
Length of output: 1817
🏁 Script executed:
# Check if the mock server or test sets up what params should be
sed -n '1,80p' pkg/mcpclient/client_test.goRepository: ConductorOne/cone
Length of output: 1991
Consider guarding the params type assertion in the mock server handler.
While this test mock server controls its own inputs, a direct type assertion can still panic if the code path changes. Since the handler processes potentially variable request shapes, use an ok-check to return a JSON-RPC error response for robustness:
🛠️ Suggested guard
case "tools/call":
- params := req["params"].(map[string]interface{})
+ params, ok := req["params"].(map[string]interface{})
+ if !ok {
+ resp = map[string]interface{}{
+ "jsonrpc": "2.0",
+ "id": req["id"],
+ "error": map[string]interface{}{
+ "code": -32602,
+ "message": "Invalid params",
+ },
+ }
+ break
+ }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| case "tools/call": | |
| params := req["params"].(map[string]interface{}) | |
| if params["name"] == "connector_analyze" { | |
| resp = map[string]interface{}{ | |
| "jsonrpc": "2.0", | |
| "id": req["id"], | |
| "result": map[string]interface{}{ | |
| "content": []map[string]interface{}{ | |
| { | |
| "type": "text", | |
| "text": `{"status":"tool_call","session_id":"test-session","tool_call":{"name":"read_files","arguments":{"paths":["go.mod"]}}}`, | |
| }, | |
| }, | |
| }, | |
| } | |
| } else if params["name"] == "tool_result" { | |
| resp = map[string]interface{}{ | |
| case "tools/call": | |
| params, ok := req["params"].(map[string]interface{}) | |
| if !ok { | |
| resp = map[string]interface{}{ | |
| "jsonrpc": "2.0", | |
| "id": req["id"], | |
| "error": map[string]interface{}{ | |
| "code": -32602, | |
| "message": "Invalid params", | |
| }, | |
| } | |
| break | |
| } | |
| if params["name"] == "connector_analyze" { | |
| resp = map[string]interface{}{ | |
| "jsonrpc": "2.0", | |
| "id": req["id"], | |
| "result": map[string]interface{}{ | |
| "content": []map[string]interface{}{ | |
| { | |
| "type": "text", | |
| "text": `{"status":"tool_call","session_id":"test-session","tool_call":{"name":"read_files","arguments":{"paths":["go.mod"]}}}`, | |
| }, | |
| }, | |
| }, | |
| } | |
| } else if params["name"] == "tool_result" { | |
| resp = map[string]interface{}{ |
🤖 Prompt for AI Agents
In `@pkg/mcpclient/client_test.go` around lines 96 - 112, In the mock server
handler in pkg/mcpclient/client_test.go (the "tools/call" case) replace the
direct type assertion params := req["params"].(map[string]interface{}) with a
safe ok-check (e.g., params, ok := req["params"].(map[string]interface{})) and
if !ok return a JSON-RPC error response (using the same resp structure with
"error" containing code/message and the incoming id) so the test mock fails
gracefully instead of panicking; update subsequent references (params["name"])
to use the guarded params variable.
Comment on lines
+141
to
+212
| // processAnalysisResponse handles the analysis response, including tool callback loops. | ||
| func (c *Client) processAnalysisResponse(ctx context.Context, resp *jsonrpcResponse) (*AnalysisResult, error) { | ||
| for { | ||
| var result struct { | ||
| Status string `json:"status"` | ||
| Message string `json:"message"` | ||
| ToolCall *struct { | ||
| Name string `json:"name"` | ||
| Arguments map[string]interface{} `json:"arguments"` | ||
| } `json:"tool_call,omitempty"` | ||
| Summary map[string]interface{} `json:"summary,omitempty"` | ||
| } | ||
|
|
||
| if err := json.Unmarshal(resp.Result, &result); err != nil { | ||
| return nil, fmt.Errorf("failed to parse analysis response: %w", err) | ||
| } | ||
|
|
||
| switch result.Status { | ||
| case "complete": | ||
| // Analysis complete | ||
| issuesFound := 0 | ||
| filesScanned := 0 | ||
| if result.Summary != nil { | ||
| if v, ok := result.Summary["issues_found"].(float64); ok { | ||
| issuesFound = int(v) | ||
| } | ||
| if v, ok := result.Summary["files_analyzed"].(float64); ok { | ||
| filesScanned = int(v) | ||
| } | ||
| } | ||
| return &AnalysisResult{ | ||
| Status: "complete", | ||
| Message: result.Message, | ||
| IssuesFound: issuesFound, | ||
| FilesScanned: filesScanned, | ||
| Summary: result.Summary, | ||
| }, nil | ||
|
|
||
| case "tool_call": | ||
| if result.ToolCall == nil { | ||
| return nil, fmt.Errorf("tool_call status but no tool_call data") | ||
| } | ||
|
|
||
| // Execute the tool locally | ||
| toolResult, err := c.ToolHandler.HandleToolCall(ctx, result.ToolCall.Name, result.ToolCall.Arguments) | ||
| if err != nil { | ||
| return nil, fmt.Errorf("tool %s failed: %w", result.ToolCall.Name, err) | ||
| } | ||
|
|
||
| // Send the result back to the server | ||
| resp, err = c.sendRequest(ctx, "tool_result", map[string]interface{}{ | ||
| "tool": result.ToolCall.Name, | ||
| "result": toolResult, | ||
| }) | ||
| if err != nil { | ||
| return nil, fmt.Errorf("failed to send tool result: %w", err) | ||
| } | ||
|
|
||
| // Continue the loop with the new response | ||
| continue | ||
|
|
||
| case "error": | ||
| return &AnalysisResult{ | ||
| Status: "error", | ||
| Message: result.Message, | ||
| }, nil | ||
|
|
||
| default: | ||
| return nil, fmt.Errorf("unexpected status: %s", result.Status) | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
Guard against nil ToolHandler on tool_call.
If ToolHandler is nil, this path panics. Return a clear error instead.
🐛 Proposed fix
case "tool_call":
if result.ToolCall == nil {
return nil, fmt.Errorf("tool_call status but no tool_call data")
}
+ if c.ToolHandler == nil {
+ return nil, fmt.Errorf("tool handler is required for tool_call responses")
+ }
// Execute the tool locally
toolResult, err := c.ToolHandler.HandleToolCall(ctx, result.ToolCall.Name, result.ToolCall.Arguments)
if err != nil {
return nil, fmt.Errorf("tool %s failed: %w", result.ToolCall.Name, err)
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| // processAnalysisResponse handles the analysis response, including tool callback loops. | |
| func (c *Client) processAnalysisResponse(ctx context.Context, resp *jsonrpcResponse) (*AnalysisResult, error) { | |
| for { | |
| var result struct { | |
| Status string `json:"status"` | |
| Message string `json:"message"` | |
| ToolCall *struct { | |
| Name string `json:"name"` | |
| Arguments map[string]interface{} `json:"arguments"` | |
| } `json:"tool_call,omitempty"` | |
| Summary map[string]interface{} `json:"summary,omitempty"` | |
| } | |
| if err := json.Unmarshal(resp.Result, &result); err != nil { | |
| return nil, fmt.Errorf("failed to parse analysis response: %w", err) | |
| } | |
| switch result.Status { | |
| case "complete": | |
| // Analysis complete | |
| issuesFound := 0 | |
| filesScanned := 0 | |
| if result.Summary != nil { | |
| if v, ok := result.Summary["issues_found"].(float64); ok { | |
| issuesFound = int(v) | |
| } | |
| if v, ok := result.Summary["files_analyzed"].(float64); ok { | |
| filesScanned = int(v) | |
| } | |
| } | |
| return &AnalysisResult{ | |
| Status: "complete", | |
| Message: result.Message, | |
| IssuesFound: issuesFound, | |
| FilesScanned: filesScanned, | |
| Summary: result.Summary, | |
| }, nil | |
| case "tool_call": | |
| if result.ToolCall == nil { | |
| return nil, fmt.Errorf("tool_call status but no tool_call data") | |
| } | |
| // Execute the tool locally | |
| toolResult, err := c.ToolHandler.HandleToolCall(ctx, result.ToolCall.Name, result.ToolCall.Arguments) | |
| if err != nil { | |
| return nil, fmt.Errorf("tool %s failed: %w", result.ToolCall.Name, err) | |
| } | |
| // Send the result back to the server | |
| resp, err = c.sendRequest(ctx, "tool_result", map[string]interface{}{ | |
| "tool": result.ToolCall.Name, | |
| "result": toolResult, | |
| }) | |
| if err != nil { | |
| return nil, fmt.Errorf("failed to send tool result: %w", err) | |
| } | |
| // Continue the loop with the new response | |
| continue | |
| case "error": | |
| return &AnalysisResult{ | |
| Status: "error", | |
| Message: result.Message, | |
| }, nil | |
| default: | |
| return nil, fmt.Errorf("unexpected status: %s", result.Status) | |
| } | |
| } | |
| } | |
| // processAnalysisResponse handles the analysis response, including tool callback loops. | |
| func (c *Client) processAnalysisResponse(ctx context.Context, resp *jsonrpcResponse) (*AnalysisResult, error) { | |
| for { | |
| var result struct { | |
| Status string `json:"status"` | |
| Message string `json:"message"` | |
| ToolCall *struct { | |
| Name string `json:"name"` | |
| Arguments map[string]interface{} `json:"arguments"` | |
| } `json:"tool_call,omitempty"` | |
| Summary map[string]interface{} `json:"summary,omitempty"` | |
| } | |
| if err := json.Unmarshal(resp.Result, &result); err != nil { | |
| return nil, fmt.Errorf("failed to parse analysis response: %w", err) | |
| } | |
| switch result.Status { | |
| case "complete": | |
| // Analysis complete | |
| issuesFound := 0 | |
| filesScanned := 0 | |
| if result.Summary != nil { | |
| if v, ok := result.Summary["issues_found"].(float64); ok { | |
| issuesFound = int(v) | |
| } | |
| if v, ok := result.Summary["files_analyzed"].(float64); ok { | |
| filesScanned = int(v) | |
| } | |
| } | |
| return &AnalysisResult{ | |
| Status: "complete", | |
| Message: result.Message, | |
| IssuesFound: issuesFound, | |
| FilesScanned: filesScanned, | |
| Summary: result.Summary, | |
| }, nil | |
| case "tool_call": | |
| if result.ToolCall == nil { | |
| return nil, fmt.Errorf("tool_call status but no tool_call data") | |
| } | |
| if c.ToolHandler == nil { | |
| return nil, fmt.Errorf("tool handler is required for tool_call responses") | |
| } | |
| // Execute the tool locally | |
| toolResult, err := c.ToolHandler.HandleToolCall(ctx, result.ToolCall.Name, result.ToolCall.Arguments) | |
| if err != nil { | |
| return nil, fmt.Errorf("tool %s failed: %w", result.ToolCall.Name, err) | |
| } | |
| // Send the result back to the server | |
| resp, err = c.sendRequest(ctx, "tool_result", map[string]interface{}{ | |
| "tool": result.ToolCall.Name, | |
| "result": toolResult, | |
| }) | |
| if err != nil { | |
| return nil, fmt.Errorf("failed to send tool result: %w", err) | |
| } | |
| // Continue the loop with the new response | |
| continue | |
| case "error": | |
| return &AnalysisResult{ | |
| Status: "error", | |
| Message: result.Message, | |
| }, nil | |
| default: | |
| return nil, fmt.Errorf("unexpected status: %s", result.Status) | |
| } | |
| } | |
| } |
🤖 Prompt for AI Agents
In `@pkg/mcpclient/client.go` around lines 141 - 212, In processAnalysisResponse,
guard against a nil ToolHandler before calling c.ToolHandler.HandleToolCall: if
c.ToolHandler == nil return a clear error (e.g., fmt.Errorf("no ToolHandler
configured for tool_call %s", result.ToolCall.Name)) instead of attempting to
call HandleToolCall; apply this check in the "tool_call" case (referencing
processAnalysisResponse, result.ToolCall, and ToolHandler.HandleToolCall) so the
function returns a helpful error instead of panicking.
Comment on lines
+214
to
+260
| // sendRequest sends a JSON-RPC request to the server. | ||
| func (c *Client) sendRequest(ctx context.Context, method string, params interface{}) (*jsonrpcResponse, error) { | ||
| c.requestID++ | ||
| req := jsonrpcRequest{ | ||
| JSONRPC: "2.0", | ||
| ID: c.requestID, | ||
| Method: method, | ||
| Params: params, | ||
| } | ||
|
|
||
| body, err := json.Marshal(req) | ||
| if err != nil { | ||
| return nil, fmt.Errorf("failed to marshal request: %w", err) | ||
| } | ||
|
|
||
| httpReq, err := http.NewRequestWithContext(ctx, http.MethodPost, c.ServerURL, bytes.NewReader(body)) | ||
| if err != nil { | ||
| return nil, fmt.Errorf("failed to create request: %w", err) | ||
| } | ||
|
|
||
| httpReq.Header.Set("Content-Type", "application/json") | ||
| if c.AuthToken != "" { | ||
| httpReq.Header.Set("Authorization", "Bearer "+c.AuthToken) | ||
| } | ||
|
|
||
| httpResp, err := c.HTTPClient.Do(httpReq) | ||
| if err != nil { | ||
| return nil, fmt.Errorf("request failed: %w", err) | ||
| } | ||
| defer httpResp.Body.Close() | ||
|
|
||
| if httpResp.StatusCode != http.StatusOK { | ||
| bodyBytes, _ := io.ReadAll(httpResp.Body) | ||
| return nil, fmt.Errorf("server returned %d: %s", httpResp.StatusCode, string(bodyBytes)) | ||
| } | ||
|
|
||
| var resp jsonrpcResponse | ||
| if err := json.NewDecoder(httpResp.Body).Decode(&resp); err != nil { | ||
| return nil, fmt.Errorf("failed to decode response: %w", err) | ||
| } | ||
|
|
||
| if resp.Error != nil { | ||
| return nil, fmt.Errorf("server error %d: %s", resp.Error.Code, resp.Error.Message) | ||
| } | ||
|
|
||
| return &resp, nil | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# First, let's look at the Client struct definition and any comments about default client
head -n 250 pkg/mcpclient/client.go | tail -n +1 | head -n 100Repository: ConductorOne/cone
Length of output: 2660
🏁 Script executed:
#!/bin/bash
# Find the Client struct definition
rg -A 30 "type Client struct" pkg/mcpclient/client.goRepository: ConductorOne/cone
Length of output: 988
🏁 Script executed:
#!/bin/bash
# Check if there's any mention of "default client" in the file
rg -i "default.*client|client.*default" pkg/mcpclient/client.go -B 2 -A 2Repository: ConductorOne/cone
Length of output: 181
🏁 Script executed:
#!/bin/bash
# Look for HTTPClient initialization or nil checks
rg "HTTPClient" pkg/mcpclient/client.go -B 2 -A 2Repository: ConductorOne/cone
Length of output: 508
🏁 Script executed:
#!/bin/bash
# Check for Timeout field references
rg "Timeout" pkg/mcpclient/client.go -B 2 -A 2Repository: ConductorOne/cone
Length of output: 377
Honor the documented "default client" contract when HTTPClient is nil.
The Client struct documentation states "If nil, a default client is used," but sendRequest directly dereferences c.HTTPClient without a nil check, causing a panic. Add a nil fallback that also respects the Timeout field.
Proposed fix
- httpResp, err := c.HTTPClient.Do(httpReq)
+ httpClient := c.HTTPClient
+ if httpClient == nil {
+ timeout := c.Timeout
+ if timeout == 0 {
+ timeout = 30 * time.Second
+ }
+ httpClient = &http.Client{Timeout: timeout}
+ }
+ httpResp, err := httpClient.Do(httpReq)
if err != nil {
return nil, fmt.Errorf("request failed: %w", err)
}🤖 Prompt for AI Agents
In `@pkg/mcpclient/client.go` around lines 214 - 260, The sendRequest method
dereferences c.HTTPClient directly which can panic when nil; change it to use a
local httpClient variable: if c.HTTPClient is nil, set httpClient =
&http.Client{Timeout: c.Timeout} (so the documented default client behavior and
the Client.Timeout field are honored), otherwise use the provided c.HTTPClient
unchanged; then call httpClient.Do(...) and adjust any uses of c.HTTPClient to
httpClient in sendRequest.
Comment on lines
+196
to
+262
| // handleEditFile applies an edit to a file. | ||
| // Security: Only edits files within ConnectorDir. | ||
| func (h *ToolHandler) handleEditFile(ctx context.Context, args map[string]interface{}) (*ToolResult, error) { | ||
| path, _ := args["path"].(string) | ||
| old, _ := args["old"].(string) | ||
| new, _ := args["new"].(string) | ||
|
|
||
| if path == "" || old == "" { | ||
| return &ToolResult{Success: false, Error: "missing required arguments (path, old)"}, nil | ||
| } | ||
|
|
||
| // Security: Resolve path relative to connector directory | ||
| fullPath := filepath.Join(h.ConnectorDir, path) | ||
| relPath, err := filepath.Rel(h.ConnectorDir, fullPath) | ||
| if err != nil || strings.HasPrefix(relPath, "..") { | ||
| return &ToolResult{Success: false, Error: "path must be within connector directory"}, nil | ||
| } | ||
|
|
||
| // Read current content | ||
| content, err := os.ReadFile(fullPath) | ||
| if err != nil { | ||
| return &ToolResult{Success: false, Error: fmt.Sprintf("failed to read file: %v", err)}, nil | ||
| } | ||
|
|
||
| // Check if old string exists | ||
| if !strings.Contains(string(content), old) { | ||
| return &ToolResult{Success: false, Error: "old string not found in file"}, nil | ||
| } | ||
|
|
||
| // Show diff and ask for confirmation | ||
| fmt.Printf("\n--- %s (before)\n", path) | ||
| fmt.Printf("+++ %s (after)\n", path) | ||
| fmt.Printf("@@ edit @@\n") | ||
| fmt.Printf("-%s\n", strings.ReplaceAll(old, "\n", "\n-")) | ||
| fmt.Printf("+%s\n", strings.ReplaceAll(new, "\n", "\n+")) | ||
| fmt.Println() | ||
|
|
||
| if h.DryRun { | ||
| return &ToolResult{ | ||
| Success: true, | ||
| Data: map[string]interface{}{"applied": false, "reason": "dry run"}, | ||
| }, nil | ||
| } | ||
|
|
||
| accepted, err := prompt.Confirm("Apply this change?") | ||
| if err != nil { | ||
| return &ToolResult{Success: false, Error: err.Error()}, nil | ||
| } | ||
|
|
||
| if !accepted { | ||
| return &ToolResult{ | ||
| Success: true, | ||
| Data: map[string]interface{}{"applied": false, "reason": "user declined"}, | ||
| }, nil | ||
| } | ||
|
|
||
| // Apply the edit | ||
| newContent := strings.Replace(string(content), old, new, 1) | ||
| if err := os.WriteFile(fullPath, []byte(newContent), 0644); err != nil { | ||
| return &ToolResult{Success: false, Error: fmt.Sprintf("failed to write file: %v", err)}, nil | ||
| } | ||
|
|
||
| return &ToolResult{ | ||
| Success: true, | ||
| Data: map[string]interface{}{"applied": true}, | ||
| }, nil | ||
| } |
There was a problem hiding this comment.
Require explicit new argument before editing.
If new is missing, it defaults to "" and can unintentionally delete content. Validate presence/type explicitly.
🐛 Proposed fix
path, _ := args["path"].(string)
old, _ := args["old"].(string)
- new, _ := args["new"].(string)
+ newRaw, ok := args["new"]
+ if !ok {
+ return &ToolResult{Success: false, Error: "missing required arguments (path, old, new)"}, nil
+ }
+ new, ok := newRaw.(string)
+ if !ok {
+ return &ToolResult{Success: false, Error: "'new' must be a string"}, nil
+ }
if path == "" || old == "" {
return &ToolResult{Success: false, Error: "missing required arguments (path, old)"}, nil
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| // handleEditFile applies an edit to a file. | |
| // Security: Only edits files within ConnectorDir. | |
| func (h *ToolHandler) handleEditFile(ctx context.Context, args map[string]interface{}) (*ToolResult, error) { | |
| path, _ := args["path"].(string) | |
| old, _ := args["old"].(string) | |
| new, _ := args["new"].(string) | |
| if path == "" || old == "" { | |
| return &ToolResult{Success: false, Error: "missing required arguments (path, old)"}, nil | |
| } | |
| // Security: Resolve path relative to connector directory | |
| fullPath := filepath.Join(h.ConnectorDir, path) | |
| relPath, err := filepath.Rel(h.ConnectorDir, fullPath) | |
| if err != nil || strings.HasPrefix(relPath, "..") { | |
| return &ToolResult{Success: false, Error: "path must be within connector directory"}, nil | |
| } | |
| // Read current content | |
| content, err := os.ReadFile(fullPath) | |
| if err != nil { | |
| return &ToolResult{Success: false, Error: fmt.Sprintf("failed to read file: %v", err)}, nil | |
| } | |
| // Check if old string exists | |
| if !strings.Contains(string(content), old) { | |
| return &ToolResult{Success: false, Error: "old string not found in file"}, nil | |
| } | |
| // Show diff and ask for confirmation | |
| fmt.Printf("\n--- %s (before)\n", path) | |
| fmt.Printf("+++ %s (after)\n", path) | |
| fmt.Printf("@@ edit @@\n") | |
| fmt.Printf("-%s\n", strings.ReplaceAll(old, "\n", "\n-")) | |
| fmt.Printf("+%s\n", strings.ReplaceAll(new, "\n", "\n+")) | |
| fmt.Println() | |
| if h.DryRun { | |
| return &ToolResult{ | |
| Success: true, | |
| Data: map[string]interface{}{"applied": false, "reason": "dry run"}, | |
| }, nil | |
| } | |
| accepted, err := prompt.Confirm("Apply this change?") | |
| if err != nil { | |
| return &ToolResult{Success: false, Error: err.Error()}, nil | |
| } | |
| if !accepted { | |
| return &ToolResult{ | |
| Success: true, | |
| Data: map[string]interface{}{"applied": false, "reason": "user declined"}, | |
| }, nil | |
| } | |
| // Apply the edit | |
| newContent := strings.Replace(string(content), old, new, 1) | |
| if err := os.WriteFile(fullPath, []byte(newContent), 0644); err != nil { | |
| return &ToolResult{Success: false, Error: fmt.Sprintf("failed to write file: %v", err)}, nil | |
| } | |
| return &ToolResult{ | |
| Success: true, | |
| Data: map[string]interface{}{"applied": true}, | |
| }, nil | |
| } | |
| // handleEditFile applies an edit to a file. | |
| // Security: Only edits files within ConnectorDir. | |
| func (h *ToolHandler) handleEditFile(ctx context.Context, args map[string]interface{}) (*ToolResult, error) { | |
| path, _ := args["path"].(string) | |
| old, _ := args["old"].(string) | |
| newRaw, ok := args["new"] | |
| if !ok { | |
| return &ToolResult{Success: false, Error: "missing required arguments (path, old, new)"}, nil | |
| } | |
| new, ok := newRaw.(string) | |
| if !ok { | |
| return &ToolResult{Success: false, Error: "'new' must be a string"}, nil | |
| } | |
| if path == "" || old == "" { | |
| return &ToolResult{Success: false, Error: "missing required arguments (path, old)"}, nil | |
| } | |
| // Security: Resolve path relative to connector directory | |
| fullPath := filepath.Join(h.ConnectorDir, path) | |
| relPath, err := filepath.Rel(h.ConnectorDir, fullPath) | |
| if err != nil || strings.HasPrefix(relPath, "..") { | |
| return &ToolResult{Success: false, Error: "path must be within connector directory"}, nil | |
| } | |
| // Read current content | |
| content, err := os.ReadFile(fullPath) | |
| if err != nil { | |
| return &ToolResult{Success: false, Error: fmt.Sprintf("failed to read file: %v", err)}, nil | |
| } | |
| // Check if old string exists | |
| if !strings.Contains(string(content), old) { | |
| return &ToolResult{Success: false, Error: "old string not found in file"}, nil | |
| } | |
| // Show diff and ask for confirmation | |
| fmt.Printf("\n--- %s (before)\n", path) | |
| fmt.Printf("+++ %s (after)\n", path) | |
| fmt.Printf("@@ edit @@\n") | |
| fmt.Printf("-%s\n", strings.ReplaceAll(old, "\n", "\n-")) | |
| fmt.Printf("+%s\n", strings.ReplaceAll(new, "\n", "\n+")) | |
| fmt.Println() | |
| if h.DryRun { | |
| return &ToolResult{ | |
| Success: true, | |
| Data: map[string]interface{}{"applied": false, "reason": "dry run"}, | |
| }, nil | |
| } | |
| accepted, err := prompt.Confirm("Apply this change?") | |
| if err != nil { | |
| return &ToolResult{Success: false, Error: err.Error()}, nil | |
| } | |
| if !accepted { | |
| return &ToolResult{ | |
| Success: true, | |
| Data: map[string]interface{}{"applied": false, "reason": "user declined"}, | |
| }, nil | |
| } | |
| // Apply the edit | |
| newContent := strings.Replace(string(content), old, new, 1) | |
| if err := os.WriteFile(fullPath, []byte(newContent), 0644); err != nil { | |
| return &ToolResult{Success: false, Error: fmt.Sprintf("failed to write file: %v", err)}, nil | |
| } | |
| return &ToolResult{ | |
| Success: true, | |
| Data: map[string]interface{}{"applied": true}, | |
| }, nil | |
| } |
🤖 Prompt for AI Agents
In `@pkg/mcpclient/tools.go` around lines 196 - 262, The handler handleEditFile
currently allows missing "new" to default to empty string which can delete
content; validate the "new" argument from args before proceeding (similar to the
existing check for "path" and "old") by ensuring args["new"] is present and is a
non-empty string, and return a ToolResult with Success:false and a clear error
message (e.g., "missing required argument: new") if it isn't; update the
early-argument validation block (where path, old, new are read) to perform this
check and abort before showing the diff or writing the file.
Comment on lines
+264
to
+317
| // handleWriteFile writes a new file. | ||
| // Security: Only writes files within ConnectorDir. | ||
| func (h *ToolHandler) handleWriteFile(ctx context.Context, args map[string]interface{}) (*ToolResult, error) { | ||
| path, _ := args["path"].(string) | ||
| content, _ := args["content"].(string) | ||
|
|
||
| if path == "" { | ||
| return &ToolResult{Success: false, Error: "missing 'path' argument"}, nil | ||
| } | ||
|
|
||
| // Security: Resolve path relative to connector directory | ||
| fullPath := filepath.Join(h.ConnectorDir, path) | ||
| relPath, err := filepath.Rel(h.ConnectorDir, fullPath) | ||
| if err != nil || strings.HasPrefix(relPath, "..") { | ||
| return &ToolResult{Success: false, Error: "path must be within connector directory"}, nil | ||
| } | ||
|
|
||
| // Check if file exists | ||
| if _, err := os.Stat(fullPath); err == nil { | ||
| // File exists, ask for confirmation | ||
| overwrite, err := prompt.Confirm(fmt.Sprintf("File %s exists. Overwrite?", path)) | ||
| if err != nil { | ||
| return &ToolResult{Success: false, Error: err.Error()}, nil | ||
| } | ||
| if !overwrite { | ||
| return &ToolResult{ | ||
| Success: true, | ||
| Data: map[string]interface{}{"written": false, "reason": "user declined overwrite"}, | ||
| }, nil | ||
| } | ||
| } | ||
|
|
||
| if h.DryRun { | ||
| return &ToolResult{ | ||
| Success: true, | ||
| Data: map[string]interface{}{"written": false, "reason": "dry run"}, | ||
| }, nil | ||
| } | ||
|
|
||
| // Ensure parent directory exists | ||
| if err := os.MkdirAll(filepath.Dir(fullPath), 0755); err != nil { | ||
| return &ToolResult{Success: false, Error: fmt.Sprintf("failed to create directory: %v", err)}, nil | ||
| } | ||
|
|
||
| // Write the file | ||
| if err := os.WriteFile(fullPath, []byte(content), 0644); err != nil { | ||
| return &ToolResult{Success: false, Error: fmt.Sprintf("failed to write file: %v", err)}, nil | ||
| } | ||
|
|
||
| return &ToolResult{ | ||
| Success: true, | ||
| Data: map[string]interface{}{"written": true}, | ||
| }, nil | ||
| } |
There was a problem hiding this comment.
DryRun should bypass overwrite prompts.
In dry-run mode, prompting for overwrite can still block or fail in non-interactive runs. Short-circuit before prompting.
🐛 Proposed fix
relPath, err := filepath.Rel(h.ConnectorDir, fullPath)
if err != nil || strings.HasPrefix(relPath, "..") {
return &ToolResult{Success: false, Error: "path must be within connector directory"}, nil
}
+ if h.DryRun {
+ return &ToolResult{
+ Success: true,
+ Data: map[string]interface{}{"written": false, "reason": "dry run"},
+ }, nil
+ }
+
// Check if file exists
if _, err := os.Stat(fullPath); err == nil {
// File exists, ask for confirmation
@@
- if h.DryRun {
- return &ToolResult{
- Success: true,
- Data: map[string]interface{}{"written": false, "reason": "dry run"},
- }, nil
- }
-
// Ensure parent directory exists
if err := os.MkdirAll(filepath.Dir(fullPath), 0755); err != nil {
return &ToolResult{Success: false, Error: fmt.Sprintf("failed to create directory: %v", err)}, nil
}🤖 Prompt for AI Agents
In `@pkg/mcpclient/tools.go` around lines 264 - 317, In handleWriteFile, avoid
calling prompt.Confirm when h.DryRun is true: after detecting the file exists
(the os.Stat(fullPath) == nil branch) short-circuit and return the dry-run
result if h.DryRun is set instead of calling prompt.Confirm; move or add the
h.DryRun check before invoking prompt.Confirm so non-interactive dry runs never
block.
Comment on lines
+272
to
+328
| // DisplayBox prints text in a simple box. | ||
| // Used for consent dialogs and other important messages. | ||
| func DisplayBox(title, content string) { | ||
| width := 70 | ||
|
|
||
| // Top border | ||
| fmt.Println("+" + strings.Repeat("-", width-2) + "+") | ||
|
|
||
| // Title | ||
| if title != "" { | ||
| padding := (width - 2 - len(title)) / 2 | ||
| fmt.Printf("|%s%s%s|\n", strings.Repeat(" ", padding), title, strings.Repeat(" ", width-2-padding-len(title))) | ||
| fmt.Println("+" + strings.Repeat("-", width-2) + "+") | ||
| } | ||
|
|
||
| // Content - word wrap | ||
| lines := wrapText(content, width-4) | ||
| for _, line := range lines { | ||
| padding := width - 2 - len(line) | ||
| fmt.Printf("| %s%s|\n", line, strings.Repeat(" ", padding-1)) | ||
| } | ||
|
|
||
| // Bottom border | ||
| fmt.Println("+" + strings.Repeat("-", width-2) + "+") | ||
| } | ||
|
|
||
| // wrapText wraps text to the given width. | ||
| func wrapText(text string, width int) []string { | ||
| var lines []string | ||
| paragraphs := strings.Split(text, "\n") | ||
|
|
||
| for _, para := range paragraphs { | ||
| if para == "" { | ||
| lines = append(lines, "") | ||
| continue | ||
| } | ||
|
|
||
| words := strings.Fields(para) | ||
| if len(words) == 0 { | ||
| lines = append(lines, "") | ||
| continue | ||
| } | ||
|
|
||
| line := words[0] | ||
| for _, word := range words[1:] { | ||
| if len(line)+1+len(word) <= width { | ||
| line += " " + word | ||
| } else { | ||
| lines = append(lines, line) | ||
| line = word | ||
| } | ||
| } | ||
| lines = append(lines, line) | ||
| } | ||
|
|
||
| return lines | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Create a test Go program to verify strings.Repeat behavior with negative count
cat > /tmp/test_repeat.go << 'EOF'
package main
import (
"fmt"
"strings"
)
func main() {
fmt.Println("Testing strings.Repeat with negative count:")
// Test with negative count
defer func() {
if r := recover(); r != nil {
fmt.Printf("Panic recovered: %v\n", r)
} else {
fmt.Println("No panic occurred")
}
}()
result := strings.Repeat(" ", -5)
fmt.Printf("Result: '%s' (length: %d)\n", result, len(result))
}
EOF
# Run the test
cd /tmp && go run test_repeat.goRepository: ConductorOne/cone
Length of output: 270
🏁 Script executed:
#!/bin/bash
# Verify the padding calculation for various title lengths
cat > /tmp/test_padding.go << 'EOF'
package main
import (
"fmt"
)
func main() {
width := 70
testTitles := []string{
"Short", // len = 5
"This is a medium title that's reasonable", // len = 40
"This is a very long title that exceeds seventy characters in total length", // len = 77
}
for _, title := range testTitles {
padding := (width - 2 - len(title)) / 2
fmt.Printf("Title: '%s' (len=%d), width=%d, padding=%d\n", title, len(title), width, padding)
}
}
EOF
cd /tmp && go run test_padding.goRepository: ConductorOne/cone
Length of output: 300
🏁 Script executed:
#!/bin/bash
# Read the actual file from the repository
cat -n pkg/prompt/prompt.go | sed -n '272,328p'Repository: ConductorOne/cone
Length of output: 1788
🏁 Script executed:
#!/bin/bash
# Check if there are any tests for DisplayBox that might reveal other guards
rg -A 10 -B 2 "DisplayBox" --type go | head -100Repository: ConductorOne/cone
Length of output: 4646
Truncate titles to prevent negative padding in DisplayBox.
Line 282 calculates padding := (width - 2 - len(title)) / 2. When a title exceeds 68 characters, padding becomes negative, causing strings.Repeat on line 283 to panic with "strings: negative Repeat count". Truncate the title and clamp padding to non-negative values.
Proposed fix
// Title
if title != "" {
+ maxTitle := width - 2
+ if len(title) > maxTitle {
+ title = title[:maxTitle]
+ }
padding := (width - 2 - len(title)) / 2
+ if padding < 0 {
+ padding = 0
+ }
fmt.Printf("|%s%s%s|\n", strings.Repeat(" ", padding), title, strings.Repeat(" ", width-2-padding-len(title)))
fmt.Println("+" + strings.Repeat("-", width-2) + "+")
}🤖 Prompt for AI Agents
In `@pkg/prompt/prompt.go` around lines 272 - 328, The DisplayBox function can
panic when a long title makes padding negative; update DisplayBox to truncate
title to at most width-2 characters (or width-4 if you want spaces around),
compute padding from the truncated title, and clamp padding to zero (e.g., if
(width-2-len(truncatedTitle)) / 2 < 0 then set padding = 0) before calling
strings.Repeat; reference the DisplayBox function and the local variables title
and padding so you replace uses of title with the truncatedTitle and ensure
padding is non-negative to avoid strings.Repeat panics.
Comment on lines
+99
to
+101
| oldWd, _ := os.Getwd() | ||
| os.Chdir(tmpDir) | ||
| defer os.Chdir(oldWd) |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
cd pkg/scaffold && wc -l scaffold_test.goRepository: ConductorOne/cone
Length of output: 81
🏁 Script executed:
cd pkg/scaffold && sed -n '95,110p' scaffold_test.goRepository: ConductorOne/cone
Length of output: 334
🏁 Script executed:
cd pkg/scaffold && sed -n '190,200p' scaffold_test.goRepository: ConductorOne/cone
Length of output: 456
🏁 Script executed:
rg "Getwd|Chdir" pkg/scaffold/scaffold_test.go -B 2 -A 2Repository: ConductorOne/cone
Length of output: 195
Add error handling for os.Getwd and os.Chdir to prevent test failures.
Lines 99-101 ignore errors from os.Getwd() and os.Chdir(). If these calls fail, the test runs in the wrong directory and cleanup may not restore the original state. Use error checks and t.Cleanup() to ensure reliable test isolation:
Suggested fix
- oldWd, _ := os.Getwd()
- os.Chdir(tmpDir)
- defer os.Chdir(oldWd)
+ oldWd, err := os.Getwd()
+ if err != nil {
+ t.Fatalf("failed to get working dir: %v", err)
+ }
+ if err := os.Chdir(tmpDir); err != nil {
+ t.Fatalf("failed to chdir to temp dir: %v", err)
+ }
+ t.Cleanup(func() {
+ if err := os.Chdir(oldWd); err != nil {
+ t.Errorf("failed to restore working dir: %v", err)
+ }
+ })📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| oldWd, _ := os.Getwd() | |
| os.Chdir(tmpDir) | |
| defer os.Chdir(oldWd) | |
| oldWd, err := os.Getwd() | |
| if err != nil { | |
| t.Fatalf("failed to get working dir: %v", err) | |
| } | |
| if err := os.Chdir(tmpDir); err != nil { | |
| t.Fatalf("failed to chdir to temp dir: %v", err) | |
| } | |
| t.Cleanup(func() { | |
| if err := os.Chdir(oldWd); err != nil { | |
| t.Errorf("failed to restore working dir: %v", err) | |
| } | |
| }) |
🧰 Tools
🪛 GitHub Check: go-lint
[failure] 101-101:
Error return value of os.Chdir is not checked (errcheck)
[failure] 100-100:
Error return value of os.Chdir is not checked (errcheck)
🤖 Prompt for AI Agents
In `@pkg/scaffold/scaffold_test.go` around lines 99 - 101, Handle errors from
os.Getwd() and os.Chdir(tmpDir) and ensure cleanup uses t.Cleanup: call oldWd,
err := os.Getwd() and if err != nil use t.Fatalf to stop the test; then if
os.Chdir(tmpDir) returns an error use t.Fatalf; register t.Cleanup(func(){ if
err := os.Chdir(oldWd); err != nil { t.Fatalf("restore cwd: %v", err) } })
instead of defer so the original working directory is reliably restored even if
the test fails.
Comment on lines
+24
to
+31
| // Generate creates a new connector project from the standard template. | ||
| func Generate(cfg *Config) error { | ||
| if cfg.Name == "" { | ||
| return fmt.Errorf("scaffold: connector name is required") | ||
| } | ||
| if cfg.ModulePath == "" { | ||
| cfg.ModulePath = fmt.Sprintf("github.com/conductorone/baton-%s", cfg.Name) | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, let's explore the scaffold.go file to see the full context
cat -n pkg/scaffold/scaffold.go | head -100Repository: ConductorOne/cone
Length of output: 3491
🏁 Script executed:
# Search for the generateFile function mentioned in the scratchpad
rg -n "func generateFile" pkg/scaffold/Repository: ConductorOne/cone
Length of output: 144
🏁 Script executed:
# Look for all usages of cfg.Name to understand how it's used
rg -n "cfg\.Name" pkg/scaffold/Repository: ConductorOne/cone
Length of output: 685
🏁 Script executed:
# Check if there are Config struct definitions and existing validations
rg -n "type Config struct" pkg/scaffold/ -A 10Repository: ConductorOne/cone
Length of output: 1529
🏁 Script executed:
# Check if there are other callers of Generate to see if nil check is important
rg -n "Generate\(" pkg/scaffold/ -B 2 -A 2Repository: ConductorOne/cone
Length of output: 1975
🏁 Script executed:
# Check tests to see how Generate is called
find . -name "*test.go" -type f | head -5 | xargs rg -l "Generate"Repository: ConductorOne/cone
Length of output: 45
Add nil check and validate connector name to prevent panics and path traversal.
The Generate function accesses cfg.Name on line 26 without first checking if cfg is nil, which will panic. Additionally, cfg.Name is used to construct file paths (via filepath.Join on line 73) without validation. This allows path traversal attacks—if a user passes a name like "../../etc", it could write files outside the intended output directory. Also, invalid characters in the name can produce malformed Go module paths (line 30). Validate that cfg is non-nil and that the name contains only lowercase letters, digits, and hyphens (as indicated by the Config documentation example of "my-app").
🔒 Suggested fix
func Generate(cfg *Config) error {
+ if cfg == nil {
+ return fmt.Errorf("scaffold: config is required")
+ }
if cfg.Name == "" {
return fmt.Errorf("scaffold: connector name is required")
}
+ for _, r := range cfg.Name {
+ if !(r >= 'a' && r <= 'z' || r >= '0' && r <= '9' || r == '-') {
+ return fmt.Errorf("scaffold: invalid connector name %q", cfg.Name)
+ }
+ }
if cfg.ModulePath == "" {
cfg.ModulePath = fmt.Sprintf("github.com/conductorone/baton-%s", cfg.Name)
}🤖 Prompt for AI Agents
In `@pkg/scaffold/scaffold.go` around lines 24 - 31, Add a nil check for the
incoming *Config at the top of Generate and validate cfg.Name before using it:
return an error if cfg is nil; ensure cfg.Name matches a safe pattern (only
lowercase letters, digits, and hyphens, e.g. regex ^[a-z0-9-]+$), reject names
containing "..", leading/trailing slashes, or any path separators to prevent
path traversal, and return a clear error if invalid; only after validation, set
cfg.ModulePath (when empty) using the validated name to form
"github.com/conductorone/baton-<name>"; ensure any later use of cfg.Name with
filepath.Join or file writes relies on the validated value to avoid writing
outside the intended directory (references: Generate, Config, cfg.Name,
cfg.ModulePath, filepath.Join).
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.
Add scaffolding command: init and also a publish command (depends upon registry-api or similar service)
Add connector subcommands for dev workflow and AI analysis: analyze, consent, dev, validate commands with supporting
packages for MCP client, consent storage, and prompts.
Summary by CodeRabbit
Release Notes
New Features
Tests
✏️ Tip: You can customize this high-level summary in your review settings.