Skip to content

Commit 3af8e25

Browse files
authored
fix(connect): keep string contents when stripping trailing commas (#1768)
## Bug `unmarshalLenientJSON` (the JSONC fallback added for #922) removes trailing commas with the regex `,\s*([}\]])`. The regex has no notion of JSON strings, so it also rewrites `, }` and `, ]` inside string values. The fallback runs whenever strict parsing fails, for example on a hand-edited `opencode.json` with one trailing comma. Connect, disconnect, undo and preview then work on the altered document, and connect writes it back to the client's config file. Another server's arguments are silently changed on disk. `stripJSONComments`, the other half of the same fallback, is already string-aware for exactly this reason. ## Trigger `~/.config/opencode/opencode.json`: ```json { "mcp": { "other": {"type": "local", "command": ["sh", "-c", "echo {a, } [b, ]"]}, }, } ``` After `mcpproxy connect opencode`, the other server's command on disk is `["sh", "-c", "echo {a} [b]"]`. ## Fix Replace the regex with `stripTrailingCommas`, a small scanner that tracks string and escape state the same way `stripJSONComments` does. It drops a `,` only when it is outside a string and the next non-whitespace byte is `}` or `]`. The now-unused `regexp` import is removed. ## Test `TestConnect_TrailingCommaCleanupPreservesStringContents` runs a real `Connect("opencode", ...)` on the file above and checks the other server's command is byte-for-byte unchanged after the write. The existing trailing-comma and JSONC tests still pass. ## Commands run - `CGO_ENABLED=0 go vet ./internal/connect`: clean - `CGO_ENABLED=0 go test -count=1 ./internal/connect`: ok - With the fix reverted (`git stash` of `connect.go`), the new test fails with `other server's command rewritten: got ["sh" "-c" "echo {a} [b]"]`. - `gofmt -l` on the edited files: no output
1 parent aedfc21 commit 3af8e25

2 files changed

Lines changed: 87 additions & 4 deletions

File tree

‎internal/connect/connect.go‎

Lines changed: 39 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -7,7 +7,6 @@ import (
77
"net/url"
88
"os"
99
"path/filepath"
10-
"regexp"
1110
"sort"
1211
"strings"
1312
"sync"
@@ -1701,8 +1700,6 @@ func entryPointsToBridge(entry map[string]interface{}, baseURL string) bool {
17011700
return hasBridgePkg && pointsToUs
17021701
}
17031702

1704-
var trailingCommaPattern = regexp.MustCompile(`,\s*([}\]])`)
1705-
17061703
func unmarshalLenientJSON(raw []byte, out interface{}) error {
17071704
if err := json.Unmarshal(raw, out); err == nil {
17081705
normalizeNilConfigMap(out)
@@ -1715,7 +1712,7 @@ func unmarshalLenientJSON(raw []byte, out interface{}) error {
17151712
if cerr != nil {
17161713
return cerr
17171714
}
1718-
cleaned = trailingCommaPattern.ReplaceAll(cleaned, []byte(`$1`))
1715+
cleaned = stripTrailingCommas(cleaned)
17191716
if err := json.Unmarshal(cleaned, out); err != nil {
17201717
return err
17211718
}
@@ -1791,6 +1788,44 @@ func stripJSONComments(raw []byte) ([]byte, error) {
17911788
return out, nil
17921789
}
17931790

1791+
// stripTrailingCommas drops a ',' that is followed (after whitespace) by '}'
1792+
// or ']'. It is string-aware like stripJSONComments: a ", }" inside a JSON
1793+
// string value (a shell snippet in args, say) is content and must survive,
1794+
// because connect writes the parsed document back to the client's file.
1795+
func stripTrailingCommas(raw []byte) []byte {
1796+
out := make([]byte, 0, len(raw))
1797+
inString := false
1798+
escaped := false
1799+
for i := 0; i < len(raw); i++ {
1800+
c := raw[i]
1801+
if inString {
1802+
switch {
1803+
case escaped:
1804+
escaped = false
1805+
case c == '\\':
1806+
escaped = true
1807+
case c == '"':
1808+
inString = false
1809+
}
1810+
out = append(out, c)
1811+
continue
1812+
}
1813+
if c == '"' {
1814+
inString = true
1815+
} else if c == ',' {
1816+
j := i + 1
1817+
for j < len(raw) && (raw[j] == ' ' || raw[j] == '\t' || raw[j] == '\n' || raw[j] == '\r') {
1818+
j++
1819+
}
1820+
if j < len(raw) && (raw[j] == '}' || raw[j] == ']') {
1821+
continue
1822+
}
1823+
}
1824+
out = append(out, c)
1825+
}
1826+
return out
1827+
}
1828+
17941829
// jsonHasComments reports whether stripping comments would change the content —
17951830
// i.e. the file actually uses JSONC comments (not just slashes inside strings).
17961831
// An unterminated block comment counts as commented (the write guard refuses).
Lines changed: 48 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,48 @@
1+
package connect
2+
3+
import (
4+
"encoding/json"
5+
"os"
6+
"path/filepath"
7+
"testing"
8+
)
9+
10+
// The trailing-comma cleanup must not touch ", }" / ", ]" inside string
11+
// values: connect writes the parsed document back, so a rewritten shell
12+
// snippet in another server's args would silently change on disk.
13+
func TestConnect_TrailingCommaCleanupPreservesStringContents(t *testing.T) {
14+
const snippet = `echo {a, } [b, ]`
15+
svc, home := testService(t)
16+
cfgPath := ConfigPath("opencode", home)
17+
if err := os.MkdirAll(filepath.Dir(cfgPath), 0o755); err != nil {
18+
t.Fatal(err)
19+
}
20+
if err := os.WriteFile(cfgPath, []byte(`{
21+
"mcp": {
22+
"other": {"type": "local", "command": ["sh", "-c", "echo {a, } [b, ]"]},
23+
},
24+
}`), 0o644); err != nil {
25+
t.Fatal(err)
26+
}
27+
28+
if res, err := svc.Connect("opencode", "mcpproxy", false); err != nil || !res.Success {
29+
t.Fatalf("connect: %+v, %v", res, err)
30+
}
31+
32+
raw, err := os.ReadFile(cfgPath)
33+
if err != nil {
34+
t.Fatal(err)
35+
}
36+
var data struct {
37+
MCP map[string]struct {
38+
Command []string `json:"command"`
39+
} `json:"mcp"`
40+
}
41+
if err := json.Unmarshal(raw, &data); err != nil {
42+
t.Fatal(err)
43+
}
44+
cmd := data.MCP["other"].Command
45+
if len(cmd) != 3 || cmd[2] != snippet {
46+
t.Fatalf("other server's command rewritten: got %q, want [sh -c %q]", cmd, snippet)
47+
}
48+
}

0 commit comments

Comments
 (0)