Merge pull request #162 from githubnext/copilot/update-jsonrpc-rendering

Author: pelikhan

internal/logger/rpc_logger.go

@@ -30,7 +30,7 @@ const (
 	// MaxPayloadPreviewLengthText is the maximum number of characters to include in text log preview (10KB)
 	MaxPayloadPreviewLengthText = 10 * 1024 // 10KB
 	// MaxPayloadPreviewLengthMarkdown is the maximum number of characters to include in markdown log preview
-	MaxPayloadPreviewLengthMarkdown = 120
+	MaxPayloadPreviewLengthMarkdown = 512
 )
 
 // RPCMessageInfo contains information about an RPC message for logging
@@ -139,9 +139,42 @@ func formatRPCMessage(info *RPCMessageInfo) string {
 	return strings.Join(parts, " ")
 }
 
+// formatJSONWithoutFields formats JSON by removing specified fields and indenting with 2 spaces
+// Returns the formatted string and a boolean indicating if the JSON was valid
+func formatJSONWithoutFields(jsonStr string, fieldsToRemove []string) (string, bool) {
+	var data map[string]interface{}
+	if err := json.Unmarshal([]byte(jsonStr), &data); err != nil {
+		// If not valid JSON, return as-is with false
+		return jsonStr, false
+	}
+
+	// Remove specified fields
+	for _, field := range fieldsToRemove {
+		delete(data, field)
+	}
+
+	// Re-marshal with 2-space indentation
+	formatted, err := json.MarshalIndent(data, "", "  ")
+	if err != nil {
+		return jsonStr, false
+	}
+
+	// Compress JSON: remove newline after opening brace and before closing brace
+	result := string(formatted)
+	// Remove newline after opening brace
+	result = strings.Replace(result, "{\n", "{ ", 1)
+	// Remove newline before closing brace
+	lastNewline := strings.LastIndex(result, "\n}")
+	if lastNewline != -1 {
+		result = result[:lastNewline] + " }"
+	}
+
+	return result, true
+}
+
 // formatRPCMessageMarkdown formats an RPC message for markdown logging
 func formatRPCMessageMarkdown(info *RPCMessageInfo) string {
-	// Concise format: **server**→method payload
+	// Concise format: **server**→method \n~~~ \n{formatted json} \n~~~
 	var dir string
 	if info.Direction == RPCDirectionOutbound {
 		dir = "→"
@@ -160,9 +193,18 @@ func formatRPCMessageMarkdown(info *RPCMessageInfo) string {
 		}
 	}
 
-	// Add size and payload inline
+	// Add formatted payload in code block
 	if info.Payload != "" {
-		message += fmt.Sprintf(" `%s`", info.Payload)
+		// Remove jsonrpc and method fields, then format
+		formatted, isValidJSON := formatJSONWithoutFields(info.Payload, []string{"jsonrpc", "method"})
+		if isValidJSON {
+			// Valid JSON: use code block for better readability (compact formatting)
+			// Empty line before ~~~ per markdown convention
+			message += fmt.Sprintf(" \n\n~~~\n%s\n~~~", formatted)
+		} else {
+			// Invalid JSON: use inline backticks to avoid malformed markdown
+			message += fmt.Sprintf(" `%s`", formatted)
+		}
 	}
 
 	// Error (if present)

internal/logger/rpc_logger_test.go

@@ -179,9 +179,10 @@ func TestFormatRPCMessage(t *testing.T) {
 
 func TestFormatRPCMessageMarkdown(t *testing.T) {
 	tests := []struct {
-		name string
-		info *RPCMessageInfo
-		want []string // Strings that should be present in output
+		name    string
+		info    *RPCMessageInfo
+		want    []string // Strings that should be present in output
+		notWant []string // Strings that should NOT be present in output
 	}{
 		{
 			name: "outbound request",
@@ -191,9 +192,10 @@ func TestFormatRPCMessageMarkdown(t *testing.T) {
 				ServerID:    "github",
 				Method:      "tools/list",
 				PayloadSize: 50,
-				Payload:     `{"jsonrpc":"2.0"}`,
+				Payload:     `{"jsonrpc":"2.0","method":"tools/list","params":{}}`,
 			},
-			want: []string{"**github**→`tools/list`", "`{\"jsonrpc\":\"2.0\"}`"},
+			want:    []string{"**github**→`tools/list`", "~~~", `"params"`, "{}"},
+			notWant: []string{`"jsonrpc"`, `"method"`},
 		},
 		{
 			name: "inbound response",
@@ -204,7 +206,8 @@ func TestFormatRPCMessageMarkdown(t *testing.T) {
 				PayloadSize: 100,
 				Payload:     `{"result":{}}`,
 			},
-			want: []string{"**github**←resp", "`{\"result\":{}}`"},
+			want:    []string{"**github**←resp", "~~~", `"result"`},
+			notWant: []string{`"jsonrpc"`, `"method"`},
 		},
 		{
 			name: "response with error",
@@ -215,7 +218,21 @@ func TestFormatRPCMessageMarkdown(t *testing.T) {
 				PayloadSize: 100,
 				Error:       "Connection timeout",
 			},
-			want: []string{"**github**←resp", "⚠️`Connection timeout`"},
+			want:    []string{"**github**←resp", "⚠️`Connection timeout`"},
+			notWant: []string{},
+		},
+		{
+			name: "invalid JSON payload uses inline backticks",
+			info: &RPCMessageInfo{
+				Direction:   RPCDirectionOutbound,
+				MessageType: RPCMessageRequest,
+				ServerID:    "github",
+				Method:      "tools/call",
+				PayloadSize: 30,
+				Payload:     `{invalid json syntax}`,
+			},
+			want:    []string{"**github**→`tools/call`", "`{invalid json syntax}`"},
+			notWant: []string{"~~~"}, // Should NOT use code blocks for invalid JSON
 		},
 	}
 
@@ -228,6 +245,78 @@ func TestFormatRPCMessageMarkdown(t *testing.T) {
 					t.Errorf("Expected result to contain %q, got:\n%s", expected, result)
 				}
 			}
+
+			for _, notExpected := range tt.notWant {
+				if strings.Contains(result, notExpected) {
+					t.Errorf("Expected result NOT to contain %q, got:\n%s", notExpected, result)
+				}
+			}
+		})
+	}
+}
+
+func TestFormatJSONWithoutFields(t *testing.T) {
+	tests := []struct {
+		name           string
+		input          string
+		fieldsToRemove []string
+		wantContains   []string
+		wantNotContain []string
+		wantValid      bool
+	}{
+		{
+			name:           "remove jsonrpc and method",
+			input:          `{"jsonrpc":"2.0","method":"tools/call","params":{"arg":"value"},"id":1}`,
+			fieldsToRemove: []string{"jsonrpc", "method"},
+			wantContains:   []string{`"params"`, `"arg"`, `"value"`, `"id"`},
+			wantNotContain: []string{`"jsonrpc"`, `"method"`},
+			wantValid:      true,
+		},
+		{
+			name:           "indent with 2 spaces",
+			input:          `{"a":"b","c":{"d":"e"}}`,
+			fieldsToRemove: []string{},
+			wantContains:   []string{"  \"a\"", "  \"c\"", "    \"d\""},
+			wantNotContain: []string{},
+			wantValid:      true,
+		},
+		{
+			name:           "invalid JSON returns as-is with false",
+			input:          `{invalid json}`,
+			fieldsToRemove: []string{"jsonrpc"},
+			wantContains:   []string{`{invalid json}`},
+			wantNotContain: []string{},
+			wantValid:      false,
+		},
+		{
+			name:           "empty object",
+			input:          `{}`,
+			fieldsToRemove: []string{"jsonrpc"},
+			wantContains:   []string{`{}`},
+			wantNotContain: []string{},
+			wantValid:      true,
+		},
+	}
+
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			result, isValid := formatJSONWithoutFields(tt.input, tt.fieldsToRemove)
+
+			if isValid != tt.wantValid {
+				t.Errorf("Expected isValid=%v, got %v", tt.wantValid, isValid)
+			}
+
+			for _, want := range tt.wantContains {
+				if !strings.Contains(result, want) {
+					t.Errorf("Expected result to contain %q, got:\n%s", want, result)
+				}
+			}
+
+			for _, notWant := range tt.wantNotContain {
+				if strings.Contains(result, notWant) {
+					t.Errorf("Expected result NOT to contain %q, got:\n%s", notWant, result)
+				}
+			}
 		})
 	}
 }
