SPDD 2026-07-05: outcome evaluators, policy compiler, and spec coverage for batch 20-24

docs/adr/43594-policy-compiler-more-restrictive-wins-merge.md

@@ -0,0 +1,53 @@
+# ADR-43594: Policy Compiler with More-Restrictive-Wins Merge Semantics
+
+**Date**: 2026-07-05
+**Status**: Draft
+**Deciders**: Unknown
+
+---
+
+### Context
+
+The gh-aw agent orchestrator needs a runtime authorization layer that controls what actions an agent may take for a given intent (e.g., which tools it may call, whether it may auto-merge, how many attempts it may make). Before this PR, the `Authorizer.AuthorizeTool` function was unimplemented — any policy defined in `.github/intent-policy.json` was purely advisory with no runtime effect.
+
+The governance spec (`specs/intent-attribution-agent-governance.md`) requires that policy precedence follows a strict hierarchy: organization constraints > repository constraints > intent-specific rules > workflow defaults > agent request. The key safety constraint is fail-closed behavior: when no rules match (or intent is ambiguous), the system MUST apply the most restrictive policy possible rather than granting elevated authority.
+
+A multi-rule policy system needs a merge strategy that determines which value wins when rules at different precedence levels specify conflicting constraints. The wrong strategy could allow a lower-precedence rule to silently weaken a security constraint established by a higher-precedence rule.
+
+### Decision
+
+We will implement a `PolicyCompiler` in `pkg/intent/policy.go` that compiles an ordered list of `PolicyRule` entries into a single `ExecutionPolicy` using **more-restrictive-wins** merge semantics. Rules are processed in declaration order (highest to lowest precedence). For each field, the merge function always keeps the value that represents the stricter constraint: `DeniedTools` and `RequiredChecks` use union, `AllowedTools` uses intersection when both sides constrain, `HumanApprovalRequired` uses OR, `AutoMergeAllowed` uses AND, and `MaxAttempts` uses min. The baseline is always `safestDefaultPolicy()` — a fail-closed policy that denies all write operations and requires human approval.
+
+### Alternatives Considered
+
+#### Alternative 1: Last-Write-Wins (Declaration Order Override)
+
+Each rule applies sequentially, with later rules fully overriding earlier rules' values. This is the simplest implementation and matches how many configuration systems work (e.g., environment variable layering).
+
+This was rejected because it allows lower-precedence rules (e.g., intent-specific) to silently weaken constraints established by higher-precedence rules (e.g., organizational). An org rule setting `human_approval_required: true` could be cleared by a downstream repo rule setting it `false`. This violates the spec's requirement that higher-precedence rules must not be weakened.
+
+#### Alternative 2: Single Flat Policy (No Rule Precedence)
+
+Eliminate rule scopes and precedence entirely. Maintain a single global `ExecutionPolicy` per repository, loaded from a config file. Intent-specific behavior is encoded as label-to-policy mappings in a flat lookup table, not as composable rules.
+
+This was rejected because it cannot express the layered governance model required by the spec (org-wide defaults + repo overrides + intent-specific adjustments). A flat model would require duplicating the most restrictive constraints in every policy entry, making the config error-prone and difficult to audit.
+
+### Consequences
+
+#### Positive
+- Fail-closed by default: when no rules match, `safestDefaultPolicy()` produces `autonomy=propose_only`, `write_scope=none`, `human_approval_required=true`, `auto_merge_allowed=false`, `max_attempts=1`.
+- Higher-precedence org/repo constraints cannot be weakened by lower-precedence intent rules; the merge invariant is enforced at the `mergePolicy()` level, making it impossible to bypass by rule ordering.
+- `RuleIDs` field records which rules contributed to the compiled policy, enabling auditability of each policy decision.
+- 3 unit tests covering org → repo → intent precedence are included and passing.
+
+#### Negative
+- The compiled `ExecutionPolicy` is currently **not wired to runtime enforcement**: no orchestrator path calls `AuthorizeTool` or checks `WriteScope`/`HumanApprovalRequired`/`RequiredChecks` at execution time. Policy today is advisory only. All fields in `ExecutionPolicy` are documented as "Not wired" in the spec's implementation audit table.
+- `PolicyCondition.Matches` checks label values as flat strings across all dimensions (domain, priority, risk). Callers must ensure label values are unique across dimensions to avoid false positives (e.g., a domain value that collides with a priority value would cause incorrect rule matches).
+
+#### Neutral
+- The `PolicyCompiler` lives in `pkg/intent/policy.go` alongside the existing attribution types; it does not yet have its own sub-package. A follow-up will move authorization logic to `pkg/intent/authz` when `AuthorizeTool` is implemented.
+- The merge semantics for `AllowedTools` are asymmetric: if neither side specifies tools, the result is unrestricted; if only one side specifies tools, that restriction is adopted; if both sides restrict, the intersection is used. This means adding `AllowedTools` to a rule makes it harder to relax at lower precedence levels.
+
+---
+
+*ADR created by [adr-writer agent]. Review and finalize before changing status from Draft to Accepted.*

