Add formal test suite for Permission Management gap analysis (T-PM-003, T-PM-005, T-PM-007)

docs/adr/52325-white-box-testing-for-pm-spec-compliance-predicates.md

@@ -0,0 +1,44 @@
+# ADR-52325: White-Box Testing for Permission Management Spec-Compliance Predicates
+
+**Date**: 2026-08-12
+**Status**: Draft
+**Deciders**: pelikhan, copilot-swe-agent
+
+---
+
+### Context
+
+`specs/security-architecture-spec-validation.md` §12 Compliance Test Matrix Gap Analysis flagged three Permission Management test cases — T-PM-003 (strict mode gating), T-PM-005 (`workflow_run` repository validation), and T-PM-007 (token precedence) — as lacking dedicated test coverage. The implementation functions that encode these behaviors (`hasWorkflowRunTrigger`, `buildWorkflowRunRepoSafetyCondition`, `validateWorkflowRunBranches`, `getEffectiveGitHubToken`, `getEffectiveSafeOutputGitHubToken`) are intentionally unexported internals within `pkg/workflow`. Closing the spec coverage gap requires a test file that can call these unexported functions directly. The same white-box pattern is already used by existing `security_architecture_*_formal_test.go` files in this package.
+
+### Decision
+
+We will declare the formal spec-compliance test file (`security_architecture_pm_formal_test.go`) as `package workflow` (same-package / white-box test) rather than `package workflow_test` (external / black-box test). This allows the test file to call unexported functions directly without requiring any changes to production code or the addition of exported shims.
+
+### Alternatives Considered
+
+#### Alternative 1: Add Exported `*ForTest()` Wrapper Functions
+
+Thin exported shims (e.g., `HasWorkflowRunTriggerForTest`) are added in a separate `_test.go` file within `package workflow`, making the internals callable from `package workflow_test`. This keeps the external-test boundary intact but requires writing and maintaining boilerplate wrapper functions for each unexported symbol under test. Rejected because it adds production-adjacent indirection without a correctness benefit over same-package membership, and the project has already established the white-box convention for this file family.
+
+#### Alternative 2: Promote the Functions to Exported Status
+
+Rename the unexported functions to uppercase exports so they become part of the package's public API. Rejected because these are deliberate implementation details — exporting them would make them an implicit contract that is harder to change later, and the motivation for testing them is compliance verification, not external use.
+
+### Consequences
+
+#### Positive
+- No changes to production code are needed; the implementation is tested exactly as written.
+- Follows the existing project convention for `security_architecture_*_formal_test.go` files, keeping this new file consistent with its siblings.
+- Predicate tests directly encode the spec invariants against the real functions, avoiding any mismatch between a shim's behavior and the underlying implementation.
+
+#### Negative
+- The test file is tightly coupled to internal function signatures; renaming or extracting methods to a different struct will break these tests without any compiler warning at the call site.
+- White-box tests carry a risk of circular reasoning: they verify properties of the same code they are derived from, so they will not catch cases where the implementation and the tests share the same misunderstanding of the spec.
+
+#### Neutral
+- The test file must be declared in `package workflow` to compile, placing it in the same namespace as production code.
+- The `//go:build !integration` build tag ensures the file is excluded from integration builds, consistent with other unit tests in the package.
+
+---
+
+*ADR created by [adr-writer agent]. Review and finalize before changing status from Draft to Accepted.*

pkg/workflow/security_architecture_pm_formal_test.go

