From befb3b4cef0bf602fff54ce2cb63f2b7aaad8e45 Mon Sep 17 00:00:00 2001
From: Sathpal-Singh <sathpal.gnec@gmail.com>
Date: Wed, 21 Oct 2026 21:47:25 +0530
Subject: [PATCH] fix(commands): keep a failed command's output in its error

When kubectl, helm, istioctl or cilium exits non-zero, executeCommand
wraps only the exit error, so the agent is told "exit status 1" and
never sees the command's own message, such as kubectl's
"Error from server (Forbidden): ...". The output is logged by the shell
executor but not returned.

Include the trimmed output in the error's cause. The original error
stays wrapped, and the error creators now see the real message, so the
suggestions they pick from it (permission, not found, already exists,
connection refused) apply instead of the generic fallback.

Signed-off-by: Sathpal-Singh <sathpal.gnec@gmail.com>
---
 internal/commands/builder.go             |  7 +++
 internal/commands/builder_errors_test.go | 63 ++++++++++++++++++++++++
 2 files changed, 70 insertions(+)
 create mode 100644 internal/commands/builder_errors_test.go

diff --git a/internal/commands/builder.go b/internal/commands/builder.go
index c9baed8..03c5b8d 100644
--- a/internal/commands/builder.go
+++ b/internal/commands/builder.go
@@ -427,6 +427,13 @@ func (cb *CommandBuilder) executeCommand(ctx context.Context, command string, ar
 	executor := cmd.GetShellExecutor(ctx)
 	output, err := executor.Exec(ctx, command, args...)
 	if err != nil {
+		// Keep what the command printed, such as kubectl's "Error from server
+		// (Forbidden): ...". The exit status alone does not say why the command
+		// failed, and the error creators below pick their suggestions from it.
+		if commandOutput := strings.TrimSpace(string(output)); commandOutput != "" {
+			err = fmt.Errorf("%w: %s", err, commandOutput)
+		}
+
 		// Create appropriate error based on command type
 		var toolError *errors.ToolError
 		switch command {
diff --git a/internal/commands/builder_errors_test.go b/internal/commands/builder_errors_test.go
new file mode 100644
index 0000000..3952c15
--- /dev/null
+++ b/internal/commands/builder_errors_test.go
@@ -0,0 +1,63 @@
+package commands
+
+import (
+	"context"
+	"fmt"
+	"testing"
+
+	"github.com/kagent-dev/tools/internal/cmd"
+	"github.com/kagent-dev/tools/internal/errors"
+	"github.com/stretchr/testify/assert"
+	"github.com/stretchr/testify/require"
+)
+
+// TestExecuteKeepsCommandOutputOnFailure checks that a failed command's own
+// message reaches the caller, not only its exit status.
+func TestExecuteKeepsCommandOutputOnFailure(t *testing.T) {
+	exitStatus := fmt.Errorf("exit status 1")
+
+	testCases := []struct {
+		name          string
+		output        string
+		wantInError   string
+		wantErrorCode string
+	}{
+		{
+			name:        "admission webhook denial",
+			output:      "Error from server (Forbidden): admission webhook \"policy.example.com\" denied the request: scaling prod needs approval\n",
+			wantInError: `admission webhook "policy.example.com" denied the request: scaling prod needs approval`,
+		},
+		{
+			name:          "RBAC denial gets the permission suggestions",
+			output:        "Error from server (Forbidden): deployments.apps \"checkout\" is forbidden: User \"system:serviceaccount:kagent:kagent-tools\" cannot patch resource \"deployments\" in API group \"apps\" in the namespace \"prod\"\n",
+			wantInError:   `cannot patch resource "deployments"`,
+			wantErrorCode: "K8S_PERMISSION_ERROR",
+		},
+		{
+			name:        "no output keeps the error as it was",
+			output:      "",
+			wantInError: "failed: exit status 1",
+		},
+	}
+
+	for _, testCase := range testCases {
+		t.Run(testCase.name, func(t *testing.T) {
+			mockExecutor := cmd.NewMockShellExecutor()
+			mockExecutor.AddPartialMatcherString("kubectl", []string{"patch"}, testCase.output, exitStatus)
+			ctx := cmd.WithShellExecutor(context.Background(), mockExecutor)
+
+			_, err := NewCommandBuilder("kubectl").
+				WithArgs("patch", "deployment", "checkout", "--type=merge", "-p", `{"spec":{"replicas":6}}`).
+				WithNamespace("prod").
+				Execute(ctx)
+
+			var toolError *errors.ToolError
+			require.ErrorAs(t, err, &toolError)
+			assert.Contains(t, err.Error(), testCase.wantInError)
+			assert.ErrorIs(t, toolError.Cause, exitStatus, "the original error stays wrapped")
+			if testCase.wantErrorCode != "" {
+				assert.Equal(t, testCase.wantErrorCode, toolError.ErrorCode)
+			}
+		})
+	}
+}