pkg/cli/outcome_eval.go

@@ -105,6 +105,8 @@ var outcomeEvaluators = map[string]outcomeEvaluator{
 	"push_to_pull_request_branch":           evalPushToPRBranch,
 	"add_reviewer":                          evalAddReviewer,
 	"submit_pull_request_review":            evalSubmitPullRequestReview,
+	"dispatch_workflow":                     evalDispatchWorkflow,
+	"update_discussion":                     evalUpdateDiscussion,
 }
 
 // EvaluateOutcomes checks the current state of all safe output items from a run.

pkg/cli/outcome_eval_workflow.go

@@ -0,0 +1,104 @@
+package cli
+
+import (
+	"fmt"
+	"math"
+
+	"github.com/github/gh-aw/pkg/logger"
+)
+
+// maxSafeFloat64Int is the largest integer that can be represented exactly as
+// float64 (2^53). GitHub run IDs decoded from JSON arrive as float64; any value
+// beyond this threshold cannot be round-tripped to int64 without precision loss.
+const maxSafeFloat64Int = 1 << 53
+
+var outcomeEvalWorkflowLog = logger.New("cli:outcome_eval_workflow")
+var workflowOutcomeGHAPIGet = ghAPIGet
+
+// evalDispatchWorkflow checks whether a dispatched workflow run completed successfully.
+// It looks for a run_id in the item metadata and queries the workflow run status.
+// Spec: specs/safe-output-outcome-evaluation.md §20
+func evalDispatchWorkflow(item CreatedItemReport, repoOverride string) OutcomeReport {
+	repo := resolveItemRepo(item, repoOverride)
+	outcomeEvalWorkflowLog.Printf("Evaluating dispatch_workflow: repo=%s, url=%s", repo, item.URL)
+
+	report := OutcomeReport{
+		Type:      item.Type,
+		ObjectURL: item.URL,
+		Repo:      repo,
+	}
+
+	// Extract run_id from metadata if available.
+	// JSON numbers unmarshal as float64; convert carefully to int64 to avoid
+	// precision loss for large GitHub run IDs (which can exceed 2^32).
+	var runID int64
+	if item.Metadata != nil {
+		if v, ok := item.Metadata["run_id"]; ok {
+			switch id := v.(type) {
+			case float64:
+				// Guard against float64 values that cannot round-trip to int64.
+				// math.MaxInt64 is not representable exactly as float64; use 2^53
+				// (maxSafeFloat64Int) as the safe upper bound for lossless conversion.
+				if id > 0 && id <= maxSafeFloat64Int && id == math.Trunc(id) {
+					runID = int64(id)
+				}
+			case int:
+				runID = int64(id)
+			case int64:
+				runID = id
+			}
+		}
+	}
+
+	if runID <= 0 {
+		// No run ID available — workflow may not have been dispatched or ID not captured
+		report.Result = OutcomePending
+		report.Detail = "no run ID available; dispatch may still be queued"
+		return report
+	}
+
+	data, err := workflowOutcomeGHAPIGet(fmt.Sprintf("actions/runs/%d", runID), repo)
+	if err != nil {
+		report.Result = OutcomeError
+		report.EvalError = err.Error()
+		return report
+	}
+
+	status, _ := data["status"].(string)
+	conclusion, _ := data["conclusion"].(string)
+	outcomeEvalWorkflowLog.Printf("dispatch_workflow run %d: status=%s, conclusion=%s", runID, status, conclusion)
+
+	switch {
+	case status == "completed" && conclusion == "success":
+		report.Result = OutcomeAccepted
+		report.Detail = "workflow run completed with success"
+	case status == "completed" && (conclusion == "failure" || conclusion == "timed_out" || conclusion == "cancelled" || conclusion == "action_required"):
+		// action_required means the run is blocked and requires manual intervention;
+		// treat it as rejected rather than ignored since it does not self-resolve.
+		report.Result = OutcomeRejected
+		report.Detail = "workflow run completed with " + conclusion
+	case status == "completed":
+		// neutral and skipped indicate the run did not contribute meaningful output.
+		report.Result = OutcomeIgnored
+		report.Detail = "workflow run completed with " + conclusion
+	default:
+		report.Result = OutcomePending
+		report.Detail = "workflow run status: " + status
+	}
+	return report
+}
+
+// evalUpdateDiscussion checks whether a discussion edit stuck.
+// Full evaluation requires GraphQL (same pattern as evalCloseDiscussion).
+// Until the GraphQL evaluator is implemented this returns OutcomeIgnored so that
+// callers do not retry indefinitely.
+// Spec: specs/safe-output-outcome-evaluation.md §12
+func evalUpdateDiscussion(item CreatedItemReport, repoOverride string) OutcomeReport {
+	return OutcomeReport{
+		Type:      item.Type,
+		ObjectURL: item.URL,
+		Repo:      resolveItemRepo(item, repoOverride),
+		Result:    OutcomeIgnored,
+		Detail:    "discussion update check requires GraphQL (not yet implemented); outcome is advisory only",
+	}
+}

