[purelock] Lock down selectHistoricalOperationalValueGrader, extractHostFromRemoteURL, extractOTLPAttributesFromObsMap with pure [Content t

docs/adr/56895-add-adrs-for-pure-function-test-lockdown.md

@@ -0,0 +1,50 @@
+# ADR-56895: Add ADRs for Pure Function Test Lockdown
+
+**Date**: 2026-08-29
+**Status**: Draft
+**Deciders**: pelikhan, adr-writer agent
+
+---
+
+### Context
+
+This pull request adds targeted tests for three existing pure functions in `pkg/cli` and `pkg/parser` to increase branch coverage and lock down current behavior around malformed remote URLs, historical grader selection, and OTLP attribute extraction. The PR body frames the work as "PureLock" coverage hardening with deterministic, side-effect-free functions and explicitly calls out before/after coverage improvements, validation steps, and residual risk. Although the changes are test-only, they codify architectural expectations about which helper functions are treated as pure and therefore safe to validate with exhaustive table-driven tests rather than broader integration flows. Because the design gate was triggered by >100 added lines in business-logic directories, the decision needs to be recorded explicitly.
+
+### Decision
+
+We will treat narrowly scoped, side-effect-free helper functions as behavior that should be locked down with targeted unit tests covering success paths, malformed input handling, and defensive branches. For this PR, that means adding focused tests for `selectHistoricalOperationalValueGrader`, `extractHostFromRemoteURL`, and `extractOTLPAttributesFromObsMap` instead of changing their production implementations or relying on broader package-level integration tests. We chose this approach because the PR evidence shows these functions are deterministic, coverage gaps are localized, and the desired outcome is to preserve existing semantics while raising confidence.
+
+### Alternatives Considered
+
+#### Alternative 1: Rely on Existing Integration and Package Tests
+
+Keep the current test suite structure and accept the uncovered branches in these helper functions.
+
+This was considered because it avoids adding over 200 lines of new test code and keeps the test surface smaller. It was not chosen because the PR evidence shows specific uncovered error and fallback paths that are important to preserve, and the existing broader tests do not exercise them directly.
+
+#### Alternative 2: Refactor the Production Functions Before Adding Tests
+
+Change the helper implementations or surrounding abstractions first, then add tests against the refactored shape.
+
+This was considered because refactoring can sometimes simplify testability or reduce edge cases. It was not chosen because the PR's stated goal is to lock down current pure behavior, not to alter the design, and changing production code would introduce a broader decision than the evidence in this PR supports.
+
+### Consequences
+
+#### Positive
+- The repository gains explicit regression protection for edge cases in three pure helper functions without changing production behavior.
+- Coverage improves in targeted areas of `pkg/cli` and `pkg/parser`, making future regressions in fallback and validation logic easier to detect.
+- The tests document which behaviors are considered stable for malformed inputs, duplicate records, missing observations, and OTLP attribute filtering.
+
+#### Negative
+- The codebase takes on additional test maintenance cost, especially if these helpers evolve and many table cases need updating.
+- Locking down current behavior may make future intentional semantic changes noisier because multiple targeted assertions will need coordinated updates.
+- Treating large test-only additions as ADR-worthy may create process overhead when the architectural decision is narrow.
+
+#### Neutral
+- The decision affects test strategy and behavioral documentation more than runtime architecture or user-facing execution semantics.
+- The ADR records purity-based testing as the rationale for this PR, but it does not require every future coverage increase to use the same pattern.
+- No new runtime dependencies or external integrations are introduced; the implementation impact remains within test files under existing packages.
+
+---
+
+*ADR created by [adr-writer agent]. Review and finalize before changing status from Draft to Accepted.*

pkg/cli/git_test.go

@@ -571,6 +571,26 @@ func TestExtractHostFromRemoteURL(t *testing.T) {
 			url:      "https://ghes.example.com:8443/org/repo.git",
 			expected: "ghes.example.com:8443",
 		},
+		{
+			name:     "malformed URL falls back to manual parsing with userinfo and path",
+			url:      "https://baduser%zz@example.com/path",
+			expected: "example.com",
+		},
+		{
+			name:     "malformed URL falls back to manual parsing with userinfo and no path",
+			url:      "https://baduser%zz@example.com",
+			expected: "example.com",
+		},
+		{
+			name:     "malformed URL falls back to manual parsing with no userinfo and no path",
+			url:      "https://%zz",
+			expected: "%zz",
+		},
+		{
+			name:     "SSH scp-like with slash before colon does not match and defaults to github.com",
+			url:      "some/path:notaport",
+			expected: "github.com",
+		},
 	}
 
 	for _, tt := range tests {

pkg/cli/graders_operational_value_regrade_test.go

@@ -252,6 +252,124 @@ func TestParseOperationalValueEvaluatorOutputRejectsFutureEvidence(t *testing.T)
 	}
 }
 
