fix: 修复技能调用白名单、脚本参数解析与工具失败结果回流问题

frontend/src/composables/useChatStreamHandler.ts

@@ -731,10 +731,27 @@ export function useChatStreamHandler(options: UseChatStreamHandlerOptions) {
       }
       case 'tool_result':
       case 'error': {
+        // An error event is now exclusively a flow-interrupting failure (model
+        // stream / pipeline error); tool failures arrive as tool_result with
+        // success=false. Handle fatal errors first, before attempting
+        // pending-tool-call matching (which can never match a flow error and
+        // would only emit a spurious warning).
+        if (responseType === 'error') {
+          const errorMsg = String(data.content || t('chat.processError'))
+          message.content = errorMsg
+          message.is_completed = true
+          isReplying.value = false
+          loading.value = false
+          fullContent.value = ''
+          currentAssistantMessageId.value = ''
+          reportError(errorMsg)
+          console.error('[Chat Error]', errorMsg)
+          break
+        }
         if (dataPayload) {
           const toolCallId = dataPayload.tool_call_id as string | undefined
           const toolName = dataPayload.tool_name as string | undefined
-          const success = responseType !== 'error' && dataPayload.success !== false
+          const success = dataPayload.success !== false
           log('[Tool Result]', {
             tool_call_id: toolCallId,
             tool_name: toolName,
@@ -760,9 +777,14 @@ export function useChatStreamHandler(options: UseChatStreamHandlerOptions) {
           if (toolCallEvent) {
             toolCallEvent.pending = false
             toolCallEvent.success = success
+            // On failure, prefer the full tool Output (stdout/stderr/exit
+            // code) which the backend ships in dataPayload.output; only fall
+            // back to the short Error label when no Output was attached.
+            // Showing just "Script exited with code N" hides the detail a
+            // human needs to diagnose the failure.
             toolCallEvent.output = success
               ? dataPayload.output || data.content
-              : dataPayload.error || data.content
+              : dataPayload.output || dataPayload.error || data.content
             toolCallEvent.error = !success ? dataPayload.error || data.content : undefined
             const duration =
               dataPayload.duration_ms !== undefined ? dataPayload.duration_ms : dataPayload.duration
@@ -774,27 +796,6 @@ export function useChatStreamHandler(options: UseChatStreamHandlerOptions) {
           } else {
             console.warn('[Tool Result] No pending tool call found for', toolCallId || toolName)
           }
-          if (responseType === 'error' && !toolName) {
-            const errorMsg = String(data.content || t('chat.processError'))
-            message.content = errorMsg
-            message.is_completed = true
-            isReplying.value = false
-            loading.value = false
-            fullContent.value = ''
-            currentAssistantMessageId.value = ''
-            reportError(errorMsg)
-            console.error('[Chat Error]', errorMsg)
-          }
-        } else if (responseType === 'error') {
-          const errorMsg = String(data.content || t('chat.processError'))
-          message.content = errorMsg
-          message.is_completed = true
-          isReplying.value = false
-          loading.value = false
-          fullContent.value = ''
-          currentAssistantMessageId.value = ''
-          reportError(errorMsg)
-          console.error('[Chat Error]', errorMsg)
         }
         break
       }

internal/agent/tools/skill_execute.go

@@ -90,9 +90,18 @@ func (i *ExecuteSkillScriptInput) UnmarshalJSON(data []byte) error {
 		return fmt.Errorf("args must be a string or an array of strings: %w", err)
 	}
 
-	// A string is interpreted as a conventional space-separated command line.
-	// The tool schema continues to advertise []string, so well-formed calls are
-	// unaffected; this is only a compatibility fallback for model output.
+	// Some providers emit the array as a stringified JSON payload
+	// (e.g. "[\"--project-name\",\"X\"]"). Treat that as an array first so the
+	// model's intent is preserved; strings.Fields would otherwise split the
+	// brackets/quotes into garbage tokens and the script would see nonsense argv.
+	if err := json.Unmarshal([]byte(argsString), &i.Args); err == nil {
+		return nil
+	}
+
+	// A plain string is interpreted as a conventional space-separated command
+	// line. The tool schema continues to advertise []string, so well-formed
+	// calls are unaffected; this is only a compatibility fallback for model
+	// output.
 	i.Args = strings.Fields(argsString)
 	return nil
 }

internal/agent/tools/skill_execute_test.go

@@ -0,0 +1,60 @@
+package tools
+
+import (
+	"encoding/json"
+	"testing"
+
+	"github.com/stretchr/testify/require"
+)
+
+// TestExecuteSkillScriptInputUnmarshalJSON covers the model-emitted shapes the
+// UnmarshalJSON fallback must tolerate. The provider does not always honor the
+// []string schema: it sometimes emits a stringified JSON array or a single
+// command-line string. Each must round-trip to the intended argv.
+func TestExecuteSkillScriptInputUnmarshalJSON(t *testing.T) {
+	t.Run("real array", func(t *testing.T) {
+		var in ExecuteSkillScriptInput
+		require.NoError(t, json.Unmarshal([]byte(`{
+			"skill_name": "s", "script_path": "p",
+			"args": ["--project-name", "X", "--creator", "admin"]
+		}`), &in))
+		require.Equal(t, []string{"--project-name", "X", "--creator", "admin"}, in.Args)
+	})
+
+	t.Run("stringified json array", func(t *testing.T) {
+		// Some providers emit args as a JSON string whose content is itself a
+		// JSON array. strings.Fields would mangle the brackets/quotes into
+		// one garbage token; the array must be recovered instead.
+		var in ExecuteSkillScriptInput
+		require.NoError(t, json.Unmarshal([]byte(`{
+			"skill_name": "s", "script_path": "p",
+			"args": "[\"--project-name\", \"X\", \"--creator\", \"admin\"]"
+		}`), &in))
+		require.Equal(t, []string{"--project-name", "X", "--creator", "admin"}, in.Args)
+	})
+
+	t.Run("single command line string", func(t *testing.T) {
+		var in ExecuteSkillScriptInput
+		require.NoError(t, json.Unmarshal([]byte(`{
+			"skill_name": "s", "script_path": "p",
+			"args": "ls --workspace /workspace/output"
+		}`), &in))
+		require.Equal(t, []string{"ls", "--workspace", "/workspace/output"}, in.Args)
+	})
+
+	t.Run("absent args", func(t *testing.T) {
+		var in ExecuteSkillScriptInput
+		require.NoError(t, json.Unmarshal([]byte(`{
+			"skill_name": "s", "script_path": "p"
+		}`), &in))
+		require.Empty(t, in.Args)
+	})
+
+	t.Run("null args", func(t *testing.T) {
+		var in ExecuteSkillScriptInput
+		require.NoError(t, json.Unmarshal([]byte(`{
+			"skill_name": "s", "script_path": "p", "args": null
+		}`), &in))
+		require.Empty(t, in.Args)
+	})
+}

internal/application/service/session_agent_qa.go

@@ -435,8 +435,12 @@ func mergeResolvedTagKnowledgeIDs(
 	return uniqueNonEmptyStrings(merged)
 }
 
-// applyPerRequestSkillScope narrows the agent's skill whitelist to the @Skill
-// mentions for this turn and records the pinned set for the <must_use> hint.
+// applyPerRequestSkillScope records the @Skill mentions for this turn as the
+// pinned set that drives the <must_use> hint. It deliberately does NOT narrow
+// the allow-gate: an agent whose prompt requires a skill the user did not
+// @mention must still be able to read and execute it. Mentioning a skill only
+// prioritizes it, it never revokes access to the agent's configured set.
+//
 // It is a no-op when no skills were mentioned or skills are disabled.
 func applyPerRequestSkillScope(
 	ctx context.Context,
@@ -454,18 +458,11 @@ func applyPerRequestSkillScope(
 	if !agentConfig.SkillsEnabled {
 		return
 	}
-	switch skillsMode {
-	case "selected":
-		agentConfig.AllowedSkills = intersectPreservingRequestOrder(requested, agentConfig.AllowedSkills)
-		if len(agentConfig.AllowedSkills) == 0 {
-			agentConfig.SkillsEnabled = false
-		}
-	case "all":
-		agentConfig.AllowedSkills = dedupPreservingOrder(requested)
-	}
-	if agentConfig.SkillsEnabled && len(agentConfig.AllowedSkills) > 0 {
-		agentConfig.PinnedSkillNames = intersectPreservingRequestOrder(requested, agentConfig.AllowedSkills)
-	}
+	// PinnedSkillNames carries only mentioned skills that are currently
+	// allowed, so the <must_use> hint never directs the model at a skill it
+	// cannot load. An empty AllowedSkills means all skills are allowed,
+	// matching Manager.isSkillAllowed, so every mention is pinned in that case.
+	agentConfig.PinnedSkillNames = pinPreservingRequestOrder(requested, agentConfig.AllowedSkills)
 	logger.Infof(ctx, "Applied per-request @skill scope: requested=%v effective=%v pinned=%v",
 		requested, agentConfig.AllowedSkills, agentConfig.PinnedSkillNames)
 }
@@ -553,6 +550,33 @@ func intersectPreservingRequestOrder(requested []string, allowed []string) []str
 	return result
 }
 
+// pinPreservingRequestOrder returns the requested skills that are allowed,
+// preserving request order. Unlike intersectPreservingRequestOrder, an empty
+// allowed list is treated as "all skills allowed" (matching
+// Manager.isSkillAllowed), so every requested skill is pinned.
+func pinPreservingRequestOrder(requested []string, allowed []string) []string {
+	allowedAll := len(allowed) == 0
+	allowedSet := make(map[string]bool, len(allowed))
+	for _, value := range allowed {
+		if value != "" {
+			allowedSet[value] = true
+		}
+	}
+	result := make([]string, 0, len(requested))
+	seen := make(map[string]bool, len(requested))
+	for _, value := range requested {
+		if value == "" || seen[value] {
+			continue
+		}
+		if !allowedAll && !allowedSet[value] {
+			continue
+		}
+		seen[value] = true
+		result = append(result, value)
+	}
+	return result
+}
+
 func dedupPreservingOrder(values []string) []string {
 	result := make([]string, 0, len(values))
 	seen := make(map[string]bool, len(values))

internal/application/service/session_agent_qa_scope_test.go

@@ -78,18 +78,33 @@ func TestApplyPerRequestMCPScope_NoneIgnoresMentionAndDoesNotPin(t *testing.T) {
 	assert.Empty(t, cfg.PinnedMCPServiceIDs)
 }
 
-func TestApplyPerRequestSkillScope_SelectedEmptyIntersectionDisables(t *testing.T) {
+func TestApplyPerRequestSkillScope_SelectedPinsMentionedAndKeepsAllowed(t *testing.T) {
+	cfg := &types.AgentConfig{SkillsEnabled: true, AllowedSkills: []string{"a", "b"}}
+	applyPerRequestSkillScope(context.Background(), cfg, "selected", []string{"a"})
+	assert.True(t, cfg.SkillsEnabled)
+	// Allow-gate is not narrowed: a prompt-mandated skill the user did not
+	// @mention (b) must remain callable.
+	assert.Equal(t, []string{"a", "b"}, cfg.AllowedSkills)
+	assert.Equal(t, []string{"a"}, cfg.PinnedSkillNames)
+}
+
+func TestApplyPerRequestSkillScope_SelectedMentionOutsideAllowedIsNotPinned(t *testing.T) {
 	cfg := &types.AgentConfig{SkillsEnabled: true, AllowedSkills: []string{"a", "b"}}
 	applyPerRequestSkillScope(context.Background(), cfg, "selected", []string{"c"})
-	assert.False(t, cfg.SkillsEnabled)
+	// Skills stay enabled (no narrowing-to-empty disable); the out-of-scope
+	// mention is simply not pinned.
+	assert.True(t, cfg.SkillsEnabled)
+	assert.Equal(t, []string{"a", "b"}, cfg.AllowedSkills)
 	assert.Empty(t, cfg.PinnedSkillNames)
 }
 
-func TestApplyPerRequestSkillScope_AllPinsMentioned(t *testing.T) {
+func TestApplyPerRequestSkillScope_AllPinsMentionedWithoutNarrowingGate(t *testing.T) {
 	cfg := &types.AgentConfig{SkillsEnabled: true}
 	applyPerRequestSkillScope(context.Background(), cfg, "all", []string{"analysis", "analysis"})
 	assert.True(t, cfg.SkillsEnabled)
-	assert.Equal(t, []string{"analysis"}, cfg.AllowedSkills)
+	// "all" mode keeps AllowedSkills empty (= all allowed); it does not narrow
+	// to only the mentioned set, so other skills the agent needs stay callable.
+	assert.Empty(t, cfg.AllowedSkills)
 	assert.Equal(t, []string{"analysis"}, cfg.PinnedSkillNames)
 }
 

internal/handler/session/agent_stream_handler.go

@@ -241,14 +241,15 @@ func (h *AgentStreamHandler) handleToolResult(ctx context.Context, evt event.Eve
 	}
 	h.mu.Unlock()
 
-	// Send SSE response (both success and failure)
+	// Tool results — success or failure — always use the tool_result event
+	// type; the success flag in metadata distinguishes them. Reserve the
+	// error event type for flow-interrupting failures (model stream /
+	// pipeline errors), so the frontend can treat any error event as fatal
+	// without inspecting tool_name.
 	responseType := types.ResponseTypeToolResult
 	content := agenttools.StreamContentForToolResult(data.ToolName, data.Success, data.Error, data.Data)
-	if !data.Success {
-		responseType = types.ResponseTypeError
-		if content == "" && data.Error != "" {
-			content = data.Error
-		}
+	if !data.Success && content == "" && data.Error != "" {
+		content = data.Error
 	}
 
 	// Build metadata including tool result data for rich frontend rendering

internal/modelcontext/registry.go

@@ -266,10 +266,19 @@ func (r *Registry) ModelToolResultForTool(toolName string, result *types.ToolRes
 		modelOutput = r.sources.ModelOutput(&copyResult)
 	} else if copyResult.Success {
 		modelOutput = copyResult.Output
-	} else if copyResult.Error != "" {
-		modelOutput = "Error: " + copyResult.Error
 	} else {
-		modelOutput = "Error: tool call failed"
+		// Failed: surface the error, and also include any diagnostic Output
+		// the tool produced on failure. Some tools (e.g. execute_skill_script)
+		// write stdout/stderr into Output on a non-zero exit; dropping it here
+		// would hide the exact detail the model needs to diagnose and recover.
+		if copyResult.Error != "" {
+			modelOutput = "Error: " + copyResult.Error
+		} else {
+			modelOutput = "Error: tool call failed"
+		}
+		if copyResult.Output != "" {
+			modelOutput += "\n\n" + copyResult.Output
+		}
 	}
 	// Even tools without structured source results can surface a known durable
 	// ID in validation errors or status text. Compact only explicitly declared

internal/modelcontext/registry_test.go

@@ -295,6 +295,51 @@ func TestRegistryCompactsDatabaseQueryIDColumnsForBuiltInFollowUps(t *testing.T)
 	require.JSONEq(t, `{"knowledge_id":"doc-real"}`, calls[0].Function.Arguments)
 }
 
+// TestModelToolResultPreservesOutputOnFailure guards against a regression
+// where a failed tool call dropped ToolResult.Output, hiding the stdout/stderr
+// the model needs to diagnose the failure (e.g. execute_skill_script writes a
+// non-zero exit's stdout/stderr into Output, not Error).
+func TestModelToolResultPreservesOutputOnFailure(t *testing.T) {
+	registry := NewRegistry(true)
+
+	// execute_skill_script has no source/handle policy, so it routes through
+	// the non-sourceOutput failure branch.
+	got := registry.ModelToolResultForTool("execute_skill_script", &types.ToolResult{
+		Success: false,
+		Error:   "Script exited with code 1",
+		Output: "=== Script Execution: foo/bar ===\n## Standard Error\n\n" +
+			"```\nTraceback (most recent call last):\n  ValueError: boom\n```",
+	})
+	require.Contains(t, got, "Error: Script exited with code 1")
+	require.Contains(t, got, "Traceback (most recent call last)")
+	require.Contains(t, got, "ValueError: boom")
+}
+
+// TestModelToolResultFailureWithoutOutputOnlyReturnsError confirms that tools
+// which set only Error (the common case) are unaffected by the Output-merge.
+func TestModelToolResultFailureWithoutOutputOnlyReturnsError(t *testing.T) {
+	registry := NewRegistry(true)
+
+	got := registry.ModelToolResultForTool("wiki_write_page", &types.ToolResult{
+		Success: false,
+		Error:   "validation failed",
+	})
+	require.Equal(t, "Error: validation failed", got)
+}
+
+// TestModelToolResultFailureWithOutputOnly invents a default error heading
+// when the tool set Output but no Error.
+func TestModelToolResultFailureWithOutputOnly(t *testing.T) {
+	registry := NewRegistry(true)
+
+	got := registry.ModelToolResultForTool("execute_skill_script", &types.ToolResult{
+		Success: false,
+		Output:  "## Standard Error\n\n```\nboom\n```",
+	})
+	require.Contains(t, got, "Error: tool call failed")
+	require.Contains(t, got, "boom")
+}
+
 func TestRegistryDecodesCanonicalArgumentsForEveryBuiltInReferenceTool(t *testing.T) {
 	registry := NewRegistry(true)
 	registry.RegisterDocument("doc-real")