pkg/cli/outcome_eval_workflow_test.go

@@ -0,0 +1,206 @@
+//go:build !integration
+
+package cli
+
+import (
+	"errors"
+	"testing"
+
+	"github.com/stretchr/testify/assert"
+)
+
+func TestEvalDispatchWorkflowNoRunID(t *testing.T) {
+	// No metadata at all → pending with an informative detail message.
+	report := evalDispatchWorkflow(CreatedItemReport{
+		Type: "dispatch_workflow",
+		Repo: "owner/repo",
+	}, "owner/repo")
+
+	assert.Equal(t, OutcomePending, report.Result)
+	assert.Contains(t, report.Detail, "no run ID available")
+}
+
+func TestEvalDispatchWorkflowRunIDZero(t *testing.T) {
+	// run_id present but zero → treated as missing.
+	report := evalDispatchWorkflow(CreatedItemReport{
+		Type:     "dispatch_workflow",
+		Repo:     "owner/repo",
+		Metadata: map[string]any{"run_id": float64(0)},
+	}, "owner/repo")
+
+	assert.Equal(t, OutcomePending, report.Result)
+}
+
+func TestEvalDispatchWorkflowAPIError(t *testing.T) {
+	old := workflowOutcomeGHAPIGet
+	t.Cleanup(func() { workflowOutcomeGHAPIGet = old })
+	workflowOutcomeGHAPIGet = func(_ string, _ string) (map[string]any, error) {
+		return nil, errors.New("connection refused")
+	}
+
+	report := evalDispatchWorkflow(CreatedItemReport{
+		Type:     "dispatch_workflow",
+		Repo:     "owner/repo",
+		Metadata: map[string]any{"run_id": float64(12345678)},
+	}, "owner/repo")
+
+	assert.Equal(t, OutcomeError, report.Result)
+	assert.Contains(t, report.EvalError, "connection refused")
+}
+
+func TestEvalDispatchWorkflowCompletedSuccess(t *testing.T) {
+	old := workflowOutcomeGHAPIGet
+	t.Cleanup(func() { workflowOutcomeGHAPIGet = old })
+	workflowOutcomeGHAPIGet = func(_ string, _ string) (map[string]any, error) {
+		return map[string]any{
+			"status":     "completed",
+			"conclusion": "success",
+		}, nil
+	}
+
+	report := evalDispatchWorkflow(CreatedItemReport{
+		Type:     "dispatch_workflow",
+		Repo:     "owner/repo",
+		Metadata: map[string]any{"run_id": float64(12345678)},
+	}, "owner/repo")
+
+	assert.Equal(t, OutcomeAccepted, report.Result)
+	assert.Contains(t, report.Detail, "success")
+}
+
+func TestEvalDispatchWorkflowCompletedFailure(t *testing.T) {
+	for _, conclusion := range []string{"failure", "timed_out", "cancelled"} {
+		t.Run(conclusion, func(t *testing.T) {
+			old := workflowOutcomeGHAPIGet
+			t.Cleanup(func() { workflowOutcomeGHAPIGet = old })
+			workflowOutcomeGHAPIGet = func(_ string, _ string) (map[string]any, error) {
+				return map[string]any{
+					"status":     "completed",
+					"conclusion": conclusion,
+				}, nil
+			}
+
+			report := evalDispatchWorkflow(CreatedItemReport{
+				Type:     "dispatch_workflow",
+				Repo:     "owner/repo",
+				Metadata: map[string]any{"run_id": float64(99999)},
+			}, "owner/repo")
+
+			assert.Equal(t, OutcomeRejected, report.Result)
+			assert.Contains(t, report.Detail, conclusion)
+		})
+	}
+}
+
+func TestEvalDispatchWorkflowCompletedOtherConclusion(t *testing.T) {
+	old := workflowOutcomeGHAPIGet
+	t.Cleanup(func() { workflowOutcomeGHAPIGet = old })
+	workflowOutcomeGHAPIGet = func(_ string, _ string) (map[string]any, error) {
+		return map[string]any{
+			"status":     "completed",
+			"conclusion": "skipped",
+		}, nil
+	}
+
+	report := evalDispatchWorkflow(CreatedItemReport{
+		Type:     "dispatch_workflow",
+		Repo:     "owner/repo",
+		Metadata: map[string]any{"run_id": float64(42)},
+	}, "owner/repo")
+
+	assert.Equal(t, OutcomeIgnored, report.Result)
+	assert.Contains(t, report.Detail, "skipped")
+}
+
+func TestEvalDispatchWorkflowInProgress(t *testing.T) {
+	old := workflowOutcomeGHAPIGet
+	t.Cleanup(func() { workflowOutcomeGHAPIGet = old })
+	workflowOutcomeGHAPIGet = func(_ string, _ string) (map[string]any, error) {
+		return map[string]any{
+			"status":     "in_progress",
+			"conclusion": "",
+		}, nil
+	}
+
+	report := evalDispatchWorkflow(CreatedItemReport{
+		Type:     "dispatch_workflow",
+		Repo:     "owner/repo",
+		Metadata: map[string]any{"run_id": float64(77)},
+	}, "owner/repo")
+
+	assert.Equal(t, OutcomePending, report.Result)
+	assert.Contains(t, report.Detail, "in_progress")
+}
+
+func TestEvalDispatchWorkflowRunIDInt64(t *testing.T) {
+	// run_id supplied as int64 (not float64) must be handled without panic.
+	old := workflowOutcomeGHAPIGet
+	t.Cleanup(func() { workflowOutcomeGHAPIGet = old })
+	workflowOutcomeGHAPIGet = func(_ string, _ string) (map[string]any, error) {
+		return map[string]any{
+			"status":     "completed",
+			"conclusion": "success",
+		}, nil
+	}
+
+	report := evalDispatchWorkflow(CreatedItemReport{
+		Type:     "dispatch_workflow",
+		Repo:     "owner/repo",
+		Metadata: map[string]any{"run_id": int64(9876543210)},
+	}, "owner/repo")
+
+	assert.Equal(t, OutcomeAccepted, report.Result)
+}
+
+func TestEvalDispatchWorkflowFloat64OverflowGuard(t *testing.T) {
+	// A float64 value above 2^53 cannot represent integers exactly and must be
+	// treated as an invalid run_id (OutcomePending) rather than silently truncated.
+	// Use 2^53 + 2 (= 9007199254740994): consecutive integers around 2^53 collapse
+	// to the same float64, so this value would be mangled if cast to int64 directly.
+	aboveMaxSafeInt := float64(maxSafeFloat64Int) + 2
+
+	report := evalDispatchWorkflow(CreatedItemReport{
+		Type:     "dispatch_workflow",
+		Repo:     "owner/repo",
+		Metadata: map[string]any{"run_id": aboveMaxSafeInt},
+	}, "owner/repo")
+
+	assert.Equal(t, OutcomePending, report.Result,
+		"a float64 run_id above 2^53 must be treated as invalid (OutcomePending)")
+}
+
+func TestEvalDispatchWorkflowActionRequired(t *testing.T) {
+	// action_required is a blocking conclusion that requires manual intervention;
+	// it must map to OutcomeRejected (not OutcomeIgnored).
+	old := workflowOutcomeGHAPIGet
+	t.Cleanup(func() { workflowOutcomeGHAPIGet = old })
+	workflowOutcomeGHAPIGet = func(_ string, _ string) (map[string]any, error) {
+		return map[string]any{
+			"status":     "completed",
+			"conclusion": "action_required",
+		}, nil
+	}
+
+	report := evalDispatchWorkflow(CreatedItemReport{
+		Type:     "dispatch_workflow",
+		Repo:     "owner/repo",
+		Metadata: map[string]any{"run_id": float64(12345678)},
+	}, "owner/repo")
+
+	assert.Equal(t, OutcomeRejected, report.Result,
+		"action_required conclusion must map to OutcomeRejected")
+	assert.Contains(t, report.Detail, "action_required")
+}
+
+func TestEvalUpdateDiscussionReturnsIgnored(t *testing.T) {
+	// evalUpdateDiscussion must return OutcomeIgnored (not OutcomePending) so that
+	// callers do not enter an infinite retry loop waiting for a terminal status.
+	report := evalUpdateDiscussion(CreatedItemReport{
+		Type: "update_discussion",
+		URL:  "https://github.com/owner/repo/discussions/1",
+	}, "owner/repo")
+
+	assert.Equal(t, OutcomeIgnored, report.Result,
+		"evalUpdateDiscussion must return OutcomeIgnored to prevent infinite retry")
+	assert.NotEmpty(t, report.Detail)
+}