From 096393f6fa8aeca6204b4105d73e4e340a3f005f Mon Sep 17 00:00:00 2001 From: Leonardo Araujo Date: Tue, 2 Jun 2026 00:51:18 -0300 Subject: [PATCH 1/3] feat(policy): built-in policy presets strict, permissive, shell_safe (#104) Ship named presets resolvable from Project defaults, direct policy references, or Policy.spec.preset with local overrides layered on top. Expand presets during normalize so validate/plan show effective rules. shell_safe classifies native shell command tokens and integrates with tool safety metadata from #103. Co-authored-by: Cursor --- CHANGELOG.md | 2 + internal/cli/initembed/policies/default.yaml | 1 + internal/engine/steps.go | 2 +- internal/policy/context.go | 2 + internal/policy/derive.go | 25 ++ internal/policy/doc.go | 4 + internal/policy/evaluator.go | 47 +++ internal/policy/presets_eval_test.go | 141 +++++++++ internal/policy/shell_safe.go | 29 ++ internal/spec/doc.go | 3 + internal/spec/kinds.go | 17 +- internal/spec/normalize.go | 5 +- internal/spec/policy_expand.go | 93 ++++++ internal/spec/policy_presets.go | 305 +++++++++++++++++++ internal/spec/presets_test.go | 86 ++++++ internal/spec/resolve.go | 4 +- internal/spec/shell_tokens.go | 75 +++++ internal/spec/shell_tokens_test.go | 120 ++++++++ internal/spec/validator.go | 23 ++ internal/tools/native/registry.go | 21 ++ 20 files changed, 996 insertions(+), 9 deletions(-) create mode 100644 internal/policy/presets_eval_test.go create mode 100644 internal/policy/shell_safe.go create mode 100644 internal/spec/policy_expand.go create mode 100644 internal/spec/policy_presets.go create mode 100644 internal/spec/presets_test.go create mode 100644 internal/spec/shell_tokens.go create mode 100644 internal/spec/shell_tokens_test.go diff --git a/CHANGELOG.md b/CHANGELOG.md index e89a446..5cce362 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -8,6 +8,8 @@ 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. +- **`shell_safe` token classification** for native `command.run` / `run` / `exec` / `shell` operations: read-only tokens (`ls`, `cat`, …) run unattended; risky/unknown tokens and tools with side-effect metadata require `--approve`. - **`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`). - **Plan risk hints** for tools that will require approval at run, including decision source (`explicit_policy_rule`, `safety_metadata`, `fail_closed_default`). diff --git a/internal/cli/initembed/policies/default.yaml b/internal/cli/initembed/policies/default.yaml index 4fe6913..b3ba70b 100644 --- a/internal/cli/initembed/policies/default.yaml +++ b/internal/cli/initembed/policies/default.yaml @@ -3,6 +3,7 @@ kind: Policy metadata: name: default spec: + preset: shell_safe execution: maxWallClockSeconds: 300 maxTotalCostUsd: 5 diff --git a/internal/engine/steps.go b/internal/engine/steps.go index c98c74b..f781cdd 100644 --- a/internal/engine/steps.go +++ b/internal/engine/steps.go @@ -46,7 +46,7 @@ func parseAgentJSONObject(content string) (map[string]any, error) { func (e *Executor) runToolStep(ctx context.Context, pol policy.PolicyEvaluator, runID string, step spec.WorkflowStep, with map[string]any, pctx policy.RunContext) (map[string]any, tools.ToolCallMeta, error) { uses := strings.TrimSpace(step.Uses) - if err := pol.CheckToolCall(ctx, policy.ToolCallContext{Run: pctx, StepID: step.ID, Uses: uses}); err != nil { + if err := pol.CheckToolCall(ctx, policy.ToolCallContext{Run: pctx, StepID: step.ID, Uses: uses, With: with}); err != nil { if e.Trace != nil { if d, ok := policy.AsDenied(err); ok { _, _ = e.Trace.Append(ctx, runID, step.ID, trace.EventPolicyDenied, d.TraceData()) diff --git a/internal/policy/context.go b/internal/policy/context.go index 22cdf95..5e03b8c 100644 --- a/internal/policy/context.go +++ b/internal/policy/context.go @@ -25,4 +25,6 @@ type ToolCallContext struct { Run RunContext StepID string Uses string + // With is the interpolated workflow step input (used by shell_safe token classification). + With map[string]any } diff --git a/internal/policy/derive.go b/internal/policy/derive.go index cab7a61..211e484 100644 --- a/internal/policy/derive.go +++ b/internal/policy/derive.go @@ -49,6 +49,31 @@ 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 && pol.Approvals != nil { + if pol.Approvals.Permissive { + return ToolDecision{ + Decision: DecisionAllow, + Source: SourceExplicitPolicyRule, + Safety: safety, + } + } + if pol.Approvals.RequireAllTools { + return ToolDecision{ + Decision: DecisionRequireApproval, + Source: SourceExplicitPolicyRule, + Safety: safety, + } + } + 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) for _, r := range pol.Approvals.RequiredFor { diff --git a/internal/policy/doc.go b/internal/policy/doc.go index 7a5cbe2..6f9b43f 100644 --- a/internal/policy/doc.go +++ b/internal/policy/doc.go @@ -5,6 +5,10 @@ // Run context should carry elapsed wall clock, accumulated cost, and repeated --approve action strings // matching policy approvals.requiredFor entries. // +// Built-in policy presets (issue #104): strict, permissive, and shell_safe. Select via +// Project.spec.defaults.policy, a Policy resource spec.preset, or by referencing a preset name +// as the workflow/agent policy. [spec.ExpandPresetsInGraph] materializes effective rules during normalize. +// // When no explicit approvals.requiredFor rule matches a tool call, [Derive] consults // [spec.ResolveToolSafety] metadata (fail-closed defaults; issue #103). // diff --git a/internal/policy/evaluator.go b/internal/policy/evaluator.go index 0bc63aa..385e49c 100644 --- a/internal/policy/evaluator.go +++ b/internal/policy/evaluator.go @@ -54,13 +54,60 @@ func (e *evaluator) CheckStep(ctx context.Context, step StepContext) error { func (e *evaluator) CheckToolCall(ctx context.Context, call ToolCallContext) error { _ = ctx p := e.spec() + if p != nil && p.Approvals != nil && p.Approvals.Permissive { + if err := checkKnownTool(e.graph, call.Uses, p.Tools); err != nil { + return err + } + return nil + } if p != nil { if err := checkKnownTool(e.graph, call.Uses, p.Tools); err != nil { return err } + if p.Approvals != nil && p.Approvals.RequireAllTools { + if actionApproved(call.Uses, call.Run.ApprovedActions) { + return nil + } + return denied( + ReasonApprovalRequired, + "policy: action requires explicit approval (--approve)", + call.Uses, + map[string]any{"requiredFor": call.Uses, "preset": spec.PresetStrict}, + ) + } + if spec.ResolvedPresetName(p) == spec.PresetShellSafe && !shellSafeRequiresApproval(e.graph, call) { + return nil + } + if presetRequiresApproval(p, e.graph, call) { + if actionApproved(call.Uses, call.Run.ApprovedActions) { + return nil + } + return denied( + ReasonApprovalRequired, + "policy: action requires explicit approval (--approve)", + call.Uses, + map[string]any{ + "requiredFor": call.Uses, + "preset": spec.ResolvedPresetName(p), + }, + ) + } if approvalRequired(call.Uses, p.Approvals) { return checkApprovalGranted(call.Uses, p.Approvals, call.Run.ApprovedActions) } } return checkSafetyDerived(e.graph, call) } + +func presetRequiresApproval(p *spec.PolicySpec, graph *spec.ProjectGraph, call ToolCallContext) bool { + if p == nil || p.Approvals == nil { + return false + } + if p.Approvals.RequireAllTools { + return true + } + if spec.ResolvedPresetName(p) == spec.PresetShellSafe { + return shellSafeRequiresApproval(graph, call) + } + return false +} diff --git a/internal/policy/presets_eval_test.go b/internal/policy/presets_eval_test.go new file mode 100644 index 0000000..d9572a0 --- /dev/null +++ b/internal/policy/presets_eval_test.go @@ -0,0 +1,141 @@ +package policy + +import ( + "context" + "testing" + + "github.com/LAA-Software-Engineering/agentic-control-plane/internal/spec" +) + +func shellSafeGraph() *spec.ProjectGraph { + return &spec.ProjectGraph{ + Tools: map[string]*spec.ToolResource{ + "shell": { + Metadata: spec.Metadata{Name: "shell"}, + Spec: spec.ToolSpec{ + Type: "native", + Safety: &spec.ToolSafety{ + SideEffects: spec.BoolPtr(true), + }, + }, + }, + }, + } +} + +func TestCheckToolCall_shellSafe_allowsLs(t *testing.T) { + pol, err := spec.BuildPreset(spec.PresetShellSafe) + if err != nil { + t.Fatal(err) + } + ev := NewEvaluator(shellSafeGraph(), &pol) + err = ev.CheckToolCall(context.Background(), ToolCallContext{ + Uses: "tool.shell.command.run", + With: map[string]any{"command": "ls -la"}, + }) + if err != nil { + t.Fatalf("ls should be allowed: %v", err) + } +} + +func TestCheckToolCall_shellSafe_gatesRm(t *testing.T) { + pol, err := spec.BuildPreset(spec.PresetShellSafe) + if err != nil { + t.Fatal(err) + } + ev := NewEvaluator(shellSafeGraph(), &pol) + err = ev.CheckToolCall(context.Background(), ToolCallContext{ + Uses: "tool.shell.command.run", + With: map[string]any{"command": "rm -rf /tmp/x"}, + }) + if err == nil { + t.Fatal("expected rm to require approval") + } + d, ok := AsDenied(err) + if !ok || d.Reason != ReasonApprovalRequired { + t.Fatalf("got %v", err) + } +} + +func TestCheckToolCall_shellSafe_unknownTokenGated(t *testing.T) { + pol, err := spec.BuildPreset(spec.PresetShellSafe) + if err != nil { + t.Fatal(err) + } + ev := NewEvaluator(shellSafeGraph(), &pol) + err = ev.CheckToolCall(context.Background(), ToolCallContext{ + Uses: "tool.shell.command.run", + With: map[string]any{"command": "totally-unknown"}, + }) + if err == nil { + t.Fatal("expected unknown token to gate") + } +} + +func TestCheckToolCall_strict_gatesAllTools(t *testing.T) { + g := testGraphWithTools("helper") + g.Tools["helper"].Spec.Safety = &spec.ToolSafety{SideEffects: spec.BoolPtr(false)} + pol, err := spec.BuildPreset(spec.PresetStrict) + if err != nil { + t.Fatal(err) + } + ev := NewEvaluator(g, &pol) + err = ev.CheckToolCall(context.Background(), ToolCallContext{ + Uses: "tool.helper.echo", + }) + if err == nil { + t.Fatal("strict should gate all tools") + } +} + +func TestCheckToolCall_permissive_allowsMutatingTool(t *testing.T) { + g := testGraphWithTools("slack") + g.Tools["slack"].Spec.Safety = &spec.ToolSafety{SideEffects: spec.BoolPtr(true)} + pol, err := spec.BuildPreset(spec.PresetPermissive) + if err != nil { + t.Fatal(err) + } + ev := NewEvaluator(g, &pol) + err = ev.CheckToolCall(context.Background(), ToolCallContext{ + Uses: "tool.slack.message.send", + }) + if err != nil { + t.Fatalf("permissive should allow: %v", err) + } +} + +func TestExpandPresetsInGraph_materializesDefault(t *testing.T) { + g := &spec.ProjectGraph{ + Spec: spec.ProjectSpec{ + Defaults: &spec.ProjectDefaults{Policy: spec.PresetShellSafe}, + }, + } + spec.ExpandPresetsInGraph(g) + pr, ok := g.Policies[spec.PresetShellSafe] + if !ok || pr == nil { + t.Fatal("expected injected shell_safe policy") + } + if pr.Spec.ResolvedPreset != spec.PresetShellSafe { + t.Fatalf("ResolvedPreset = %q", pr.Spec.ResolvedPreset) + } +} + +func TestExpandPresetsInGraph_userPolicyOverridesBuiltin(t *testing.T) { + g := &spec.ProjectGraph{ + Spec: spec.ProjectSpec{ + Defaults: &spec.ProjectDefaults{Policy: spec.PresetStrict}, + }, + Policies: map[string]*spec.PolicyResource{ + spec.PresetStrict: { + Metadata: spec.Metadata{Name: spec.PresetStrict}, + Spec: spec.PolicySpec{ + Execution: &spec.PolicyExecution{MaxWallClockSeconds: 99}, + }, + }, + }, + } + spec.ExpandPresetsInGraph(g) + if g.Policies[spec.PresetStrict].Spec.Execution.MaxWallClockSeconds != 99 { + t.Fatal("user policy should not be replaced by builtin") + } +} diff --git a/internal/policy/shell_safe.go b/internal/policy/shell_safe.go new file mode 100644 index 0000000..5764df5 --- /dev/null +++ b/internal/policy/shell_safe.go @@ -0,0 +1,29 @@ +package policy + +import ( + "github.com/LAA-Software-Engineering/agentic-control-plane/internal/spec" + "github.com/LAA-Software-Engineering/agentic-control-plane/internal/tools" +) + +// shellSafeRequiresApproval reports whether a tool call should be gated under shell_safe. +func shellSafeRequiresApproval(graph *spec.ProjectGraph, call ToolCallContext) bool { + toolName, operation, err := tools.ParseUses(call.Uses) + if err != nil { + return true + } + if spec.IsShellCommandOperation(operation) { + cmd := spec.ExtractShellCommand(call.With) + token := spec.FirstShellToken(cmd) + switch spec.ClassifyShellToken(token) { + case spec.ShellTokenReadOnly: + return false + case spec.ShellTokenGate, spec.ShellTokenUnknown: + return true + } + } + safety := resolvedSafetyForTool(graph, toolName) + if safety.RequiresApproval || safety.SideEffects { + return true + } + return false +} diff --git a/internal/spec/doc.go b/internal/spec/doc.go index c100a60..af232c8 100644 --- a/internal/spec/doc.go +++ b/internal/spec/doc.go @@ -6,4 +6,7 @@ // // Tool resources may declare spec.safety (trusted, sideEffects, requiresApproval) for // fail-closed policy derivation when explicit Policy rules do not apply (issue #103). +// +// Built-in policy presets (strict, permissive, shell_safe) are defined in this package and +// expanded during [NormalizeProjectGraph] via [ExpandPresetsInGraph] (issue #104). package spec diff --git a/internal/spec/kinds.go b/internal/spec/kinds.go index c1c661c..30dec10 100644 --- a/internal/spec/kinds.go +++ b/internal/spec/kinds.go @@ -159,10 +159,15 @@ type WorkflowOutput struct { // --- Policy (design doc §7.5, MVP) --- type PolicySpec struct { - Execution *PolicyExecution `yaml:"execution,omitempty" json:"execution,omitempty"` - Tools *PolicyTools `yaml:"tools,omitempty" json:"tools,omitempty"` - Approvals *PolicyApprovals `yaml:"approvals,omitempty" json:"approvals,omitempty"` - Security *PolicySecurity `yaml:"security,omitempty" json:"security,omitempty"` + // Preset references a built-in policy preset (strict, permissive, shell_safe) as a base + // for this Policy resource; local spec fields layer on top (issue #104). + Preset string `yaml:"preset,omitempty" json:"preset,omitempty"` + // ResolvedPreset is populated during [NormalizeProjectGraph] when a preset is expanded; not author YAML. + ResolvedPreset string `yaml:"-" json:"-"` + Execution *PolicyExecution `yaml:"execution,omitempty" json:"execution,omitempty"` + Tools *PolicyTools `yaml:"tools,omitempty" json:"tools,omitempty"` + Approvals *PolicyApprovals `yaml:"approvals,omitempty" json:"approvals,omitempty"` + Security *PolicySecurity `yaml:"security,omitempty" json:"security,omitempty"` } type PolicyExecution struct { @@ -177,6 +182,10 @@ type PolicyTools struct { type PolicyApprovals struct { RequiredFor []string `yaml:"requiredFor,omitempty" json:"requiredFor,omitempty"` + // RequireAllTools is set when the strict preset is expanded (every tool call requires approval). + RequireAllTools bool `yaml:"requireAllTools,omitempty" json:"requireAllTools,omitempty"` + // Permissive is set when the permissive preset is expanded (never gate tool calls). + Permissive bool `yaml:"permissive,omitempty" json:"permissive,omitempty"` } type PolicySecurity struct { diff --git a/internal/spec/normalize.go b/internal/spec/normalize.go index 694853e..dd66c73 100644 --- a/internal/spec/normalize.go +++ b/internal/spec/normalize.go @@ -3,8 +3,8 @@ package spec import "strings" // NormalizeProjectGraph applies Project.spec.defaults to resources that omit matching -// fields, materializes Tool safety defaults (issue #103), and performs trivial string -// canonicalization (trim surrounding ASCII space). +// fields, materializes Tool safety defaults (issue #103), expands built-in policy presets +// (issue #104), and performs trivial string canonicalization (trim surrounding ASCII space). // // Default application (§7.1 → effective config): // - Agent.spec.model ← defaults.model when the agent omits model (empty / whitespace-only). @@ -41,6 +41,7 @@ func NormalizeProjectGraph(g *ProjectGraph) { } NormalizeToolSafety(&tr.Spec) } + ExpandPresetsInGraph(g) } func normalizeAgentSpec(spec *AgentSpec, defModel, defPolicy, defRuntime string) { diff --git a/internal/spec/policy_expand.go b/internal/spec/policy_expand.go new file mode 100644 index 0000000..f4d5cc2 --- /dev/null +++ b/internal/spec/policy_expand.go @@ -0,0 +1,93 @@ +package spec + +import "strings" + +// ExpandPresetsInGraph materializes built-in policy presets referenced by Project defaults, +// agents, workflows, and Policy.spec.preset (issue #104). User-defined Policy resources with +// the same metadata.name override built-ins. Mutates g in place. +func ExpandPresetsInGraph(g *ProjectGraph) { + if g == nil { + return + } + if g.Policies == nil { + g.Policies = make(map[string]*PolicyResource) + } + for _, name := range collectReferencedPolicyNames(g) { + if !IsBuiltinPreset(name) { + continue + } + if _, exists := g.Policies[name]; exists { + continue + } + preset, err := BuildPreset(name) + if err != nil { + continue + } + g.Policies[name] = &PolicyResource{ + APIVersion: APIVersionV0, + Kind: KindPolicy, + Metadata: Metadata{Name: name}, + Spec: preset, + } + } + for _, pr := range g.Policies { + if pr == nil { + continue + } + resolved, err := resolvePolicyResourcePreset(&pr.Spec) + if err != nil || resolved == nil { + continue + } + pr.Spec = *resolved + } +} + +func resolvePolicyResourcePreset(pol *PolicySpec) (*PolicySpec, error) { + if pol == nil { + return nil, nil + } + presetName := strings.TrimSpace(pol.Preset) + if presetName == "" { + if pol.ResolvedPreset != "" { + return nil, nil + } + return nil, nil + } + if !IsBuiltinPreset(presetName) { + return nil, nil + } + return ResolvePolicySpec(pol) +} + +func collectReferencedPolicyNames(g *ProjectGraph) []string { + seen := make(map[string]struct{}) + add := func(name string) { + name = strings.TrimSpace(name) + if name == "" { + return + } + seen[name] = struct{}{} + } + if g.Spec.Defaults != nil { + add(g.Spec.Defaults.Policy) + } + for _, ar := range g.Agents { + if ar != nil { + add(ar.Spec.Policy) + } + } + for _, wr := range g.Workflows { + if wr != nil { + add(wr.Spec.Policy) + } + } + for name, pr := range g.Policies { + add(name) + _ = pr + } + out := make([]string, 0, len(seen)) + for name := range seen { + out = append(out, name) + } + return out +} diff --git a/internal/spec/policy_presets.go b/internal/spec/policy_presets.go new file mode 100644 index 0000000..6a08b75 --- /dev/null +++ b/internal/spec/policy_presets.go @@ -0,0 +1,305 @@ +package spec + +import ( + "fmt" + "sort" + "strings" +) + +// Built-in policy preset names (issue #104). +const ( + PresetStrict = "strict" + PresetPermissive = "permissive" + PresetShellSafe = "shell_safe" +) + +// shellCommandOperations are native tool operations that carry a shell command in step input. +var shellCommandOperations = []string{"command.run", "run", "exec", "shell"} + +var shellGateTokens = map[string]struct{}{ + "rm": {}, "mv": {}, "cp": {}, "chmod": {}, "chown": {}, "mkfifo": {}, "dd": {}, + "curl": {}, "wget": {}, "ssh": {}, "exec": {}, "eval": {}, "write": {}, "delete": {}, +} + +// ErrUnknownPreset is returned when a policy references an unrecognized preset name. +type ErrUnknownPreset struct { + Name string +} + +func (e *ErrUnknownPreset) Error() string { + if e == nil || e.Name == "" { + return "policy: unknown preset" + } + return fmt.Sprintf("policy: unknown preset %q (valid: %s)", e.Name, strings.Join(BuiltinPresetNames(), ", ")) +} + +// BuiltinPresetNames returns sorted built-in preset identifiers. +func BuiltinPresetNames() []string { + names := make([]string, 0, len(builtinPresetBuilders)) + for name := range builtinPresetBuilders { + names = append(names, name) + } + sort.Strings(names) + return names +} + +// IsBuiltinPreset reports whether name is a built-in preset identifier. +func IsBuiltinPreset(name string) bool { + name = strings.TrimSpace(name) + if name == "" { + return false + } + _, ok := builtinPresetBuilders[name] + return ok +} + +var builtinPresetBuilders = map[string]func() PolicySpec{ + PresetStrict: buildStrictPreset, + PresetPermissive: buildPermissivePreset, + PresetShellSafe: buildShellSafePreset, +} + +func buildStrictPreset() PolicySpec { + return PolicySpec{ + ResolvedPreset: PresetStrict, + Approvals: &PolicyApprovals{ + RequireAllTools: true, + }, + } +} + +func buildPermissivePreset() PolicySpec { + return PolicySpec{ + ResolvedPreset: PresetPermissive, + Approvals: &PolicyApprovals{ + Permissive: true, + }, + } +} + +func buildShellSafePreset() PolicySpec { + return PolicySpec{ + ResolvedPreset: PresetShellSafe, + Approvals: &PolicyApprovals{ + RequiredFor: shellSafeExpandedRequiredFor(), + }, + } +} + +func shellSafeExpandedRequiredFor() []string { + out := make([]string, 0, len(shellGateTokens)*len(shellCommandOperations)) + for _, op := range shellCommandOperations { + for token := range shellGateTokens { + out = append(out, fmt.Sprintf("preset:%s:%s:%s", PresetShellSafe, op, token)) + } + } + sort.Strings(out) + return out +} + +// BuildPreset returns a fresh [PolicySpec] for a built-in preset name. +func BuildPreset(name string) (PolicySpec, error) { + name = strings.TrimSpace(name) + build, ok := builtinPresetBuilders[name] + if !ok { + return PolicySpec{}, &ErrUnknownPreset{Name: name} + } + return build(), nil +} + +// MergePolicySpec layers local policy fields on top of a preset base (issue #104). +func MergePolicySpec(base, overlay PolicySpec) PolicySpec { + out := base + if overlay.Execution != nil { + out.Execution = clonePolicyExecution(overlay.Execution) + } + if overlay.Tools != nil { + out.Tools = clonePolicyTools(overlay.Tools) + } + if overlay.Approvals != nil { + out.Approvals = mergePolicyApprovals(base.Approvals, overlay.Approvals) + } + if overlay.Security != nil { + out.Security = clonePolicySecurity(overlay.Security) + } + if overlay.ResolvedPreset != "" { + out.ResolvedPreset = overlay.ResolvedPreset + } + return out +} + +// ResolvePolicySpec expands Preset (when set) and returns the effective merged policy. +func ResolvePolicySpec(pol *PolicySpec) (*PolicySpec, error) { + if pol == nil { + return nil, nil + } + presetName := strings.TrimSpace(pol.Preset) + if presetName == "" { + cp := *pol + return &cp, nil + } + base, err := BuildPreset(presetName) + if err != nil { + return nil, err + } + merged := MergePolicySpec(base, *pol) + merged.Preset = presetName + merged.ResolvedPreset = presetName + return &merged, nil +} + +func mergePolicyApprovals(base, overlay *PolicyApprovals) *PolicyApprovals { + if overlay == nil { + return clonePolicyApprovals(base) + } + out := &PolicyApprovals{ + RequireAllTools: overlay.RequireAllTools, + Permissive: overlay.Permissive, + } + if base != nil { + if !out.RequireAllTools { + out.RequireAllTools = base.RequireAllTools + } + if !out.Permissive { + out.Permissive = base.Permissive + } + } + out.RequiredFor = mergePresetRequiredFor( + presetRequiredForSlice(base), + presetRequiredForSlice(overlay), + ) + if out.RequiredFor == nil && !out.RequireAllTools && !out.Permissive { + return nil + } + return out +} + +func presetRequiredForSlice(a *PolicyApprovals) []string { + if a == nil { + return nil + } + return append([]string(nil), a.RequiredFor...) +} + +func mergePresetRequiredFor(base, overlay []string) []string { + if len(overlay) == 0 { + return append([]string(nil), base...) + } + if len(base) == 0 { + return append([]string(nil), overlay...) + } + overrideTools := make(map[string]struct{}, len(overlay)) + for _, r := range overlay { + if tn := toolNameFromPresetRequiredFor(r); tn != "" { + overrideTools[tn] = struct{}{} + } + } + var kept []string + for _, r := range base { + if tn := toolNameFromPresetRequiredFor(r); tn != "" { + if _, overridden := overrideTools[tn]; overridden { + continue + } + } + kept = append(kept, r) + } + out := append(kept, overlay...) + sort.Strings(out) + return dedupePresetStrings(out) +} + +func toolNameFromPresetRequiredFor(entry string) string { + entry = strings.TrimSpace(entry) + const prefix = "tool." + if !strings.HasPrefix(entry, prefix) { + return "" + } + rest := strings.TrimPrefix(entry, prefix) + i := strings.IndexByte(rest, '.') + if i <= 0 { + return "" + } + return rest[:i] +} + +func dedupePresetStrings(in []string) []string { + if len(in) == 0 { + return nil + } + seen := make(map[string]struct{}, len(in)) + out := make([]string, 0, len(in)) + for _, s := range in { + s = strings.TrimSpace(s) + if s == "" { + continue + } + if _, ok := seen[s]; ok { + continue + } + seen[s] = struct{}{} + out = append(out, s) + } + return out +} + +func clonePolicyExecution(in *PolicyExecution) *PolicyExecution { + if in == nil { + return nil + } + cp := *in + return &cp +} + +func clonePolicyTools(in *PolicyTools) *PolicyTools { + if in == nil { + return nil + } + cp := *in + return &cp +} + +func clonePolicySecurity(in *PolicySecurity) *PolicySecurity { + if in == nil { + return nil + } + cp := *in + return &cp +} + +func clonePolicyApprovals(in *PolicyApprovals) *PolicyApprovals { + if in == nil { + return nil + } + cp := *in + if in.RequiredFor != nil { + cp.RequiredFor = append([]string(nil), in.RequiredFor...) + } + return &cp +} + +// ResolvedPresetName returns the effective preset mode for a policy spec. +func ResolvedPresetName(pol *PolicySpec) string { + if pol == nil { + return "" + } + if pol.ResolvedPreset != "" { + return pol.ResolvedPreset + } + return strings.TrimSpace(pol.Preset) +} + +// ShellCommandOperations returns native operations subject to shell_safe token classification. +func ShellCommandOperations() []string { + return append([]string(nil), shellCommandOperations...) +} + +// IsShellCommandOperation reports whether operation is a shell command carrier for shell_safe. +func IsShellCommandOperation(operation string) bool { + op := strings.ToLower(strings.TrimSpace(operation)) + for _, candidate := range shellCommandOperations { + if op == candidate { + return true + } + } + return false +} diff --git a/internal/spec/presets_test.go b/internal/spec/presets_test.go new file mode 100644 index 0000000..0aa3c99 --- /dev/null +++ b/internal/spec/presets_test.go @@ -0,0 +1,86 @@ +package spec + +import ( + "strings" + "testing" +) + +func TestValidateProjectGraph_unknownPresetOnPolicy(t *testing.T) { + g := &ProjectGraph{ + Policies: map[string]*PolicyResource{ + "custom": { + Metadata: Metadata{Name: "custom"}, + Spec: PolicySpec{ + Preset: "bogus", + }, + }, + }, + } + err := ValidateProjectGraph(g, t.TempDir()) + if err == nil { + t.Fatal("expected validation error") + } + if !strings.Contains(err.Error(), `unknown preset "bogus"`) { + t.Fatalf("got %v", err) + } +} + +func TestValidateProjectGraph_defaultsPolicyBuiltinPreset(t *testing.T) { + g := &ProjectGraph{ + Spec: ProjectSpec{ + Defaults: &ProjectDefaults{Policy: PresetShellSafe}, + }, + } + NormalizeProjectGraph(g) + if err := ValidateProjectGraph(g, t.TempDir()); err != nil { + t.Fatal(err) + } + if pr, ok := g.Policies[PresetShellSafe]; !ok || pr.Spec.ResolvedPreset != PresetShellSafe { + t.Fatalf("expected expanded shell_safe policy, got %+v", g.Policies[PresetShellSafe]) + } +} + +func TestNormalizeProjectGraph_expandsPolicyPreset(t *testing.T) { + g := &ProjectGraph{ + Policies: map[string]*PolicyResource{ + "team": { + Metadata: Metadata{Name: "team"}, + Spec: PolicySpec{ + Preset: PresetStrict, + Execution: &PolicyExecution{ + MaxWallClockSeconds: 120, + }, + }, + }, + }, + } + NormalizeProjectGraph(g) + sp := g.Policies["team"].Spec + if sp.ResolvedPreset != PresetStrict { + t.Fatalf("ResolvedPreset = %q", sp.ResolvedPreset) + } + if sp.Approvals == nil || !sp.Approvals.RequireAllTools { + t.Fatalf("strict expansion: %+v", sp.Approvals) + } + if sp.Execution == nil || sp.Execution.MaxWallClockSeconds != 120 { + t.Fatalf("local execution override lost: %+v", sp.Execution) + } +} + +func TestResolveReferences_builtinPresetPolicyOK(t *testing.T) { + g := &ProjectGraph{ + Spec: ProjectSpec{ + Defaults: &ProjectDefaults{Policy: PresetPermissive}, + }, + Workflows: map[string]*WorkflowResource{ + "wf": { + Metadata: Metadata{Name: "wf"}, + Spec: WorkflowSpec{}, + }, + }, + } + NormalizeProjectGraph(g) + if err := ResolveReferences(g); err != nil { + t.Fatal(err) + } +} diff --git a/internal/spec/resolve.go b/internal/spec/resolve.go index 7de355a..a67f7ba 100644 --- a/internal/spec/resolve.go +++ b/internal/spec/resolve.go @@ -43,7 +43,7 @@ func collectReferenceErrors(g *ProjectGraph) []error { } } for agentName, pol := range ix.AgentPolicies { - if _, ok := g.Policies[pol]; !ok { + if _, ok := g.Policies[pol]; !ok && !IsBuiltinPreset(pol) { errs = append(errs, &MissingRefError{ Referrer: ResourceID{Kind: KindAgent, Name: agentName}, Missing: ResourceID{Kind: KindPolicy, Name: pol}, @@ -73,7 +73,7 @@ func collectReferenceErrors(g *ProjectGraph) []error { } } if pol := ix.WorkflowPolicies[wfName]; pol != "" { - if _, ok := g.Policies[pol]; !ok { + if _, ok := g.Policies[pol]; !ok && !IsBuiltinPreset(pol) { errs = append(errs, &MissingRefError{ Referrer: ResourceID{Kind: KindWorkflow, Name: wfName}, Missing: ResourceID{Kind: KindPolicy, Name: pol}, diff --git a/internal/spec/shell_tokens.go b/internal/spec/shell_tokens.go new file mode 100644 index 0000000..013d610 --- /dev/null +++ b/internal/spec/shell_tokens.go @@ -0,0 +1,75 @@ +package spec + +import "strings" + +var shellReadOnlyTokens = map[string]struct{}{ + "ls": {}, "cat": {}, "grep": {}, "head": {}, "tail": {}, "stat": {}, + "find": {}, "pwd": {}, "wc": {}, "which": {}, +} + +// ShellTokenClass classifies the first token of a shell command for shell_safe policy. +type ShellTokenClass int + +const ( + ShellTokenUnknown ShellTokenClass = iota + ShellTokenReadOnly + ShellTokenGate +) + +// ClassifyShellToken maps the first command token to read-only, gate, or unknown (fail-closed → gate). +func ClassifyShellToken(token string) ShellTokenClass { + token = normalizeShellToken(token) + if token == "" { + return ShellTokenUnknown + } + if _, ok := shellReadOnlyTokens[token]; ok { + return ShellTokenReadOnly + } + if _, ok := shellGateTokens[token]; ok { + return ShellTokenGate + } + return ShellTokenUnknown +} + +func normalizeShellToken(token string) string { + token = strings.TrimSpace(token) + if token == "" { + return "" + } + token = strings.Trim(token, `"'`) + if i := strings.LastIndexAny(token, "/\\"); i >= 0 { + token = token[i+1:] + } + if idx := strings.IndexByte(token, '='); idx >= 0 { + token = token[:idx] + } + return strings.ToLower(strings.TrimSpace(token)) +} + +// FirstShellToken returns the first whitespace-delimited token from a shell command string. +func FirstShellToken(command string) string { + command = strings.TrimSpace(command) + if command == "" { + return "" + } + fields := strings.Fields(command) + if len(fields) == 0 { + return "" + } + return strings.Trim(fields[0], `"'`) +} + +// ExtractShellCommand reads a command string from a workflow step input map. +func ExtractShellCommand(with map[string]any) string { + if with == nil { + return "" + } + for _, key := range []string{"command", "cmd", "script"} { + if v, ok := with[key]; ok { + if s, ok := v.(string); ok { + return strings.TrimSpace(s) + } + } + } + return "" +} diff --git a/internal/spec/shell_tokens_test.go b/internal/spec/shell_tokens_test.go new file mode 100644 index 0000000..397c7b1 --- /dev/null +++ b/internal/spec/shell_tokens_test.go @@ -0,0 +1,120 @@ +package spec + +import ( + "errors" + "testing" +) + +func TestClassifyShellToken(t *testing.T) { + tests := []struct { + token string + want ShellTokenClass + }{ + {"ls", ShellTokenReadOnly}, + {"/bin/ls", ShellTokenReadOnly}, + {"LS", ShellTokenReadOnly}, + {"cat", ShellTokenReadOnly}, + {"grep", ShellTokenReadOnly}, + {"rm", ShellTokenGate}, + {"rm -rf", ShellTokenGate}, + {"/usr/bin/rm", ShellTokenGate}, + {"curl", ShellTokenGate}, + {"wget", ShellTokenGate}, + {"unknown-cmd", ShellTokenUnknown}, + {"", ShellTokenUnknown}, + {" ", ShellTokenUnknown}, + } + for _, tt := range tests { + t.Run(tt.token, func(t *testing.T) { + if got := ClassifyShellToken(FirstShellToken(tt.token)); got != tt.want { + t.Fatalf("ClassifyShellToken(%q) = %v want %v", tt.token, got, tt.want) + } + }) + } +} + +func TestFirstShellToken_adversarial(t *testing.T) { + tests := []struct { + cmd string + want string + }{ + {"ls -la /tmp", "ls"}, + {" rm -rf /", "rm"}, + {`"cat" file`, "cat"}, + {"", ""}, + } + for _, tt := range tests { + if got := FirstShellToken(tt.cmd); got != tt.want { + t.Fatalf("FirstShellToken(%q) = %q want %q", tt.cmd, got, tt.want) + } + } +} + +func TestBuildPreset_unknown(t *testing.T) { + _, err := BuildPreset("nope") + if err == nil { + t.Fatal("expected error") + } + var pe *ErrUnknownPreset + if !errors.As(err, &pe) || pe.Name != "nope" { + t.Fatalf("got %v", err) + } +} + +func TestMergePolicySpec_localRequiredForOverridesPresetTool(t *testing.T) { + base, err := BuildPreset(PresetShellSafe) + if err != nil { + t.Fatal(err) + } + overlay := PolicySpec{ + Approvals: &PolicyApprovals{ + RequiredFor: []string{"tool.helper.echo"}, + }, + } + merged := MergePolicySpec(base, overlay) + if merged.Approvals == nil { + t.Fatal("nil approvals") + } + foundEcho := false + for _, r := range merged.Approvals.RequiredFor { + if r == "tool.helper.echo" { + foundEcho = true + } + } + if !foundEcho { + t.Fatalf("missing local override: %v", merged.Approvals.RequiredFor) + } +} + +func TestBuildPreset_strict_expanded(t *testing.T) { + p, err := BuildPreset(PresetStrict) + if err != nil { + t.Fatal(err) + } + if p.Approvals == nil || !p.Approvals.RequireAllTools { + t.Fatalf("strict preset: %+v", p.Approvals) + } +} + +func TestBuildPreset_permissive_expanded(t *testing.T) { + p, err := BuildPreset(PresetPermissive) + if err != nil { + t.Fatal(err) + } + if p.Approvals == nil || !p.Approvals.Permissive { + t.Fatalf("permissive preset: %+v", p.Approvals) + } +} + +func TestBuildPreset_shellSafe_hasGatePatterns(t *testing.T) { + p, err := BuildPreset(PresetShellSafe) + if err != nil { + t.Fatal(err) + } + if p.ResolvedPreset != PresetShellSafe { + t.Fatalf("ResolvedPreset = %q", p.ResolvedPreset) + } + if p.Approvals == nil || len(p.Approvals.RequiredFor) == 0 { + t.Fatal("expected expanded gate patterns") + } +} diff --git a/internal/spec/validator.go b/internal/spec/validator.go index 2860304..83b9bcd 100644 --- a/internal/spec/validator.go +++ b/internal/spec/validator.go @@ -26,6 +26,7 @@ func ValidateProjectGraph(g *ProjectGraph, projectRoot string) error { errs = append(errs, validateMVPRuntimes(g)...) errs = append(errs, validateToolSpecs(g)...) errs = append(errs, validatePolicySpecs(g)...) + errs = append(errs, validatePolicyPresets(g)...) errs = append(errs, validateAgentSpecs(g)...) errs = append(errs, validateEnvironmentOverrides(g)...) errs = append(errs, validateSchemaFiles(g, root)...) @@ -253,6 +254,28 @@ func validatePolicySpecs(g *ProjectGraph) []error { return errs } +func validatePolicyPresets(g *ProjectGraph) []error { + var errs []error + if g.Spec.Defaults != nil { + if p := strings.TrimSpace(g.Spec.Defaults.Policy); p != "" { + if _, ok := g.Policies[p]; !ok && !IsBuiltinPreset(p) { + errs = append(errs, fmt.Errorf("Project: defaults.policy %q is not a Policy resource or built-in preset (%s)", + p, strings.Join(BuiltinPresetNames(), ", "))) + } + } + } + for name, pr := range g.Policies { + if pr == nil { + continue + } + if preset := strings.TrimSpace(pr.Spec.Preset); preset != "" && !IsBuiltinPreset(preset) { + errs = append(errs, fmt.Errorf("Policy/%s: unknown preset %q (valid: %s)", + name, preset, strings.Join(BuiltinPresetNames(), ", "))) + } + } + return errs +} + func validateAgentSpecs(g *ProjectGraph) []error { var errs []error for name, ar := range g.Agents { diff --git a/internal/tools/native/registry.go b/internal/tools/native/registry.go index 9a9f3c9..a15f5ef 100644 --- a/internal/tools/native/registry.go +++ b/internal/tools/native/registry.go @@ -38,6 +38,13 @@ func (r *Registry) Dispatch(ctx context.Context, operation string, with map[stri v, ok := with["value"] meta.DurationMs = time.Since(start).Milliseconds() return map[string]any{"value": v, "ok": ok}, meta, nil + case "command.run", "run", "exec", "shell": + meta.DurationMs = time.Since(start).Milliseconds() + cmd := shellCommandFromWith(with) + if cmd == "" { + return nil, meta, fmt.Errorf("native: %s requires string field command, cmd, or script", operation) + } + return map[string]any{"command": cmd}, meta, nil case "pull_request.fetch": // Offline demo: parse JSON from `pr` (interpolated from workflow input). No network. meta.DurationMs = time.Since(start).Milliseconds() @@ -127,3 +134,17 @@ func shallowCopy(m map[string]any) map[string]any { } return out } + +func shellCommandFromWith(with map[string]any) string { + if with == nil { + return "" + } + for _, key := range []string{"command", "cmd", "script"} { + if v, ok := with[key]; ok { + if s, ok := v.(string); ok { + return strings.TrimSpace(s) + } + } + } + return "" +} From 40b953573b5e52b766bc0f5d3f81f6f2adcdea7b Mon Sep 17 00:00:00 2001 From: Leonardo Araujo Date: Tue, 2 Jun 2026 00:58:49 -0300 Subject: [PATCH 2/3] =?UTF-8?q?fix(policy):=20address=20PR=20#126=20review?= =?UTF-8?q?=20=E2=80=94=20shell=5Fsafe=20hardening=20and=20preset=20merge?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Fail closed on shell metacharacters; remove dead synthetic requiredFor entries; use tri-state *bool merge for requireAllTools/permissive overlays; unify shell command helpers; simplify evaluator decision path; add adversarial and integration tests. Co-authored-by: Cursor --- CHANGELOG.md | 2 +- internal/cli/init_test.go | 28 ++++++++ internal/policy/derive.go | 6 +- internal/policy/doc.go | 3 + internal/policy/evaluator.go | 74 ++++++++++----------- internal/policy/presets_eval_test.go | 81 ++++++++++++++++++++++- internal/policy/shell_safe.go | 14 +--- internal/spec/kinds.go | 8 +-- internal/spec/policy_expand.go | 19 ++---- internal/spec/policy_presets.go | 96 +++++++++++++--------------- internal/spec/presets_test.go | 2 +- internal/spec/shell_tokens.go | 52 +++++++++++++++ internal/spec/shell_tokens_test.go | 66 +++++++++++++++++-- internal/spec/validator.go | 3 + internal/tools/native/registry.go | 31 +++------ 15 files changed, 337 insertions(+), 148 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 5cce362..114cba5 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -9,7 +9,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. -- **`shell_safe` token classification** for native `command.run` / `run` / `exec` / `shell` operations: read-only tokens (`ls`, `cat`, …) run unattended; risky/unknown tokens and tools with side-effect metadata require `--approve`. +- **`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`). - **Plan risk hints** for tools that will require approval at run, including decision source (`explicit_policy_rule`, `safety_metadata`, `fail_closed_default`). diff --git a/internal/cli/init_test.go b/internal/cli/init_test.go index 68295df..387e2f3 100644 --- a/internal/cli/init_test.go +++ b/internal/cli/init_test.go @@ -35,6 +35,34 @@ func TestInit_thenValidateSucceeds(t *testing.T) { } } +func TestInit_defaultPolicyExpandsShellSafePreset(t *testing.T) { + parent := t.TempDir() + name := "shellsafe" + + ResetGlobalsForTest() + cmd := NewRootCmd() + cmd.SetOut(io.Discard) + cmd.SetErr(io.Discard) + cmd.SetArgs([]string{"init", name, "--parent-dir", parent}) + if err := cmd.Execute(); err != nil { + t.Fatal(err) + } + + ResetGlobalsForTest() + g := &Global{ProjectRoot: filepath.Join(parent, name)} + graph, _, err := prepareProjectGraph(g.ProjectRoot, g) + if err != nil { + t.Fatal(err) + } + pr, ok := graph.Policies["default"] + 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) + } +} + func TestInit_rejectsExistingDir(t *testing.T) { parent := t.TempDir() name := "dup" diff --git a/internal/policy/derive.go b/internal/policy/derive.go index 211e484..c264f6d 100644 --- a/internal/policy/derive.go +++ b/internal/policy/derive.go @@ -50,20 +50,22 @@ func EffectiveToolDecision(graph *spec.ProjectGraph, pol *spec.PolicySpec, toolN toolName = strings.TrimSpace(toolName) safety := resolvedSafetyForTool(graph, toolName) if pol != nil && pol.Approvals != nil { - if pol.Approvals.Permissive { + if spec.ApprovalPermissive(pol.Approvals) { return ToolDecision{ Decision: DecisionAllow, Source: SourceExplicitPolicyRule, Safety: safety, } } - if pol.Approvals.RequireAllTools { + if spec.ApprovalRequireAllTools(pol.Approvals) { return ToolDecision{ Decision: DecisionRequireApproval, Source: SourceExplicitPolicyRule, 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{ diff --git a/internal/policy/doc.go b/internal/policy/doc.go index 6f9b43f..17ca584 100644 --- a/internal/policy/doc.go +++ b/internal/policy/doc.go @@ -9,6 +9,9 @@ // Project.spec.defaults.policy, a Policy resource spec.preset, or by referencing a preset name // as the workflow/agent policy. [spec.ExpandPresetsInGraph] materializes effective rules during normalize. // +// shell_safe uses first-token heuristics plus metacharacter fail-closed checks — not a sandbox. +// Plan risk for shell_safe is tool-granular (conservative); runtime applies per-command classification. +// // When no explicit approvals.requiredFor rule matches a tool call, [Derive] consults // [spec.ResolveToolSafety] metadata (fail-closed defaults; issue #103). // diff --git a/internal/policy/evaluator.go b/internal/policy/evaluator.go index 385e49c..d03a189 100644 --- a/internal/policy/evaluator.go +++ b/internal/policy/evaluator.go @@ -54,60 +54,62 @@ func (e *evaluator) CheckStep(ctx context.Context, step StepContext) error { func (e *evaluator) CheckToolCall(ctx context.Context, call ToolCallContext) error { _ = ctx p := e.spec() - if p != nil && p.Approvals != nil && p.Approvals.Permissive { - if err := checkKnownTool(e.graph, call.Uses, p.Tools); err != nil { - return err - } - return nil - } if p != nil { if err := checkKnownTool(e.graph, call.Uses, p.Tools); err != nil { return err } - if p.Approvals != nil && p.Approvals.RequireAllTools { - if actionApproved(call.Uses, call.Run.ApprovedActions) { - return nil - } - return denied( - ReasonApprovalRequired, - "policy: action requires explicit approval (--approve)", - call.Uses, - map[string]any{"requiredFor": call.Uses, "preset": spec.PresetStrict}, - ) - } - if spec.ResolvedPresetName(p) == spec.PresetShellSafe && !shellSafeRequiresApproval(e.graph, call) { + if p.Approvals != nil && spec.ApprovalPermissive(p.Approvals) { return nil } - if presetRequiresApproval(p, e.graph, call) { + } + switch { + case p != nil && spec.ResolvedPresetName(p) == spec.PresetShellSafe: + if shellSafeRequiresApproval(e.graph, call) { if actionApproved(call.Uses, call.Run.ApprovedActions) { return nil } - return denied( - ReasonApprovalRequired, - "policy: action requires explicit approval (--approve)", - call.Uses, - map[string]any{ - "requiredFor": call.Uses, - "preset": spec.ResolvedPresetName(p), - }, - ) + return toolCallApprovalDenied(call, p) } - if approvalRequired(call.Uses, p.Approvals) { - return checkApprovalGranted(call.Uses, p.Approvals, call.Run.ApprovedActions) + return nil + case requiresToolCallApproval(e.graph, p, call): + if actionApproved(call.Uses, call.Run.ApprovedActions) { + return nil } + return toolCallApprovalDenied(call, p) + } + if p != nil && approvalRequired(call.Uses, p.Approvals) { + return checkApprovalGranted(call.Uses, p.Approvals, call.Run.ApprovedActions) } return checkSafetyDerived(e.graph, call) } -func presetRequiresApproval(p *spec.PolicySpec, graph *spec.ProjectGraph, call ToolCallContext) bool { - if p == nil || p.Approvals == nil { +func requiresToolCallApproval(graph *spec.ProjectGraph, pol *spec.PolicySpec, call ToolCallContext) bool { + if pol == nil { return false } - if p.Approvals.RequireAllTools { - return true - } - if spec.ResolvedPresetName(p) == spec.PresetShellSafe { + 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 } + +func toolCallApprovalDenied(call ToolCallContext, pol *spec.PolicySpec) error { + extra := map[string]any{"requiredFor": call.Uses} + if pol != nil { + if preset := spec.ResolvedPresetName(pol); preset != "" { + extra["preset"] = preset + } + } + return denied( + ReasonApprovalRequired, + "policy: action requires explicit approval (--approve)", + call.Uses, + extra, + ) +} diff --git a/internal/policy/presets_eval_test.go b/internal/policy/presets_eval_test.go index d9572a0..3c81172 100644 --- a/internal/policy/presets_eval_test.go +++ b/internal/policy/presets_eval_test.go @@ -57,6 +57,38 @@ func TestCheckToolCall_shellSafe_gatesRm(t *testing.T) { } } +func TestCheckToolCall_shellSafe_gatesChainedCommand(t *testing.T) { + pol, err := spec.BuildPreset(spec.PresetShellSafe) + if err != nil { + t.Fatal(err) + } + ev := NewEvaluator(shellSafeGraph(), &pol) + err = ev.CheckToolCall(context.Background(), ToolCallContext{ + Uses: "tool.shell.run", + With: map[string]any{"command": "ls; rm -rf /"}, + }) + if err == nil { + t.Fatal("expected chained command to require approval") + } +} + +func TestCheckToolCall_shellSafe_approveGrantsRm(t *testing.T) { + pol, err := spec.BuildPreset(spec.PresetShellSafe) + if err != nil { + t.Fatal(err) + } + ev := NewEvaluator(shellSafeGraph(), &pol) + uses := "tool.shell.command.run" + err = ev.CheckToolCall(context.Background(), ToolCallContext{ + Run: RunContext{ApprovedActions: []string{uses}}, + Uses: uses, + With: map[string]any{"command": "rm -rf /tmp/x"}, + }) + if err != nil { + t.Fatalf("--approve should grant gated command: %v", err) + } +} + func TestCheckToolCall_shellSafe_unknownTokenGated(t *testing.T) { pol, err := spec.BuildPreset(spec.PresetShellSafe) if err != nil { @@ -64,7 +96,7 @@ func TestCheckToolCall_shellSafe_unknownTokenGated(t *testing.T) { } ev := NewEvaluator(shellSafeGraph(), &pol) err = ev.CheckToolCall(context.Background(), ToolCallContext{ - Uses: "tool.shell.command.run", + Uses: "tool.shell.exec", With: map[string]any{"command": "totally-unknown"}, }) if err == nil { @@ -72,6 +104,22 @@ func TestCheckToolCall_shellSafe_unknownTokenGated(t *testing.T) { } } +func TestCheckToolCall_shellSafe_nonShellSideEffectToolGated(t *testing.T) { + g := testGraphWithTools("slack") + g.Tools["slack"].Spec.Safety = &spec.ToolSafety{SideEffects: spec.BoolPtr(true)} + pol, err := spec.BuildPreset(spec.PresetShellSafe) + if err != nil { + t.Fatal(err) + } + ev := NewEvaluator(g, &pol) + err = ev.CheckToolCall(context.Background(), ToolCallContext{ + Uses: "tool.slack.message.send", + }) + if err == nil { + t.Fatal("side-effecting non-shell tool should gate under shell_safe") + } +} + func TestCheckToolCall_strict_gatesAllTools(t *testing.T) { g := testGraphWithTools("helper") g.Tools["helper"].Spec.Safety = &spec.ToolSafety{SideEffects: spec.BoolPtr(false)} @@ -104,6 +152,25 @@ func TestCheckToolCall_permissive_allowsMutatingTool(t *testing.T) { } } +func TestEngine_Evaluator_resolvesBuiltinShellSafeAfterNormalize(t *testing.T) { + g := &spec.ProjectGraph{ + Spec: spec.ProjectSpec{ + Defaults: &spec.ProjectDefaults{Policy: spec.PresetShellSafe}, + }, + Tools: shellSafeGraph().Tools, + } + spec.NormalizeProjectGraph(g) + eng := NewEngine(g) + ev := eng.Evaluator(spec.PresetShellSafe) + err := ev.CheckToolCall(context.Background(), ToolCallContext{ + Uses: "tool.shell.command.run", + With: map[string]any{"command": "ls"}, + }) + if err != nil { + t.Fatalf("builtin shell_safe after normalize should allow ls: %v", err) + } +} + func TestExpandPresetsInGraph_materializesDefault(t *testing.T) { g := &spec.ProjectGraph{ Spec: spec.ProjectSpec{ @@ -139,3 +206,15 @@ func TestExpandPresetsInGraph_userPolicyOverridesBuiltin(t *testing.T) { t.Fatal("user policy should not be replaced by builtin") } } + +func TestEffectiveToolDecision_shellSafe_toolGranularPlan(t *testing.T) { + g := shellSafeGraph() + pol, err := spec.BuildPreset(spec.PresetShellSafe) + if err != nil { + t.Fatal(err) + } + td := EffectiveToolDecision(g, &pol, "shell") + if td.Decision != DecisionRequireApproval { + t.Fatalf("plan should conservatively flag side-effecting shell tool: %+v", td) + } +} diff --git a/internal/policy/shell_safe.go b/internal/policy/shell_safe.go index 5764df5..16d9bce 100644 --- a/internal/policy/shell_safe.go +++ b/internal/policy/shell_safe.go @@ -12,18 +12,8 @@ func shellSafeRequiresApproval(graph *spec.ProjectGraph, call ToolCallContext) b return true } if spec.IsShellCommandOperation(operation) { - cmd := spec.ExtractShellCommand(call.With) - token := spec.FirstShellToken(cmd) - switch spec.ClassifyShellToken(token) { - case spec.ShellTokenReadOnly: - return false - case spec.ShellTokenGate, spec.ShellTokenUnknown: - return true - } + return spec.ShellCommandRequiresApproval(spec.ExtractShellCommand(call.With)) } safety := resolvedSafetyForTool(graph, toolName) - if safety.RequiresApproval || safety.SideEffects { - return true - } - return false + return safety.RequiresApproval || safety.SideEffects } diff --git a/internal/spec/kinds.go b/internal/spec/kinds.go index 30dec10..9c72461 100644 --- a/internal/spec/kinds.go +++ b/internal/spec/kinds.go @@ -182,10 +182,10 @@ type PolicyTools struct { type PolicyApprovals struct { RequiredFor []string `yaml:"requiredFor,omitempty" json:"requiredFor,omitempty"` - // RequireAllTools is set when the strict preset is expanded (every tool call requires approval). - RequireAllTools bool `yaml:"requireAllTools,omitempty" json:"requireAllTools,omitempty"` - // Permissive is set when the permissive preset is expanded (never gate tool calls). - Permissive bool `yaml:"permissive,omitempty" json:"permissive,omitempty"` + // RequireAllTools gates every tool call when true (strict preset). Pointer preserves tri-state merge. + RequireAllTools *bool `yaml:"requireAllTools,omitempty" json:"requireAllTools,omitempty"` + // Permissive skips tool-call approval when true (permissive preset). Pointer preserves tri-state merge. + Permissive *bool `yaml:"permissive,omitempty" json:"permissive,omitempty"` } type PolicySecurity struct { diff --git a/internal/spec/policy_expand.go b/internal/spec/policy_expand.go index f4d5cc2..159f98f 100644 --- a/internal/spec/policy_expand.go +++ b/internal/spec/policy_expand.go @@ -19,10 +19,7 @@ func ExpandPresetsInGraph(g *ProjectGraph) { if _, exists := g.Policies[name]; exists { continue } - preset, err := BuildPreset(name) - if err != nil { - continue - } + preset, _ := BuildPreset(name) g.Policies[name] = &PolicyResource{ APIVersion: APIVersionV0, Kind: KindPolicy, @@ -34,11 +31,9 @@ func ExpandPresetsInGraph(g *ProjectGraph) { if pr == nil { continue } - resolved, err := resolvePolicyResourcePreset(&pr.Spec) - if err != nil || resolved == nil { - continue + if resolved, err := resolvePolicyResourcePreset(&pr.Spec); err == nil && resolved != nil { + pr.Spec = *resolved } - pr.Spec = *resolved } } @@ -47,10 +42,7 @@ func resolvePolicyResourcePreset(pol *PolicySpec) (*PolicySpec, error) { return nil, nil } presetName := strings.TrimSpace(pol.Preset) - if presetName == "" { - if pol.ResolvedPreset != "" { - return nil, nil - } + if presetName == "" || pol.ResolvedPreset != "" { return nil, nil } if !IsBuiltinPreset(presetName) { @@ -81,9 +73,8 @@ func collectReferencedPolicyNames(g *ProjectGraph) []string { add(wr.Spec.Policy) } } - for name, pr := range g.Policies { + for name := range g.Policies { add(name) - _ = pr } out := make([]string, 0, len(seen)) for name := range seen { diff --git a/internal/spec/policy_presets.go b/internal/spec/policy_presets.go index 6a08b75..9ce2884 100644 --- a/internal/spec/policy_presets.go +++ b/internal/spec/policy_presets.go @@ -13,14 +13,6 @@ const ( PresetShellSafe = "shell_safe" ) -// shellCommandOperations are native tool operations that carry a shell command in step input. -var shellCommandOperations = []string{"command.run", "run", "exec", "shell"} - -var shellGateTokens = map[string]struct{}{ - "rm": {}, "mv": {}, "cp": {}, "chmod": {}, "chown": {}, "mkfifo": {}, "dd": {}, - "curl": {}, "wget": {}, "ssh": {}, "exec": {}, "eval": {}, "write": {}, "delete": {}, -} - // ErrUnknownPreset is returned when a policy references an unrecognized preset name. type ErrUnknownPreset struct { Name string @@ -63,7 +55,7 @@ func buildStrictPreset() PolicySpec { return PolicySpec{ ResolvedPreset: PresetStrict, Approvals: &PolicyApprovals{ - RequireAllTools: true, + RequireAllTools: BoolPtr(true), }, } } @@ -72,7 +64,7 @@ func buildPermissivePreset() PolicySpec { return PolicySpec{ ResolvedPreset: PresetPermissive, Approvals: &PolicyApprovals{ - Permissive: true, + Permissive: BoolPtr(true), }, } } @@ -80,23 +72,10 @@ func buildPermissivePreset() PolicySpec { func buildShellSafePreset() PolicySpec { return PolicySpec{ ResolvedPreset: PresetShellSafe, - Approvals: &PolicyApprovals{ - RequiredFor: shellSafeExpandedRequiredFor(), - }, + // Runtime gating uses ShellCommandRequiresApproval + safety metadata; no synthetic requiredFor. } } -func shellSafeExpandedRequiredFor() []string { - out := make([]string, 0, len(shellGateTokens)*len(shellCommandOperations)) - for _, op := range shellCommandOperations { - for token := range shellGateTokens { - out = append(out, fmt.Sprintf("preset:%s:%s:%s", PresetShellSafe, op, token)) - } - } - sort.Strings(out) - return out -} - // BuildPreset returns a fresh [PolicySpec] for a built-in preset name. func BuildPreset(name string) (PolicySpec, error) { name = strings.TrimSpace(name) @@ -153,27 +132,50 @@ func mergePolicyApprovals(base, overlay *PolicyApprovals) *PolicyApprovals { return clonePolicyApprovals(base) } out := &PolicyApprovals{ - RequireAllTools: overlay.RequireAllTools, - Permissive: overlay.Permissive, - } - if base != nil { - if !out.RequireAllTools { - out.RequireAllTools = base.RequireAllTools - } - if !out.Permissive { - out.Permissive = base.Permissive - } + RequireAllTools: mergeOptionalBool(optionalRequireAllTools(base), overlay.RequireAllTools), + Permissive: mergeOptionalBool(optionalPermissive(base), overlay.Permissive), } out.RequiredFor = mergePresetRequiredFor( presetRequiredForSlice(base), presetRequiredForSlice(overlay), ) - if out.RequiredFor == nil && !out.RequireAllTools && !out.Permissive { + if out.RequiredFor == nil && out.RequireAllTools == nil && out.Permissive == nil { return nil } return out } +func optionalRequireAllTools(a *PolicyApprovals) *bool { + if a == nil { + return nil + } + return a.RequireAllTools +} + +func optionalPermissive(a *PolicyApprovals) *bool { + if a == nil { + return nil + } + return a.Permissive +} + +func mergeOptionalBool(base, overlay *bool) *bool { + if overlay != nil { + return overlay + } + return base +} + +// ApprovalPermissive reports whether merged approvals enable permissive mode. +func ApprovalPermissive(a *PolicyApprovals) bool { + return a != nil && a.Permissive != nil && *a.Permissive +} + +// ApprovalRequireAllTools reports whether merged approvals gate every tool call. +func ApprovalRequireAllTools(a *PolicyApprovals) bool { + return a != nil && a.RequireAllTools != nil && *a.RequireAllTools +} + func presetRequiredForSlice(a *PolicyApprovals) []string { if a == nil { return nil @@ -274,6 +276,14 @@ func clonePolicyApprovals(in *PolicyApprovals) *PolicyApprovals { if in.RequiredFor != nil { cp.RequiredFor = append([]string(nil), in.RequiredFor...) } + if in.RequireAllTools != nil { + v := *in.RequireAllTools + cp.RequireAllTools = &v + } + if in.Permissive != nil { + v := *in.Permissive + cp.Permissive = &v + } return &cp } @@ -287,19 +297,3 @@ func ResolvedPresetName(pol *PolicySpec) string { } return strings.TrimSpace(pol.Preset) } - -// ShellCommandOperations returns native operations subject to shell_safe token classification. -func ShellCommandOperations() []string { - return append([]string(nil), shellCommandOperations...) -} - -// IsShellCommandOperation reports whether operation is a shell command carrier for shell_safe. -func IsShellCommandOperation(operation string) bool { - op := strings.ToLower(strings.TrimSpace(operation)) - for _, candidate := range shellCommandOperations { - if op == candidate { - return true - } - } - return false -} diff --git a/internal/spec/presets_test.go b/internal/spec/presets_test.go index 0aa3c99..4102ec4 100644 --- a/internal/spec/presets_test.go +++ b/internal/spec/presets_test.go @@ -59,7 +59,7 @@ func TestNormalizeProjectGraph_expandsPolicyPreset(t *testing.T) { if sp.ResolvedPreset != PresetStrict { t.Fatalf("ResolvedPreset = %q", sp.ResolvedPreset) } - if sp.Approvals == nil || !sp.Approvals.RequireAllTools { + if sp.Approvals == nil || !ApprovalRequireAllTools(sp.Approvals) { t.Fatalf("strict expansion: %+v", sp.Approvals) } if sp.Execution == nil || sp.Execution.MaxWallClockSeconds != 120 { diff --git a/internal/spec/shell_tokens.go b/internal/spec/shell_tokens.go index 013d610..094f155 100644 --- a/internal/spec/shell_tokens.go +++ b/internal/spec/shell_tokens.go @@ -2,11 +2,19 @@ package spec import "strings" +// Shell command operation names for native tools (single source of truth, issue #104). +var ShellCommandOperations = []string{"command.run", "run", "exec", "shell"} + var shellReadOnlyTokens = map[string]struct{}{ "ls": {}, "cat": {}, "grep": {}, "head": {}, "tail": {}, "stat": {}, "find": {}, "pwd": {}, "wc": {}, "which": {}, } +var shellGateTokens = map[string]struct{}{ + "rm": {}, "mv": {}, "cp": {}, "chmod": {}, "chown": {}, "mkfifo": {}, "dd": {}, + "curl": {}, "wget": {}, "ssh": {}, "exec": {}, "eval": {}, "write": {}, "delete": {}, +} + // ShellTokenClass classifies the first token of a shell command for shell_safe policy. type ShellTokenClass int @@ -16,6 +24,39 @@ const ( ShellTokenGate ) +// ShellCommandRequiresApproval reports whether a command string must be gated under shell_safe. +// +// This is a first-token heuristic with metacharacter fail-closed checks — not a sandbox. +// Commands containing shell composition syntax (;|&$`, newlines, $(…)) always require approval. +func ShellCommandRequiresApproval(command string) bool { + command = strings.TrimSpace(command) + if command == "" { + return true + } + if containsShellMetacharacters(command) { + return true + } + switch ClassifyShellToken(FirstShellToken(command)) { + case ShellTokenReadOnly: + return false + default: + return true + } +} + +func containsShellMetacharacters(command string) bool { + if strings.Contains(command, "$(") { + return true + } + for _, r := range command { + switch r { + case ';', '|', '&', '\n', '\r', '`', '$': + return true + } + } + return false +} + // ClassifyShellToken maps the first command token to read-only, gate, or unknown (fail-closed → gate). func ClassifyShellToken(token string) ShellTokenClass { token = normalizeShellToken(token) @@ -73,3 +114,14 @@ func ExtractShellCommand(with map[string]any) string { } return "" } + +// IsShellCommandOperation reports whether operation is a shell command carrier for shell_safe. +func IsShellCommandOperation(operation string) bool { + op := strings.ToLower(strings.TrimSpace(operation)) + for _, candidate := range ShellCommandOperations { + if op == candidate { + return true + } + } + return false +} diff --git a/internal/spec/shell_tokens_test.go b/internal/spec/shell_tokens_test.go index 397c7b1..cc83064 100644 --- a/internal/spec/shell_tokens_test.go +++ b/internal/spec/shell_tokens_test.go @@ -50,6 +50,26 @@ func TestFirstShellToken_adversarial(t *testing.T) { } } +func TestShellCommandRequiresApproval_metacharactersFailClosed(t *testing.T) { + adversarial := []string{ + "ls; rm -rf /", + "ls | rm -rf /", + "ls && rm -rf /", + "$(rm -rf /)", + "`rm -rf /`", + "ls\nrm -rf /", + "echo $(whoami)", + } + for _, cmd := range adversarial { + if !ShellCommandRequiresApproval(cmd) { + t.Fatalf("expected gate for %q", cmd) + } + } + if ShellCommandRequiresApproval("ls -la") { + t.Fatal("plain ls should not require approval") + } +} + func TestBuildPreset_unknown(t *testing.T) { _, err := BuildPreset("nope") if err == nil { @@ -86,12 +106,28 @@ func TestMergePolicySpec_localRequiredForOverridesPresetTool(t *testing.T) { } } +func TestMergePolicySpec_overlayRelaxesStrict(t *testing.T) { + base, err := BuildPreset(PresetStrict) + if err != nil { + t.Fatal(err) + } + overlay := PolicySpec{ + Approvals: &PolicyApprovals{ + RequireAllTools: BoolPtr(false), + }, + } + merged := MergePolicySpec(base, overlay) + if ApprovalRequireAllTools(merged.Approvals) { + t.Fatal("overlay requireAllTools: false should relax strict preset") + } +} + func TestBuildPreset_strict_expanded(t *testing.T) { p, err := BuildPreset(PresetStrict) if err != nil { t.Fatal(err) } - if p.Approvals == nil || !p.Approvals.RequireAllTools { + if p.Approvals == nil || !ApprovalRequireAllTools(p.Approvals) { t.Fatalf("strict preset: %+v", p.Approvals) } } @@ -101,12 +137,12 @@ func TestBuildPreset_permissive_expanded(t *testing.T) { if err != nil { t.Fatal(err) } - if p.Approvals == nil || !p.Approvals.Permissive { + if p.Approvals == nil || !ApprovalPermissive(p.Approvals) { t.Fatalf("permissive preset: %+v", p.Approvals) } } -func TestBuildPreset_shellSafe_hasGatePatterns(t *testing.T) { +func TestBuildPreset_shellSafe_noSyntheticRequiredFor(t *testing.T) { p, err := BuildPreset(PresetShellSafe) if err != nil { t.Fatal(err) @@ -114,7 +150,27 @@ func TestBuildPreset_shellSafe_hasGatePatterns(t *testing.T) { if p.ResolvedPreset != PresetShellSafe { t.Fatalf("ResolvedPreset = %q", p.ResolvedPreset) } - if p.Approvals == nil || len(p.Approvals.RequiredFor) == 0 { - t.Fatal("expected expanded gate patterns") + if p.Approvals != nil && len(p.Approvals.RequiredFor) > 0 { + t.Fatalf("shell_safe should not expand synthetic requiredFor: %v", p.Approvals.RequiredFor) + } +} + +func TestValidatePolicySpecs_conflictingApprovalFlags(t *testing.T) { + g := &ProjectGraph{ + Policies: map[string]*PolicyResource{ + "bad": { + Metadata: Metadata{Name: "bad"}, + Spec: PolicySpec{ + Approvals: &PolicyApprovals{ + RequireAllTools: BoolPtr(true), + Permissive: BoolPtr(true), + }, + }, + }, + }, + } + errs := validatePolicySpecs(g) + if len(errs) == 0 { + t.Fatal("expected validation error") } } diff --git a/internal/spec/validator.go b/internal/spec/validator.go index 83b9bcd..c794140 100644 --- a/internal/spec/validator.go +++ b/internal/spec/validator.go @@ -237,6 +237,9 @@ func validatePolicySpecs(g *ProjectGraph) []error { } } if ap := pr.Spec.Approvals; ap != nil { + if ApprovalRequireAllTools(ap) && ApprovalPermissive(ap) { + errs = append(errs, fmt.Errorf("Policy/%s: approvals.requireAllTools and approvals.permissive are mutually exclusive", name)) + } seen := make(map[string]struct{}) for i, act := range ap.RequiredFor { a := strings.TrimSpace(act) diff --git a/internal/tools/native/registry.go b/internal/tools/native/registry.go index a15f5ef..9714e41 100644 --- a/internal/tools/native/registry.go +++ b/internal/tools/native/registry.go @@ -7,6 +7,8 @@ import ( "fmt" "strings" "time" + + "github.com/LAA-Software-Engineering/agentic-control-plane/internal/spec" ) // ErrUnknownOperation indicates the operation name is not implemented by this registry. @@ -30,6 +32,14 @@ func NewRegistry() *Registry { func (r *Registry) Dispatch(ctx context.Context, operation string, with map[string]any) (map[string]any, ExecMeta, error) { start := time.Now() meta := ExecMeta{CostUSD: 0} + if spec.IsShellCommandOperation(operation) { + meta.DurationMs = time.Since(start).Milliseconds() + cmd := spec.ExtractShellCommand(with) + if cmd == "" { + return nil, meta, fmt.Errorf("native: %s requires string field command, cmd, or script", operation) + } + return map[string]any{"command": cmd}, meta, nil + } switch operation { case "echo": meta.DurationMs = time.Since(start).Milliseconds() @@ -38,13 +48,6 @@ func (r *Registry) Dispatch(ctx context.Context, operation string, with map[stri v, ok := with["value"] meta.DurationMs = time.Since(start).Milliseconds() return map[string]any{"value": v, "ok": ok}, meta, nil - case "command.run", "run", "exec", "shell": - meta.DurationMs = time.Since(start).Milliseconds() - cmd := shellCommandFromWith(with) - if cmd == "" { - return nil, meta, fmt.Errorf("native: %s requires string field command, cmd, or script", operation) - } - return map[string]any{"command": cmd}, meta, nil case "pull_request.fetch": // Offline demo: parse JSON from `pr` (interpolated from workflow input). No network. meta.DurationMs = time.Since(start).Milliseconds() @@ -134,17 +137,3 @@ func shallowCopy(m map[string]any) map[string]any { } return out } - -func shellCommandFromWith(with map[string]any) string { - if with == nil { - return "" - } - for _, key := range []string{"command", "cmd", "script"} { - if v, ok := with[key]; ok { - if s, ok := v.(string); ok { - return strings.TrimSpace(s) - } - } - } - return "" -} From 46b44675ff0fe8bce49f41325fad666afecb5a62 Mon Sep 17 00:00:00 2001 From: Leonardo Araujo Date: Tue, 2 Jun 2026 01:04:27 -0300 Subject: [PATCH 3/3] 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 --- CHANGELOG.md | 2 +- internal/cli/init_test.go | 6 +++-- internal/policy/derive.go | 21 ++++++++-------- internal/policy/evaluator.go | 16 ++++--------- internal/policy/presets_eval_test.go | 36 ++++++++++++++++++++++++++++ 5 files changed, 55 insertions(+), 26 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 114cba5..c860bb4 100644 --- a/CHANGELOG.md +++ b/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`). diff --git a/internal/cli/init_test.go b/internal/cli/init_test.go index 387e2f3..8ef341c 100644 --- a/internal/cli/init_test.go +++ b/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) } } diff --git a/internal/policy/derive.go b/internal/policy/derive.go index c264f6d..4bc9bad 100644 --- a/internal/policy/derive.go +++ b/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) diff --git a/internal/policy/evaluator.go b/internal/policy/evaluator.go index d03a189..2d69b3a 100644 --- a/internal/policy/evaluator.go +++ b/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 { diff --git a/internal/policy/presets_eval_test.go b/internal/policy/presets_eval_test.go index 3c81172..a3c50c5 100644 --- a/internal/policy/presets_eval_test.go +++ b/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)