[purelock] Lock down removeUnsafeEngineEnvKeys, migrateMessagesEffectiveTokensSuffixToAICreditsSuffix with pure-function test suites

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.*

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)
+}