+func TestSelectHistoricalOperationalValueGrader(t *testing.T) {
+	t.Parallel()
+
+	makeManifest := func(entries ...operationalValueGraderManifestEntry) *operationalValueGraderManifest {
+		return &operationalValueGraderManifest{Version: 1, Graders: entries}
+	}
+	makeArtifact := func(results ...graderArtifactResult) *graderResultsArtifact {
+		return &graderResultsArtifact{Version: 1, Results: results}
+	}
+	enabledEntry := operationalValueGraderManifestEntry{ID: "operational-value", Source: "operational-value", Enabled: true}
+	resultWithObservation := graderArtifactResult{ID: "operational-value", Observation: &graderArtifactObservation{}}
+
+	tests := []struct {
+		name        string
+		manifest    *operationalValueGraderManifest
+		artifact    *graderResultsArtifact
+		runID       string
+		wantErr     string
+		wantEntryID string
+	}{
+		{
+			name:     "nil manifest",
+			manifest: nil,
+			artifact: makeArtifact(resultWithObservation),
+			runID:    "123",
+			wantErr:  "run 123 has no grader data",
+		},
+		{
+			name:     "nil artifact",
+			manifest: makeManifest(enabledEntry),
+			artifact: nil,
+			runID:    "123",
+			wantErr:  "run 123 has no grader data",
+		},
+		{
+			name:     "duplicate manifest entries",
+			manifest: makeManifest(enabledEntry, enabledEntry),
+			artifact: makeArtifact(resultWithObservation),
+			runID:    "456",
+			wantErr:  "run 456 grader manifest contains duplicate operational-value graders",
+		},
+		{
+			name:     "duplicate artifact results",
+			manifest: makeManifest(enabledEntry),
+			artifact: makeArtifact(resultWithObservation, resultWithObservation),
+			runID:    "789",
+			wantErr:  "run 789 grader artifact contains duplicate operational-value results",
+		},
+		{
+			name:     "no matching manifest entry",
+			manifest: makeManifest(operationalValueGraderManifestEntry{ID: "other-grader"}),
+			artifact: makeArtifact(resultWithObservation),
+			runID:    "111",
+			wantErr:  "run 111 did not use an enabled operational-value grader",
+		},
+		{
+			name:     "manifest entry not enabled",
+			manifest: makeManifest(operationalValueGraderManifestEntry{ID: "operational-value", Source: "operational-value", Enabled: false}),
+			artifact: makeArtifact(resultWithObservation),
+			runID:    "222",
+			wantErr:  "run 222 did not use an enabled operational-value grader",
+		},
+		{
+			name:     "manifest entry has wrong source",
+			manifest: makeManifest(operationalValueGraderManifestEntry{ID: "operational-value", Source: "other-source", Enabled: true}),
+			artifact: makeArtifact(resultWithObservation),
+			runID:    "333",
+			wantErr:  "run 333 did not use an enabled operational-value grader",
+		},
+		{
+			name:     "no matching artifact result",
+			manifest: makeManifest(enabledEntry),
+			artifact: makeArtifact(graderArtifactResult{ID: "other-result"}),
+			runID:    "444",
+			wantErr:  "run 444 has no replayable operational-value observation",
+		},
+		{
+			name:     "artifact result missing observation",
+			manifest: makeManifest(enabledEntry),
+			artifact: makeArtifact(graderArtifactResult{ID: "operational-value", Observation: nil}),
+			runID:    "555",
+			wantErr:  "run 555 has no replayable operational-value observation",
+		},
+		{
+			name:        "success",
+			manifest:    makeManifest(enabledEntry),
+			artifact:    makeArtifact(resultWithObservation),
+			runID:       "666",
+			wantEntryID: "operational-value",
+		},
+	}
+
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			t.Parallel()
+			entry, result, err := selectHistoricalOperationalValueGrader(tt.manifest, tt.artifact, tt.runID)
+			if tt.wantErr != "" {
+				if err == nil || !strings.Contains(err.Error(), tt.wantErr) {
+					t.Fatalf("expected error containing %q, got %v", tt.wantErr, err)
+				}
+				if entry != nil || result != nil {
+					t.Fatalf("expected nil entry/result on error, got entry=%v result=%v", entry, result)
+				}
+				return
+			}
+			if err != nil {
+				t.Fatalf("unexpected error: %v", err)
+			}
+			if entry == nil || entry.ID != tt.wantEntryID {
+				t.Fatalf("expected entry with ID %q, got %v", tt.wantEntryID, entry)
+			}
+			if result == nil || result.ID != "operational-value" {
+				t.Fatalf("expected result with ID operational-value, got %v", result)
+			}
+		})
+	}
+}
+
 func TestNewGradersCommand(t *testing.T) {
 	t.Parallel()
 	command := NewGradersCommand()

pkg/parser/import_observability_test.go

@@ -0,0 +1,104 @@
+package parser
+
+import (
+	"testing"
+
+	"github.com/stretchr/testify/assert"
+)
+
+// TestExtractOTLPAttributesFromObsMap verifies extractOTLPAttributesFromObsMap
+// correctly extracts string-valued OTLP custom attributes from a raw
+// observability map, and returns nil for absent/malformed/empty input.
+func TestExtractOTLPAttributesFromObsMap(t *testing.T) {
+	t.Parallel()
+
+	tests := []struct {
+		name     string
+		obs      map[string]any
+		expected map[string]string
+	}{
+		{
+			name:     "nil map returns nil",
+			obs:      nil,
+			expected: nil,
+		},
+		{
+			name:     "empty map returns nil",
+			obs:      map[string]any{},
+			expected: nil,
+		},
+		{
+			name:     "missing otlp key returns nil",
+			obs:      map[string]any{"other": "value"},
+			expected: nil,
+		},
+		{
+			name:     "otlp value not a map returns nil",
+			obs:      map[string]any{"otlp": "not-a-map"},
+			expected: nil,
+		},
+		{
+			name:     "otlp map missing attributes returns nil",
+			obs:      map[string]any{"otlp": map[string]any{"other": "value"}},
+			expected: nil,
+		},
+		{
+			name:     "attributes value not a map returns nil",
+			obs:      map[string]any{"otlp": map[string]any{"attributes": "not-a-map"}},
+			expected: nil,
+		},
+		{
+			name:     "empty attributes map returns empty result",
+			obs:      map[string]any{"otlp": map[string]any{"attributes": map[string]any{}}},
+			expected: map[string]string{},
+		},
+		{
+			name: "string attributes are extracted",
+			obs: map[string]any{
+				"otlp": map[string]any{
+					"attributes": map[string]any{
+						"env":     "prod",
+						"service": "gh-aw",
+					},
+				},
+			},
+			expected: map[string]string{"env": "prod", "service": "gh-aw"},
+		},
+		{
+			name: "non-string attribute values are silently ignored",
+			obs: map[string]any{
+				"otlp": map[string]any{
+					"attributes": map[string]any{
+						"env":     "prod",
+						"count":   42,
+						"enabled": true,
+						"nested":  map[string]any{"a": "b"},
+						"list":    []any{"x", "y"},
+						"missing": nil,
+					},
+				},
+			},
+			expected: map[string]string{"env": "prod"},
+		},
+		{
+			name: "empty string key is ignored",
+			obs: map[string]any{
+				"otlp": map[string]any{
+					"attributes": map[string]any{
+						"":     "should-be-ignored",
+						"real": "value",
+					},
+				},
+			},
+			expected: map[string]string{"real": "value"},
+		},
+	}
+
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			t.Parallel()
+			got := extractOTLPAttributesFromObsMap(tt.obs)
+			assert.Equal(t, tt.expected, got)
+		})
+	}
+}