Skip to content

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

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
pelikhan merged 2 commits into from
Apr 24, 2026
Merged

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
5a24c8b
Initial plan
Copilot Apr 24, 2026
fb8109c
fix: audit tool returns IsError=false with suggestions for graceful J…
Copilot Apr 24, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
11 changes: 10 additions & 1 deletion pkg/cli/mcp_server_error_codes_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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")
Expand All @@ -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)
})
}
16 changes: 8 additions & 8 deletions pkg/cli/mcp_server_json_integration_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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{
Expand All @@ -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
}
Comment on lines 246 to 251
Copy link

Copilot AI Apr 24, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In this integration test, any protocol-level error from session.CallTool is treated as acceptable and the test returns early. That makes the test unable to catch regressions where audit unexpectedly returns an MCP protocol error (the behavior this PR is trying to prevent). Consider only skipping/returning for known CI credential/network conditions (e.g., match specific auth/permission messages like isTestEnvPermissionError does in mcp_server_error_codes_test.go) and failing the test for other errors.

Copilot uses AI. Check for mistakes.

Expand All @@ -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)
Expand Down
14 changes: 12 additions & 2 deletions pkg/cli/mcp_tools_privileged.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
}
Expand Down Expand Up @@ -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
}
Expand Down
27 changes: 17 additions & 10 deletions pkg/cli/mcp_tools_privileged_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
}
Expand All @@ -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)
Expand All @@ -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
}
Expand Down Expand Up @@ -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
}
Expand All @@ -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)
Expand All @@ -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
}
Expand Down
Loading