Skip to content

Commit 4f4e1bb

Browse files
rudyceleklirudycelekli
authored
fix(cli): keep truncated table cells on a rune boundary (#1727)
# Pull Request ## Description Finishes the UTF-8-safe truncation sweep of #1647 / #1664 / #1665 in the part of `cmd/mcpproxy` those PRs explicitly set aside ("I left the CLI table description truncations in `cmd/mcpproxy` alone to keep this focused" — #1664). Companion to #1726, which covers the remaining `internal/` sites. Four table cuts existed as independent copies of the same arithmetic, and all four cut at a raw byte offset: | Site | Column | Data | |---|---|---| | `shorten` (`profile_cmd.go`) | 30 | `EffectiveServers`, comma-joined | | `truncateStr` (`registry_cmd.go`, `catalog_cmd.go`) | 60 / 45 / 40 | registry `description`, catalog `Title` | | inline `desc[:57] + "..."` (`upstream_cmd.go`) | 60 | upstream tool `Description` | | inline `result[:maxLen-3] + "..."` (`token_cmd.go`) | 23 | `allowed_servers`, comma-joined | **Reachability.** Every one of these cells is upstream- or operator-authored text: - `mcpproxy registry list` / `registry search` print descriptions straight from the registry API response (`mapString(r, "description")`), and `catalog list` prints `r.Title`. Both are free text from a public registry. - `mcpproxy upstream tools` prints the tool `Description` advertised by the upstream server. - `allowed_servers` and `EffectiveServers` are server names, which the config only forbids from being empty or containing `:` (`internal/config/server_edition_config.go:176`) — a non-ASCII name is legal. A value longer than its column whose byte at the cut offset is a UTF-8 continuation byte ends in a partial rune on the terminal. With a 3-byte rune, 2 of every 3 offsets are mid-rune; the test sweeps all three at each of the six real column widths. All four now go through one `truncateCell` built on `stringutil.UTF8Prefix`, in a file of its own so a fifth table does not grow a fifth copy. `joinInterfaceSlice` keeps `maxLen == 0` meaning "do not truncate". ASCII output is byte-for-byte unchanged and the result still never exceeds the column width — both asserted. **Left alone, deliberately:** - `cmd/mcpproxy/security_cmd.go` and `cmd/mcpproxy/main.go` have the same shape but are touched by open PRs (#1714, #1720, #1725), so I have not gone near them. Happy to follow up once those land. - `activity_cmd.go`'s `actType[:12]` and `upstream_cmd.go`'s `hash[:12]` cut an internal activity-type enum and a hex digest, where no multi-byte rune can reach the boundary. Not touched — a guard there would protect against a state the code cannot produce. ## Testing - [x] I have tested these changes locally - [x] I have added/updated tests that prove my fix is effective or my feature works - [x] All existing tests pass New tests in `cmd/mcpproxy/table_cell_utf8_test.go`: - `TestTruncateCellKeepsRuneBoundary` — 6 real column widths x 3 byte offsets. - `TestTruncateCellASCIIUnchanged` — pins the ASCII output of all four removed helpers, including the `maxLen <= 3` edge. - `TestJoinInterfaceSliceTruncationIsRuneSafe` — drives the composition (`joinInterfaceSlice` -> `truncateCell`), not just the helper, and pins `maxLen == 0`. **Mutation check.** With `truncateCell` reverted to the raw byte cut (`s[:maxLen-3] + "..."`) the two boundary tests fail by name — 12 sub-cases in `TestTruncateCellKeepsRuneBoundary`, plus `TestJoinInterfaceSliceTruncationIsRuneSafe` — while `TestTruncateCellASCIIUnchanged` still passes, which is what shows the change is behaviour-preserving for ASCII. Restoring the fix returns the package to green. Commands run (exit status in brackets): ``` gofmt -l cmd/mcpproxy [0, no output] git diff --check [0] CGO_ENABLED=0 go vet ./cmd/mcpproxy [0] CGO_ENABLED=0 go build ./cmd/mcpproxy [0] CGO_ENABLED=0 go test -count=1 ./cmd/mcpproxy [0, whole package] ``` `CGO_ENABLED=0` because this machine's Command Line Tools SDK is currently broken for cgo linking (`arm64e.x1-macos: unknown architecture` out of `CoreFoundation.tbd`), which is unrelated to this change. Nothing here is cgo-dependent. I did not run `-race` or `./scripts/run-all-tests.sh` (local disk constraints); none of this code is concurrent.
1 parent f321b47 commit 4f4e1bb

8 files changed

Lines changed: 119 additions & 34 deletions

File tree

‎cmd/mcpproxy/catalog_cmd.go‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -429,7 +429,7 @@ func printCatalogTable(formatter clioutput.OutputFormatter, results []registries
429429
if r.FromCache {
430430
source += " (cached)"
431431
}
432-
rows = append(rows, []string{source, r.ID, truncateStr(r.Title, 40), r.Transport, added})
432+
rows = append(rows, []string{source, r.ID, truncateCell(r.Title, 40), r.Transport, added})
433433
}
434434
out, err := formatter.FormatTable(headers, rows)
435435
if err == nil {

‎cmd/mcpproxy/credential_cmd.go‎

Lines changed: 0 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -333,16 +333,6 @@ func renderCredentialDetail(c cliclient.CredentialStatus) string {
333333
return strings.TrimRight(b.String(), "\n")
334334
}
335335

336-
func truncateCell(s string, maxLen int) string {
337-
if len(s) <= maxLen {
338-
return s
339-
}
340-
if maxLen <= 3 {
341-
return s[:maxLen]
342-
}
343-
return s[:maxLen-3] + "..."
344-
}
345-
346336
// printCredentialBanner emits the FR-034 honesty line. Human-readable output
347337
// begins with it on stdout (contracts/rest-endpoints.md §12); structured
348338
// json/yaml output must stay machine-parseable, so there it goes to stderr.

‎cmd/mcpproxy/profile_cmd.go‎

Lines changed: 1 addition & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -90,7 +90,7 @@ func runProfileList(_ *cobra.Command, _ []string) error {
9090
rows := make([][]string, 0, len(list.Profiles))
9191
for _, p := range list.Profiles {
9292
rows = append(rows, []string{
93-
p.Name, dash(p.Title), shorten(dash(strings.Join(p.EffectiveServers, ",")), 30), dash(p.MaxTier),
93+
p.Name, dash(p.Title), truncateCell(dash(strings.Join(p.EffectiveServers, ",")), 30), dash(p.MaxTier),
9494
strconv.Itoa(p.ToolCounts.Read), strconv.Itoa(p.ToolCounts.Write), strconv.Itoa(p.ToolCounts.Destructive),
9595
strconv.Itoa(p.ToolCounts.UnannotatedHidden), usedBySummary(p.UsedBy),
9696
strconv.Itoa(p.Calls24h), strconv.Itoa(p.Blocked24h),
@@ -125,13 +125,6 @@ func plural(n int) string {
125125
return "s"
126126
}
127127

128-
func shorten(s string, max int) string {
129-
if len(s) <= max {
130-
return s
131-
}
132-
return s[:max-3] + "..."
133-
}
134-
135128
type profileShowOpts struct {
136129
effective bool
137130
client string

‎cmd/mcpproxy/registry_cmd.go‎

Lines changed: 2 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -559,7 +559,7 @@ func renderRegistries(formatter clioutput.OutputFormatter, regs []map[string]int
559559
headers := []string{"ID", "NAME", "DESCRIPTION"}
560560
rows := make([][]string, 0, len(regs))
561561
for _, r := range regs {
562-
rows = append(rows, []string{mapString(r, "id"), mapString(r, "name"), truncateStr(mapString(r, "description"), 60)})
562+
rows = append(rows, []string{mapString(r, "id"), mapString(r, "name"), truncateCell(mapString(r, "description"), 60)})
563563
}
564564
out, err := formatter.FormatTable(headers, rows)
565565
if err != nil {
@@ -586,7 +586,7 @@ func renderRegistryServers(formatter clioutput.OutputFormatter, servers []map[st
586586
if installCmd == "" {
587587
installCmd = "-"
588588
}
589-
rows = append(rows, []string{mapString(s, "id"), mapString(s, "name"), truncateStr(mapString(s, "description"), 45), installCmd})
589+
rows = append(rows, []string{mapString(s, "id"), mapString(s, "name"), truncateCell(mapString(s, "description"), 45), installCmd})
590590
}
591591
out, err := formatter.FormatTable(headers, rows)
592592
if err != nil {
@@ -611,13 +611,6 @@ func mapString(m map[string]interface{}, key string) string {
611611
return ""
612612
}
613613

614-
func truncateStr(s string, max int) string {
615-
if len(s) > max {
616-
return s[:max-3] + "..."
617-
}
618-
return s
619-
}
620-
621614
// loadRegistryConfig loads config for the registry commands, honoring the
622615
// command's --config flag and the global --data-dir, falling back to defaults
623616
// so 'list'/'search' still work without a config file.

‎cmd/mcpproxy/table_cell.go‎

Lines changed: 30 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,30 @@
1+
package main
2+
3+
import (
4+
"github.com/smart-mcp-proxy/mcpproxy-go/internal/stringutil"
5+
)
6+
7+
// truncateCell bounds one CLI table cell to maxLen bytes, marking a shortened
8+
// value with an ellipsis. It is the single cut used by every table in this
9+
// package: `shorten` (profile), `truncateStr` (registry, catalog) and two
10+
// inline `s[:n] + "..."` sites (upstream tool list, token list) each carried
11+
// an independent copy of the same arithmetic, and all of them cut at a raw
12+
// byte offset.
13+
//
14+
// The cut goes through stringutil.UTF8Prefix, finishing the sweep of #1647 /
15+
// #1664 / #1665 in the part of cmd/mcpproxy those PRs set aside. Table cells
16+
// carry upstream- and operator-authored text — registry and tool
17+
// descriptions, catalog titles, server names — so a non-ASCII value longer
18+
// than its column can put a multi-byte rune astride the cut, and a raw byte
19+
// slice then writes a partial rune to the terminal.
20+
//
21+
// The result never exceeds maxLen; ASCII behaviour is unchanged.
22+
func truncateCell(s string, maxLen int) string {
23+
if len(s) <= maxLen {
24+
return s
25+
}
26+
if maxLen <= 3 {
27+
return stringutil.UTF8Prefix(s, maxLen)
28+
}
29+
return stringutil.UTF8Prefix(s, maxLen-3) + "..."
30+
}

‎cmd/mcpproxy/table_cell_utf8_test.go‎

Lines changed: 82 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,82 @@
1+
package main
2+
3+
import (
4+
"fmt"
5+
"strings"
6+
"testing"
7+
"unicode/utf8"
8+
)
9+
10+
// Finishing the sweep of #1647 / #1664 / #1665 in cmd/mcpproxy. Every CLI
11+
// table cut was a raw byte slice, so a non-ASCII registry/tool description,
12+
// catalog title or server name longer than its column ended in a partial
13+
// rune on the terminal.
14+
//
15+
// The 1-byte padding shifts the cut through all three offsets inside a 3-byte
16+
// rune; only two of the three are mid-rune, so a single fixed width would
17+
// pass by luck.
18+
func TestTruncateCellKeepsRuneBoundary(t *testing.T) {
19+
for _, maxLen := range []int{10, 24, 30, 40, 45, 60} {
20+
for pad := 0; pad < 3; pad++ {
21+
cell := strings.Repeat("a", pad) + strings.Repeat("語", 100)
22+
23+
got := truncateCell(cell, maxLen)
24+
if len(got) > maxLen {
25+
t.Errorf("maxLen=%d pad=%d: %d bytes, over the column width", maxLen, pad, len(got))
26+
}
27+
if !utf8.ValidString(got) {
28+
t.Errorf("maxLen=%d pad=%d: cell ends in a partial rune: % x", maxLen, pad, []byte(got))
29+
}
30+
}
31+
}
32+
}
33+
34+
// Short cells and exact fits are returned verbatim, and ASCII truncation is
35+
// byte-for-byte what the four removed helpers produced.
36+
func TestTruncateCellASCIIUnchanged(t *testing.T) {
37+
cases := []struct {
38+
s string
39+
maxLen int
40+
want string
41+
}{
42+
{"short", 10, "short"},
43+
{"exactlyten", 10, "exactlyten"},
44+
{strings.Repeat("x", 30), 10, strings.Repeat("x", 7) + "..."},
45+
{strings.Repeat("x", 100), 60, strings.Repeat("x", 57) + "..."},
46+
{strings.Repeat("x", 10), 3, "xxx"},
47+
{strings.Repeat("x", 10), 2, "xx"},
48+
}
49+
for _, c := range cases {
50+
if got := truncateCell(c.s, c.maxLen); got != c.want {
51+
t.Errorf("truncateCell(%d chars, %d) = %q, want %q", len(c.s), c.maxLen, got, c.want)
52+
}
53+
}
54+
}
55+
56+
// joinInterfaceSlice keeps maxLen == 0 meaning "do not truncate", and takes
57+
// the rune-safe cut for a positive width. allowed_servers holds server names,
58+
// which the config only forbids from being empty or containing ':'.
59+
func TestJoinInterfaceSliceTruncationIsRuneSafe(t *testing.T) {
60+
name := strings.Repeat("語", 20)
61+
m := map[string]interface{}{"allowed_servers": []interface{}{name, name}}
62+
63+
if got := joinInterfaceSlice(m, "allowed_servers", 0); got != name+","+name {
64+
t.Errorf("maxLen 0 must not truncate: got %d bytes, want %d", len(got), len(name)*2+1)
65+
}
66+
67+
got := joinInterfaceSlice(m, "allowed_servers", 23)
68+
if len(got) > 23 {
69+
t.Errorf("got %d bytes, over the 23-byte column", len(got))
70+
}
71+
if !utf8.ValidString(got) {
72+
t.Errorf("joined server names end in a partial rune: % x", []byte(got))
73+
}
74+
if !strings.HasSuffix(got, "...") {
75+
t.Errorf("a shortened value must stay marked: %q", got)
76+
}
77+
fmt.Fprint(nopWriter{}, got)
78+
}
79+
80+
type nopWriter struct{}
81+
82+
func (nopWriter) Write(p []byte) (int, error) { return len(p), nil }

‎cmd/mcpproxy/token_cmd.go‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -668,8 +668,8 @@ func joinInterfaceSlice(m map[string]interface{}, key string, maxLen int) string
668668
strs[i] = fmt.Sprintf("%v", s)
669669
}
670670
result := strings.Join(strs, ",")
671-
if maxLen > 0 && len(result) > maxLen {
672-
result = result[:maxLen-3] + "..."
671+
if maxLen > 0 {
672+
result = truncateCell(result, maxLen)
673673
}
674674
return result
675675
}

‎cmd/mcpproxy/upstream_cmd.go‎

Lines changed: 1 addition & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -2536,10 +2536,7 @@ func runUpstreamInspect(_ *cobra.Command, args []string) error {
25362536
approvedCount++
25372537
}
25382538

2539-
desc := r.Description
2540-
if len(desc) > 60 {
2541-
desc = desc[:57] + "..."
2542-
}
2539+
desc := truncateCell(r.Description, 60)
25432540

25442541
hash := r.Hash
25452542
if len(hash) > 12 {

0 commit comments

Comments
 (0)