fix: address 16 code review issues (3 critical, security + concurrency)

Critical fixes:
- executor: nil TrustEngine now defaults to DenyAllTrustEvaluator (fail-closed, not open)
- executor: ActionAsk with no Approver configured now denies (fail-safe); with Approver
  calls Approver.Request then blocks on WaitForResolution instead of returning error
- orchestrator: ContainerManager.EnsureContainer now accepts executor.SandboxConfig so
  *ContainerManager satisfies executor.ContainerExecutor (type mismatch resolved)

Security / correctness:
- executor: Action is now a type alias for policy.Action (compile-time parity, no drift)
- executor: Approver interface gains Request() method for creating approvals before waiting
- orchestrator: bind mount Source/Target validated with filepath.IsAbs + path traversal check
- orchestrator: EnsureContainer called before ExecInContainer in sandbox path

policy/store.go:
- Grant: DELETE + INSERT wrapped in a single transaction (atomic upsert)
- Check: real DB errors logged separately from "no rows found"
- List: time.Parse errors logged instead of silently discarded

orchestrator/container_manager.go:
- Init command failures now logged (were silently dropped)
- rows.Err() checked after container cache load loop

Tests added:
- TestDenyAllTrustEvaluator (fail-closed default)
- TestTrustEngineDataRace (concurrent SetMode/Evaluate under -race)
- TestExecute_ActionAsk_NoApprover_Denies
- TestExecute_ActionAsk_WithApprover_Blocks

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>

Author: intel352

executor/executor.go

