Add formal test predicates P16–P21 for otel-observability-spec

docs/adr/49809-formalize-otel-observability-spec.md

@@ -0,0 +1,47 @@
+# ADR-49809: Formalize OTLP Observability Spec with Executable Predicates P16–P21
+
+**Date**: 2026-08-02
+**Status**: Accepted
+
+---
+
+### Context
+
+The `gh-aw` workflow runtime has an OTLP observability integration (`pkg/workflow/observability_otlp.go`) that enforces several security-critical and semantic invariants: resource-attribute values must not reference `secrets.*` or `vars.*` expressions, span attributes and resource attributes are parsed from disjoint frontmatter keys, and the `mergeOTLPStringMaps` utility follows base-wins precedence with a nil-as-sentinel contract for empty inputs. Additionally, the system is expected to enforce metric cardinality bounds and distinct instrumentation scope names in future work. These invariants were previously verified only through informal tests or not at all, making it difficult to audit, communicate, or regress-test the spec. An existing formal predicate convention (P1–P15) in `otel_observability_formal_test.go` already establishes a numbered, property-named test style that serves as a machine-verifiable living specification.
+
+### Decision
+
+We will extend the formal OTLP observability test suite by adding predicates P16–P21 to `pkg/workflow/otel_observability_formal_test.go`. P16–P19 test concrete behaviors already present in `observability_otlp.go` (secret-ref rejection, attribute independence, merge precedence, nil-as-sentinel). P20 and P21 are forward-looking pending predicates: each test function immediately calls `t.Skip("pending: ...")` with an explicit message explaining what production implementation is required before they can exercise real behavior. The stub interface types (`metricAttributeRegistry`, `instrumentationScopeResolver`) that were previously defined alongside test-only implementations have been removed; the expected contract is instead described in the pending-test comment. This avoids the tautological anti-pattern of a test that constructs its own oracle and validates against that same oracle. When production implementations of the cardinality filter and instrumentation-scope resolver land in `pkg/workflow`, the `t.Skip` calls will be replaced with assertions against those implementations.
+
+### Alternatives Considered
+
+#### Alternative 1: Informal Go unit tests without predicate naming
+
+Add standard `TestXxx` functions without the `P16`-style numbering convention. This was rejected because the numbered predicate convention ties directly to the prose specification (`specs/otel-observability-spec.md`) and makes it easy to cross-reference which behaviors are covered. Informal tests provide the same runtime coverage but lose the self-documenting, spec-linked quality.
+
+#### Alternative 2: Documentation-only approach in `specs/otel-observability-spec.md`
+
+Write the invariants as prose in the specification document without adding executable tests. This was rejected because documentation drifts from implementation over time; executable predicates remain accurate by construction — if the implementation drifts, the test fails. A prose-only spec cannot prevent regressions.
+
+#### Alternative 3: Use Go's built-in fuzzing (`go test -fuzz`) for attribute validation
+
+Apply fuzz testing to `validateOTLPResourceAttributes` and `mergeOTLPStringMaps` to explore the input space rather than using fixed test cases. This was not chosen because the existing P1–P15 suite uses deterministic scenario-based tests, and consistency with the established testing pattern was preferred. Fuzzing could complement the formal predicates in a future iteration.
+
+### Consequences
+
+#### Positive
+- P16–P19 are backed by concrete implementations; regressions in security-critical behaviors (secret rejection, attribute isolation, merge semantics) will be caught immediately by CI.
+- P20 and P21 establish interface contracts (`metricAttributeRegistry`, `instrumentationScopeResolver`) before the production implementations exist, enabling interface-first design and reducing future coupling.
+- The numbered predicate style makes it straightforward to audit which invariants from `specs/otel-observability-spec.md` are machine-verified vs. not yet covered.
+
+#### Negative
+- P20 and P21 are currently skipped (`t.Skip("pending: ...")`); they do not exercise real production code until the cardinality filter and instrumentation-scope resolver are implemented.
+- When those implementations land, contributors must remove the `t.Skip` calls and supply real assertions; this is enforced only by code-review discipline, not by tooling.
+
+#### Neutral
+- The formal test suite now spans P1–P21 across a single file; continued growth will require either continued sequential numbering or a decision to split the file by concern.
+- Future contributors must follow the P-numbered predicate naming convention when adding invariants; this convention is not enforced by tooling, only by code-review discipline.
+
+---
+
+*ADR created by [adr-writer agent], finalized in [PR #49809](https://github.com/github/gh-aw/pull/49809).*

pkg/workflow/otel_observability_formal_test.go

@@ -243,3 +243,169 @@ func TestFormal_AbsentObservabilityProducesNoEndpoints(t *testing.T) {
 	assert.Empty(t, collectAllOTLPEndpoints(map[string]any{}))
 	assert.Empty(t, collectAllOTLPEndpoints(map[string]any{"observability": nil}))
 }
+
+// P16 — SecretRefResourceAttributeRejected
+// validateOTLPResourceAttributes must reject any resource-attribute value that
+// references secrets.* or vars.* and must accept literal string values and nil
+// workflow data without error.
+func TestFormal_SecretRefResourceAttributeRejected(t *testing.T) {
+	// Nil workflow data is safe — no attributes to reject.
+	assert.NoError(t, validateOTLPResourceAttributes(nil))
+
+	// Literal string values are accepted.
+	literalData := &WorkflowData{
+		RawFrontmatter: map[string]any{
+			"observability": map[string]any{
+				"otlp": map[string]any{
+					"resource-attributes": map[string]any{
+						"deployment.environment": "production",
+						"team.name":              "platform",
+					},
+				},
+			},
+		},
+	}
+	assert.NoError(t, validateOTLPResourceAttributes(literalData))
+
+	// References to secrets.* must be rejected.
+	secretsData := &WorkflowData{
+		RawFrontmatter: map[string]any{
+			"observability": map[string]any{
+				"otlp": map[string]any{
+					"resource-attributes": map[string]any{
+						"auth.token": "${{ secrets.MY_TOKEN }}",
+					},
+				},
+			},
+		},
+	}
+	require.Error(t, validateOTLPResourceAttributes(secretsData))
+
+	// References to vars.* must also be rejected.
+	varsData := &WorkflowData{
+		RawFrontmatter: map[string]any{
+			"observability": map[string]any{
+				"otlp": map[string]any{
+					"resource-attributes": map[string]any{
+						"config.value": "${{ vars.SOME_VAR }}",
+					},
+				},
+			},
+		},
+	}
+	require.Error(t, validateOTLPResourceAttributes(varsData))
+}
+
+// P17 — CustomAttributesResourceAttributesIndependent
+// collectOTLPCustomAttributes and collectOTLPResourceAttributes must read from
+// their respective `otlp.attributes` and `otlp.resource-attributes` keys
+// independently; writing to one must not affect the other.
+func TestFormal_CustomAttributesResourceAttributesIndependent(t *testing.T) {
+	frontmatter := map[string]any{
+		"observability": map[string]any{
+			"otlp": map[string]any{
+				"attributes": map[string]any{
+					"span.key": "span-value",
+				},
+				"resource-attributes": map[string]any{
+					"resource.key": "resource-value",
+				},
+			},
+		},
+	}
+
+	customAttrs := collectOTLPCustomAttributes(frontmatter)
+	resourceAttrs := collectOTLPResourceAttributes(frontmatter)
+
+	// Each collector returns only its own entries.
+	require.Len(t, customAttrs, 1)
+	assert.Equal(t, "span-value", customAttrs["span.key"])
+	assert.NotContains(t, customAttrs, "resource.key")
+
+	require.Len(t, resourceAttrs, 1)
+	assert.Equal(t, "resource-value", resourceAttrs["resource.key"])
+	assert.NotContains(t, resourceAttrs, "span.key")
+
+	// Absent sibling does not bleed into the other field.
+	onlyCustom := map[string]any{
+		"observability": map[string]any{
+			"otlp": map[string]any{
+				"attributes": map[string]any{"k": "v"},
+			},
+		},
+	}
+	assert.Nil(t, collectOTLPResourceAttributes(onlyCustom))
+
+	onlyResource := map[string]any{
+		"observability": map[string]any{
+			"otlp": map[string]any{
+				"resource-attributes": map[string]any{"k": "v"},
+			},
+		},
+	}
+	assert.Nil(t, collectOTLPCustomAttributes(onlyResource))
+}
+
+// P18 — MergePrecedenceBaseWinsOverOverride
+// mergeOTLPStringMaps must give base values priority over override values when
+// the same key exists in both maps. Disjoint keys from both maps must appear in
+// the result.
+func TestFormal_MergePrecedenceBaseWinsOverOverride(t *testing.T) {
+	base := map[string]string{
+		"shared.key":    "base-value",
+		"base-only.key": "base-only-value",
+	}
+	override := map[string]string{
+		"shared.key":        "override-value",
+		"override-only.key": "override-only-value",
+	}
+
+	merged := mergeOTLPStringMaps(base, override)
+
+	// Base wins on collision.
+	assert.Equal(t, "base-value", merged["shared.key"])
+	// Disjoint keys from both sides are present.
+	assert.Equal(t, "base-only-value", merged["base-only.key"])
+	assert.Equal(t, "override-only-value", merged["override-only.key"])
+	assert.Len(t, merged, 3)
+}
+
+// P19 — MergeOfEmptyMapsYieldsNil
+// mergeOTLPStringMaps must return nil (not an allocated empty map) when both
+// inputs are nil or empty, so callers can rely on nil as a sentinel for
+// "no attributes configured".
+func TestFormal_MergeOfEmptyMapsYieldsNil(t *testing.T) {
+	assert.Nil(t, mergeOTLPStringMaps(nil, nil))
+	assert.Nil(t, mergeOTLPStringMaps(map[string]string{}, nil))
+	assert.Nil(t, mergeOTLPStringMaps(nil, map[string]string{}))
+	assert.Nil(t, mergeOTLPStringMaps(map[string]string{}, map[string]string{}))
+}
+
+// P20 — MetricResourceCardinalityBound
+// High-cardinality per-run/per-user identifiers (gh-aw.run.id, gh-aw.run.uuid,
+// user.id, session.id, trace.id, job.id, span.id, git.commit.sha, pr.number,
+// issue.number, actor.id, url, conversation.id) must be excluded from default
+// metric dimensions to prevent unbounded label growth. Stable, bounded
+// attributes such as service.name and gh-aw.workflow.name must be allowed.
+//
+// Pending: no production metric-cardinality filter exists in pkg/workflow yet.
+// Wire this predicate to the real filter once
+// https://github.com/github/gh-aw/issues is addressed.
+// See also specs/otel-observability-spec.md §metric-cardinality and ADR-49809.
+func TestFormal_MetricResourceCardinalityBound(t *testing.T) {
+	t.Skip("pending: no production metricAttributeRegistry implementation in pkg/workflow; " +
+		"replace t.Skip with assertions against the real filter once it lands")
+}
+
+// P21 — InstrumentationScopeNaming
+// The core instrumentation scope must be "gh-aw" and the MCP gateway scope
+// must be "gh-aw-mcpg". The two scopes must be distinct so traces from each
+// component can be filtered independently.
+//
+// Pending: no production instrumentation-scope resolver exists in pkg/workflow yet.
+// Wire this predicate to the real resolver once it is implemented.
+// See specs/otel-observability-spec.md §instrumentation-scope and ADR-49809.
+func TestFormal_InstrumentationScopeNaming(t *testing.T) {
+	t.Skip("pending: no production instrumentationScopeResolver implementation in pkg/workflow; " +
+		"replace t.Skip with assertions against the real resolver once it lands")
+}