fix(hooks): also preserve stdout in failed-hook results, pin contract

The previous fix (64a00e7e8) restored Stderr preservation in runHook's
err and timeout paths but constructed a fresh HandlerResult with only
Stderr + ExitCode — silently dropping any Stdout the handler had
captured before failing. The original pre-refactor code preserved
both, and a future caller in aggregate consuming r.Stdout on the err
branch would silently see "" instead of the captured output.

Replace the explicit field listing with a small markFailed closure
that mutates the already-constructed r in place, exactly matching the
old "set ExitCode = -1, clear Output, attach err" pattern. This:

- preserves both Stdout and Stderr (matching pre-refactor behaviour),
- is robust to future fields on HandlerResult — they propagate
  automatically rather than needing each call site updated,
- makes the contract explicit in one named place that aggregate's
  err-branch reads stderr from.

Add TestExecutorHandlerErrorPreservesStderr to pin the contract
aggregate already relies on. The original regression slipped through
because no existing test asserted result.Stderr for the PreToolUse
fail-closed path; this test would have failed against the
pre-64a00e7e8 code.

Assisted-By: docker-agent

Author: dgageot

pkg/hooks/executor.go

@@ -211,23 +211,31 @@ func (e *Executor) runHook(ctx context.Context, hook Hook, inputJSON []byte) hoo
 	res, err := handler.Run(timeoutCtx, inputJSON)
 	r := hookResult{HandlerResult: res}
 
+	// markFailed turns r into a "did not complete" outcome: the
+	// handler's diagnostic stdout/stderr survive (aggregate surfaces
+	// stderr in the PreToolUse fail-closed message), ExitCode is
+	// pinned to -1 to match the documented [Result.ExitCode]
+	// convention, any partial Output is dropped (it can't have been
+	// authoritative if the run didn't complete), and rerr lands in
+	// hookResult.err for the err-!= nil branch in [aggregate].
+	markFailed := func(rerr error) hookResult {
+		r.ExitCode = -1
+		r.Output = nil
+		r.err = rerr
+		return r
+	}
+
 	// Normalize timeout/cancellation: handler error types vary, so we
 	// rewrite to a uniform error so PreToolUse fails closed cleanly.
 	if ctxErr := timeoutCtx.Err(); ctxErr != nil {
 		reason := "cancelled"
 		if errors.Is(ctxErr, context.DeadlineExceeded) {
 			reason = fmt.Sprintf("timed out after %s", hook.GetTimeout())
 		}
-		return hookResult{
-			HandlerResult: HandlerResult{Stderr: res.Stderr, ExitCode: -1},
-			err:           fmt.Errorf("hook %q %s: %w", hook.Command, reason, ctxErr),
-		}
+		return markFailed(fmt.Errorf("hook %q %s: %w", hook.Command, reason, ctxErr))
 	}
 	if err != nil {
-		return hookResult{
-			HandlerResult: HandlerResult{Stderr: res.Stderr, ExitCode: -1},
-			err:           err,
-		}
+		return markFailed(err)
 	}
 
 	// Fall back to the legacy "parse JSON from stdout" protocol.

pkg/hooks/handler_test.go

@@ -160,6 +160,46 @@ func TestExecutorHandlerErrorDeniesPreToolUse(t *testing.T) {
 	assert.Equal(t, -1, result.ExitCode)
 }
 
+// TestExecutorHandlerErrorPreservesStderr pins the contract that when a
+// handler returns an error, the diagnostic stderr it captured before
+// failing survives all the way to [Result.Stderr]. aggregate routes
+// that field into the user-visible PreToolUse fail-closed message; if
+// runHook ever drops it on the floor (as it briefly did during the
+// HandlerResult-embedding refactor) PreToolUse denials would lose
+// their explanatory text.
+func TestExecutorHandlerErrorPreservesStderr(t *testing.T) {
+	t.Parallel()
+
+	const customType HookType = "errors-with-stderr"
+	const diagnostic = "BOOM: subprocess crashed at line 42"
+
+	registry := NewRegistry()
+	registry.Register(customType, func(_ HandlerEnv, _ Hook) (Handler, error) {
+		return handlerFunc(func(context.Context, []byte) (HandlerResult, error) {
+			// Mirrors what commandHandler does on a spawn failure: it
+			// captured stderr, then surfaces an exec-level error.
+			return HandlerResult{Stderr: diagnostic, ExitCode: -1}, errors.New("spawn failed")
+		}), nil
+	})
+
+	config := &Config{
+		PreToolUse: []MatcherConfig{
+			{Matcher: "*", Hooks: []Hook{{Type: customType, Timeout: 5}}},
+		},
+	}
+
+	exec := NewExecutorWithRegistry(config, t.TempDir(), nil, registry)
+	result, err := exec.Dispatch(t.Context(), EventPreToolUse, &Input{
+		SessionID: "s", ToolName: "shell", ToolUseID: "t",
+	})
+	require.NoError(t, err)
+
+	assert.False(t, result.Allowed)
+	assert.Equal(t, -1, result.ExitCode)
+	assert.Equal(t, diagnostic, result.Stderr,
+		"handler-captured stderr must survive the err != nil normalization in runHook")
+}
+
 // handlerFunc adapts a function value into a [Handler] for terse tests.
 type handlerFunc func(context.Context, []byte) (HandlerResult, error)