@@ -0,0 +1,265 @@
+//go:build !integration
+
+package workflow
+
+import (
+	"strings"
+	"testing"
+
+	"github.com/stretchr/testify/assert"
+	"github.com/stretchr/testify/require"
+)
+
+// This file formalizes the §12 Compliance Test Matrix Gap Analysis for
+// Permission Management (T-PM-003, T-PM-005, T-PM-007) described in
+// specs/security-architecture-spec-validation.md. It derives predicates
+// directly from the exported/unexported implementation in role_checks.go,
+// agent_validation.go, github_token.go and compiler_mutators.go.
+
+// P1: WorkflowRunRepoSafetyCondition
+// The compiled if: guard must check repo-id equality and non-fork status
+// only for workflow_run events, and must allow all other events unconditionally.
+func TestFormalPM005_WorkflowRunRepoSafetyCondition(t *testing.T) {
+	c := NewCompiler()
+	condition := c.buildWorkflowRunRepoSafetyCondition()
+
+	assert.Equal(t,
+		"${{ github.event_name != 'workflow_run' || github.event.workflow_run.repository.id == github.repository_id && (!(github.event.workflow_run.repository.fork)) }}",
+		condition,
+	)
+}
+
+// P2: WorkflowRunRepoSafetyOnlyAppliesWhenTriggerPresent
+// hasWorkflowRunTrigger must correctly detect map/string/absent "on:" forms.
+func TestFormalPM005_WorkflowRunRepoSafetyOnlyWhenTriggerDeclared(t *testing.T) {
+	c := NewCompiler()
+
+	tests := []struct {
+		name        string
+		frontmatter map[string]any
+		expected    bool
+	}{
+		{
+			name:        "nil frontmatter",
+			frontmatter: nil,
+			expected:    false,
+		},
+		{
+			name:        "no on section",
+			frontmatter: map[string]any{},
+			expected:    false,
+		},
+		{
+			name: "map form with workflow_run",
+			frontmatter: map[string]any{
+				"on": map[string]any{"workflow_run": map[string]any{}},
+			},
+			expected: true,
+		},
+		{
+			name: "map form without workflow_run",
+			frontmatter: map[string]any{
+				"on": map[string]any{"push": map[string]any{}},
+			},
+			expected: false,
+		},
+		{
+			name: "string form matching workflow_run",
+			frontmatter: map[string]any{
+				"on": "workflow_run",
+			},
+			expected: true,
+		},
+		{
+			name: "string form not matching workflow_run",
+			frontmatter: map[string]any{
+				"on": "push",
+			},
+			expected: false,
+		},
+		{
+			name: "slice form containing workflow_run (currently unsupported)",
+			frontmatter: map[string]any{
+				"on": []string{"workflow_run", "push"},
+			},
+			expected: false,
+		},
+	}
+
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			assert.Equal(t, tt.expected, c.hasWorkflowRunTrigger(tt.frontmatter))
+		})
+	}
+}
+
+// P3: WorkflowRunRequiresNonEmptyWorkflowsField
+func TestFormalPM_WorkflowRunRequiresNonEmptyWorkflows(t *testing.T) {
+	tests := []struct {
+		name     string
+		value    any
+		expected bool
+	}{
+		{"non-empty string", "my-workflow", true},
+		{"blank string", "   ", false},
+		{"empty string", "", false},
+		{"non-empty []string", []string{"my-workflow"}, true},
+		{"[]string with only blanks", []string{"  ", ""}, false},
+		{"empty []string", []string{}, false},
+		{"non-empty []any of strings", []any{"my-workflow"}, true},
+		{"[]any with only blanks", []any{"  ", ""}, false},
+		{"empty []any", []any{}, false},
+		{"nil value", nil, false},
+		{"unsupported type", 42, false},
+	}
+
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			assert.Equal(t, tt.expected, hasNonEmptyWorkflowRunWorkflows(tt.value))
+		})
+	}
+}
+
+// Shared workflow_run "on:" fixtures used across the P4-P6 predicates below.
+const (
+	workflowRunOnMissingBranches = "on:\n  workflow_run:\n    workflows: [\"ci\"]\n    types: [completed]\n"
+	workflowRunOnWithBranches    = "on:\n  workflow_run:\n    workflows: [\"ci\"]\n    types: [completed]\n    branches: [main]\n"
+	pushOnNoWorkflowRun          = "on:\n  push:\n    branches: [main]\n"
+)
+
+// P4: WorkflowRunBranchRestrictionModeSensitive
+// Missing branch restrictions must error in strict mode and warn otherwise.
+func TestFormalPM003_StrictModeGatesMissingBranchRestriction(t *testing.T) {
+	workflowData := &WorkflowData{On: workflowRunOnMissingBranches}
+
+	t.Run("strict mode errors on missing branches", func(t *testing.T) {
+		c := NewCompiler()
+		c.SetStrictMode(true)
+		err := c.validateWorkflowRunBranches(workflowData, "workflow.md")
+		require.Error(t, err)
+		assert.Contains(t, err.Error(), "branch restrictions")
+	})
+
+	t.Run("non-strict mode warns on missing branches", func(t *testing.T) {
+		c := NewCompiler()
+		c.SetStrictMode(false)
+		c.ResetWarningCount()
+		err := c.validateWorkflowRunBranches(workflowData, "workflow.md")
+		require.NoError(t, err)
+		assert.Equal(t, 1, c.GetWarningCount())
+	})
+}
+
+// P5: WorkflowRunBranchRestrictionSatisfiedNoOp
+// Present branch restrictions never trigger the strict/warn path, regardless
+// of strict mode.
+func TestFormalPM_BranchRestrictionPresentIsNoOp(t *testing.T) {
+	workflowData := &WorkflowData{On: workflowRunOnWithBranches}
+
+	for _, strict := range []bool{true, false} {
+		c := NewCompiler()
+		c.SetStrictMode(strict)
+		c.ResetWarningCount()
+		err := c.validateWorkflowRunBranches(workflowData, "workflow.md")
+		require.NoError(t, err, "strict=%v", strict)
+		assert.Equal(t, 0, c.GetWarningCount(), "strict=%v", strict)
+	}
+}
+
+// P6: NoWorkflowRunTriggerIsNoOp
+// Non-workflow_run triggers must bypass validation entirely, even in strict mode.
+func TestFormalPM_NoWorkflowRunTriggerSkipsValidation(t *testing.T) {
+	// validateWorkflowRunBranches short-circuits using strings.Contains on
+	// workflowData.On, so this fixture intentionally avoids any incidental
+	// "workflow_run" substrings.
+	workflowData := &WorkflowData{On: pushOnNoWorkflowRun}
+
+	c := NewCompiler()
+	c.SetStrictMode(true)
+	err := c.validateWorkflowRunBranches(workflowData, "workflow.md")
+	require.NoError(t, err)
+}
+
+// P7: DefaultGitHubTokenPrecedence
+// A custom token always wins; otherwise the 3-tier secret fallback chain is used.
+func TestFormalPM007_DefaultGitHubTokenPrecedence(t *testing.T) {
+	fallback := getEffectiveGitHubToken("")
+	assert.NotEqual(t, -1, strings.Index(fallback, "GH_AW_GITHUB_MCP_SERVER_TOKEN"))
+	assert.NotEqual(t, -1, strings.Index(fallback, "GH_AW_GITHUB_TOKEN"))
+	assert.NotEqual(t, -1, strings.Index(fallback, "GITHUB_TOKEN"))
+	assert.Less(t, strings.Index(fallback, "GH_AW_GITHUB_MCP_SERVER_TOKEN"), strings.Index(fallback, "GH_AW_GITHUB_TOKEN"))
+	assert.Less(t, strings.Index(fallback, "GH_AW_GITHUB_TOKEN"), strings.Index(fallback, "GITHUB_TOKEN"))
+}
+
+// P8: SafeOutputGitHubTokenPrecedence
+// A custom token always wins; otherwise the 2-tier safe-output fallback chain is used.
+func TestFormalPM007_SafeOutputTokenPrecedence(t *testing.T) {
+	fallback := getEffectiveSafeOutputGitHubToken("")
+	assert.NotContains(t, fallback, "GH_AW_GITHUB_MCP_SERVER_TOKEN")
+	assert.NotEqual(t, -1, strings.Index(fallback, "GH_AW_GITHUB_TOKEN"))
+	assert.NotEqual(t, -1, strings.Index(fallback, "GITHUB_TOKEN"))
+	assert.Less(t, strings.Index(fallback, "GH_AW_GITHUB_TOKEN"), strings.Index(fallback, "GITHUB_TOKEN"))
+}
+
+// P9: TokenChainsAreDistinctByJobRole
+// The tool-token chain includes the MCP-server secret; the safe-output chain
+// deliberately excludes it.
+func TestFormalPM007_TokenChainsDifferByRole(t *testing.T) {
+	toolChain := getEffectiveGitHubToken("")
+	safeOutputChain := getEffectiveSafeOutputGitHubToken("")
+
+	assert.Contains(t, toolChain, "GH_AW_GITHUB_MCP_SERVER_TOKEN")
+	assert.NotContains(t, safeOutputChain, "GH_AW_GITHUB_MCP_SERVER_TOKEN")
+
+	// Both chains share the common tail of the fallback chain.
+	assert.Contains(t, toolChain, "GH_AW_GITHUB_TOKEN")
+	assert.Contains(t, toolChain, "GITHUB_TOKEN")
+	assert.Contains(t, safeOutputChain, "GH_AW_GITHUB_TOKEN")
+	assert.Contains(t, safeOutputChain, "GITHUB_TOKEN")
+}
+
+// P10: StrictModeIsPerCompilerInstance
+// SetStrictMode toggles the strictMode field deterministically and does not
+// leak across compiler instances.
+func TestFormalPM003_SetStrictModeIsIdempotentSetter(t *testing.T) {
+	c1 := NewCompiler()
+	c2 := NewCompiler()
+
+	c1.SetStrictMode(true)
+	c2.SetStrictMode(false)
+	assert.False(t, c2.strictMode)
+
+	// Idempotent: setting the same value repeatedly is a no-op.
+	c1.SetStrictMode(true)
+	assert.True(t, c1.strictMode)
+
+	// Toggling flips the field.
+	c1.SetStrictMode(false)
+	assert.False(t, c1.strictMode)
+}
+
+// P11: BashRestrictionWildcardSafe
+// Boundary matrix for HasBashExplicitRestriction: nil/wildcard/false/empty-list.
+func TestFormalPM_BashExplicitRestrictionBoundary(t *testing.T) {
+	tests := []struct {
+		name     string
+		tools    map[string]any
+		expected bool
+	}{
+		{"nil tools", nil, false},
+		{"no bash key", map[string]any{}, false},
+		{"bash true (no restriction)", map[string]any{"bash": true}, false},
+		{"bash false (explicit restriction)", map[string]any{"bash": false}, true},
+		{"bash nil value", map[string]any{"bash": nil}, false},
+		{"bash wildcard list", map[string]any{"bash": []any{"*"}}, false},
+		{"bash mixed list with wildcard (no restriction)", map[string]any{"bash": []any{"ls", "*", "cat"}}, false},
+		{"bash empty list (explicit restriction)", map[string]any{"bash": []any{}}, true},
+		{"bash named list (explicit restriction)", map[string]any{"bash": []any{"ls"}}, true},
+	}
+
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			assert.Equal(t, tt.expected, HasBashExplicitRestriction(tt.tools))
+		})
+	}
+}