From 24739243af46082ee778c6828b6d2ac0ece1bd1f Mon Sep 17 00:00:00 2001 From: "github-actions[bot]" <41898282+github-actions[bot]@users.noreply.github.com> Date: Mon, 10 Aug 2026 12:53:09 +0000 Subject: [PATCH 1/2] test(purelock): add coverage for removeUnsafeEngineEnvKeys and migrateMessagesEffectiveTokensSuffixToAICreditsSuffix Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- .../codemod_engine_env_secrets_pure_test.go | 210 ++++++++++++++++++ ...s_suffix_to_ai_credits_suffix_pure_test.go | 180 +++++++++++++++ 2 files changed, 390 insertions(+) create mode 100644 pkg/cli/codemod_engine_env_secrets_pure_test.go create mode 100644 pkg/cli/codemod_messages_effective_tokens_suffix_to_ai_credits_suffix_pure_test.go diff --git a/pkg/cli/codemod_engine_env_secrets_pure_test.go b/pkg/cli/codemod_engine_env_secrets_pure_test.go new file mode 100644 index 00000000000..f1d538a9c09 --- /dev/null +++ b/pkg/cli/codemod_engine_env_secrets_pure_test.go @@ -0,0 +1,210 @@ +package cli + +import ( + "testing" + + "github.com/stretchr/testify/assert" +) + +// TestRemoveUnsafeEngineEnvKeys covers removeUnsafeEngineEnvKeys, a pure function that +// rewrites frontmatter YAML lines to drop unsafe engine.env: keys while leaving +// everything else (including unrelated top-level env: blocks) untouched. +func TestRemoveUnsafeEngineEnvKeys(t *testing.T) { + t.Parallel() + + tests := []struct { + name string + lines []string + unsafeKeys map[string]struct{} + wantModified bool + wantLines []string + }{ + { + name: "no engine key present", + lines: []string{"on: push", "permissions: {}"}, + unsafeKeys: map[string]struct{}{"FOO": {}}, + wantModified: false, + wantLines: []string{"on: push", "permissions: {}"}, + }, + { + name: "no env under engine", + lines: []string{ + "engine:", + " id: copilot", + }, + unsafeKeys: map[string]struct{}{"FOO": {}}, + wantModified: false, + wantLines: []string{ + "engine:", + " id: copilot", + }, + }, + { + name: "removes single unsafe simple key", + lines: []string{ + "engine:", + " id: copilot", + " env:", + " FOO: bar", + " BAZ: qux", + }, + unsafeKeys: map[string]struct{}{"FOO": {}}, + wantModified: true, + wantLines: []string{ + "engine:", + " id: copilot", + " env:", + " BAZ: qux", + }, + }, + { + name: "removes unsafe key with nested/multiline value", + lines: []string{ + "engine:", + " env:", + " FOO: |", + " ${{ secrets.FOO }}", + " more", + " BAZ: qux", + }, + unsafeKeys: map[string]struct{}{"FOO": {}}, + wantModified: true, + wantLines: []string{ + "engine:", + " env:", + " BAZ: qux", + }, + }, + { + name: "removes unsafe key followed by comment continuation", + lines: []string{ + "engine:", + " env:", + " FOO: bar", + " # trailing comment nested under FOO", + " BAZ: qux", + }, + unsafeKeys: map[string]struct{}{"FOO": {}}, + wantModified: true, + wantLines: []string{ + "engine:", + " env:", + " BAZ: qux", + }, + }, + { + name: "leaves blank lines inside env untouched when not removing", + lines: []string{ + "engine:", + " env:", + " BAZ: qux", + "", + " QUX: zap", + }, + unsafeKeys: map[string]struct{}{"FOO": {}}, + wantModified: false, + wantLines: []string{ + "engine:", + " env:", + " BAZ: qux", + "", + " QUX: zap", + }, + }, + { + name: "removes multiple unsafe keys", + lines: []string{ + "engine:", + " env:", + " FOO: bar", + " BAZ: qux", + " QUX: zap", + }, + unsafeKeys: map[string]struct{}{"FOO": {}, "QUX": {}}, + wantModified: true, + wantLines: []string{ + "engine:", + " env:", + " BAZ: qux", + }, + }, + { + name: "stops treating lines as engine.env after exiting engine block", + lines: []string{ + "engine:", + " env:", + " FOO: bar", + "on: push", + "env:", + " FOO: unrelated-top-level", + }, + unsafeKeys: map[string]struct{}{"FOO": {}}, + wantModified: true, + wantLines: []string{ + "engine:", + " env:", + "on: push", + "env:", + " FOO: unrelated-top-level", + }, + }, + { + name: "keeps keys not in unsafe set", + lines: []string{ + "engine:", + " env:", + " SAFE: value", + }, + unsafeKeys: map[string]struct{}{"FOO": {}}, + wantModified: false, + wantLines: []string{ + "engine:", + " env:", + " SAFE: value", + }, + }, + { + name: "empty input", + lines: []string{}, + unsafeKeys: map[string]struct{}{"FOO": {}}, + wantModified: false, + wantLines: []string{}, + }, + } + + for _, tt := range tests { + tt := tt + t.Run(tt.name, func(t *testing.T) { + t.Parallel() + gotLines, gotModified := removeUnsafeEngineEnvKeys(tt.lines, tt.unsafeKeys) + assert.Equal(t, tt.wantModified, gotModified, "modified flag mismatch") + assert.Equal(t, tt.wantLines, gotLines, "resulting lines mismatch") + }) + } +} + +// TestRemoveUnsafeEngineEnvKeysPurity ensures the function does not mutate its +// input slice or map arguments (a hallmark of purity), and is deterministic +// across repeated invocations with identical inputs. +func TestRemoveUnsafeEngineEnvKeysPurity(t *testing.T) { + t.Parallel() + + original := []string{ + "engine:", + " env:", + " FOO: bar", + " BAZ: qux", + } + inputCopy := make([]string, len(original)) + copy(inputCopy, original) + + unsafeKeys := map[string]struct{}{"FOO": {}} + + result1, modified1 := removeUnsafeEngineEnvKeys(inputCopy, unsafeKeys) + // Input slice must remain unchanged. + assert.Equal(t, original, inputCopy, "input lines were mutated") + + result2, modified2 := removeUnsafeEngineEnvKeys(inputCopy, unsafeKeys) + assert.Equal(t, result1, result2, "results differ across repeated calls with identical input") + assert.Equal(t, modified1, modified2) +} diff --git a/pkg/cli/codemod_messages_effective_tokens_suffix_to_ai_credits_suffix_pure_test.go b/pkg/cli/codemod_messages_effective_tokens_suffix_to_ai_credits_suffix_pure_test.go new file mode 100644 index 00000000000..e24b4cfef5a --- /dev/null +++ b/pkg/cli/codemod_messages_effective_tokens_suffix_to_ai_credits_suffix_pure_test.go @@ -0,0 +1,180 @@ +package cli + +import ( + "testing" + + "github.com/stretchr/testify/assert" +) + +// TestMigrateMessagesEffectiveTokensSuffixToAICreditsSuffix covers the pure +// line-based rewrite that migrates safe-outputs.messages placeholders from +// {effective_tokens_suffix} to {ai_credits_suffix}. +func TestMigrateMessagesEffectiveTokensSuffixToAICreditsSuffix(t *testing.T) { + t.Parallel() + + tests := []struct { + name string + lines []string + wantModified bool + wantLines []string + }{ + { + name: "no safe-outputs block", + lines: []string{"on: push", "permissions: {}"}, + wantModified: false, + wantLines: []string{"on: push", "permissions: {}"}, + }, + { + name: "no messages block under safe-outputs", + lines: []string{ + "safe-outputs:", + " create-issue:", + }, + wantModified: false, + wantLines: []string{ + "safe-outputs:", + " create-issue:", + }, + }, + { + name: "replaces placeholder in simple scalar message", + lines: []string{ + "safe-outputs:", + " messages:", + " footer: \"Cost {effective_tokens_suffix}\"", + }, + wantModified: true, + wantLines: []string{ + "safe-outputs:", + " messages:", + " footer: \"Cost {ai_credits_suffix}\"", + }, + }, + { + name: "no placeholder present leaves lines untouched", + lines: []string{ + "safe-outputs:", + " messages:", + " footer: \"Cost: $0\"", + }, + wantModified: false, + wantLines: []string{ + "safe-outputs:", + " messages:", + " footer: \"Cost: $0\"", + }, + }, + { + name: "replaces placeholder inside block scalar", + lines: []string{ + "safe-outputs:", + " messages:", + " footer: |", + " Cost {effective_tokens_suffix}", + " more {effective_tokens_suffix} text", + " other: value", + }, + wantModified: true, + wantLines: []string{ + "safe-outputs:", + " messages:", + " footer: |", + " Cost {ai_credits_suffix}", + " more {ai_credits_suffix} text", + " other: value", + }, + }, + { + name: "does not touch keys outside messages block", + lines: []string{ + "safe-outputs:", + " create-issue:", + " title-prefix: \"{effective_tokens_suffix}\"", + " messages:", + " footer: \"{effective_tokens_suffix}\"", + }, + wantModified: true, + wantLines: []string{ + "safe-outputs:", + " create-issue:", + " title-prefix: \"{effective_tokens_suffix}\"", + " messages:", + " footer: \"{ai_credits_suffix}\"", + }, + }, + { + name: "stops applying once outside safe-outputs block", + lines: []string{ + "safe-outputs:", + " messages:", + " footer: \"{effective_tokens_suffix}\"", + "on: push", + "env:", + " X: \"{effective_tokens_suffix}\"", + }, + wantModified: true, + wantLines: []string{ + "safe-outputs:", + " messages:", + " footer: \"{ai_credits_suffix}\"", + "on: push", + "env:", + " X: \"{effective_tokens_suffix}\"", + }, + }, + { + name: "empty input", + lines: []string{}, + wantModified: false, + wantLines: []string{}, + }, + { + name: "comments are not treated as keys", + lines: []string{ + "safe-outputs:", + " messages:", + " # a comment mentioning {effective_tokens_suffix}", + " footer: \"{effective_tokens_suffix}\"", + }, + wantModified: true, + wantLines: []string{ + "safe-outputs:", + " messages:", + " # a comment mentioning {effective_tokens_suffix}", + " footer: \"{ai_credits_suffix}\"", + }, + }, + } + + for _, tt := range tests { + tt := tt + t.Run(tt.name, func(t *testing.T) { + t.Parallel() + gotLines, gotModified := migrateMessagesEffectiveTokensSuffixToAICreditsSuffix(tt.lines) + assert.Equal(t, tt.wantModified, gotModified, "modified flag mismatch") + assert.Equal(t, tt.wantLines, gotLines, "resulting lines mismatch") + }) + } +} + +// TestMigrateMessagesEffectiveTokensSuffixToAICreditsSuffixPurity verifies the +// function neither mutates its input slice nor produces different output +// across repeated calls with identical input (purity/determinism check). +func TestMigrateMessagesEffectiveTokensSuffixToAICreditsSuffixPurity(t *testing.T) { + t.Parallel() + + original := []string{ + "safe-outputs:", + " messages:", + " footer: \"{effective_tokens_suffix}\"", + } + inputCopy := make([]string, len(original)) + copy(inputCopy, original) + + result1, modified1 := migrateMessagesEffectiveTokensSuffixToAICreditsSuffix(inputCopy) + assert.Equal(t, original, inputCopy, "input lines were mutated") + + result2, modified2 := migrateMessagesEffectiveTokensSuffixToAICreditsSuffix(inputCopy) + assert.Equal(t, result1, result2, "results differ across repeated calls with identical input") + assert.Equal(t, modified1, modified2) +} From b1936eeeee14eda737553a8da603ac16ad2c5730 Mon Sep 17 00:00:00 2001 From: "github-actions[bot]" <41898282+github-actions[bot]@users.noreply.github.com> Date: Mon, 10 Aug 2026 15:50:12 +0000 Subject: [PATCH 2/2] chore(adr): add draft ADR-51783 for pure-function test suites with purity assertions Co-Authored-By: Claude Sonnet 4.6 --- ...tion-test-suites-with-purity-assertions.md | 44 +++++++++++++++++++ 1 file changed, 44 insertions(+) create mode 100644 docs/adr/51783-pure-function-test-suites-with-purity-assertions.md diff --git a/docs/adr/51783-pure-function-test-suites-with-purity-assertions.md b/docs/adr/51783-pure-function-test-suites-with-purity-assertions.md new file mode 100644 index 00000000000..c2d7df7a215 --- /dev/null +++ b/docs/adr/51783-pure-function-test-suites-with-purity-assertions.md @@ -0,0 +1,44 @@ +# ADR-51783: Pure-Function Test Suites with Purity Assertions + +**Date**: 2026-08-10 +**Status**: Draft +**Deciders**: PureLock automation (pelikhan) + +--- + +### Context + +PureLock automated analysis identified two pure Go functions in `pkg/cli` with 0% test coverage: `removeUnsafeEngineEnvKeys` (a YAML-frontmatter line-based state machine that strips unsafe `engine.env:` keys) and `migrateMessagesEffectiveTokensSuffixToAICreditsSuffix` (a single-pass rewriter that migrates `{effective_tokens_suffix}` placeholders to `{ai_credits_suffix}` within `safe-outputs.messages:` blocks). Both functions are non-trivial: they implement multi-state YAML parsers that track block nesting, handle scalar and block-scalar values, skip blank lines and comments, and exit cleanly when they cross block boundaries. The absence of any test coverage made regressions undetectable by CI. + +### Decision + +We will test pure functions using a **two-layer test pattern**: a primary table-driven subtest suite that covers every meaningful branch of the state machine using YAML-line fixtures, and a dedicated purity test that asserts no input slice is mutated and that repeated invocations with identical inputs return identical results. This approach was applied to both `removeUnsafeEngineEnvKeys` and `migrateMessagesEffectiveTokensSuffixToAICreditsSuffix`. + +### Alternatives Considered + +#### Alternative 1: Integration tests via the codemod command infrastructure + +Exercise the functions indirectly by constructing real workflow YAML files and invoking the top-level codemod command. This would provide realistic end-to-end coverage but requires filesystem setup, command plumbing, and expensive test infrastructure. It cannot easily enumerate every internal state-machine branch in isolation, and the signal-to-noise ratio for pinpointing which branch a failure exercises is low. + +#### Alternative 2: Fuzzing with `go test -fuzz` + +Use Go's native fuzzer to discover edge cases automatically. The PR body explicitly evaluated and rejected this: the line-oriented state machines achieve full branch coverage with a carefully chosen set of table fixtures, and the exhaustive fixture set provides clearer failure messages than a corpus-based fuzzer. Fuzzing would be redundant once full branch coverage is confirmed. + +### Consequences + +#### Positive +- Coverage jumps from 0% to 93.8% (`removeUnsafeEngineEnvKeys`) and 100% (`migrateMessagesEffectiveTokensSuffixToAICreditsSuffix`), providing a CI safety net for regression. +- The purity test acts as a machine-enforced contract: any future change that introduces input mutation or non-determinism will fail a test immediately. +- Table-driven fixtures are self-documenting — each subtest name describes a distinct state-machine scenario, making the expected behavior readable without consulting the implementation. + +#### Negative +- Tests are coupled to the line-based implementation strategy. If the parser is replaced with a proper YAML library, all fixture tests will require significant rewriting. +- The two-layer pattern (behavioral tests + purity test) adds boilerplate per function; this overhead scales with the number of pure functions targeted. + +#### Neutral +- These tests reside in the `cli` package (same package as the functions under test), giving them access to unexported symbols without an additional export file. +- PureLock identified the coverage gap; this ADR codifies the testing pattern that PureLock-driven PRs should follow for pure functions. + +--- + +*ADR created by [adr-writer agent]. Review and finalize before changing status from Draft to Accepted.*