fix(mcp): audit/audit-diff return graceful JSON errors instead of IsError=true (#28291)

Author: Copilot

pkg/cli/mcp_server_error_codes_test.go

@@ -171,6 +171,12 @@ func TestMCPServer_ErrorCodes_InternalError(t *testing.T) {
 			return
 		}
 
+		// The tool must not set IsError=true – that would cause the bridge to exit with code 1.
+		if result.IsError {
+			t.Errorf("Expected IsError=false from audit tool (graceful JSON error), got IsError=true; content: %v", result.Content)
+			return
+		}
+
 		// The tool must return text content that is valid JSON containing an "error" field.
 		if len(result.Content) == 0 {
 			t.Fatal("Expected non-empty content from audit tool")
@@ -191,6 +197,9 @@ func TestMCPServer_ErrorCodes_InternalError(t *testing.T) {
 		if _, hasRunID := envelope["run_id_or_url"]; !hasRunID {
 			t.Errorf("Expected 'run_id_or_url' field in audit JSON response, got: %s", textContent.Text)
 		}
-		t.Logf("✓ audit returned JSON error envelope: %s", textContent.Text)
+		if _, hasSuggestions := envelope["suggestions"]; !hasSuggestions {
+			t.Errorf("Expected 'suggestions' field in audit JSON response, got: %s", textContent.Text)
+		}
+		t.Logf("✓ audit returned graceful JSON error envelope (IsError=false): %s", textContent.Text)
 	})
 }

pkg/cli/mcp_server_json_integration_test.go

