RFR: 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
|
Important Review skippedReview was skipped due to path filters ⛔ Files ignored due to path filters (1)
CodeRabbit blocks several paths by default. You can override this behavior by explicitly including those paths in the path filters. For example, including You can disable this status message by setting the
WalkthroughAdds a versioned consent manager, a JSON‑RPC MCP client with tool‑callback handling and mock server, a file‑scoped ToolHandler, terminal prompting utilities, a connector scaffold generator, and extensive unit/integration tests across these components. Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant Server
participant ToolHandler
Client->>Server: initialize (JSON‑RPC)
Server-->>Client: initialize response
Client->>Server: connector_analyze (JSON‑RPC)
Server-->>Client: tool_call (e.g., read_files)
Client->>ToolHandler: HandleToolCall(read_files)
ToolHandler-->>Client: ToolResult (files/data)
Client->>Server: tool_result (JSON‑RPC)
Server-->>Client: tool_call (e.g., ask_user)
Client->>ToolHandler: HandleToolCall(ask_user)
ToolHandler-->>Client: ToolResult (user response)
Client->>Server: tool_result
Server-->>Client: complete (summary)
Client->>Client: produce final AnalysisResult
sequenceDiagram
participant User
participant System
participant Disk
User->>System: Load()
System->>Disk: Read ~/.cone/consent.json
alt file missing
Disk-->>System: ENOENT
System-->>User: ErrNoConsent
else file present
Disk-->>System: ConsentRecord JSON
alt version == CurrentConsentVersion
System-->>User: Valid ConsentRecord (timestamp, version)
else version mismatch
System-->>User: ErrConsentVersionMismatch
end
end
opt Save consent
User->>System: Save()
System->>Disk: Ensure ~/.cone (0700), write consent.json (0600)
Disk-->>System: success
System-->>User: OK
end
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 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🧪 Generate unit tests (beta)
Important Action Needed: IP Allowlist UpdateIf your organization protects your Git platform with IP whitelisting, please add the new CodeRabbit IP address to your allowlist:
Reviews will stop working after February 8, 2026 if the new IP is not added to your allowlist. 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.
pkg/consent/consent_test.go
Outdated
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.
pkg/mcpclient/client_test.go
Outdated
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.
pkg/mcpclient/client_test.go
Outdated
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.
pkg/mcpclient/tools.go
Outdated
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.
pkg/mcpclient/tools.go
Outdated
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.
pkg/scaffold/scaffold_test.go
Outdated
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).
- errcheck: handle ignored error returns (os.Chdir, json.Encode, etc.) - gosec: add ReadHeaderTimeout, use 0600 file permissions - errorlint: use errors.Is instead of direct comparison - predeclared: rename old/new to avoid shadowing - goconst: add statusToolCall constant - goimports: fix import ordering - godot: end comments with periods - gocritic: convert if-else chains to switches - tenv: use t.Setenv instead of os.Setenv in tests - noctx: use http.NewRequestWithContext - unused: remove dead code - scaffold: remove unused imports from generated templates - .golangci.yml: add exclusion rules for legitimate fmt.Print usage in CLI code and intentional nil error returns
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@pkg/prompt/prompt.go`:
- Around line 288-292: The loop printing boxed lines can panic when wrapText
returns an unwrappable long token because padding (calculated in the loop
iterating over lines) can be negative; update the logic in the for loop that
processes lines from wrapText so you clamp padding to a minimum of 0 before
using strings.Repeat (or skip repeating when negative), e.g., compute padding :=
width-2-len(line) and set padding = 0 if it's negative, then call fmt.Printf("|
%s%s|\n", line, strings.Repeat(" ", padding-1 >= 0 ? padding-1 : 0)); ensure the
change is applied to the block handling lines returned by wrapText so extremely
long words/URLs no longer cause a panic.
🧹 Nitpick comments (5)
pkg/consent/consent_test.go (1)
513-526: Consider usingstrings.Containsfrom the standard library.These custom helper functions duplicate functionality already available in the
stringspackage. Usingstrings.Contains(s, substr)would simplify the code and eliminate potential edge-case bugs.♻️ Suggested refactor
-// contains is a helper for string containment checks. -func contains(s, substr string) bool { - return len(s) >= len(substr) && (s == substr || len(substr) == 0 || - (len(s) > 0 && len(substr) > 0 && findSubstring(s, substr))) -} - -func findSubstring(s, substr string) bool { - for i := 0; i <= len(s)-len(substr); i++ { - if s[i:i+len(substr)] == substr { - return true - } - } - return false -}Then replace all
contains(...)calls withstrings.Contains(...)and add"strings"to the imports.pkg/mcpclient/integration_test.go (2)
134-141: Unsafe type assertions can cause test panics on unexpected responses.The type assertions at lines 134, 138, 155, and 156 don't use the comma-ok idiom. If the mock server returns an unexpected response structure, these will panic instead of providing a clear test failure.
♻️ Proposed fix
- result := resp["result"].(map[string]interface{}) - if result["status"] != "tool_call" { - t.Fatalf("expected status 'tool_call', got %v", result["status"]) - } - toolCall := result["tool_call"].(map[string]interface{}) - if toolCall["name"] != "read_files" { + result, ok := resp["result"].(map[string]interface{}) + if !ok { + t.Fatalf("expected result to be map, got %T", resp["result"]) + } + if result["status"] != "tool_call" { + t.Fatalf("expected status 'tool_call', got %v", result["status"]) + } + toolCall, ok := result["tool_call"].(map[string]interface{}) + if !ok { + t.Fatalf("expected tool_call to be map, got %T", result["tool_call"]) + } + if toolCall["name"] != "read_files" {
193-193: Consider handling json.Marshal error in test helper.While unlikely to fail with the simple map structure, handling this error would make debugging easier if test data becomes malformed.
♻️ Proposed fix
- body, _ := json.Marshal(req) + body, err := json.Marshal(req) + if err != nil { + t.Fatalf("failed to marshal request: %v", err) + }pkg/mcpclient/mock/server.go (2)
60-64: Usetime.Durationconstant for better readability.
10 * 1e9works but is less clear than using thetimepackage constants.♻️ Proposed fix
+import ( + // ... existing imports + "time" +) + s.server = &http.Server{ Addr: addr, Handler: mux, - ReadHeaderTimeout: 10 * 1e9, // 10 seconds + ReadHeaderTimeout: 10 * time.Second, }
88-93: ToolResults returns internal slice reference, allowing external mutation.The method returns the internal
toolResultsslice directly. Callers could inadvertently modify it, potentially causing flaky tests. Consider returning a copy.♻️ Proposed fix
func (s *Server) ToolResults() []map[string]interface{} { s.mu.Lock() defer s.mu.Unlock() - return s.toolResults + // Return a copy to prevent external mutation + result := make([]map[string]interface{}, len(s.toolResults)) + copy(result, s.toolResults) + return result }
Run `make install-hooks` to install a git pre-push hook that runs `make lint` before each push, preventing lint failures in CI.
- Migrate .golangci.yml to v2 format using golangci-lint migrate - Remove deprecated linters: tenv, varcheck, typecheck - Replace tenv with usetesting - Move goimports to formatters section - Fix noctx: use exec.CommandContext - Fix staticcheck ST1005: error strings shouldn't be capitalized or end with punctuation - add separate error message constants - Fix staticcheck QF1008: simplify embedded field selectors - Fix staticcheck QF1003: use tagged switch instead of if-else - Fix usetesting: use t.TempDir() and t.Chdir() instead of os.MkdirTemp/os.Chdir in tests - Add make install-hooks target to install pre-push hook
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Fix all issues with AI agents
In `@pkg/client/entitlement.go`:
- Around line 158-164: The loop constructing EntitlementWithBindings
dereferences v.AppEntitlementView.AppEntitlement without checking for nil;
update the construction in the loop that builds rv to first verify
v.AppEntitlementView != nil and v.AppEntitlementView.AppEntitlement != nil
(similar to GetEntitlement's two-step guard) and skip or handle entries with a
nil AppEntitlementView.AppEntitlement (e.g., continue the loop or set a safe
default) before calling AppEntitlement(*v.AppEntitlementView.AppEntitlement) so
PopulateExpandedMap and bindings remain valid for non-nil entries.
In `@pkg/mcpclient/integration_test.go`:
- Around line 177-213: sendJSONRPC currently ignores json.Marshal errors, can
hang without timeouts, and doesn’t check non-2xx HTTP status; update sendJSONRPC
to capture and handle the error from json.Marshal (fail the test with t.Fatalf),
use a context with timeout or ensure the provided *http.Client has a timeout
(e.g., create ctx, cancel := context.WithTimeout(context.Background(),
time.Second*5) and pass ctx to http.NewRequestWithContext), check
resp.StatusCode and t.Fatalf on non-2xx responses (include status and
body/snippet), and ensure you add the missing import (time) used for the
timeout; keep assertNoRPCError as-is for JSON-RPC payload checks.
In `@pkg/scaffold/scaffold_test.go`:
- Around line 61-65: The test currently only fails when os.Stat returns
IsNotExist, ignoring other errors; update the loop that iterates over
expectedFiles and checks path := filepath.Join(outputDir, f) so that after
calling if _, err := os.Stat(path); you treat any non-nil err as a test failure
(e.g., t.Fatalf or t.Errorf including err) and only skip when err is nil — i.e.,
replace the IsNotExist-only check with a check that fails the test for any err,
referencing the existing os.Stat call, expectedFiles slice, outputDir variable,
and the test's t instance.
pkg/mcpclient/integration_test.go
Outdated
Comment on lines
177
to
213
| func sendJSONRPC(t *testing.T, client *http.Client, url, method string, params interface{}) map[string]interface{} { | ||
| t.Helper() | ||
|
|
||
| req := map[string]interface{}{ | ||
| "jsonrpc": "2.0", | ||
| "id": 1, | ||
| "method": method, | ||
| } | ||
| if params != nil { | ||
| req["params"] = params | ||
| } | ||
|
|
||
| body, _ := json.Marshal(req) | ||
| httpReq, err := http.NewRequestWithContext(context.Background(), http.MethodPost, url, bytes.NewReader(body)) | ||
| if err != nil { | ||
| t.Fatalf("failed to create request: %v", err) | ||
| } | ||
| httpReq.Header.Set("Content-Type", "application/json") | ||
| resp, err := client.Do(httpReq) | ||
| if err != nil { | ||
| t.Fatalf("request failed: %v", err) | ||
| } | ||
| defer resp.Body.Close() | ||
|
|
||
| var result map[string]interface{} | ||
| if err := json.NewDecoder(resp.Body).Decode(&result); err != nil { | ||
| t.Fatalf("failed to decode response: %v", err) | ||
| } | ||
|
|
||
| return result | ||
| } | ||
|
|
||
| func assertNoRPCError(t *testing.T, resp map[string]interface{}) { | ||
| t.Helper() | ||
| if errObj, ok := resp["error"]; ok && errObj != nil { | ||
| t.Fatalf("unexpected JSON-RPC error: %v", errObj) | ||
| } |
There was a problem hiding this comment.
Harden sendJSONRPC error handling and timeouts.
json.Marshal errors are ignored, requests can hang indefinitely, and non‑2xx responses aren’t surfaced explicitly. Recommend handling marshal errors, adding a timeout, and checking HTTP status.
✅ Proposed fix
func sendJSONRPC(t *testing.T, client *http.Client, url, method string, params interface{}) map[string]interface{} {
t.Helper()
req := map[string]interface{}{
"jsonrpc": "2.0",
"id": 1,
"method": method,
}
if params != nil {
req["params"] = params
}
- body, _ := json.Marshal(req)
- httpReq, err := http.NewRequestWithContext(context.Background(), http.MethodPost, url, bytes.NewReader(body))
+ body, err := json.Marshal(req)
+ if err != nil {
+ t.Fatalf("failed to marshal request: %v", err)
+ }
+ ctx, cancel := context.WithTimeout(context.Background(), 5*time.Second)
+ defer cancel()
+ httpReq, err := http.NewRequestWithContext(ctx, http.MethodPost, url, bytes.NewReader(body))
if err != nil {
t.Fatalf("failed to create request: %v", err)
}
httpReq.Header.Set("Content-Type", "application/json")
resp, err := client.Do(httpReq)
if err != nil {
t.Fatalf("request failed: %v", err)
}
defer resp.Body.Close()
+ if resp.StatusCode < 200 || resp.StatusCode >= 300 {
+ t.Fatalf("unexpected HTTP status: %s", resp.Status)
+ }
var result map[string]interface{}
if err := json.NewDecoder(resp.Body).Decode(&result); err != nil {
t.Fatalf("failed to decode response: %v", err)
}Also add the missing import:
+ "time"📝 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
| func sendJSONRPC(t *testing.T, client *http.Client, url, method string, params interface{}) map[string]interface{} { | |
| t.Helper() | |
| req := map[string]interface{}{ | |
| "jsonrpc": "2.0", | |
| "id": 1, | |
| "method": method, | |
| } | |
| if params != nil { | |
| req["params"] = params | |
| } | |
| body, _ := json.Marshal(req) | |
| httpReq, err := http.NewRequestWithContext(context.Background(), http.MethodPost, url, bytes.NewReader(body)) | |
| if err != nil { | |
| t.Fatalf("failed to create request: %v", err) | |
| } | |
| httpReq.Header.Set("Content-Type", "application/json") | |
| resp, err := client.Do(httpReq) | |
| if err != nil { | |
| t.Fatalf("request failed: %v", err) | |
| } | |
| defer resp.Body.Close() | |
| var result map[string]interface{} | |
| if err := json.NewDecoder(resp.Body).Decode(&result); err != nil { | |
| t.Fatalf("failed to decode response: %v", err) | |
| } | |
| return result | |
| } | |
| func assertNoRPCError(t *testing.T, resp map[string]interface{}) { | |
| t.Helper() | |
| if errObj, ok := resp["error"]; ok && errObj != nil { | |
| t.Fatalf("unexpected JSON-RPC error: %v", errObj) | |
| } | |
| func sendJSONRPC(t *testing.T, client *http.Client, url, method string, params interface{}) map[string]interface{} { | |
| t.Helper() | |
| req := map[string]interface{}{ | |
| "jsonrpc": "2.0", | |
| "id": 1, | |
| "method": method, | |
| } | |
| if params != nil { | |
| req["params"] = params | |
| } | |
| body, err := json.Marshal(req) | |
| if err != nil { | |
| t.Fatalf("failed to marshal request: %v", err) | |
| } | |
| ctx, cancel := context.WithTimeout(context.Background(), 5*time.Second) | |
| defer cancel() | |
| httpReq, err := http.NewRequestWithContext(ctx, http.MethodPost, url, bytes.NewReader(body)) | |
| if err != nil { | |
| t.Fatalf("failed to create request: %v", err) | |
| } | |
| httpReq.Header.Set("Content-Type", "application/json") | |
| resp, err := client.Do(httpReq) | |
| if err != nil { | |
| t.Fatalf("request failed: %v", err) | |
| } | |
| defer resp.Body.Close() | |
| if resp.StatusCode < 200 || resp.StatusCode >= 300 { | |
| t.Fatalf("unexpected HTTP status: %s", resp.Status) | |
| } | |
| var result map[string]interface{} | |
| if err := json.NewDecoder(resp.Body).Decode(&result); err != nil { | |
| t.Fatalf("failed to decode response: %v", err) | |
| } | |
| return result | |
| } | |
| func assertNoRPCError(t *testing.T, resp map[string]interface{}) { | |
| t.Helper() | |
| if errObj, ok := resp["error"]; ok && errObj != nil { | |
| t.Fatalf("unexpected JSON-RPC error: %v", errObj) | |
| } | |
| } |
🤖 Prompt for AI Agents
In `@pkg/mcpclient/integration_test.go` around lines 177 - 213, sendJSONRPC
currently ignores json.Marshal errors, can hang without timeouts, and doesn’t
check non-2xx HTTP status; update sendJSONRPC to capture and handle the error
from json.Marshal (fail the test with t.Fatalf), use a context with timeout or
ensure the provided *http.Client has a timeout (e.g., create ctx, cancel :=
context.WithTimeout(context.Background(), time.Second*5) and pass ctx to
http.NewRequestWithContext), check resp.StatusCode and t.Fatalf on non-2xx
responses (include status and body/snippet), and ensure you add the missing
import (time) used for the timeout; keep assertNoRPCError as-is for JSON-RPC
payload checks.
Comment on lines
+61
to
+65
| for _, f := range expectedFiles { | ||
| path := filepath.Join(outputDir, f) | ||
| if _, err := os.Stat(path); os.IsNotExist(err) { | ||
| t.Errorf("expected file %s to exist", f) | ||
| } |
There was a problem hiding this comment.
Handle non-IsNotExist errors from os.Stat.
Right now any stat error other than “not exists” is silently ignored, which can hide failures (e.g., permission or transient IO). Treat any error as a test failure.
💡 Suggested fix
for _, f := range expectedFiles {
path := filepath.Join(outputDir, f)
- if _, err := os.Stat(path); os.IsNotExist(err) {
- t.Errorf("expected file %s to exist", f)
- }
+ if _, err := os.Stat(path); err != nil {
+ if os.IsNotExist(err) {
+ t.Errorf("expected file %s to exist", f)
+ } else {
+ t.Fatalf("failed to stat %s: %v", f, 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
| for _, f := range expectedFiles { | |
| path := filepath.Join(outputDir, f) | |
| if _, err := os.Stat(path); os.IsNotExist(err) { | |
| t.Errorf("expected file %s to exist", f) | |
| } | |
| for _, f := range expectedFiles { | |
| path := filepath.Join(outputDir, f) | |
| if _, err := os.Stat(path); err != nil { | |
| if os.IsNotExist(err) { | |
| t.Errorf("expected file %s to exist", f) | |
| } else { | |
| t.Fatalf("failed to stat %s: %v", f, err) | |
| } | |
| } | |
| } |
🤖 Prompt for AI Agents
In `@pkg/scaffold/scaffold_test.go` around lines 61 - 65, The test currently only
fails when os.Stat returns IsNotExist, ignoring other errors; update the loop
that iterates over expectedFiles and checks path := filepath.Join(outputDir, f)
so that after calling if _, err := os.Stat(path); you treat any non-nil err as a
test failure (e.g., t.Fatalf or t.Errorf including err) and only skip when err
is nil — i.e., replace the IsNotExist-only check with a check that fails the
test for any err, referencing the existing os.Stat call, expectedFiles slice,
outputDir variable, and the test's t instance.
CodeRabbit feedback: - pkg/client/entitlement.go: add nil check before dereferencing AppEntitlementView.AppEntitlement in loop - pkg/mcpclient/client.go: guard against nil ToolHandler before calling HandleToolCall - pkg/mcpclient/mock/server.go: log encoding errors in sendResult and sendError instead of ignoring them - pkg/mcpclient/mock/server_test.go: handle json.Marshal errors with t.Fatalf instead of ignoring - pkg/prompt/prompt.go: prevent DisplayBox panic when title or content line exceeds box width Scaffold template improvements based on learnings: - Add Entity Confusion warning to Grant/Revoke - documents which entity provides which data (principal=who, entitlement=what) - Add idempotency handling examples - show proper handling of "already exists" and "not found" errors - Add pagination termination comment - use API's next token, not result count
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@pkg/mcpclient/client.go`:
- Around line 26-27: The doc comment for HTTPClient promises a nil-safe default
but sendRequest dereferences HTTPClient directly; either ensure a nil fallback
is used or update the comment — to fix, add a nil-check in sendRequest that uses
http.DefaultClient (or c.NewClient-created default) when c.HTTPClient == nil,
e.g., reference the HTTPClient field and the sendRequest method to locate where
to insert the fallback, and keep NewClient as-is (or alternatively update the
HTTPClient comment to state it must be non-nil if you choose not to add the
fallback).
In `@pkg/scaffold/scaffold.go`:
- Around line 79-88: The current file creation in the scaffold logic uses
os.OpenFile with os.O_TRUNC which silently overwrites existing files; change the
open flags in the scaffold creation code (the os.OpenFile call that uses
fullPath and mode derived from tf.Mode) to use os.O_CREATE|os.O_EXCL|os.O_WRONLY
so the call fails if the target file already exists, then detect that error
(os.IsExist) and return a clear error message indicating the file already exists
(so callers can implement a --force behavior later); keep the existing default
mode fallback (mode == 0 -> 0644) and ensure the code still defers f.Close()
only on successful open.
🧹 Nitpick comments (5)
pkg/client/entitlement.go (1)
68-87: Consider adding defensive nil check for consistency.The
GetPathsmethod dereferencese.AppEntitlementViewat line 72 without a nil check. While theNewExpandableEntitlementWithBindingsconstructor guarantees non-nilAppEntitlementViewfor valid instances, adding a defensive check would be consistent with the guard added at lines 161-163 inSearchEntitlements.♻️ Optional defensive check
func (e *ExpandableEntitlementWithBindings) GetPaths() []PathDetails { if e == nil { return nil } + if e.AppEntitlementView == nil { + return nil + } view := *e.AppEntitlementViewpkg/mcpclient/mock/server.go (2)
61-74: Usetime.Durationconstant instead of numeric multiplication.
10 * 1e9is less readable than10 * time.Secondand requires the reader to know that nanoseconds are expected.Also,
Start()returns before the server is confirmed listening, which can cause race conditions in tests if callers immediately try to connect.♻️ Suggested improvement
s.server = &http.Server{ Addr: addr, Handler: mux, - ReadHeaderTimeout: 10 * 1e9, // 10 seconds + ReadHeaderTimeout: 10 * time.Second, }
89-94: Consider returning a copy of the slice for thread safety.The current implementation returns the internal slice reference after releasing the lock. Callers could inadvertently modify it, or observe modifications made by concurrent
handleToolResultcalls.For a test mock this is likely acceptable, but a defensive copy would be safer.
♻️ Defensive copy
func (s *Server) ToolResults() []map[string]interface{} { s.mu.Lock() defer s.mu.Unlock() - return s.toolResults + result := make([]map[string]interface{}, len(s.toolResults)) + copy(result, s.toolResults) + return result }pkg/prompt/prompt.go (2)
17-24: Consider using or removingErrCancelled.
ErrCancelledis defined but never returned by any function in this package. If signal handling (Ctrl+C) is planned, this is fine to keep. Otherwise, consider removing dead code or adding a TODO comment indicating the intended future use.
247-268: Optional: Consider deduplicating selected indices.If the user enters
1,2,1, the function returns[0, 1, 0]with duplicates. This may be intentional, but typically multi-select returns unique indices. A simple map-based dedup could prevent unexpected behavior.
Comment on lines
+79
to
+88
| // Create file | ||
| mode := tf.Mode | ||
| if mode == 0 { | ||
| mode = 0644 | ||
| } | ||
| f, err := os.OpenFile(fullPath, os.O_CREATE|os.O_WRONLY|os.O_TRUNC, mode) | ||
| if err != nil { | ||
| return fmt.Errorf("failed to create file: %w", err) | ||
| } | ||
| defer f.Close() |
There was a problem hiding this comment.
Prevent silent overwrites of existing files.
os.O_TRUNC will overwrite any existing files if the user runs scaffold in a non-empty directory, risking data loss. Consider failing when a target file already exists (and optionally add a --force flag later).
💡 Safer default (fail if file exists)
import (
+ "errors"
"fmt"
"os"
"path/filepath"
"strings"
"text/template"
)
@@
- f, err := os.OpenFile(fullPath, os.O_CREATE|os.O_WRONLY|os.O_TRUNC, mode)
+ f, err := os.OpenFile(fullPath, os.O_CREATE|os.O_WRONLY|os.O_EXCL, mode)
if err != nil {
+ if errors.Is(err, os.ErrExist) {
+ return fmt.Errorf("file already exists: %s", fullPath)
+ }
return fmt.Errorf("failed to create file: %w", 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
| // Create file | |
| mode := tf.Mode | |
| if mode == 0 { | |
| mode = 0644 | |
| } | |
| f, err := os.OpenFile(fullPath, os.O_CREATE|os.O_WRONLY|os.O_TRUNC, mode) | |
| if err != nil { | |
| return fmt.Errorf("failed to create file: %w", err) | |
| } | |
| defer f.Close() | |
| import ( | |
| "errors" | |
| "fmt" | |
| "os" | |
| "path/filepath" | |
| "strings" | |
| "text/template" | |
| ) |
Suggested change
| // Create file | |
| mode := tf.Mode | |
| if mode == 0 { | |
| mode = 0644 | |
| } | |
| f, err := os.OpenFile(fullPath, os.O_CREATE|os.O_WRONLY|os.O_TRUNC, mode) | |
| if err != nil { | |
| return fmt.Errorf("failed to create file: %w", err) | |
| } | |
| defer f.Close() | |
| // Create file | |
| mode := tf.Mode | |
| if mode == 0 { | |
| mode = 0644 | |
| } | |
| f, err := os.OpenFile(fullPath, os.O_CREATE|os.O_WRONLY|os.O_EXCL, mode) | |
| if err != nil { | |
| if errors.Is(err, os.ErrExist) { | |
| return fmt.Errorf("file already exists: %s", fullPath) | |
| } | |
| return fmt.Errorf("failed to create file: %w", err) | |
| } | |
| defer f.Close() |
🤖 Prompt for AI Agents
In `@pkg/scaffold/scaffold.go` around lines 79 - 88, The current file creation in
the scaffold logic uses os.OpenFile with os.O_TRUNC which silently overwrites
existing files; change the open flags in the scaffold creation code (the
os.OpenFile call that uses fullPath and mode derived from tf.Mode) to use
os.O_CREATE|os.O_EXCL|os.O_WRONLY so the call fails if the target file already
exists, then detect that error (os.IsExist) and return a clear error message
indicating the file already exists (so callers can implement a --force behavior
later); keep the existing default mode fallback (mode == 0 -> 0644) and ensure
the code still defers f.Close() only on successful open.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@pkg/mcpclient/mock/server.go`:
- Around line 90-95: ToolResults currently returns the internal slice
s.toolResults while holding the lock, which allows handleToolResult to mutate
the slice after the lock is released and causes a data race; fix by, inside
Server.ToolResults (while mu is locked), allocate a new slice of the same
length, copy the elements from s.toolResults into it (e.g.
make([]map[string]interface{}, len(s.toolResults)); copy(...)), then return that
new slice so callers get a snapshot and the internal slice can't be mutated
concurrently; keep the mu.Lock()/defer mu.Unlock() around the copy operation and
reference Server.ToolResults, s.toolResults, and handleToolResult to locate the
code.
🧹 Nitpick comments (1)
pkg/mcpclient/mock/server.go (1)
56-75: Server may not be listening whenStartreturns.
Start()returns immediately whileListenAndServeruns in a goroutine. Tests that connect right afterStart()may experience intermittent failures if the socket isn't bound yet. Additionally, startup errors (e.g., port already in use) are silently swallowed.Consider waiting for the server to be ready before returning, or at minimum providing a mechanism for callers to wait.
♻️ Suggested fix using a channel to signal readiness
func (s *Server) Start(addr string) error { s.addr = addr mux := http.NewServeMux() mux.HandleFunc("/", s.HandleMCP) - s.server = &http.Server{ - Addr: addr, - Handler: mux, - ReadHeaderTimeout: 10 * time.Second, - } + ln, err := net.Listen("tcp", addr) + if err != nil { + return fmt.Errorf("failed to listen on %s: %w", addr, err) + } + s.addr = ln.Addr().String() // capture actual address (useful for ":0") + + s.server = &http.Server{ + Handler: mux, + ReadHeaderTimeout: 10 * time.Second, + } go func() { - if err := s.server.ListenAndServe(); err != http.ErrServerClosed { + if err := s.server.Serve(ln); err != http.ErrServerClosed { fmt.Printf("mock server error: %v\n", err) } }() return nil }This also requires adding
"net"to the imports.
robert-chiniquy
changed the title
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@pkg/mcpclient/mock/server.go`:
- Around line 56-74: The Start method currently sets s.addr to the requested
addr and starts ListenAndServe in a goroutine, which hides bind failures and
leaves s.addr wrong for ephemeral ports; change Server.Start (s.addr, s.server)
to explicitly bind using net.Listen (e.g., net.Listen("tcp", addr)), return any
listen error immediately, create http.Server and call Serve(listener) in the
goroutine, and set s.addr to listener.Addr().String() after successful listen so
callers can use Addr() to connect; also propagate any listen error instead of
always returning nil.
- Around line 172-186: handleToolsCall resets s.currentStep but leaves
s.toolResults intact and doesn't guard against a nil scenario, causing results
to accumulate across runs and a possible panic in sendNextToolCallback; update
handleToolsCall to also clear s.toolResults (e.g., reset or reinitialize the
slice/map) while holding s.mu, and check that s.scenario is not nil before
calling sendNextToolCallback (returning a proper error via s.sendError if nil)
so each run starts fresh and avoids nil derefs in sendNextToolCallback.
pkg/mcpclient/mock/server.go
Outdated
Comment on lines
56
to
74
| // Start starts the mock server on the given address. | ||
| func (s *Server) Start(addr string) error { | ||
| s.addr = addr | ||
| mux := http.NewServeMux() | ||
| mux.HandleFunc("/", s.HandleMCP) | ||
|
|
||
| s.server = &http.Server{ | ||
| Addr: addr, | ||
| Handler: mux, | ||
| ReadHeaderTimeout: 10 * time.Second, | ||
| } | ||
|
|
||
| go func() { | ||
| if err := s.server.ListenAndServe(); err != http.ErrServerClosed { | ||
| fmt.Printf("mock server error: %v\n", err) | ||
| } | ||
| }() | ||
|
|
||
| return nil |
There was a problem hiding this comment.
Expose actual bound address and propagate bind failures.
Line 58 sets s.addr to the requested address verbatim, so passing ":0" leaves Addr() unusable for clients. Also, Line 69 logs ListenAndServe failures but Start still returns nil. Consider binding explicitly and storing the real address.
🐛 Proposed fix
import (
"context"
"encoding/json"
"fmt"
+ "net"
"net/http"
"os"
"sync"
"time"
)
func (s *Server) Start(addr string) error {
- s.addr = addr
mux := http.NewServeMux()
mux.HandleFunc("/", s.HandleMCP)
+ ln, err := net.Listen("tcp", addr)
+ if err != nil {
+ return err
+ }
+ s.addr = ln.Addr().String()
+
s.server = &http.Server{
Addr: addr,
Handler: mux,
ReadHeaderTimeout: 10 * time.Second,
}
go func() {
- if err := s.server.ListenAndServe(); err != http.ErrServerClosed {
+ if err := s.server.Serve(ln); err != nil && err != http.ErrServerClosed {
fmt.Printf("mock server error: %v\n", err)
}
}()
return 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
| // Start starts the mock server on the given address. | |
| func (s *Server) Start(addr string) error { | |
| s.addr = addr | |
| mux := http.NewServeMux() | |
| mux.HandleFunc("/", s.HandleMCP) | |
| s.server = &http.Server{ | |
| Addr: addr, | |
| Handler: mux, | |
| ReadHeaderTimeout: 10 * time.Second, | |
| } | |
| go func() { | |
| if err := s.server.ListenAndServe(); err != http.ErrServerClosed { | |
| fmt.Printf("mock server error: %v\n", err) | |
| } | |
| }() | |
| return nil | |
| import ( | |
| "context" | |
| "encoding/json" | |
| "fmt" | |
| "net" | |
| "net/http" | |
| "os" | |
| "sync" | |
| "time" | |
| ) | |
| // Start starts the mock server on the given address. | |
| func (s *Server) Start(addr string) error { | |
| mux := http.NewServeMux() | |
| mux.HandleFunc("/", s.HandleMCP) | |
| ln, err := net.Listen("tcp", addr) | |
| if err != nil { | |
| return err | |
| } | |
| s.addr = ln.Addr().String() | |
| s.server = &http.Server{ | |
| Addr: addr, | |
| Handler: mux, | |
| ReadHeaderTimeout: 10 * time.Second, | |
| } | |
| go func() { | |
| if err := s.server.Serve(ln); err != nil && err != http.ErrServerClosed { | |
| fmt.Printf("mock server error: %v\n", err) | |
| } | |
| }() | |
| return nil | |
| } |
🤖 Prompt for AI Agents
In `@pkg/mcpclient/mock/server.go` around lines 56 - 74, The Start method
currently sets s.addr to the requested addr and starts ListenAndServe in a
goroutine, which hides bind failures and leaves s.addr wrong for ephemeral
ports; change Server.Start (s.addr, s.server) to explicitly bind using
net.Listen (e.g., net.Listen("tcp", addr)), return any listen error immediately,
create http.Server and call Serve(listener) in the goroutine, and set s.addr to
listener.Addr().String() after successful listen so callers can use Addr() to
connect; also propagate any listen error instead of always returning nil.
pkg/mcpclient/mock/server.go
Outdated
Comment on lines
172
to
186
| func (s *Server) handleToolsCall(w http.ResponseWriter, id interface{}, params map[string]interface{}) { | ||
| name, _ := params["name"].(string) | ||
|
|
||
| if name != "connector_analyze" { | ||
| s.sendError(w, id, -32602, fmt.Sprintf("unknown tool: %s", name)) | ||
| return | ||
| } | ||
|
|
||
| s.mu.Lock() | ||
| s.currentStep = 0 | ||
| s.mu.Unlock() | ||
|
|
||
| // Return the first tool callback | ||
| s.sendNextToolCallback(w, id) | ||
| } |
There was a problem hiding this comment.
Reset per-run state and guard missing scenario.
Line 180 resets currentStep but leaves toolResults intact, so results accumulate across multiple runs and the summary becomes misleading. Also, a nil scenario will panic in sendNextToolCallback.
🐛 Proposed fix
func (s *Server) handleToolsCall(w http.ResponseWriter, id interface{}, params map[string]interface{}) {
name, _ := params["name"].(string)
if name != "connector_analyze" {
s.sendError(w, id, -32602, fmt.Sprintf("unknown tool: %s", name))
return
}
+ if s.scenario == nil {
+ s.sendError(w, id, -32603, "scenario not configured")
+ return
+ }
s.mu.Lock()
s.currentStep = 0
+ s.toolResults = s.toolResults[:0]
s.mu.Unlock()
// Return the first tool callback
s.sendNextToolCallback(w, id)
}🤖 Prompt for AI Agents
In `@pkg/mcpclient/mock/server.go` around lines 172 - 186, handleToolsCall resets
s.currentStep but leaves s.toolResults intact and doesn't guard against a nil
scenario, causing results to accumulate across runs and a possible panic in
sendNextToolCallback; update handleToolsCall to also clear s.toolResults (e.g.,
reset or reinitialize the slice/map) while holding s.mu, and check that
s.scenario is not nil before calling sendNextToolCallback (returning a proper
error via s.sendError if nil) so each run starts fresh and avoids nil derefs in
sendNextToolCallback.
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
New Features
Bug Fixes
Tests
Chores
✏️ Tip: You can customize this high-level summary in your review settings.