fix(policy): shell_safe requiredFor layering and plan preset path (#126)

Combine shell_safe token gating with explicit approvals.requiredFor at
runtime; move EffectiveToolDecision shell_safe check outside Approvals
guard; drop unreachable requiresToolCallApproval branch.

Co-authored-by: Cursor <cursoragent@cursor.com>

Author: leo-aa88

CHANGELOG.md

@@ -8,7 +8,7 @@ The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.1.0/).
 
 ### Added
 
-- **Built-in policy presets** (issue #104): `strict`, `permissive`, and `shell_safe`. Select via `Project.spec.defaults.policy`, by referencing a preset name on agents/workflows, or with `Policy.spec.preset` (local rules layer on top). Presets expand during [NormalizeProjectGraph] so plan/validate show effective rules.
+- **Built-in policy presets** (issue #104): `strict`, `permissive`, and `shell_safe`. Select via `Project.spec.defaults.policy`, by referencing a preset name on agents/workflows, or with `Policy.spec.preset` (local rules layer on top). Presets expand during [NormalizeProjectGraph]; `strict`/`permissive` materialize approval flags, while `shell_safe` sets `ResolvedPreset` and relies on runtime token classification plus tool safety metadata for plan risk.
 - **`shell_safe` token classification** for native `command.run` / `run` / `exec` / `shell` operations: read-only first tokens (`ls`, `cat`, …) run unattended when the command contains no shell metacharacters (`;|&$`, newlines, `` ` ``, `$(…)`); risky tokens, unknown tokens, and side-effecting non-shell tools require `--approve`. **Heuristic only — not a sandbox.**
 - **`spec.safety` on Tool resources** (issue #103): optional `trusted`, `sideEffects`, and `requiresApproval` fields. [NormalizeProjectGraph] materializes fail-closed defaults on load.
 - **Policy safety fallback**: when no `approvals.requiredFor` entry matches the exact `uses` string, [policy.Derive] consults resolved safety metadata. Unattended mutating tools require `--approve` (exit code **5**, `approval_required`).

internal/cli/init_test.go

@@ -5,6 +5,8 @@ import (
 	"os"
 	"path/filepath"
 	"testing"
+
+	"github.com/LAA-Software-Engineering/agentic-control-plane/internal/spec"
 )
 
 func TestInit_thenValidateSucceeds(t *testing.T) {
@@ -58,8 +60,8 @@ func TestInit_defaultPolicyExpandsShellSafePreset(t *testing.T) {
 	if !ok || pr == nil {
 		t.Fatal("expected default policy")
 	}
-	if pr.Spec.ResolvedPreset != "shell_safe" {
-		t.Fatalf("default policy ResolvedPreset = %q want shell_safe", pr.Spec.ResolvedPreset)
+	if pr.Spec.ResolvedPreset != spec.PresetShellSafe {
+		t.Fatalf("default policy ResolvedPreset = %q want %s", pr.Spec.ResolvedPreset, spec.PresetShellSafe)
 	}
 }
 

internal/policy/derive.go

@@ -49,6 +49,16 @@ func Derive(safety spec.ResolvedToolSafety) Decision {
 func EffectiveToolDecision(graph *spec.ProjectGraph, pol *spec.PolicySpec, toolName string) ToolDecision {
 	toolName = strings.TrimSpace(toolName)
 	safety := resolvedSafetyForTool(graph, toolName)
+	if pol != nil && spec.ResolvedPresetName(pol) == spec.PresetShellSafe {
+		// shell_safe plan risk is tool-granular (conservative); runtime uses per-command classification.
+		if safety.RequiresApproval || safety.SideEffects {
+			return ToolDecision{
+				Decision: DecisionRequireApproval,
+				Source:   SourceExplicitPolicyRule,
+				Safety:   safety,
+			}
+		}
+	}
 	if pol != nil && pol.Approvals != nil {
 		if spec.ApprovalPermissive(pol.Approvals) {
 			return ToolDecision{
@@ -64,17 +74,6 @@ func EffectiveToolDecision(graph *spec.ProjectGraph, pol *spec.PolicySpec, toolN
 				Safety:   safety,
 			}
 		}
-		// shell_safe plan risk is tool-granular (conservative): side-effecting tools flag approval;
-		// runtime applies per-command token classification via shellSafeRequiresApproval.
-		if spec.ResolvedPresetName(pol) == spec.PresetShellSafe {
-			if safety.RequiresApproval || safety.SideEffects {
-				return ToolDecision{
-					Decision: DecisionRequireApproval,
-					Source:   SourceExplicitPolicyRule,
-					Safety:   safety,
-				}
-			}
-		}
 	}
 	if pol != nil && pol.Approvals != nil {
 		prefix := toolUsesPrefix(toolName)

internal/policy/evaluator.go

@@ -64,7 +64,8 @@ func (e *evaluator) CheckToolCall(ctx context.Context, call ToolCallContext) err
 	}
 	switch {
 	case p != nil && spec.ResolvedPresetName(p) == spec.PresetShellSafe:
-		if shellSafeRequiresApproval(e.graph, call) {
+		needApproval := shellSafeRequiresApproval(e.graph, call) || approvalRequired(call.Uses, p.Approvals)
+		if needApproval {
 			if actionApproved(call.Uses, call.Run.ApprovedActions) {
 				return nil
 			}
@@ -84,19 +85,10 @@ func (e *evaluator) CheckToolCall(ctx context.Context, call ToolCallContext) err
 }
 
 func requiresToolCallApproval(graph *spec.ProjectGraph, pol *spec.PolicySpec, call ToolCallContext) bool {
-	if pol == nil {
+	if pol == nil || pol.Approvals == nil {
 		return false
 	}
-	if spec.ResolvedPresetName(pol) == spec.PresetShellSafe {
-		return shellSafeRequiresApproval(graph, call)
-	}
-	if pol.Approvals == nil {
-		return false
-	}
-	if spec.ApprovalRequireAllTools(pol.Approvals) {
-		return true
-	}
-	return false
+	return spec.ApprovalRequireAllTools(pol.Approvals)
 }
 
 func toolCallApprovalDenied(call ToolCallContext, pol *spec.PolicySpec) error {

internal/policy/presets_eval_test.go

@@ -57,6 +57,27 @@ func TestCheckToolCall_shellSafe_gatesRm(t *testing.T) {
 	}
 }
 
+func TestCheckToolCall_shellSafe_requiredForLayering(t *testing.T) {
+	g := testGraphWithTools("helper")
+	g.Tools["helper"].Spec.Safety = &spec.ToolSafety{SideEffects: spec.BoolPtr(false)}
+	base, err := spec.BuildPreset(spec.PresetShellSafe)
+	if err != nil {
+		t.Fatal(err)
+	}
+	pol := spec.MergePolicySpec(base, spec.PolicySpec{
+		Approvals: &spec.PolicyApprovals{
+			RequiredFor: []string{"tool.helper.echo"},
+		},
+	})
+	ev := NewEvaluator(g, &pol)
+	err = ev.CheckToolCall(context.Background(), ToolCallContext{
+		Uses: "tool.helper.echo",
+	})
+	if err == nil {
+		t.Fatal("shell_safe with local requiredFor should gate exact uses")
+	}
+}
+
 func TestCheckToolCall_shellSafe_gatesChainedCommand(t *testing.T) {
 	pol, err := spec.BuildPreset(spec.PresetShellSafe)
 	if err != nil {
@@ -207,6 +228,21 @@ func TestExpandPresetsInGraph_userPolicyOverridesBuiltin(t *testing.T) {
 	}
 }
 
+func TestEffectiveToolDecision_shellSafe_builtinPresetNoApprovals(t *testing.T) {
+	g := shellSafeGraph()
+	pol, err := spec.BuildPreset(spec.PresetShellSafe)
+	if err != nil {
+		t.Fatal(err)
+	}
+	if pol.Approvals != nil {
+		t.Fatal("builtin shell_safe should not set Approvals")
+	}
+	td := EffectiveToolDecision(g, &pol, "shell")
+	if td.Decision != DecisionRequireApproval {
+		t.Fatalf("plan should flag side-effecting shell tool via ResolvedPreset: %+v", td)
+	}
+}
+
 func TestEffectiveToolDecision_shellSafe_toolGranularPlan(t *testing.T) {
 	g := shellSafeGraph()
 	pol, err := spec.BuildPreset(spec.PresetShellSafe)