Repository navigation
fix(cli): keep truncated table cells on a rune boundary - #1727
Merged
Dumbris merged 1 commit intoOct 22, 2026
Merged
Conversation
Finishes the UTF-8-safe truncation sweep (smart-mcp-proxy#1647, smart-mcp-proxy#1664, smart-mcp-proxy#1665) in the part of cmd/mcpproxy those PRs set aside. Four table cuts existed as independent copies of the same arithmetic — shorten (profile list), truncateStr (registry list/search, catalog list) and inline s[:n] + "..." in the upstream tool list and the token list — and every one cut at a raw byte offset. Table cells carry upstream- and operator-authored text: registry and tool descriptions, catalog titles, server names. A non-ASCII value longer than its column puts a multi-byte rune astride the cut, so the cell written to the terminal ends in a partial rune. 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. Left alone: security_cmd.go and main.go carry the same shape but are under open PRs (smart-mcp-proxy#1714, smart-mcp-proxy#1720, smart-mcp-proxy#1725). The activity-type and hash cuts are over internal enums and hex, where no multi-byte rune can reach the boundary.
3 tasks done
|
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
Dumbris
pushed a commit
that referenced
this pull request
Oct 22, 2026
…1728) # Pull Request ## Description ### The refusal names a character that is not in the name `ValidateName` walks the name byte by byte — correctly, that loop is the confinement boundary — but reported the offending character as `string(name[i])`. Converting a *byte* to a string yields the code point of the same value, so the first byte of `語` (0xE8) was rendered as `è`: ``` before: invalid script name "語": character "è" is not allowed (names are 1-64 characters of A-Z, a-z, 0-9, '-' or '_' …) after: invalid script name "語": character "語" is not allowed (names are 1-64 characters of A-Z, a-z, 0-9, '-' or '_' …) ``` A caller is told to remove a character it never sent. A byte that begins no valid rune is not a character at all, and is now reported as one: ``` before: invalid script name "a\x80b": character "\u0080" is not allowed … after: invalid script name "a\x80b": byte 0x80 is not allowed … ``` **Reachability.** `codescripts.Resolve` / `ResolveScoped` call `ValidateName` before any filesystem access (`codescripts.go:394`), and `internal/httpapi/code_exec.go:244` maps `*InvalidNameError` straight to an HTTP 400 `INVALID_SCRIPT_NAME` whose message body **is** `invalidName.Error()`. So any `code_execution` call naming a non-ASCII script — i.e. the exact case the validator is there to refuse — returns this message to the agent. The input is one the caller chooses freely; nothing upstream narrows it to ASCII first. Note the order of the checks: the byte-length check fires first, so the mojibake path needs a *short* non-ASCII name (`語` is 3 bytes, under the 64 limit) and the echo path below needs a long one. Both are ordinary requests. ### The echoed name is cut inside a rune `truncateForMessage` bounds the echoed name at `MaxNameLen+16` bytes with a raw byte slice. For a long non-ASCII name the cut lands inside a rune and `%q` then renders the dangling bytes as `\xNN` escapes, so the refusal ends in hex noise instead of the name it is quoting. It now goes through `stringutil.UTF8Prefix`, the helper added in #1664 (see also #1647, #1665, and #1726 / #1727 which finish that sweep elsewhere). The byte-wise validation loop, the confinement property and every ASCII refusal are unchanged. ### One thing I did *not* change `len(name) > MaxNameLen` is a **byte** check, while `MaxNameLen` is documented and reported as a **character** count ("names are 1-64 characters"). They agree for every name the token can accept, since a valid name is ASCII — but a 30-character Japanese name is refused as "name is 90 characters long", which reads as a contradiction against a 64-character limit. Reporting the rune count instead makes it worse ("name is 30 characters long" against a 64-character limit), because the real divergence is in the check, not the message. That is a product decision about whether the limit is bytes or characters, so I left it exactly as it is rather than guess. Happy to follow up either way. ## 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 `internal/codescripts/invalid_name_message_test.go`: | Test | Asserts | |---|---| | `TestInvalidNameErrorNamesTheRealCharacter` | the refusal contains `語` and does not contain `è` | | `TestInvalidNameErrorReportsMalformedByte` | a stray `0x80` is reported as `byte 0x80` | | `TestInvalidNameErrorEchoIsNotCutInsideARune` | 3 byte offsets inside a 3-byte rune; no `\x` escape survives in the echo | | `TestValidateNameASCIIMessagesUnchanged` | pins the full text of the empty / disallowed-character / oversized ASCII refusals, and that a valid name is still accepted | **Mutation check.** With `internal/codescripts/codescripts.go` reverted to the base SHA (`14cb288`) and the test file kept, the first three fail by name and `TestValidateNameASCIIMessagesUnchanged` passes — which is what shows the change is behaviour-preserving for ASCII. With the fix applied all four pass. Commands run (exit status in brackets): ``` gofmt -l internal/codescripts [0, no output] git diff --check [0] go vet ./internal/codescripts [0] go test -count=1 ./internal/codescripts [0, whole package] ``` Not run: `-race` and `./scripts/run-all-tests.sh` (local disk constraints on this machine). Nothing here is concurrent.
Member
|
Merged. Thank you, @rudycelekli! Four separate copies of the same table-cell arithmetic are now one helper that cuts on a rune boundary, so server lists, profiles, registries and credentials all render non-ASCII names the same way. With #1726 that completes the sweep you started in #1647. Contributions like this are what keep MCPProxy moving, and they matter a lot to the project. One small ask, only if you feel like it: a short public note about your change (a tweet, a LinkedIn post, a line in a community you are part of) helps more people discover MCPProxy. No pressure at all, and I would be glad to repost it. |
Dumbris
added a commit
that referenced
this pull request
Oct 22, 2026
## Description `TestInformationalRescan_BusyServerIsRetriedNotDropped` (Spec 118, #1689) failed on `main` in 4 of the last 8 unit-test runs and was the only red check on contributor PRs #1727, #1725 and #1715. **Cause.** `fireInformationalRescan` deletes the server's entry from `infoRescanTimers` when the debounce timer fires and re-creates it only after the gates and the claim ran. On every busy fire there is a short window in which "is a timer armed" is false. The test sampled the map at one fixed instant (`time.Sleep(4 * debounce)`), which under CI load lands in that window. Reproduced locally: 1 failure in 150 runs with `GOMAXPROCS=1` under 2×ncpu CPU burners. **Fix (test only).** Read `infoRescanSeq` under the mutex before scheduling; the change itself takes `before+1`, so `require.Eventually(rescanSeq("srv") > before+1)` is a timing-free proof that a busy fire re-armed the timer. The "no scan attempted on top of a claimed slot" and "a busy slot is not a failed attempt" assertions are unchanged. **Verification.** 400 runs of the fixed test under the same CPU contention: 0 failures. `go test -race -count=3 -run TestInformationalRescan ./internal/server/` green. `golangci-lint` with the CI v2 config clean. Cross-reviewed with opencode (gpt-6.1-sol), two rounds: round 1 found the baseline read could hit the same gap (fixed by reading the counter before scheduling), round 2 CLEAN. No production code changes. ## Second flake, same PR `TestRefreshConcurrency_ScansDuringASlowSelfCheckSeeOldOrNew` (from #1720) failed the darwin-amd64 Build Binaries job on this PR: `119.6ms is not less than 109.7ms (baseline median 1.19ms)`. Both refresh-concurrency guards compared the single worst of 300 scans against `50*median+50ms`, which one GC pause on a macOS runner exceeds. They now bound p90 relative to the baseline and keep an absolute 2 s ceiling on the worst scan (no scheduler stall reaches it, any wait behind the gate or a fetch deadline does). 30 runs under CPU contention: 0 failures. opencode Sol 6.1 round 1 flagged that p90 alone could hide one scan absorbing a timed-out wait; the absolute ceiling is the answer to that.
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
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.
Pull Request
Description
Finishes the UTF-8-safe truncation sweep of #1647 / #1664 / #1665 in the part of
cmd/mcpproxythose PRs explicitly set aside ("I left the CLI table description truncations incmd/mcpproxyalone to keep this focused" — #1664). Companion to #1726, which covers the remaininginternal/sites.Four table cuts existed as independent copies of the same arithmetic, and all four cut at a raw byte offset:
shorten(profile_cmd.go)EffectiveServers, comma-joinedtruncateStr(registry_cmd.go,catalog_cmd.go)description, catalogTitledesc[:57] + "..."(upstream_cmd.go)Descriptionresult[:maxLen-3] + "..."(token_cmd.go)allowed_servers, comma-joinedReachability. Every one of these cells is upstream- or operator-authored text:
mcpproxy registry list/registry searchprint descriptions straight from the registry API response (mapString(r, "description")), andcatalog listprintsr.Title. Both are free text from a public registry.mcpproxy upstream toolsprints the toolDescriptionadvertised by the upstream server.allowed_serversandEffectiveServersare 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
truncateCellbuilt onstringutil.UTF8Prefix, in a file of its own so a fifth table does not grow a fifth copy.joinInterfaceSlicekeepsmaxLen == 0meaning "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.goandcmd/mcpproxy/main.gohave the same shape but are touched by open PRs (fix(security): close the round-2 TPA red-team findings — scanned baseline, diacritics, tr/el corpus, name squat (R2-1..R2-5) #1714, feat(security): Spec 101 US3 TPA bundle refresh lifecycle and publication channel #1720, fix(runtime): require an access token for OAuth status #1725), so I have not gone near them. Happy to follow up once those land.activity_cmd.go'sactType[:12]andupstream_cmd.go'shash[: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
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 themaxLen <= 3edge.TestJoinInterfaceSliceTruncationIsRuneSafe— drives the composition (joinInterfaceSlice->truncateCell), not just the helper, and pinsmaxLen == 0.Mutation check. With
truncateCellreverted to the raw byte cut (s[:maxLen-3] + "...") the two boundary tests fail by name — 12 sub-cases inTestTruncateCellKeepsRuneBoundary, plusTestJoinInterfaceSliceTruncationIsRuneSafe— whileTestTruncateCellASCIIUnchangedstill 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):
CGO_ENABLED=0because this machine's Command Line Tools SDK is currently broken for cgo linking (arm64e.x1-macos: unknown architectureout ofCoreFoundation.tbd), which is unrelated to this change. Nothing here is cgo-dependent. I did not run-raceor./scripts/run-all-tests.sh(local disk constraints); none of this code is concurrent.