From 8acc6ec056b576ae16f9a901133e4abcbd320f59 Mon Sep 17 00:00:00 2001 From: "github-actions[bot]" <41898282+github-actions[bot]@users.noreply.github.com> Date: Fri, 17 Jul 2026 13:16:33 +0000 Subject: [PATCH 1/2] perf(logger): avoid double sanitize pass per RPC hop in logRPCMessageToAll MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Previously, logRPCMessageToAll called SanitizeString (10 compiled regexes) to build text/markdown previews, then LogRPCMessageJSONLWithTags called SanitizeJSON -> SanitizeString again on the same payload bytes — tripling the regex work on the hot logging path. This commit: - Adds SanitizeJSONFromString to the sanitize package: compacts an already-sanitized string into json.RawMessage, skipping the regex pass. - Splits LogRPCMessageJSONLWithTags into a public wrapper (unchanged API) and an internal logRPCMessageJSONLWithTagsAndSanitized that accepts a pre-sanitized json.RawMessage. - Updates logRPCMessageToAll to reuse the sanitized string already computed for preview truncation, eliminating two redundant regex passes per hop. - Adds unit tests for SanitizeJSONFromString including a consistency check and a benchmark for the new fast path. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- internal/logger/global_state.go | 2 +- internal/logger/jsonl_logger.go | 8 ++++- internal/logger/rpc_logger.go | 12 ++++--- internal/sanitize/sanitize.go | 9 +++-- internal/sanitize/sanitize_test.go | 58 ++++++++++++++++++++++++++++++ 5 files changed, 80 insertions(+), 9 deletions(-) diff --git a/internal/logger/global_state.go b/internal/logger/global_state.go index 05f7b44ee..ea0df5b96 100644 --- a/internal/logger/global_state.go +++ b/internal/logger/global_state.go @@ -297,7 +297,7 @@ var logFuncs = map[LogLevel]func(string, string, ...interface{}){ // The withGlobalLogger helper is used in: // - file_logger.go: logWithLevel (for FileLogger) // - markdown_logger.go: logWithMarkdown (for MarkdownLogger) -// - jsonl_logger.go: LogRPCMessageJSONLWithTags (for JSONLLogger) +// - jsonl_logger.go: LogRPCMessageJSONLWithTags and logRPCMessageJSONLWithTagsAndSanitized (for JSONLLogger) // - server_file_logger.go: logWithLevelAndServer (for ServerFileLogger) // - tools_logger.go: LogToolsForServer (for ToolsLogger) // - rpc_logger.go: logRPCMessageToAll and LogRPCMessage (for MarkdownLogger) diff --git a/internal/logger/jsonl_logger.go b/internal/logger/jsonl_logger.go index 7c2df2525..6b42b2efd 100644 --- a/internal/logger/jsonl_logger.go +++ b/internal/logger/jsonl_logger.go @@ -106,6 +106,12 @@ func LogRPCMessageJSONL(direction RPCMessageDirection, messageType RPCMessageTyp // LogRPCMessageJSONLWithTags logs an RPC message to the global JSONL logger with optional agent tag snapshots. // It uses the withGlobalLogger helper from global_helpers.go to handle mutex locking and nil-checking. func LogRPCMessageJSONLWithTags(direction RPCMessageDirection, messageType RPCMessageType, serverID, method string, payloadBytes []byte, err error, agentSecrecy, agentIntegrity []string) { + logRPCMessageJSONLWithTagsAndSanitized(direction, messageType, serverID, method, sanitize.SanitizeJSON(payloadBytes), err, agentSecrecy, agentIntegrity) +} + +// logRPCMessageJSONLWithTagsAndSanitized is the internal variant that accepts a pre-sanitized +// payload to avoid redundant regex passes when the caller already holds a sanitized copy. +func logRPCMessageJSONLWithTagsAndSanitized(direction RPCMessageDirection, messageType RPCMessageType, serverID, method string, sanitizedPayload json.RawMessage, err error, agentSecrecy, agentIntegrity []string) { withGlobalLogger(&globalJSONLMu, &globalJSONLLogger, func(logger *JSONLLogger) { entry := &JSONLRPCMessage{ Timestamp: time.Now().UTC().Format(jsonTimestampLayout), @@ -114,7 +120,7 @@ func LogRPCMessageJSONLWithTags(direction RPCMessageDirection, messageType RPCMe Direction: string(direction), ServerID: serverID, Method: method, - Payload: sanitize.SanitizeJSON(payloadBytes), + Payload: sanitizedPayload, } if len(agentSecrecy) > 0 { diff --git a/internal/logger/rpc_logger.go b/internal/logger/rpc_logger.go index 2194cda7c..5220ac53f 100644 --- a/internal/logger/rpc_logger.go +++ b/internal/logger/rpc_logger.go @@ -97,9 +97,10 @@ func newRPCMessageInfoFromSanitized(direction RPCMessageDirection, messageType R // logRPCMessageToAll routes a single RPC message to all log sinks (text, markdown, JSONL). // It uses the withGlobalLogger helper from global_helpers.go to handle mutex locking and nil-checking. func logRPCMessageToAll(direction RPCMessageDirection, messageType RPCMessageType, serverID, method string, payload []byte, err error, agentSecrecy, agentIntegrity []string) { - // Sanitize the payload string once, then truncate to different preview lengths. - // SanitizeString runs 10 compiled regex patterns; sharing the sanitized string - // between the text and markdown previews halves the sanitization work per RPC hop. + // Sanitize the payload string once, then share across all sinks. + // SanitizeString runs 10 compiled regex patterns; computing it once and + // passing the result to both preview builders and the JSONL logger avoids + // running the same patterns three times per RPC hop. sanitized := sanitize.SanitizeString(string(payload)) // Log to text file (with larger payload preview) @@ -112,8 +113,9 @@ func logRPCMessageToAll(direction RPCMessageDirection, messageType RPCMessageTyp logger.Log(LogLevelDebug, "rpc", "%s", formatRPCMessageMarkdown(infoMarkdown)) }) - // Log to JSONL file (full payload, sanitized) - LogRPCMessageJSONLWithTags(direction, messageType, serverID, method, payload, err, agentSecrecy, agentIntegrity) + // Log to JSONL file (full payload, sanitized). + // Use the pre-sanitized string variant to avoid running the 10 regex patterns again. + logRPCMessageJSONLWithTagsAndSanitized(direction, messageType, serverID, method, sanitize.SanitizeJSONFromString(sanitized), err, agentSecrecy, agentIntegrity) } // LogRPCRequest logs an RPC request message to text, markdown, and JSONL logs. diff --git a/internal/sanitize/sanitize.go b/internal/sanitize/sanitize.go index 116e9dc65..487128418 100644 --- a/internal/sanitize/sanitize.go +++ b/internal/sanitize/sanitize.go @@ -114,9 +114,14 @@ func RedactSecretMap(env map[string]string) map[string]string { // SanitizeJSON sanitizes a JSON payload by applying regex patterns to the entire string // It takes raw bytes, applies regex sanitization in one pass, and returns sanitized bytes func SanitizeJSON(payloadBytes []byte) json.RawMessage { - // Apply regex sanitization to the entire string in one pass - sanitized := SanitizeString(string(payloadBytes)) + return SanitizeJSONFromString(SanitizeString(string(payloadBytes))) +} +// SanitizeJSONFromString compacts an already-sanitized JSON string into a +// json.RawMessage. It skips the regex sanitization pass — callers that have +// already called SanitizeString on the payload string can use this to avoid +// running the 10 compiled regex patterns a second time. +func SanitizeJSONFromString(sanitized string) json.RawMessage { // Use json.Compact to validate and compact in one pass (avoids a full unmarshal+marshal cycle) var buf bytes.Buffer if err := json.Compact(&buf, []byte(sanitized)); err != nil { diff --git a/internal/sanitize/sanitize_test.go b/internal/sanitize/sanitize_test.go index b916182e2..ae0f33cc8 100644 --- a/internal/sanitize/sanitize_test.go +++ b/internal/sanitize/sanitize_test.go @@ -813,3 +813,61 @@ func BenchmarkSanitizeJSON_WithPrettyPrint(b *testing.B) { _ = SanitizeJSON(input) } } + +// TestSanitizeJSONFromString verifies that SanitizeJSONFromString produces the same +// output as SanitizeJSON when the caller pre-sanitizes the string, and that it correctly +// handles already-sanitized inputs without running the regex patterns again. +func TestSanitizeJSONFromString(t *testing.T) { + tests := []struct { + name string + input string + }{ + { + name: "clean compact JSON", + input: `{"method":"tools/list","id":1}`, + }, + { + name: "already redacted payload", + input: `{"token":"[REDACTED]","id":2}`, + }, + { + name: "pretty-printed JSON", + input: "{\n \"session\": \"abc\",\n \"ok\": true\n}", + }, + { + name: "invalid JSON falls back to error envelope", + input: `not valid json {`, + }, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + got := SanitizeJSONFromString(tt.input) + require.NotNil(t, got) + // Must always be valid JSON + var tmp interface{} + err := json.Unmarshal(got, &tmp) + assert.NoError(t, err, "SanitizeJSONFromString must always return valid JSON") + }) + } +} + +// TestSanitizeJSONFromString_Consistency verifies that SanitizeJSONFromString(SanitizeString(s)) +// produces the same result as SanitizeJSON([]byte(s)) for clean payloads. +func TestSanitizeJSONFromString_Consistency(t *testing.T) { + input := `{"method":"tools/call","id":42,"params":{"name":"search_code"}}` + direct := SanitizeJSON([]byte(input)) + twoStep := SanitizeJSONFromString(SanitizeString(input)) + assert.Equal(t, string(direct), string(twoStep), "SanitizeJSONFromString(SanitizeString) must match SanitizeJSON") +} + +// BenchmarkSanitizeJSONFromString_Compact benchmarks the fast path used by +// logRPCMessageToAll: sanitize once with SanitizeString, compact with SanitizeJSONFromString. +func BenchmarkSanitizeJSONFromString_Compact(b *testing.B) { + input := `{"session":"abc123","tool":"github___get_file_contents","result":{"content":"hello world","path":"README.md"}}` + sanitized := SanitizeString(input) + b.ReportAllocs() + b.ResetTimer() + for i := 0; i < b.N; i++ { + _ = SanitizeJSONFromString(sanitized) + } +} From e5fa39af491ad07e0fb5f04ba806e94a3b358ff3 Mon Sep 17 00:00:00 2001 From: Landon Cox Date: Fri, 17 Jul 2026 07:36:38 -0700 Subject: [PATCH 2/2] Potential fix for pull request finding Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com> --- internal/logger/rpc_logger.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/internal/logger/rpc_logger.go b/internal/logger/rpc_logger.go index 5220ac53f..1dd81cf34 100644 --- a/internal/logger/rpc_logger.go +++ b/internal/logger/rpc_logger.go @@ -100,7 +100,7 @@ func logRPCMessageToAll(direction RPCMessageDirection, messageType RPCMessageTyp // Sanitize the payload string once, then share across all sinks. // SanitizeString runs 10 compiled regex patterns; computing it once and // passing the result to both preview builders and the JSONL logger avoids - // running the same patterns three times per RPC hop. + // running the same patterns a second time per RPC hop. sanitized := sanitize.SanitizeString(string(payload)) // Log to text file (with larger payload preview)