From 5a24c8bb73f8e562e5d8b85ed38edb66e7f27956 Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Fri, 24 Apr 2026 14:34:00 +0000 Subject: [PATCH 1/2] Initial plan From fb8109ccb22408aa51af4957d36f66d9dc6ac33d Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Fri, 24 Apr 2026 14:57:44 +0000 Subject: [PATCH 2/2] fix: audit tool returns IsError=false with suggestions for graceful JSON errors - Change IsError from true to false in registerAuditTool error handler - Change IsError from true to false in registerAuditDiffTool error handler - Add 'suggestions' field to both error envelopes - Update unit tests to assert IsError=false and check suggestions field - Update integration tests to check IsError=false and remove stale comments Agent-Logs-Url: https://github.com/github/gh-aw/sessions/967df33d-882f-4c79-8d5b-172cddc4013c Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com> --- pkg/cli/mcp_server_error_codes_test.go | 11 ++++++++- pkg/cli/mcp_server_json_integration_test.go | 16 ++++++------ pkg/cli/mcp_tools_privileged.go | 14 +++++++++-- pkg/cli/mcp_tools_privileged_test.go | 27 +++++++++++++-------- 4 files changed, 47 insertions(+), 21 deletions(-) diff --git a/pkg/cli/mcp_server_error_codes_test.go b/pkg/cli/mcp_server_error_codes_test.go index 9ceeff6ad7e..0d3f0b7bba6 100644 --- a/pkg/cli/mcp_server_error_codes_test.go +++ b/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) }) } diff --git a/pkg/cli/mcp_server_json_integration_test.go b/pkg/cli/mcp_server_json_integration_test.go index b96a8a8576f..7b33107966c 100644 --- a/pkg/cli/mcp_server_json_integration_test.go +++ b/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) diff --git a/pkg/cli/mcp_tools_privileged.go b/pkg/cli/mcp_tools_privileged.go index d8efe2557c2..f15314f8d94 100644 --- a/pkg/cli/mcp_tools_privileged.go +++ b/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 } diff --git a/pkg/cli/mcp_tools_privileged_test.go b/pkg/cli/mcp_tools_privileged_test.go index ba608f144da..25839d9db2b 100644 --- a/pkg/cli/mcp_tools_privileged_test.go +++ b/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 }