Add formal test predicates P16–P21 for otel-observability-spec - #49809
Conversation
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
|
🧠 Matt Pocock Skills Reviewer has completed the skills-based review. ✅ |
|
✅ Design Decision Gate 🏗️ completed the design decision gate check. |
|
✅ PR Code Quality Reviewer completed the code quality review. |
|
✅ Test Quality Sentinel completed test quality analysis. |
There was a problem hiding this comment.
Pull request overview
Adds six OTLP observability predicates, extending formal coverage from P1–P15 to P1–P21.
Changes:
- Tests resource-attribute validation and independence.
- Tests map merge precedence and nil semantics.
- Adds provisional cardinality and scope-name predicates.
Show a summary per file
| File | Description |
|---|---|
pkg/workflow/otel_observability_formal_test.go |
Adds predicates P16–P21 and test-only stubs. |
Review details
Tip
Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Suppressed comments (2)
pkg/workflow/otel_observability_formal_test.go:434
gh-aw.repositorymust not be classified as a bounded default metric dimension. The observability specification explicitly says repository names MUST NOT be metric dimensions by default (specs/otel-observability-spec.md:784), so this assertion would encode non-conforming behavior.
"gh-aw.repository",
pkg/workflow/otel_observability_formal_test.go:464
- This test resolves only the test-local stub whose methods hardcode the two strings asserted below; no production scope creation implements or uses this interface. As a result, P21 remains green regardless of the scopes actually emitted. Please bind the predicate to the production instrumentation-scope path, or mark it pending until that path exists.
var resolver instrumentationScopeResolver = &staticInstrumentationScopeResolver{}
- Files reviewed: 1/1 changed files
- Comments generated: 1
- Review effort level: Balanced
| var registry metricAttributeRegistry = &staticMetricAttributeRegistry{ | ||
| highCardinalityKeys: map[string]bool{ | ||
| "gh-aw.run.id": true, | ||
| "gh-aw.run.uuid": true, | ||
| "user.id": true, |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Review: P16–21 Formal Test Predicates
P16–P19 are well-structured — they exercise real production functions (validateOTLPResourceAttributes, collectOTLPCustomAttributes, collectOTLPResourceAttributes, mergeOTLPStringMaps) with clear acceptance/rejection cases.
Non-blocking concern for P20 and P21: These tests are tautological stubs. They define staticMetricAttributeRegistry and staticInstrumentationScopeResolver themselves and then assert those stub implementations behave as hardcoded. No production code is exercised. The PR notes this is intentional, but P20/P21 currently provide zero regression protection.
Consider adding t.Skip("stub: replace with real implementation") or a prominent // TODO: to signal incompleteness to future readers.
Overall P16–P19 add genuine value; no blocking issues.
🧵 Reviewed using Impeccable skills by Impeccable Skills Reviewer · sonnet46 · 16.7 AIC · ⌖ 12.1 AIC · ⊞ 5.4K
There was a problem hiding this comment.
Verdict: Request changes — P20/P21 test only self-referential stubs, providing no real regression coverage.
P16-P19 are solid: they exercise real production functions (validateOTLPResourceAttributes, collectOTLPCustomAttributes/collectOTLPResourceAttributes, mergeOTLPStringMaps) with reasonable assertions on precedence and nil-sentinel semantics.
However, P20 (MetricResourceCardinalityBound) and P21 (InstrumentationScopeNaming) introduce brand-new interfaces (metricAttributeRegistry, instrumentationScopeResolver) with test-only stub implementations, then assert the stubs return the exact hardcoded values they were constructed with. There is no production code implementing either interface in pkg/workflow, so these tests can never fail due to a real behavioral regression — they are unfalsifiable by construction and provide false confidence under the same TestFormal_* naming convention used for the genuinely production-backed P1-P19 tests. I'd block merge on this: either skip these two with a tracking-issue reference (t.Skip("pending #issue")), or hold them until real implementations exist to bind against.
💡 Additional notes
- P16's secret/var-reference rejection test also lacks a negative case for benign strings that merely contain the substrings "secrets"/"vars" without being a
${{ }}expression — worth adding to pin down the regex's exact matching semantics given it gates user config. - The
grumpy-codersub-agent's first-pass findings were reviewed; overlapping items were consolidated into the three comments above. A few of its lower-confidence findings (style/blank-line nits, minor comment-consistency notes) were dropped as non-blocking/non-actionable.
🔎 Code quality review by PR Code Quality Reviewer · auto · 34.9 AIC · ⌖ 5.6 AIC · ⊞ 7.9K
Comment /review to run again
| // High-cardinality per-run/per-user identifiers must be excluded from default | ||
| // metric dimensions to prevent unbounded label growth. Stable, bounded | ||
| // attributes such as service.name and workflow.name must be allowed. | ||
| func TestFormal_MetricResourceCardinalityBound(t *testing.T) { |
There was a problem hiding this comment.
This test only verifies a test-local hardcoded stub returns its own hardcoded values — it provides zero regression protection for actual gh-aw metric-cardinality behavior.
💡 Details
staticMetricAttributeRegistry.highCardinalityKeys is populated with the exact same keys the assertions then check. There is no production implementation of metricAttributeRegistry in pkg/workflow — the interface exists solely for this test. So when a real cardinality filter is eventually implemented (with possibly different/buggy classification logic), this test keeps passing forever because it never touches that code. The TestFormal_* naming matches the rest of the suite (P1-P19), which do exercise real production functions (validateOTLPResourceAttributes, mergeOTLPStringMaps), creating false confidence that P20 has equivalent coverage. There is also no tracking-issue reference committing to replace the stub, so it risks becoming permanent dead scaffolding.
Suggested fix: t.Skip("pending #issue — no production cardinality-filter implementation yet"), or hold off adding a TestFormal_*-named test until a real metricAttributeRegistry implementation exists to exercise.
| // 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. | ||
| func TestFormal_InstrumentationScopeNaming(t *testing.T) { |
There was a problem hiding this comment.
Same problem as P20: this asserts a locally-defined stub's hardcoded return values against themselves, exercising no production code at all.
💡 Details
staticInstrumentationScopeResolver.CoreScope()/GatewayScope() are literal constants declared a few lines above in this same file. resolver.CoreScope() returning "gh-aw" is guaranteed by construction — the assertion cannot fail unless someone edits the stub itself. If gh-aw's actual OTel setup ever names its instrumentation scope something other than "gh-aw" (e.g. a typo, a rename, or divergence between core and gateway), this test will not catch it, because instrumentationScopeResolver has no production implementation anywhere in pkg/workflow.
As written, this is a spec/contract placeholder disguised as a TestFormal_* regression test. Recommend t.Skip with a tracking-issue reference, or move the interface/stub out of the test file into a design-doc comment until the real resolver exists.
| "observability": map[string]any{ | ||
| "otlp": map[string]any{ | ||
| "resource-attributes": map[string]any{ | ||
| "deployment.environment": "production", |
There was a problem hiding this comment.
P16 verifies exact ${{ secrets.X }}/${{ vars.X }} patterns but has no negative case proving benign values containing those substrings are accepted, nor coverage for case variants.
💡 Details
Looking at validateOTLPResourceAttributes (uses otlpResourceAttributeSecretRefPattern.MatchString(value)), the test only exercises: nil input, clean literals, and the two canonical secret/var expression forms. It never asserts what happens for values that legitimately mention the words "secrets" or "vars" without being an actual ${{ }} expression (e.g. team.name: "secrets-rotation-squad"), which would validate whether the regex is anchored/scoped correctly and not simply doing a substring match that could produce false-positive rejections on innocuous attribute values. Given this validator gates user configuration and its regex behavior isn't inspected here, add at least one accept-case with a benign string containing "secrets"/"vars" as a substring to pin down the intended matching semantics.
🧪 Test Quality Sentinel Report
📊 Metrics (6 tests)
|
…s P16-P21 Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
There was a problem hiding this comment.
Skills-Based Review 🧠
Applied /tdd — requesting changes on test coverage gaps and tautological stubs.
📋 Key Themes & Highlights
Key Themes
- P16 coverage gap:
env.*expressions are not tested — the production pattern only blockssecrets.*/vars.*, soenv.*acceptance is undocumented and untested. - P18/P19 boundary cases: asymmetric nil/non-empty inputs are untested; these are the cases most likely to regress.
- P20/P21 tautological stubs: both tests confirm the test-only struct returns what it was coded to return — no production behaviour is under test. They pass trivially but could mislead CI readers into thinking the real feature is verified. A
t.Skipguard would communicate their aspirational status clearly.
Positive Highlights
- ✅ P16–P19 exercise real production functions (
validateOTLPResourceAttributes,collectOTLP*,mergeOTLPStringMaps) — solid spec coverage. - ✅ P17 thoroughly documents the disjoint-key invariant with cross-contamination probes.
- ✅ Predicate comments follow the established pattern from P1–P15 consistently.
- ✅ Using stub interfaces for P20/P21 is the right forward-looking design; the concern is only around how CI presents their results.
🧠 Reviewed using Matt Pocock's skills by Matt Pocock Skills Reviewer · sonnet46 · 51.8 AIC · ⌖ 8.4 AIC · ⊞ 7.1K
Comment /matt to run again
| require.Error(t, validateOTLPResourceAttributes(varsData)) | ||
| } | ||
|
|
||
| // P17 — CustomAttributesResourceAttributesIndependent |
There was a problem hiding this comment.
[/tdd] P16 does not cover ${{ env.* }} expressions — the production regex only rejects secrets.* and vars.*, so env.* is silently accepted. Add a case that explicitly documents whether that is intentional or a gap.
💡 Suggested addition
// ${{ env.* }} — document the intent: accepted or should be rejected?
envData := &WorkflowData{
RawFrontmatter: map[string]any{
"observability": map[string]any{
"otlp": map[string]any{
"resource-attributes": map[string]any{
"some.key": "${{ env.SOME_VAR }}",
},
},
},
},
}
assert.NoError(t, validateOTLPResourceAttributes(envData), "env.* is intentionally accepted")@copilot please address this.
| 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{})) |
There was a problem hiding this comment.
[/tdd] P19 only tests nil/empty → nil, but does not assert what mergeOTLPStringMaps returns when one side is nil and the other is non-empty. Add the asymmetric cases to fully nail the nil-as-sentinel contract.
💡 Missing cases
// One-sided non-empty — result should equal the non-empty map (not nil)
result := mergeOTLPStringMaps(map[string]string{"k": "v"}, nil)
assert.NotNil(t, result)
assert.Equal(t, "v", result["k"])
result2 := mergeOTLPStringMaps(nil, map[string]string{"k": "v"})
assert.NotNil(t, result2)
assert.Equal(t, "v", result2["k"])Without these, a regression where a non-empty map is accidentally dropped would not be caught here.
@copilot please address this.
|
|
||
| // 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 |
There was a problem hiding this comment.
[/tdd] P20's staticMetricAttributeRegistry is a tautological stub: the test asserts that the keys it was constructed with are classified as high-cardinality — which is true by construction. The test provides no value until it exercises a real implementation that decides cardinality from the key name/pattern. Consider adding a // TODO(P20): replace with real pkg/workflow impl comment and a t.Skip("stub — no real implementation yet") guard so CI doesn't present this as evidence of a working feature.
💡 Rationale
A stub that only confirms its own hardcoded data is correct cannot catch a real regression. Adding t.Skip makes it clear this predicate is aspirational, and removes the risk of misleading green CI.
@copilot please address this.
| assert.Equal(t, "gh-aw", resolver.CoreScope()) | ||
| assert.Equal(t, "gh-aw-mcpg", resolver.GatewayScope()) | ||
| assert.NotEqual(t, resolver.CoreScope(), resolver.GatewayScope()) | ||
| } |
There was a problem hiding this comment.
[/tdd] Same tautology concern as P20: P21 tests that a hand-written struct returns the strings it was coded to return — there is no production behaviour being verified. The same t.Skip guard pattern recommended for P20 applies here.
@copilot please address this.
| // 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) |
There was a problem hiding this comment.
[/tdd] P18 asserts assert.Len(t, merged, 3) but does not test the case where override-only keys are present and base is nil/empty. Add a case asserting that a nil base returns all override keys (or vice-versa) to fully specify the boundary behaviour.
💡 Suggested addition
// Base-nil: all override keys should appear
result := mergeOTLPStringMaps(nil, map[string]string{"a": "1", "b": "2"})
assert.Equal(t, map[string]string{"a": "1", "b": "2"}, result)@copilot please address this.
🏗️ Design Decision Gate — ADR RequiredThis PR makes significant changes to a core business logic directory ( 📄 Draft ADR committed:
📋 What to do next
Once an ADR is linked in the PR body, this gate will re-run and verify the implementation matches the decision. ❓ Why ADRs Matter
ADRs create a searchable, permanent record of why the codebase looks the way it does. Future contributors (and your future self) will thank you. 📋 Michael Nygard ADR Format ReferenceAn ADR must contain these four sections to be considered complete:
All ADRs are stored in
|
|
@copilot Please continue triage on this PR. Remaining reviewer-visible blockers:
Run details: https://github.com/github/gh-aw/actions/runs/30757146330
|
Removes the tautological test-only stub implementations (staticMetricAttributeRegistry, staticInstrumentationScopeResolver) that made P20/P21 self-fulfilling. Both predicates now immediately call t.Skip with an explanation of what production implementation is required before they can exercise real behavior. Updates ADR-49809 from Draft to Accepted, revising the Decision and Consequences sections to reflect the pending-predicate approach. Co-authored-by: gh-aw-bot <259018956+gh-aw-bot@users.noreply.github.com>
Done. Changes are in commit P20/P21 — tautological stubs removed:
ADR-49809 — updated to Accepted:
Maintainer handoff: When a metric-cardinality filter or instrumentation-scope resolver lands in |
|
@copilot Please continue triage on this PR. Remaining reviewer-visible blockers:
Run details: https://github.com/github/gh-aw/actions/runs/30758413653
|
|
@copilot Please continue triage on this PR. Remaining reviewer-visible blockers:
Branch refresh was requested for this PR. Failed check still visible from candidate scan:
Run details: https://github.com/github/gh-aw/actions/runs/30759528407
|
|
🎉 This pull request is included in a new release. Release: |
Extends the existing OTLP observability formal test suite (
otel_observability_formal_test.go, P1–P15) with 6 additional predicates covering resource-attribute secret rejection, attribute/resource-attribute independence, merge semantics, and forward-looking stubs for metric cardinality and instrumentation scope naming.New predicates
SecretRefResourceAttributeRejected—validateOTLPResourceAttributesmust rejectsecrets.*/vars.*expressions inresource-attributeswhile accepting literal values and nil inputCustomAttributesResourceAttributesIndependent—collectOTLPCustomAttributesandcollectOTLPResourceAttributesread from disjoint frontmatter keys with no cross-contaminationMergePrecedenceBaseWinsOverOverride—mergeOTLPStringMapsgives base priority on collision and unions disjoint keys from both sidesMergeOfEmptyMapsYieldsNil—mergeOTLPStringMapsreturnsnil(not an allocated empty map) when both inputs are nil/empty, preserving the nil-as-sentinel contractMetricResourceCardinalityBound(stub) — definesmetricAttributeRegistryinterface; high-cardinality run/user/session identifiers are excluded from default metric dimensions; stable keys (service.name,gh-aw.workflow.name) are allowedInstrumentationScopeNaming(stub) — definesinstrumentationScopeResolverinterface; core scope ="gh-aw", gateway scope ="gh-aw-mcpg", and the two must remain distinctP16–P19 exercise concrete implementations already in
observability_otlp.go. P20 and P21 ship test-only stub implementations of their respective interfaces, to be replaced once a metrics-cardinality filter and instrumentation-scope resolver land inpkg/workflow.