diff --git a/docs/adr/53123-purelock-automated-pure-function-test-coverage.md b/docs/adr/53123-purelock-automated-pure-function-test-coverage.md new file mode 100644 index 00000000000..10e4841f2c9 --- /dev/null +++ b/docs/adr/53123-purelock-automated-pure-function-test-coverage.md @@ -0,0 +1,44 @@ +# ADR-53123: PureLock — Automated Pure-Function Test Coverage + +**Date**: 2026-08-16 +**Status**: Draft +**Deciders**: Unknown (automation-generated PR by PureLock) + +--- + +### Context + +Several pure Go functions in `pkg/cli/` and `pkg/workflow/` had statement coverage in the 46–61% range (`recommendAuditComparisonAction` at 46.7%, `gatewayEntryToTimelineEvent` at 48.5%, `prepareNestedMapValueForYAML` at 60.9%). These functions contain non-trivial branching logic — priority ordering, type switches, and fallback chains — that create regression risk. No existing workflow systematically targeted pure functions for comprehensive branch coverage; coverage improvements happened reactively during feature work, leaving gaps that were difficult to close without dedicated effort. + +### Decision + +We will use PureLock, an automated precompute pass that ranks pure Go functions by coverage deficit and cyclomatic complexity, and generates comprehensive testify table-driven test suites for the top-ranked candidates. PureLock targets only functions with no observable side effects (no I/O, no global mutation), enabling full branch coverage via pure input/output assertions. Each generated test file is reviewed and committed to the PR branch before merge. + +### Alternatives Considered + +#### Alternative 1: Manual test authoring by feature owners + +Tests are written by the engineer implementing a feature, as part of normal PR workflow. This is the existing practice for most of the codebase. Why not chosen: coverage improvements for pre-existing low-coverage functions depend entirely on whether an engineer proactively adds tests outside their feature scope. In practice, low-coverage pure functions persist indefinitely because no one has an incentive to write tests for code they did not change. + +#### Alternative 2: Integration or end-to-end tests + +Cover the target functions indirectly by exercising the higher-level workflows that call them. Why not chosen: integration tests cannot guarantee 100% branch-level coverage of a specific function, take significantly longer to run, require network-dependent infrastructure (e.g., `httptest` servers that are blocked in the sandbox), and obscure which branches were actually exercised. + +### Consequences + +#### Positive +- Achieves 100% function-level statement coverage for each targeted pure function, closing coverage gaps that would otherwise persist indefinitely. +- Scales automatically: PureLock can identify and generate tests for many low-coverage functions without requiring human initiative or scope creep into feature work. +- Table-driven tests with explicit subtest names serve as living documentation of expected function behavior and branch semantics. + +#### Negative +- Generated test files can drift from production code as function signatures or branch logic evolve; reviewers must ensure tests remain meaningful after refactors. +- Automation-generated PRs consume merge queue capacity and require reviewer bandwidth to inspect generated test cases for correctness, even when no production code changes. + +#### Neutral +- PureLock PRs are generated by a bot (`github-actions`) rather than a human author; they carry `automation`, `testing`, and `coverage` labels for filtering. +- Full-package test runs may be blocked in the sandbox by network-dependent tests in unrelated files; targeted `-run` flags are required to validate the generated tests in CI. + +--- + +*ADR created by [adr-writer agent]. Review and finalize before changing status from Draft to Accepted.* diff --git a/pkg/cli/audit_comparison_test.go b/pkg/cli/audit_comparison_test.go index bec4311f9f0..2dfed456419 100644 --- a/pkg/cli/audit_comparison_test.go +++ b/pkg/cli/audit_comparison_test.go @@ -160,3 +160,146 @@ func TestScoreAuditComparisonCandidateFallsBackToLatestSuccess(t *testing.T) { assert.Equal(t, "latest_success", candidate.Selection) assert.Nil(t, candidate.MatchedOn) } + +func TestRecommendAuditComparisonAction(t *testing.T) { + t.Parallel() + + newlyPresentMCPFailure := &AuditComparisonMCPFailureDelta{NewlyPresent: true} + + tests := []struct { + name string + label string + currentConclusion string + delta *AuditComparisonDelta + expectedContains string + }{ + { + name: "non-success failure conclusion", + label: "changed", + currentConclusion: "failure", + delta: &AuditComparisonDelta{}, + expectedContains: "Investigate failure; run concluded with errors", + }, + { + name: "non-success non-failure conclusion", + label: "changed", + currentConclusion: "action_required", + delta: &AuditComparisonDelta{}, + expectedContains: "Investigate the action required conclusion", + }, + { + name: "nil delta with success conclusion", + label: "changed", + currentConclusion: "success", + delta: nil, + expectedContains: "No action needed", + }, + { + name: "stable label with success conclusion", + label: "stable", + currentConclusion: "success", + delta: &AuditComparisonDelta{}, + expectedContains: "No action needed", + }, + { + name: "empty conclusion treated as success-equivalent, stable label", + label: "stable", + currentConclusion: "", + delta: &AuditComparisonDelta{}, + expectedContains: "No action needed", + }, + { + name: "posture read_only to write_capable", + label: "risky", + currentConclusion: "success", + delta: &AuditComparisonDelta{ + Posture: AuditComparisonStringDelta{Before: "read_only", After: "write_capable"}, + }, + expectedContains: "Review first-time write-capable behavior", + }, + { + name: "newly present MCP failure", + label: "risky", + currentConclusion: "success", + delta: &AuditComparisonDelta{ + MCPFailure: newlyPresentMCPFailure, + }, + expectedContains: "Inspect the new MCP failure", + }, + { + name: "MCP failure present but not newly present falls through", + label: "changed", + currentConclusion: "success", + delta: &AuditComparisonDelta{ + MCPFailure: &AuditComparisonMCPFailureDelta{NewlyPresent: false}, + }, + expectedContains: "Review the behavior change", + }, + { + name: "blocked requests increased", + label: "risky", + currentConclusion: "success", + delta: &AuditComparisonDelta{ + BlockedRequests: AuditComparisonIntDelta{Before: 1, After: 3}, + }, + expectedContains: "Review network policy changes", + }, + { + name: "turns increased", + label: "changed", + currentConclusion: "success", + delta: &AuditComparisonDelta{ + Turns: AuditComparisonIntDelta{Before: 2, After: 5}, + }, + expectedContains: "Compare prompt or task-shape changes", + }, + { + name: "fallback default review message", + label: "changed", + currentConclusion: "success", + delta: &AuditComparisonDelta{ + Turns: AuditComparisonIntDelta{Before: 5, After: 3}, + BlockedRequests: AuditComparisonIntDelta{Before: 3, After: 1}, + }, + expectedContains: "Review the behavior change against the selected successful baseline", + }, + { + name: "priority order: posture change wins over MCP failure", + label: "risky", + currentConclusion: "success", + delta: &AuditComparisonDelta{ + Posture: AuditComparisonStringDelta{Before: "read_only", After: "write_capable"}, + MCPFailure: newlyPresentMCPFailure, + }, + expectedContains: "Review first-time write-capable behavior", + }, + { + name: "priority order: MCP failure wins over blocked requests", + label: "risky", + currentConclusion: "success", + delta: &AuditComparisonDelta{ + MCPFailure: newlyPresentMCPFailure, + BlockedRequests: AuditComparisonIntDelta{Before: 1, After: 5}, + }, + expectedContains: "Inspect the new MCP failure", + }, + { + name: "priority order: blocked requests wins over turns", + label: "risky", + currentConclusion: "success", + delta: &AuditComparisonDelta{ + BlockedRequests: AuditComparisonIntDelta{Before: 1, After: 5}, + Turns: AuditComparisonIntDelta{Before: 2, After: 8}, + }, + expectedContains: "Review network policy changes", + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + t.Parallel() + result := recommendAuditComparisonAction(tt.label, tt.currentConclusion, tt.delta) + assert.Contains(t, result, tt.expectedContains) + }) + } +} diff --git a/pkg/cli/gateway_logs_timeline_test.go b/pkg/cli/gateway_logs_timeline_test.go index d3fc7370295..adb63ac2bbb 100644 --- a/pkg/cli/gateway_logs_timeline_test.go +++ b/pkg/cli/gateway_logs_timeline_test.go @@ -990,3 +990,191 @@ func TestRenderUnifiedTimelineStream_SteeringEvent(t *testing.T) { t.Errorf("output missing steering message; got:\n%s", out) } } + +// ─── gatewayEntryToTimelineEvent ─────────────────────────────────────────────── + +func TestGatewayEntryToTimelineEvent(t *testing.T) { + baseTS := "2024-01-15T10:00:00Z" + baseTime := time.Date(2024, 1, 15, 10, 0, 0, 0, time.UTC) + + tests := []struct { + name string + entry GatewayLogEntry + wantOK bool + wantKind TimelineEventKind + wantServer string + wantTool string + wantStatus string + wantError string + wantReason string + wantAuthor string + }{ + { + name: "unparseable timestamp returns false", + entry: GatewayLogEntry{Timestamp: "not-a-timestamp", Event: "tool_call"}, + wantOK: false, + }, + { + name: "empty timestamp returns false", + entry: GatewayLogEntry{Timestamp: "", Event: "tool_call"}, + wantOK: false, + }, + { + name: "DIFC_FILTERED uses ServerID when present", + entry: GatewayLogEntry{ + Timestamp: baseTS, + Type: "DIFC_FILTERED", + ServerID: "srv-1", + ServerName: "srv-fallback", + ToolName: "tool-a", + Reason: "blocked secrecy", + AuthorLogin: "octocat", + }, + wantOK: true, + wantKind: TimelineKindDIFCFiltered, + wantServer: "srv-1", + wantTool: "tool-a", + wantReason: "blocked secrecy", + wantAuthor: "octocat", + }, + { + name: "DIFC_FILTERED falls back to ServerName when ServerID empty", + entry: GatewayLogEntry{ + Timestamp: baseTS, + Type: "DIFC_FILTERED", + ServerName: "srv-fallback", + }, + wantOK: true, + wantKind: TimelineKindDIFCFiltered, + wantServer: "srv-fallback", + }, + { + name: "GUARD_POLICY_BLOCKED uses ServerID when present", + entry: GatewayLogEntry{ + Timestamp: baseTS, + Type: "GUARD_POLICY_BLOCKED", + ServerID: "srv-2", + ToolName: "tool-b", + Reason: "policy violation", + Message: "guard rejected the call", + }, + wantOK: true, + wantKind: TimelineKindGuardPolicyBlocked, + wantServer: "srv-2", + wantTool: "tool-b", + wantReason: "policy violation", + wantError: "guard rejected the call", + }, + { + name: "GUARD_POLICY_BLOCKED falls back to ServerName when ServerID empty", + entry: GatewayLogEntry{ + Timestamp: baseTS, + Type: "GUARD_POLICY_BLOCKED", + ServerName: "srv-fallback-guard", + }, + wantOK: true, + wantKind: TimelineKindGuardPolicyBlocked, + wantServer: "srv-fallback-guard", + }, + { + name: "tool_call event with explicit status is preserved", + entry: GatewayLogEntry{ + Timestamp: baseTS, + Event: "tool_call", + ServerName: "srv-3", + ToolName: "tool-c", + Method: "call", + Duration: 12.5, + Status: "success", + }, + wantOK: true, + wantKind: TimelineKindToolCall, + wantServer: "srv-3", + wantTool: "tool-c", + wantStatus: "success", + }, + { + name: "rpc_call event with error sets status to error", + entry: GatewayLogEntry{ + Timestamp: baseTS, + Event: "rpc_call", + ToolName: "tool-d", + Error: "boom", + }, + wantOK: true, + wantKind: TimelineKindToolCall, + wantTool: "tool-d", + wantStatus: "error", + wantError: "boom", + }, + { + name: "request event with error level sets status to error", + entry: GatewayLogEntry{ + Timestamp: baseTS, + Event: "request", + Level: "error", + }, + wantOK: true, + wantKind: TimelineKindToolCall, + wantStatus: "error", + }, + { + name: "request event with no error defaults to success", + entry: GatewayLogEntry{ + Timestamp: baseTS, + Event: "request", + }, + wantOK: true, + wantKind: TimelineKindToolCall, + wantStatus: "success", + }, + { + name: "unknown type and unknown event returns false", + entry: GatewayLogEntry{ + Timestamp: baseTS, + Type: "SOMETHING_ELSE", + Event: "unknown_event", + }, + wantOK: false, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + evt, ok := gatewayEntryToTimelineEvent(tt.entry) + if ok != tt.wantOK { + t.Fatalf("gatewayEntryToTimelineEvent() ok = %v, want %v", ok, tt.wantOK) + } + if !tt.wantOK { + return + } + if !evt.Time.Equal(baseTime) { + t.Errorf("Time = %v, want %v", evt.Time, baseTime) + } + if evt.Source != TimelineSourceGateway { + t.Errorf("Source = %v, want %v", evt.Source, TimelineSourceGateway) + } + if evt.Kind != tt.wantKind { + t.Errorf("Kind = %v, want %v", evt.Kind, tt.wantKind) + } + if tt.wantServer != "" && evt.ServerName != tt.wantServer { + t.Errorf("ServerName = %q, want %q", evt.ServerName, tt.wantServer) + } + if tt.wantTool != "" && evt.ToolName != tt.wantTool { + t.Errorf("ToolName = %q, want %q", evt.ToolName, tt.wantTool) + } + if tt.wantStatus != "" && evt.Status != tt.wantStatus { + t.Errorf("Status = %q, want %q", evt.Status, tt.wantStatus) + } + if tt.wantError != "" && evt.Error != tt.wantError { + t.Errorf("Error = %q, want %q", evt.Error, tt.wantError) + } + if tt.wantReason != "" && evt.Reason != tt.wantReason { + t.Errorf("Reason = %q, want %q", evt.Reason, tt.wantReason) + } + if tt.wantAuthor != "" && evt.AuthorLogin != tt.wantAuthor { + t.Errorf("AuthorLogin = %q, want %q", evt.AuthorLogin, tt.wantAuthor) + } + }) + } +} diff --git a/pkg/workflow/yaml_test.go b/pkg/workflow/yaml_test.go index 07834aa4526..e01865cd2e3 100644 --- a/pkg/workflow/yaml_test.go +++ b/pkg/workflow/yaml_test.go @@ -613,3 +613,144 @@ func TestFormatYAMLValue(t *testing.T) { }) } } + +func TestPrepareNestedMapValueForYAML(t *testing.T) { + t.Run("map[string]any under with field is ordered", func(t *testing.T) { + value := map[string]any{"b": 1, "a": 2} + result := prepareNestedMapValueForYAML("with", value) + ms, ok := result.(yaml.MapSlice) + if !ok { + t.Fatalf("expected yaml.MapSlice, got %T", result) + } + if len(ms) != 2 || ms[0].Key != "a" || ms[1].Key != "b" { + t.Errorf("expected sorted keys a,b; got %+v", ms) + } + }) + + t.Run("map[string]any under env field is ordered", func(t *testing.T) { + value := map[string]any{"z": 1, "a": 2} + result := prepareNestedMapValueForYAML("env", value) + ms, ok := result.(yaml.MapSlice) + if !ok { + t.Fatalf("expected yaml.MapSlice, got %T", result) + } + if len(ms) != 2 || ms[0].Key != "a" { + t.Errorf("expected sorted keys; got %+v", ms) + } + }) + + t.Run("map[string]any under secrets field is ordered", func(t *testing.T) { + value := map[string]any{"token": "x"} + result := prepareNestedMapValueForYAML("secrets", value) + if _, ok := result.(yaml.MapSlice); !ok { + t.Fatalf("expected yaml.MapSlice, got %T", result) + } + }) + + t.Run("map[string]any under other field is recursively copied not ordered", func(t *testing.T) { + value := map[string]any{"nested": map[string]any{"x": 1}} + result := prepareNestedMapValueForYAML("other", value) + copied, ok := result.(map[string]any) + if !ok { + t.Fatalf("expected map[string]any, got %T", result) + } + if _, ok := copied["nested"]; !ok { + t.Errorf("expected nested key to be preserved") + } + // original map must not be mutated by copy + if &copied == &value { + t.Errorf("expected a distinct copied map") + } + }) + + t.Run("map[string]string under with field is ordered", func(t *testing.T) { + value := map[string]string{"b": "1", "a": "2"} + result := prepareNestedMapValueForYAML("with", value) + if _, ok := result.(yaml.MapSlice); !ok { + t.Fatalf("expected yaml.MapSlice, got %T", result) + } + }) + + t.Run("map[string]string under other field returned unchanged", func(t *testing.T) { + value := map[string]string{"b": "1", "a": "2"} + result := prepareNestedMapValueForYAML("other", value) + m, ok := result.(map[string]string) + if !ok { + t.Fatalf("expected map[string]string, got %T", result) + } + if m["a"] != "2" || m["b"] != "1" { + t.Errorf("expected value unchanged; got %+v", m) + } + }) + + t.Run("[]any recursively processes elements", func(t *testing.T) { + value := []any{ + map[string]any{"b": 1, "a": 2}, + "plain", + } + result := prepareNestedMapValueForYAML("other", value) + arr, ok := result.([]any) + if !ok { + t.Fatalf("expected []any, got %T", result) + } + if len(arr) != 2 { + t.Fatalf("expected 2 elements, got %d", len(arr)) + } + if arr[1] != "plain" { + t.Errorf("expected second element unchanged; got %v", arr[1]) + } + if _, ok := arr[0].(map[string]any); !ok { + t.Errorf("expected nested map to remain a map[string]any (fieldName empty means no special ordering); got %T", arr[0]) + } + }) + + t.Run("yaml.MapSlice with string keys recurses using key as fieldName", func(t *testing.T) { + value := yaml.MapSlice{ + {Key: "with", Value: map[string]any{"b": 1, "a": 2}}, + {Key: "plain", Value: "value"}, + } + result := prepareNestedMapValueForYAML("other", value) + ms, ok := result.(yaml.MapSlice) + if !ok { + t.Fatalf("expected yaml.MapSlice, got %T", result) + } + if len(ms) != 2 { + t.Fatalf("expected 2 items, got %d", len(ms)) + } + if ms[0].Key != "with" { + t.Errorf("expected first key 'with', got %v", ms[0].Key) + } + if _, ok := ms[0].Value.(yaml.MapSlice); !ok { + t.Errorf("expected nested 'with' value to become ordered yaml.MapSlice, got %T", ms[0].Value) + } + if ms[1].Value != "value" { + t.Errorf("expected plain value unchanged; got %v", ms[1].Value) + } + }) + + t.Run("yaml.MapSlice with non-string key falls back to empty fieldName", func(t *testing.T) { + value := yaml.MapSlice{ + {Key: 42, Value: "x"}, + } + result := prepareNestedMapValueForYAML("other", value) + ms, ok := result.(yaml.MapSlice) + if !ok { + t.Fatalf("expected yaml.MapSlice, got %T", result) + } + if len(ms) != 1 || ms[0].Key != 42 { + t.Errorf("expected non-string key preserved; got %+v", ms) + } + if ms[0].Value != "x" { + t.Errorf("expected value unchanged; got %v", ms[0].Value) + } + }) + + t.Run("default case returns value unchanged for scalar types", func(t *testing.T) { + for _, v := range []any{42, "hello", true, 3.14, nil} { + result := prepareNestedMapValueForYAML("field", v) + if result != v { + t.Errorf("expected %v unchanged, got %v", v, result) + } + } + }) +}