@@ -105,6 +105,9 @@ func Execute(ctx context.Context, cfg Config, systemPrompt, userTask, agentID st
 	if cfg.RequestTimeout <= 0 {
 		cfg.RequestTimeout = 60 * time.Minute
 	}
+	// Track whether the caller configured a real Approver. A nil Approver means no
+	// approval system — trust-ask actions must be denied rather than auto-approved.
+	hasRealApprover := cfg.Approver != nil
 	if cfg.Approver == nil {
 		cfg.Approver = &NullApprover{}
 	}
@@ -120,10 +123,10 @@ func Execute(ctx context.Context, cfg Config, systemPrompt, userTask, agentID st
 	if cfg.Memory == nil {
 		cfg.Memory = &NullMemoryStore{}
 	}
-	// TrustEngine defaults to NullTrustEvaluator (fail-open / allow all).
-	// This is intentional for dev/testing. Replace with a real TrustEvaluator in production.
+	// TrustEngine defaults to DenyAllTrustEvaluator when nil — fail-closed, not open.
+	// Set cfg.TrustEngine = &NullTrustEvaluator{} explicitly for dev/testing environments.
 	if cfg.TrustEngine == nil {
-		cfg.TrustEngine = &NullTrustEvaluator{}
+		cfg.TrustEngine = &DenyAllTrustEvaluator{}
 	}
 
 	if cfg.Provider == nil {
@@ -332,23 +335,43 @@ func Execute(ctx context.Context, cfg Config, systemPrompt, userTask, agentID st
 						Content:   fmt.Sprintf("[TRUST DENY] %s by %s", tc.Name, agentID),
 					})
 				case ActionAsk:
-					// Block on human approval before allowing execution.
-					approvalID := uuid.New().String()
-					_ = cfg.Transcript.Record(ctx, TranscriptEntry{
-						ID:        approvalID,
-						AgentID:   agentID,
-						TaskID:    cfg.TaskID,
-						ProjectID: cfg.ProjectID,
-						Iteration: iterCount,
-						Role:      provider.RoleUser,
-						Content:   fmt.Sprintf("[TRUST ASK] %s by %s — waiting for human approval", tc.Name, agentID),
-					})
-					record, waitErr := cfg.Approver.WaitForResolution(ctx, approvalID, cfg.ApprovalTimeout)
-					if waitErr != nil || record == nil || record.Status != ApprovalApproved {
-						resultStr = fmt.Sprintf("Tool %q requires human approval (trust policy: ask) — not approved", tc.Name)
+					// Deny immediately when no real Approver is configured — fail-safe.
+					if !hasRealApprover {
+						resultStr = fmt.Sprintf("Tool %q denied: trust policy requires user approval but no Approver is configured", tc.Name)
 						isError = true
+						_ = cfg.Transcript.Record(ctx, TranscriptEntry{
+							ID:        uuid.New().String(),
+							AgentID:   agentID,
+							TaskID:    cfg.TaskID,
+							ProjectID: cfg.ProjectID,
+							Iteration: iterCount,
+							Role:      provider.RoleUser,
+							Content:   fmt.Sprintf("[TRUST DENY] %s by %s (ask, no approver)", tc.Name, agentID),
+						})
+					} else {
+						// Create an approval entry and block until a human resolves it.
+						approvalID, reqErr := cfg.Approver.Request(ctx, tc.Name, tc.Arguments)
+						if reqErr != nil {
+							resultStr = fmt.Sprintf("Tool %q denied: could not create approval request: %v", tc.Name, reqErr)
+							isError = true
+						} else {
+							_ = cfg.Transcript.Record(ctx, TranscriptEntry{
+								ID:        uuid.New().String(),
+								AgentID:   agentID,
+								TaskID:    cfg.TaskID,
+								ProjectID: cfg.ProjectID,
+								Iteration: iterCount,
+								Role:      provider.RoleUser,
+								Content:   fmt.Sprintf("[TRUST ASK] %s by %s — waiting for human approval (id=%s)", tc.Name, agentID, approvalID),
+							})
+							record, waitErr := cfg.Approver.WaitForResolution(ctx, approvalID, cfg.ApprovalTimeout)
+							if waitErr != nil || record == nil || record.Status != ApprovalApproved {
+								resultStr = fmt.Sprintf("Tool %q requires human approval (trust policy: ask) — not approved", tc.Name)
+								isError = true
+							}
+							// If approved, fall through to normal tool execution.
+						}
 					}
-					// If approved, fall through to normal tool execution.
 				}
 			}
 

executor/executor_test.go

@@ -4,6 +4,7 @@ import (
 	"context"
 	"fmt"
 	"testing"
+	"time"
 
 	"github.com/GoCodeAlone/workflow-plugin-agent/provider"
 	"github.com/GoCodeAlone/workflow-plugin-agent/tools"
@@ -129,6 +130,7 @@ func TestExecute_ToolExecution(t *testing.T) {
 	cfg := Config{
 		Provider:     p,
 		ToolRegistry: reg,
+		TrustEngine:  &NullTrustEvaluator{}, // allow all tools in this unit test
 	}
 	result, err := Execute(context.Background(), cfg, "sys", "task", "agent-1")
 	if err != nil {
@@ -528,6 +530,7 @@ func TestExecute_ToolArgsEventIsCopy(t *testing.T) {
 	cfg := Config{
 		Provider:     p,
 		ToolRegistry: reg,
+		TrustEngine:  &NullTrustEvaluator{}, // allow all tools in this unit test
 		OnEvent: func(e Event) {
 			// Mutate the event's ToolArgs — this should NOT affect tool execution.
 			if e.Type == EventToolCallStart && e.ToolArgs != nil {
@@ -768,3 +771,117 @@ func (s *simpleTool) Definition() provider.ToolDef   { return s.def }
 func (s *simpleTool) Execute(ctx context.Context, args map[string]any) (any, error) {
 	return s.fn(ctx, args)
 }
+
+// TestExecute_ActionAsk_NoApprover_Denies verifies that when no Approver is configured,
+// a trust ActionAsk tool call is denied rather than auto-approved (fail-safe behavior).
+func TestExecute_ActionAsk_NoApprover_Denies(t *testing.T) {
+	var toolExecuted bool
+	reg := tools.NewRegistry()
+	reg.Register(&simpleTool{
+		name: "risky_tool",
+		def:  provider.ToolDef{Name: "risky_tool", Description: "risky"},
+		fn: func(_ context.Context, _ map[string]any) (any, error) {
+			toolExecuted = true
+			return "executed", nil
+		},
+	})
+
+	callN := 0
+	p := &callCountProvider{
+		onChat: func() (*provider.Response, error) {
+			callN++
+			if callN == 1 {
+				return &provider.Response{
+					ToolCalls: []provider.ToolCall{
+						{ID: "tc-1", Name: "risky_tool", Arguments: map[string]any{}},
+					},
+				}, nil
+			}
+			return &provider.Response{Content: "done"}, nil
+		},
+	}
+
+	// TrustEngine returns Ask for risky_tool; Approver is nil (not configured).
+	te := &mockTrustEvaluator{toolAction: ActionAsk}
+	cfg := Config{
+		Provider:     p,
+		ToolRegistry: reg,
+		TrustEngine:  te,
+		// Approver intentionally nil — should default to deny behavior
+	}
+
+	_, err := Execute(context.Background(), cfg, "sys", "task", "agent-1")
+	if err != nil {
+		t.Fatalf("Execute: %v", err)
+	}
+	if toolExecuted {
+		t.Error("risky_tool should NOT have been executed when no Approver is configured and trust policy is ask")
+	}
+}
+
+// TestExecute_ActionAsk_WithApprover_Blocks verifies that when an Approver is configured,
+// a trust ActionAsk call blocks until the Approver resolves it.
+func TestExecute_ActionAsk_WithApprover_Blocks(t *testing.T) {
+	var toolExecuted bool
+	var requestReceived bool
+	reg := tools.NewRegistry()
+	reg.Register(&simpleTool{
+		name: "reviewed_tool",
+		def:  provider.ToolDef{Name: "reviewed_tool", Description: "needs approval"},
+		fn: func(_ context.Context, _ map[string]any) (any, error) {
+			toolExecuted = true
+			return "executed", nil
+		},
+	})
+
+	callN := 0
+	p := &callCountProvider{
+		onChat: func() (*provider.Response, error) {
+			callN++
+			if callN == 1 {
+				return &provider.Response{
+					ToolCalls: []provider.ToolCall{
+						{ID: "tc-1", Name: "reviewed_tool", Arguments: map[string]any{}},
+					},
+				}, nil
+			}
+			return &provider.Response{Content: "done"}, nil
+		},
+	}
+
+	approver := &recordingApprover{status: ApprovalApproved}
+	te := &mockTrustEvaluator{toolAction: ActionAsk}
+	cfg := Config{
+		Provider:     p,
+		ToolRegistry: reg,
+		TrustEngine:  te,
+		Approver:     approver,
+	}
+
+	_, err := Execute(context.Background(), cfg, "sys", "task", "agent-1")
+	if err != nil {
+		t.Fatalf("Execute: %v", err)
+	}
+	requestReceived = approver.requested
+	if !requestReceived {
+		t.Error("Approver.Request should have been called for ActionAsk")
+	}
+	if !toolExecuted {
+		t.Error("reviewed_tool should have been executed after approval")
+	}
+}
+
+// recordingApprover records whether Request was called and auto-resolves with a fixed status.
+type recordingApprover struct {
+	requested bool
+	status    ApprovalStatus
+}
+
+func (r *recordingApprover) Request(_ context.Context, _ string, _ map[string]any) (string, error) {
+	r.requested = true
+	return "test-approval-id", nil
+}
+
+func (r *recordingApprover) WaitForResolution(_ context.Context, _ string, _ time.Duration) (*ApprovalRecord, error) {
+	return &ApprovalRecord{Status: r.status}, nil
+}

executor/interfaces.go

@@ -5,7 +5,9 @@ import (
 	"context"
 	"time"
 
+	"github.com/GoCodeAlone/workflow-plugin-agent/policy"
 	"github.com/GoCodeAlone/workflow-plugin-agent/provider"
+	"github.com/google/uuid"
 )
 
 // TranscriptEntry is a single recorded message in the agent conversation.
@@ -46,6 +48,10 @@ type ApprovalRecord struct {
 
 // Approver manages approval gate blocking.
 type Approver interface {
+	// Request creates a pending approval for the given tool call and returns its ID.
+	// Implementers should persist the pending approval so WaitForResolution can query it.
+	Request(ctx context.Context, toolName string, args map[string]any) (approvalID string, err error)
+
 	// WaitForResolution blocks until the approval with the given ID is resolved
 	// or the timeout elapses.
 	WaitForResolution(ctx context.Context, approvalID string, timeout time.Duration) (*ApprovalRecord, error)
@@ -120,9 +126,13 @@ type NullTranscript struct{}
 
 func (n *NullTranscript) Record(_ context.Context, _ TranscriptEntry) error { return nil }
 
-// NullApprover is a no-op Approver — always returns "approved".
+// NullApprover is a no-op Approver for dev/testing — auto-approves every request.
 type NullApprover struct{}
 
+func (n *NullApprover) Request(_ context.Context, _ string, _ map[string]any) (string, error) {
+	return uuid.New().String(), nil
+}
+
 func (n *NullApprover) WaitForResolution(_ context.Context, _ string, _ time.Duration) (*ApprovalRecord, error) {
 	return &ApprovalRecord{Status: ApprovalApproved}, nil
 }
@@ -154,13 +164,14 @@ func (n *NullSecretRedactor) Redact(text string) string { return text }
 
 func (n *NullSecretRedactor) CheckAndRedact(_ *provider.Message) {}
 
-// Action mirrors policy.Action to avoid circular imports.
-type Action string
+// Action is a type alias for policy.Action, providing compile-time parity between
+// the executor and policy packages with no string duplication.
+type Action = policy.Action
 
 const (
-	ActionAllow Action = "allow"
-	ActionDeny  Action = "deny"
-	ActionAsk   Action = "ask"
+	ActionAllow Action = policy.Allow
+	ActionDeny  Action = policy.Deny
+	ActionAsk   Action = policy.Ask
 )
 
 // TrustEvaluator checks whether a tool call is permitted.
@@ -182,13 +193,13 @@ type ContainerExecutor interface {
 
 // SandboxConfig holds per-agent Docker sandbox settings.
 type SandboxConfig struct {
-	Enabled      bool          `json:"enabled" yaml:"enabled"`
-	Image        string        `json:"image" yaml:"image"`
-	Network      string        `json:"network" yaml:"network"`
-	Memory       string        `json:"memory" yaml:"memory"`
-	CPU          float64       `json:"cpu" yaml:"cpu"`
+	Enabled      bool           `json:"enabled" yaml:"enabled"`
+	Image        string         `json:"image" yaml:"image"`
+	Network      string         `json:"network" yaml:"network"`
+	Memory       string         `json:"memory" yaml:"memory"`
+	CPU          float64        `json:"cpu" yaml:"cpu"`
 	Mounts       []SandboxMount `json:"mounts" yaml:"mounts"`
-	InitCommands []string      `json:"init" yaml:"init"`
+	InitCommands []string       `json:"init" yaml:"init"`
 }
 
 // SandboxMount is a bind mount for a sandbox container.
@@ -198,11 +209,23 @@ type SandboxMount struct {
 	ReadOnly bool   `json:"readonly" yaml:"readonly"`
 }
 
-// NullTrustEvaluator allows everything (no trust enforcement).
+// NullTrustEvaluator allows everything — intended for dev/testing only.
+// In production, configure a real TrustEvaluator. When Config.TrustEngine is nil,
+// the executor defaults to DenyAllTrustEvaluator for fail-safe behavior.
 type NullTrustEvaluator struct{}
 
 func (n *NullTrustEvaluator) Evaluate(_ context.Context, _ string, _ map[string]any) Action {
 	return ActionAllow
 }
 func (n *NullTrustEvaluator) EvaluateCommand(_ string) Action { return ActionAllow }
 func (n *NullTrustEvaluator) EvaluatePath(_ string) Action    { return ActionAllow }
+
+// DenyAllTrustEvaluator denies every tool call. Used as the default when no TrustEngine
+// is configured, so missing configuration fails closed rather than open.
+type DenyAllTrustEvaluator struct{}
+
+func (d *DenyAllTrustEvaluator) Evaluate(_ context.Context, _ string, _ map[string]any) Action {
+	return ActionDeny
+}
+func (d *DenyAllTrustEvaluator) EvaluateCommand(_ string) Action { return ActionDeny }
+func (d *DenyAllTrustEvaluator) EvaluatePath(_ string) Action    { return ActionDeny }

executor/mesh_support_test.go

@@ -74,6 +74,7 @@ func TestMeshSupport_EndToEnd(t *testing.T) {
 	cfg := Config{
 		Provider:      p,
 		ToolRegistry:  reg,
+		TrustEngine:   &NullTrustEvaluator{}, // allow all tools in this unit test
 		MaxIterations: 10,
 		Inbox:         inbox,
 		OnEvent: func(e Event) {

executor/trust_test.go

@@ -15,6 +15,19 @@ func TestNullTrustEvaluator(t *testing.T) {
 	}
 }
 
+func TestDenyAllTrustEvaluator(t *testing.T) {
+	te := &DenyAllTrustEvaluator{}
+	if te.Evaluate(context.Background(), "file_write", nil) != ActionDeny {
+		t.Error("DenyAllTrustEvaluator should always deny")
+	}
+	if te.EvaluateCommand("echo hello") != ActionDeny {
+		t.Error("DenyAllTrustEvaluator should always deny commands")
+	}
+	if te.EvaluatePath("/tmp/file") != ActionDeny {
+		t.Error("DenyAllTrustEvaluator should always deny paths")
+	}
+}
+
 type mockTrustEvaluator struct {
 	toolAction Action
 	cmdAction  Action