@@ -236,7 +236,7 @@ func TestMCPServer_AuditToolReturnsValidJSON(t *testing.T) {
 	defer session.Close()
 
 	// Call audit tool with an invalid run ID
-	// The tool should return an MCP error for invalid run IDs
+	// The tool should return a graceful JSON error (IsError=false), not a protocol-level MCP error.
 	params := &mcp.CallToolParams{
 		Name: "audit",
 		Arguments: map[string]any{
@@ -245,8 +245,8 @@ func TestMCPServer_AuditToolReturnsValidJSON(t *testing.T) {
 	}
 	result, err := session.CallTool(ctx, params)
 	if err != nil {
-		// Expected behavior: audit command fails with invalid run ID
-		t.Logf("Audit tool correctly returned error for invalid run ID: %v", err)
+		// A permission or network error is acceptable in environments without GitHub credentials.
+		t.Logf("Audit tool returned protocol error (acceptable in CI without credentials): %v", err)
 		return
 	}
 
@@ -265,14 +265,14 @@ func TestMCPServer_AuditToolReturnsValidJSON(t *testing.T) {
 		t.Fatal("Expected non-empty text content from audit tool")
 	}
 
-	// The audit tool may return an error message for invalid run IDs
-	// We check if the output contains "Error:" which indicates an error message
-	if len(textContent.Text) >= 6 && textContent.Text[0:6] == "Error:" {
-		t.Logf("Audit tool returned error message as expected for invalid run ID")
+	// The audit tool must return valid JSON (graceful error envelope or success)
+	// and must NOT set IsError=true.
+	if result.IsError {
+		t.Errorf("Audit tool set IsError=true; expected graceful JSON error (IsError=false). Content: %s", textContent.Text)
 		return
 	}
 
-	// If not an error, verify JSON is valid
+	// Verify JSON is valid
 	if !isValidJSON(textContent.Text) {
 		// Try extracting JSON from output
 		jsonOutput := extractJSONFromOutput(textContent.Text)

pkg/cli/mcp_tools_privileged.go

@@ -351,16 +351,22 @@ Returns JSON with the following structure:
 
 			// Return a JSON error envelope instead of an MCP protocol error so
 			// callers always receive consistent JSON and the run ID is always present.
+			// IsError must be false so that callers (e.g. mcp_cli_bridge) treat this as
+			// a graceful not-found / failure response rather than a fatal protocol error.
 			errorEnvelope := map[string]any{
 				"error":         "failed to audit workflow run: " + mainMsg,
 				"run_id_or_url": args.RunIDOrURL,
+				"suggestions": []string{
+					"Verify the run ID is correct",
+					"Use the 'logs' tool to list recent run IDs",
+				},
 			}
 			jsonBytes, jsonErr := json.Marshal(errorEnvelope)
 			if jsonErr != nil {
 				return nil, nil, newMCPError(jsonrpc.CodeInternalError, "failed to audit workflow run: "+mainMsg, nil)
 			}
 			return &mcp.CallToolResult{
-				IsError: true,
+				IsError: false,
 				Content: []mcp.Content{&mcp.TextContent{Text: string(jsonBytes)}},
 			}, nil, nil
 		}
@@ -457,13 +463,17 @@ Returns JSON describing the differences between the base run and each comparison
 				"error":        "failed to diff workflow runs: " + mainMsg,
 				"base_run_id":  args.BaseRunID,
 				"compare_runs": args.CompareRunIDs,
+				"suggestions": []string{
+					"Verify the run IDs are correct",
+					"Use the 'logs' tool to list recent run IDs",
+				},
 			}
 			jsonBytes, jsonErr := json.Marshal(errorEnvelope)
 			if jsonErr != nil {
 				return nil, nil, newMCPError(jsonrpc.CodeInternalError, "failed to diff workflow runs: "+mainMsg, nil)
 			}
 			return &mcp.CallToolResult{
-				IsError: true,
+				IsError: false,
 				Content: []mcp.Content{&mcp.TextContent{Text: string(jsonBytes)}},
 			}, nil, nil
 		}

pkg/cli/mcp_tools_privileged_test.go

@@ -257,11 +257,12 @@ func TestAuditToolPassesGithubRepositoryAsRepoFlag(t *testing.T) {
 	}
 }
 
-// TestAuditToolErrorEnvelopeSetsIsErrorTrue verifies that audit command failures
-// returned as JSON envelopes are marked with IsError=true in the MCP response.
-func TestAuditToolErrorEnvelopeSetsIsErrorTrue(t *testing.T) {
+// TestAuditToolErrorEnvelopeSetsIsErrorFalse verifies that audit command failures
+// returned as JSON envelopes use IsError=false so callers receive graceful JSON
+// rather than a fatal MCP protocol error.
+func TestAuditToolErrorEnvelopeSetsIsErrorFalse(t *testing.T) {
 	mockExecCmd := func(ctx context.Context, args ...string) *exec.Cmd {
-		cmd := exec.CommandContext(ctx, os.Args[0], "-test.run=TestAuditToolErrorEnvelopeSetsIsErrorTrueHelperProcess")
+		cmd := exec.CommandContext(ctx, os.Args[0], "-test.run=TestAuditToolErrorEnvelopeHelperProcess")
 		cmd.Env = append(os.Environ(), "GH_AW_AUDIT_HELPER_PROCESS=1")
 		return cmd
 	}
@@ -278,7 +279,7 @@ func TestAuditToolErrorEnvelopeSetsIsErrorTrue(t *testing.T) {
 	})
 	require.NoError(t, err, "audit tool should return result envelope without protocol error")
 	require.NotNil(t, result, "result should not be nil")
-	assert.True(t, result.IsError, "audit error envelope should set IsError=true")
+	assert.False(t, result.IsError, "audit error envelope should set IsError=false (graceful JSON error)")
 	require.NotEmpty(t, result.Content, "result should contain text content")
 
 	textContent, ok := result.Content[0].(*mcp.TextContent)
@@ -290,9 +291,12 @@ func TestAuditToolErrorEnvelopeSetsIsErrorTrue(t *testing.T) {
 	errorMessage, ok := envelope["error"].(string)
 	require.True(t, ok, "error envelope should include string error field")
 	assert.Contains(t, errorMessage, "failed to audit workflow run", "error envelope should include contextual prefix")
+	suggestions, hasSuggestions := envelope["suggestions"]
+	assert.True(t, hasSuggestions, "error envelope should include suggestions")
+	assert.NotEmpty(t, suggestions, "suggestions should not be empty")
 }
 
-func TestAuditToolErrorEnvelopeSetsIsErrorTrueHelperProcess(t *testing.T) {
+func TestAuditToolErrorEnvelopeHelperProcess(t *testing.T) {
 	if os.Getenv("GH_AW_AUDIT_HELPER_PROCESS") != "1" {
 		return
 	}
@@ -331,9 +335,9 @@ func TestAuditTool_AcceptsDeprecatedMaxTokensParameter(t *testing.T) {
 	assert.NotContains(t, strings.Join(capturedArgs, " "), "max_tokens", "audit command args should ignore max_tokens")
 }
 
-func TestAuditDiffToolErrorEnvelopeSetsIsErrorTrue(t *testing.T) {
+func TestAuditDiffToolErrorEnvelopeSetsIsErrorFalse(t *testing.T) {
 	mockExecCmd := func(ctx context.Context, args ...string) *exec.Cmd {
-		cmd := exec.CommandContext(ctx, os.Args[0], "-test.run=TestAuditDiffToolErrorEnvelopeSetsIsErrorTrueHelperProcess")
+		cmd := exec.CommandContext(ctx, os.Args[0], "-test.run=TestAuditDiffToolErrorEnvelopeHelperProcess")
 		cmd.Env = append(os.Environ(), "GH_AW_AUDIT_DIFF_HELPER_PROCESS=1")
 		return cmd
 	}
@@ -353,7 +357,7 @@ func TestAuditDiffToolErrorEnvelopeSetsIsErrorTrue(t *testing.T) {
 	})
 	require.NoError(t, err, "audit-diff tool should return result envelope without protocol error")
 	require.NotNil(t, result, "result should not be nil")
-	assert.True(t, result.IsError, "audit-diff error envelope should set IsError=true")
+	assert.False(t, result.IsError, "audit-diff error envelope should set IsError=false (graceful JSON error)")
 	require.NotEmpty(t, result.Content, "result should contain text content")
 
 	textContent, ok := result.Content[0].(*mcp.TextContent)
@@ -365,9 +369,12 @@ func TestAuditDiffToolErrorEnvelopeSetsIsErrorTrue(t *testing.T) {
 	errorMessage, ok := envelope["error"].(string)
 	require.True(t, ok, "error envelope should include string error field")
 	assert.Contains(t, errorMessage, "failed to diff workflow runs", "error envelope should include contextual prefix")
+	suggestions, hasSuggestions := envelope["suggestions"]
+	assert.True(t, hasSuggestions, "error envelope should include suggestions")
+	assert.NotEmpty(t, suggestions, "suggestions should not be empty")
 }
 
-func TestAuditDiffToolErrorEnvelopeSetsIsErrorTrueHelperProcess(t *testing.T) {
+func TestAuditDiffToolErrorEnvelopeHelperProcess(t *testing.T) {
 	if os.Getenv("GH_AW_AUDIT_DIFF_HELPER_PROCESS") != "1" {
 		return
 	}