fix: address CodeRabbit review round 5

- Reject symlinks and non-regular files in TmpStorage.Put
- Validate cpus param is non-negative in docker/run
- Add timeout context and error logging to container cleanup/stop
- Create fresh artifacts scratch dir per retry attempt

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>

Author: michaelmcnees

internal/artifact/tmp.go

@@ -48,6 +48,15 @@ func (fs *FilesystemTmpStorage) Put(ctx context.Context, key string, localPath s
 		return "", fmt.Errorf("creating artifact dir: %w", err)
 	}
 
+	// Reject symlinks and non-regular files to prevent exfiltration.
+	fi, err := os.Lstat(localPath)
+	if err != nil {
+		return "", fmt.Errorf("stat source file: %w", err)
+	}
+	if !fi.Mode().IsRegular() {
+		return "", fmt.Errorf("source file %q is not a regular file (mode: %s)", localPath, fi.Mode())
+	}
+
 	src, err := os.Open(localPath)
 	if err != nil {
 		return "", fmt.Errorf("opening source file: %w", err)

internal/connector/docker.go

@@ -11,6 +11,7 @@ import (
 	"path/filepath"
 	"strconv"
 	"strings"
+	"time"
 
 	"github.com/docker/docker/api/types/container"
 	"github.com/docker/docker/api/types/image"
@@ -110,10 +111,19 @@ func parseDockerParams(params map[string]any) (*dockerConfig, error) {
 	// cpus
 	switch v := params["cpus"].(type) {
 	case float64:
+		if v < 0 {
+			return nil, fmt.Errorf("docker/run: cpus must be non-negative, got %v", v)
+		}
 		cfg.CPUs = v
 	case int:
+		if v < 0 {
+			return nil, fmt.Errorf("docker/run: cpus must be non-negative, got %v", v)
+		}
 		cfg.CPUs = float64(v)
 	case int64:
+		if v < 0 {
+			return nil, fmt.Errorf("docker/run: cpus must be non-negative, got %v", v)
+		}
 		cfg.CPUs = float64(v)
 	}
 
@@ -309,7 +319,11 @@ func (c *DockerRunConnector) Execute(ctx context.Context, params map[string]any)
 	// Ensure cleanup.
 	defer func() {
 		if cfg.Remove {
-			cli.ContainerRemove(context.Background(), containerID, container.RemoveOptions{Force: true})
+			cleanupCtx, cleanupCancel := context.WithTimeout(context.Background(), 30*time.Second) // #nosec G118 -- intentional: parent ctx may be cancelled
+			defer cleanupCancel()
+			if err := cli.ContainerRemove(cleanupCtx, containerID, container.RemoveOptions{Force: true}); err != nil {
+				log.Printf("docker/run: failed to remove container %s: %v", containerID, err)
+			}
 		}
 	}()
 
@@ -343,9 +357,13 @@ func (c *DockerRunConnector) Execute(ctx context.Context, params map[string]any)
 		select {
 		case <-ctx.Done():
 			gracePeriod := 10
-			cli.ContainerStop(context.Background(), containerID, container.StopOptions{
+			stopCtx, stopCancel := context.WithTimeout(context.Background(), 30*time.Second)
+			defer stopCancel()
+			if err := cli.ContainerStop(stopCtx, containerID, container.StopOptions{
 				Timeout: &gracePeriod,
-			})
+			}); err != nil {
+				log.Printf("docker/run: failed to stop container %s: %v", containerID, err)
+			}
 		case <-doneCh:
 		}
 	}()

internal/engine/engine.go

@@ -408,22 +408,13 @@ func (e *Engine) executeStepLogic(ctx context.Context, execID string, step workf
 		resolvedParams["_registry_credential"] = regCredData
 	}
 
-	// Create artifacts scratch dir if step declares artifacts.
+	// Validate artifact subsystem is configured if step declares artifacts.
 	var artifactsDir string
 	if len(step.Artifacts) > 0 {
 		if e.TmpStorage == nil || e.ArtifactStore == nil {
 			return nil, fmt.Errorf("step %q declares artifacts but artifact subsystem is not configured (set tmp storage in mantle.yaml)", step.Name)
 		}
 	}
-	if len(step.Artifacts) > 0 && e.TmpStorage != nil {
-		var tmpErr error
-		artifactsDir, tmpErr = os.MkdirTemp("", "mantle-artifacts-*")
-		if tmpErr != nil {
-			return nil, fmt.Errorf("creating artifacts dir: %v", tmpErr)
-		}
-		defer os.RemoveAll(artifactsDir) // clean local scratch after persisting to tmp storage
-		ctx = artifact.WithArtifactsDir(ctx, artifactsDir)
-	}
 
 	// Inject workflow/step context for AI observability metrics.
 	resolvedParams["_workflow"] = workflowName
@@ -453,6 +444,20 @@ func (e *Engine) executeStepLogic(ctx context.Context, execID string, step workf
 	var lastErr error
 
 	for attempt := 1; attempt <= maxAttempts; attempt++ {
+		// Create a fresh artifacts scratch dir per attempt.
+		if len(step.Artifacts) > 0 && e.TmpStorage != nil {
+			if artifactsDir != "" {
+				os.RemoveAll(artifactsDir) // clean previous attempt's scratch
+			}
+			var tmpErr error
+			artifactsDir, tmpErr = os.MkdirTemp("", "mantle-artifacts-*")
+			if tmpErr != nil {
+				return nil, fmt.Errorf("creating artifacts dir: %v", tmpErr)
+			}
+			defer os.RemoveAll(artifactsDir)
+			ctx = artifact.WithArtifactsDir(ctx, artifactsDir)
+		}
+
 		// Apply per-step timeout.
 		execCtx := ctx
 		var cancel context.CancelFunc