@@ -410,7 +499,7 @@ func TestLogRPCRequestPayloadTruncation(t *testing.T) {
 	}
 	defer CloseMarkdownLogger()
 
-	// Create a large payload (> 10KB for text, > 120 chars for markdown)
+	// Create a large payload (> 10KB for text, > 512 chars for markdown)
 	largeData := strings.Repeat("x", 12*1024) // 12KB of x's
 	payload := []byte(`{"jsonrpc":"2.0","id":1,"method":"test","params":{"data":"` + largeData + `"}}`)
 	LogRPCRequest(RPCDirectionOutbound, "backend", "test", payload)
@@ -438,7 +527,7 @@ func TestLogRPCRequestPayloadTruncation(t *testing.T) {
 		t.Errorf("Text log contains more data than expected after truncation (should be ~10KB)")
 	}
 
-	// Check markdown log - should be truncated at 120 chars
+	// Check markdown log - should be truncated at 512 chars
 	mdLog := filepath.Join(logDir, "test.md")
 	mdContent, err := os.ReadFile(mdLog)
 	if err != nil {
@@ -450,9 +539,9 @@ func TestLogRPCRequestPayloadTruncation(t *testing.T) {
 		t.Errorf("Markdown log does not show truncation marker")
 	}
 
-	// Markdown should have much less data (truncated at 120 chars)
-	xCountMd := strings.Count(mdStr, strings.Repeat("x", 150))
+	// Markdown should have much less data (truncated at 512 chars)
+	xCountMd := strings.Count(mdStr, strings.Repeat("x", 600))
 	if xCountMd > 0 {
-		t.Errorf("Markdown log contains more data than expected after truncation (should be ~120 chars)")
+		t.Errorf("Markdown log contains more data than expected after truncation (should be ~512 chars)")
 	}
 }