Merge pull request #124 from LAA-Software-Engineering/feat/tool-safety-metadata-103

feat(spec,policy): tool safety metadata + fail-closed policy derivation

Author: leo-aa88

CHANGELOG.md

@@ -0,0 +1,33 @@
+# Changelog
+
+All notable changes to this project are documented in this file.
+
+The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.1.0/).
+
+## [Unreleased]
+
+### Added
+
+- **`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`).
+
+### Changed
+
+- **Breaking — tool calls without explicit policy are no longer unrestricted.** Previously, `CheckToolCall` with a nil [spec.PolicySpec] allowed all tools. Now fail-closed safety always applies from the project graph (even when the workflow omits `spec.policy` or the Policy resource is missing).
+- Tools with **no** `spec.safety` block behave as **untrusted with side effects** after normalization → require `--approve` unless an explicit `approvals.requiredFor` rule matches.
+
+### Migration
+
+1. For **read-only** native or mock tools (echo, fetch, identity), add:
+   ```yaml
+   spec:
+     safety:
+       sideEffects: false
+   ```
+2. For tools where you accept **tool-wide** unattended use but still gate specific operations, set `trusted: true` and list write operations under `Policy.spec.approvals.requiredFor` (exact `uses` strings).
+3. Do **not** set `trusted: true` unless you intend every operation on that tool to run without safety-derived approval; per-action gating remains `requiredFor` only (exact match at runtime).
+
+### Not yet wired
+
+- MCP discovery does **not** yet apply [spec.SafetyFromMCPMeta] / [spec.MergeToolSafety]; author-set `spec.safety` in YAML is the source of truth until MCP merge lands (tracked separately from #103).

CONTRIBUTING.md

@@ -22,6 +22,8 @@ Run **`make`** or **`make help`** for a full list of targets.
 
 ## Before you open a pull request
 
+User-visible behavior changes should include an entry under **[Unreleased]** in [`CHANGELOG.md`](CHANGELOG.md) (especially breaking changes and migrations).
+
 1. **Format** — `make fmt` or ensure `gofmt -l .` prints nothing (same check as CI).
 2. **Static analysis** — `make vet` (or `go vet ./...`).
 3. **Tests** — `make test` (`go test ./... -race`).

examples/example1/tools/helper.yaml

@@ -4,3 +4,5 @@ metadata:
   name: helper
 spec:
   type: native
+  safety:
+    sideEffects: false

examples/pr-review-demo/tools/github.yaml

@@ -4,3 +4,5 @@ metadata:
   name: github
 spec:
   type: native
+  safety:
+    trusted: true

examples/pr-review-github-actions/tools/github.yaml

@@ -4,3 +4,5 @@ metadata:
   name: github
 spec:
   type: native
+  safety:
+    trusted: true

examples/pr-review-github/tools/github.yaml

@@ -4,3 +4,5 @@ metadata:
   name: github
 spec:
   type: native
+  safety:
+    trusted: true

internal/cli/initembed/tools/helper.yaml

@@ -4,3 +4,5 @@ metadata:
   name: helper
 spec:
   type: native
+  safety:
+    sideEffects: false

internal/cli/run_test.go

@@ -29,6 +29,16 @@ func runPolicyRoot(t *testing.T) string {
 	return abs
 }
 
+func runSafetyRoot(t *testing.T) string {
+	t.Helper()
+	p := filepath.Join("testdata", "run_safety")
+	abs, err := filepath.Abs(p)
+	if err != nil {
+		t.Fatal(err)
+	}
+	return abs
+}
+
 func TestRun_demo_integration_succeeds(t *testing.T) {
 	db := filepath.Join(t.TempDir(), "run-cli.db")
 	root := runProjRoot(t)
@@ -54,6 +64,53 @@ func TestRun_demo_integration_succeeds(t *testing.T) {
 	}
 }
 
+func TestRun_safetyOnlyDenial_exit5(t *testing.T) {
+	db := filepath.Join(t.TempDir(), "run-safety.db")
+	root := runSafetyRoot(t)
+
+	ResetGlobalsForTest()
+	cmd := NewRootCmd()
+	cmd.SetOut(io.Discard)
+	cmd.SetErr(io.Discard)
+	cmd.SetArgs([]string{
+		"run", "workflow/echo",
+		"--project", root,
+		"--state", db,
+		"--input", "topic=x",
+	})
+	err := cmd.Execute()
+	if err == nil {
+		t.Fatal("expected safety-derived policy denial")
+	}
+	if ExitCodeOf(err) != ExitPolicyDenied {
+		t.Fatalf("exit=%d want %d err=%v", ExitCodeOf(err), ExitPolicyDenied, err)
+	}
+}
+
+func TestRun_safetyOnly_withApprove_succeeds(t *testing.T) {
+	db := filepath.Join(t.TempDir(), "run-safety-ok.db")
+	root := runSafetyRoot(t)
+
+	ResetGlobalsForTest()
+	var out bytes.Buffer
+	cmd := NewRootCmd()
+	cmd.SetOut(&out)
+	cmd.SetErr(&out)
+	cmd.SetArgs([]string{
+		"run", "workflow/echo",
+		"--project", root,
+		"--state", db,
+		"--input", "topic=x",
+		"--approve", "tool.helper.echo",
+	})
+	if err := cmd.Execute(); err != nil {
+		t.Fatal(err)
+	}
+	if !strings.Contains(out.String(), "Status: succeeded") {
+		t.Fatalf("output:\n%s", out.String())
+	}
+}
+
 func TestRun_policyDenial_exit5(t *testing.T) {
 	db := filepath.Join(t.TempDir(), "run-pol.db")
 	root := runPolicyRoot(t)

internal/cli/testdata/fmt_messy/tool.yaml

@@ -4,3 +4,5 @@ metadata:
   name: helper
 spec:
   type: native
+  safety:
+    sideEffects: false

internal/cli/testdata/plan_project/tool.yaml

@@ -4,3 +4,5 @@ metadata:
   name: helper
 spec:
   type: native
+  safety:
+    sideEffects: false