Always configure OTLP from enterprise default variables and secrets#56353
Conversation
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
|
✅ PR Code Quality Reviewer completed the code quality review.
|
|
✅ Ponytail Reviewer completed successfully!
|
|
✅ Design Decision Gate 🏗️ completed the design decision gate check. See the comment below for the result and any generated ADR draft.
|
|
✅ Test Quality Sentinel completed test quality analysis. Test Quality Sentinel skipped because pre-fetch PR data was unavailable: unable to fetch test file diff
|
|
🧠 Matt Pocock Skills Reviewer has completed the skills-based review. ✅
|
Comment MemoryNote This comment is managed by comment memory.It stores persistent context for this thread in the code block at the top of this comment.
|
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Verdict
No blocking changed-line issues found in this OTLP enterprise-default fallback change.
Themes reviewed
- default OTLP fallback precedence and no-op behavior when defaults are unset
- runtime failure path when endpoint is configured without headers
- early masking / checkout ordering interactions
- MCP gateway env propagation and observability summary gating
- safe-update allowlisting for compiler-injected secret references
I did not find a correctness regression in the changed lines that justifies blocking merge.
🔎 Code quality review by PR Code Quality Reviewer · pi · gpt54 · 34.7 AIC · ⌖ 6.87 AIC · ⊞ 7K
Comment /review to run again
There was a problem hiding this comment.
Ponytail review: two tiny abstractions can be collapsed without losing clarity. net: -24 lines possible.
Generated by ✂️ Ponytail Reviewer for #56353 · codex · mai10 · 12 AIC · ⌖ 0.943 AIC · ⊞ 16.7K
Comment /ponytail to run again
| // GH_AW_OTLP_ALL_HEADERS = all endpoint headers comma-joined (for masking). | ||
| if firstHeaders != "" { | ||
| otlpEnvLines += "\n OTEL_EXPORTER_OTLP_HEADERS: " + firstHeaders | ||
| // buildOTLPHeaderEnvLines returns the workflow env: lines carrying OTLP header values. |
There was a problem hiding this comment.
pkg/workflow/observability_otlp.go:839: yagni: two tiny env-builder helpers with one caller each. Inline their string assembly into injectOTLPConfig.
| // The credentials check then fails fast when the enterprise default OTLP endpoint is | ||
| // configured without the matching credentials secret; it is a no-op when the endpoint | ||
| // variable is unset. | ||
| func (c *Compiler) generateOTLPTelemetrySteps(yaml *strings.Builder, data *WorkflowData) { |
There was a problem hiding this comment.
pkg/workflow/compiler_yaml_checkout.go:22: yagni: generateOTLPTelemetrySteps is a one-call wrapper around three step emissions. Inline those writes in generateInitialAndCheckoutSteps and drop the helper.
…efaults Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
🏗️ Design Decision Gate — ADR RequiredThis PR makes significant changes to core business logic (376 new lines in business logic directories) but does not have a linked Architecture Decision Record (ADR). 📄 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
|
There was a problem hiding this comment.
Pull request overview
Adds enterprise-wide OTLP defaults so compiled workflows inherit telemetry configuration without frontmatter.
Changes:
- Adds default OTLP endpoint/header expressions and runtime credential validation.
- Integrates telemetry steps, tests, CLI configuration, and documentation.
- Regenerates affected workflow lock files.
Show a summary per file
| File | Description |
|---|---|
pkg/workflow/workflow_data.go |
Tracks enterprise-default OTLP usage. |
pkg/workflow/safe_update_enforcement.go |
Registers the internal OTLP secret. |
pkg/workflow/observability_otlp.go |
Implements fallback resolution and env generation. |
pkg/workflow/observability_otlp_test.go |
Tests fallback injection. |
pkg/workflow/observability_otlp_default_credentials_script_test.go |
Tests the credential guard. |
pkg/workflow/observability_job_summary_test.go |
Updates summary expectations. |
pkg/workflow/compilerenv/manager.go |
Defines default OTLP expressions. |
pkg/workflow/compiler_yaml_checkout.go |
Emits telemetry validation steps. |
pkg/workflow/checkout_runtime_order_test.go |
Accounts for injected telemetry steps. |
pkg/cli/env_command.go |
Adds endpoint management to gh aw env. |
docs/src/content/docs/reference/open-telemetry.mdx |
Documents organization-wide defaults. |
docs/src/content/docs/reference/compiler-enterprise-environment-controls.md |
Documents controls and precedence. |
actions/setup/sh/check_otlp_default_credentials.sh |
Validates endpoint/header pairing. |
.github/workflows/smoke-copilot-small.lock.yml |
Regenerates compiled OTLP configuration. |
.github/workflows/smoke-copilot-sdk.lock.yml |
Regenerates compiled OTLP configuration. |
.github/workflows/sighthound-security-scan.lock.yml |
Regenerates compiled OTLP configuration. |
.github/workflows/pr-description-caveman.lock.yml |
Regenerates compiled OTLP configuration. |
.github/workflows/example-failure-category-filter.lock.yml |
Regenerates compiled OTLP configuration. |
.github/workflows/designer-drift-audit.lock.yml |
Regenerates compiled OTLP configuration. |
.github/workflows/daily-trajectory-grader-implementer.lock.yml |
Regenerates compiled OTLP configuration. |
.github/workflows/daily-team-status.lock.yml |
Regenerates compiled OTLP configuration. |
.github/workflows/daily-squid-image-scan.lock.yml |
Regenerates compiled OTLP configuration. |
.github/workflows/daily-max-ai-credits-test.lock.yml |
Regenerates compiled OTLP configuration. |
.github/workflows/daily-github-docs-seo-optimizer.lock.yml |
Regenerates compiled OTLP configuration. |
.github/workflows/daily-byok-ollama-test.lock.yml |
Regenerates compiled OTLP configuration. |
.github/workflows/copilot-centralization-optimizer.lock.yml |
Regenerates compiled OTLP configuration. |
.github/workflows/copilot-centralization-drilldown.lock.yml |
Regenerates compiled OTLP configuration. |
Review details
- Files reviewed: 60/60 changed files
- Comments generated: 3
- Review effort level: Balanced
| if isOTLPDefaultCredentialsCheckNeeded(data) { | ||
| yaml.WriteString(generateOTLPDefaultCredentialsCheckStep()) |
| // workflow-specific when WorkflowID is available. | ||
| // If the user has already defined OTEL_SERVICE_NAME in their env block, | ||
| // we respect their value and skip injection to avoid duplicate key errors. | ||
| otlpEnvLines := " OTEL_EXPORTER_OTLP_ENDPOINT: " + firstEndpoint |
| return []otlpEndpointEntry{{ | ||
| URL: compilerenv.BuildDefaultOTLPEndpointExpression(), | ||
| Headers: compilerenv.BuildDefaultOTLPHeadersExpression(), | ||
| }}, true |
There was a problem hiding this comment.
Skills-Based Review 🧠
Applied /tdd and /codebase-design — overall solid implementation with one important test gap and two minor hardening opportunities.
📋 Key Themes & Highlights
Key Themes
- Missing regression guard: no test asserts
OTLPUsesEnterpriseDefaults == falsewhen explicit frontmatter OTLP is configured — the most important invariant of the precedence contract is not verified. GH_AW_OTLP_IF_MISSINGmask step always emits: the enterprise-default headers expression${{ secrets.GH_AW_DEFAULT_OTLP_HEADERS }}is non-empty at compile time, soisOTLPHeadersPresentis always true and the mask step fires for every workflow. This is correct runtime behaviour (masking an empty resolved value is harmless), but it's surprising enough to warrant a comment.safe_update_enforcement.goomits the variable entry:DefaultOTLPHeadersis registered butDefaultOTLPEndpointis not. Variables don't require the same secrecy treatment, but the surrounding map is documented as covering "infrastructure secrets/vars managed by gh-aw itself", so consistency argues for adding the entry.
Positive Highlights
- ✅
GH_AW_OTLP_IF_MISSING: ignorecorrectly forced for the enterprise-defaults path. - ✅ Expression endpoints skip the firewall allowlist — correct and documented.
- ✅
setup.shcopies allsh/*.shvia glob, so no explicit registration is needed for the new script. - ✅ Shell script test coverage is thorough and uses
filteredEnvisolation. - ✅
safe_update_enforcement.goregisters the secret to avoid spurious safe-update failures. - ✅ Documentation clearly explains the fallback semantics and the unauthenticated-collector limitation.
🧠 Reviewed using Matt Pocock's skills by Matt Pocock Skills Reviewer · copilot · sonnet46 · 105.3 AIC · ⌖ 10.2 AIC · ⊞ 7.6K
Comment /matt to run again
| assert.True(t, wd.OTLPUsesEnterpriseDefaults, "enterprise defaults should be flagged") | ||
| assert.Contains(t, wd.Env, "OTEL_EXPORTER_OTLP_ENDPOINT: ${{ vars.GH_AW_DEFAULT_OTLP_ENDPOINT }}") | ||
| }) | ||
|
|
There was a problem hiding this comment.
[/tdd] Missing regression guard: no test asserts OTLPUsesEnterpriseDefaults == false when explicit frontmatter OTLP is configured.
This is the most important invariant of the precedence contract — frontmatter always wins over enterprise defaults — and it goes unverified. A future refactor of resolveOTLPEndpointEntries could silently break it.
💡 Suggested test case
t.Run("does not use enterprise defaults when OTLP is configured in frontmatter", func(t *testing.T) {
c := newCompiler()
wd := &WorkflowData{
ParsedFrontmatter: &FrontmatterConfig{
Observability: &ObservabilityConfig{
OTLP: &OTLPConfig{Endpoint: "(traces.example.com/redacted)"},
},
},
}
c.injectOTLPConfig(wd)
assert.False(t, wd.OTLPUsesEnterpriseDefaults, "frontmatter endpoint must suppress enterprise defaults")
assert.NotContains(t, wd.Env, "GH_AW_DEFAULT_OTLP_ENDPOINT")
})@copilot please address this.
| "COPILOT_GITHUB_TOKEN": true, | ||
| // Enterprise-wide OTLP exporter credentials injected by injectOTLPConfig when | ||
| // no observability.otlp endpoint is configured in frontmatter. | ||
| compilerenv.DefaultOTLPHeaders: true, |
There was a problem hiding this comment.
[/codebase-design] DefaultOTLPEndpoint (the variable) is not registered here, only DefaultOTLPHeaders (the secret).
The surrounding map is documented as covering all infrastructure env/vars managed by gh-aw. Omitting the endpoint variable creates an asymmetry: if safe-update ever gains variable checking, this entry will need adding under pressure. Registering it now keeps the map complete and consistent.
💡 Suggested addition
// Enterprise-wide OTLP endpoint variable injected by injectOTLPConfig when
// no observability.otlp endpoint is configured in frontmatter.
compilerenv.DefaultOTLPEndpoint: true,
// Enterprise-wide OTLP exporter credentials injected by injectOTLPConfig when
// no observability.otlp endpoint is configured in frontmatter.
compilerenv.DefaultOTLPHeaders: true,@copilot please address this.
| // GH_AW_OTLP_ALL_HEADERS holds all endpoint headers comma-joined (used for masking). | ||
| func buildOTLPHeaderEnvLines(entries []otlpEndpointEntry) string { | ||
| var lines string | ||
| if firstHeaders := entries[0].Headers; firstHeaders != "" { |
There was a problem hiding this comment.
[/codebase-design] The mask step will always be emitted for every workflow, even those where the enterprise-default secret resolves to empty at runtime.
Because entries[0].Headers is always ${{ secrets.GH_AW_DEFAULT_OTLP_HEADERS }} (a non-empty string at compile time) on the enterprise-defaults path, isOTLPHeadersPresent will always return true. Masking an empty string is harmless, but a brief comment here would prevent future readers from wondering whether the mask step is redundant.
💡 Suggested comment
// Note: when usesEnterpriseDefaults is true, entries[0].Headers is always the
// expression "${{ secrets.GH_AW_DEFAULT_OTLP_HEADERS }}", which is non-empty at
// compile time. The mask step therefore always emits; masking an empty resolved
// value at runtime is a no-op.@copilot please address this.
|
I also attempted a branch refresh where allowed. After addressing the above, please run the
|
|
@copilot update docs and specs |
…tials leak Co-authored-by: gh-aw-bot <259018956+gh-aw-bot@users.noreply.github.com>
Addressed the three review threads:
Note: I don't have a |
Co-authored-by: gh-aw-bot <259018956+gh-aw-bot@users.noreply.github.com>
…ized guard + env dedup Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Updated docs and specs to reflect the guard fixes:
|
|
@copilot resolve the merge conflicts on this branch. |
…er-config-otel # Conflicts: # .github/workflows/agentic-token-audit.lock.yml # .github/workflows/agentic-token-optimizer.lock.yml # .github/workflows/agentic-token-trend-audit.lock.yml # .github/workflows/archivx-agentic-workflows-analyzer.lock.yml # .github/workflows/copilot-centralization-drilldown.lock.yml # .github/workflows/copilot-centralization-optimizer.lock.yml # .github/workflows/daily-arxiv-researcher.lock.yml # .github/workflows/daily-byok-ollama-test.lock.yml # .github/workflows/daily-credit-limit-test.lock.yml # .github/workflows/daily-dashboard-language-spec-review.lock.yml # .github/workflows/daily-documentation-diagram.lock.yml # .github/workflows/daily-elixir-credo-snippet-audit.lock.yml # .github/workflows/daily-github-docs-seo-optimizer.lock.yml # .github/workflows/daily-go-test-parallelizer.lock.yml # .github/workflows/daily-graft-intelligence.lock.yml # .github/workflows/daily-max-ai-credits-test.lock.yml # .github/workflows/daily-safeoutputs-git-simulator.lock.yml # .github/workflows/daily-spending-forecast.lock.yml # .github/workflows/daily-squid-image-scan.lock.yml # .github/workflows/daily-team-status.lock.yml # .github/workflows/daily-trajectory-grader-implementer.lock.yml # .github/workflows/daily-windows-terminal-integration-builder.lock.yml # .github/workflows/designer-drift-audit.lock.yml # .github/workflows/eslint-miner.lock.yml # .github/workflows/evoskill-evolver.lock.yml # .github/workflows/example-failure-category-filter.lock.yml # .github/workflows/objective-impact-report.lock.yml # .github/workflows/portfolio-analyst.lock.yml # .github/workflows/pr-description-caveman.lock.yml # .github/workflows/ruflo-backed-task.lock.yml # .github/workflows/sighthound-security-scan.lock.yml # .github/workflows/smoke-aider.lock.yml # .github/workflows/smoke-claude-on-copilot.lock.yml # .github/workflows/smoke-copilot-auto.lock.yml # .github/workflows/smoke-copilot-mai.lock.yml # .github/workflows/smoke-copilot-sdk.lock.yml # .github/workflows/smoke-copilot-small.lock.yml # .github/workflows/smoke-copilot-sub-agents.lock.yml # .github/workflows/smoke-drive.lock.yml # .github/workflows/smoke-github-claude.lock.yml # .github/workflows/smoke-pydantic.lock.yml # .github/workflows/squad-game-planner.lock.yml # .github/workflows/squad-implement-worker.lock.yml # .github/workflows/squad-plan.lock.yml # .github/workflows/squad.lock.yml # .github/workflows/weekly-network-domains-audit.lock.yml Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Resolved in 092fca9. Unshallowed the repo, merged |
OTLP telemetry previously had to be opted into per workflow via
observability.otlpfrontmatter (typically through a shared import). This makes the compiler always emit OTEL configuration, falling back to org/enterprise-level Actions defaults so telemetry can be configured once and inherited by every agentic workflow.Two new default env entries, reusing the existing
compilerenvdefault-env infrastructure:GH_AW_DEFAULT_OTLP_ENDPOINTGH_AW_DEFAULT_OTLP_HEADERSA workflow with no OTLP frontmatter now compiles to:
Semantics
observability.otlpendpoint from frontmatter or an import wins; precedence is unchanged for workflows already configuring OTLP.GH_AW_OTLP_IF_MISSING: ignore, so unresolved expressions collapse to empty strings and the existing runtime parsers drop endpoints with an empty URL.actions/setup/sh/check_otlp_default_credentials.sh) rather than a stepif:, since thesecretscontext isn't available in step-level expressions.Changes
pkg/workflow/compilerenv/manager.go— constants and expression builders for the default endpoint/headers.pkg/workflow/observability_otlp.go—resolveOTLPEndpointEntries()holds the fallback decision; env-line construction split intobuildOTLPHeaderEnvLines()/buildOTLPPayloadEnvLines(); new credentials-check step generator.pkg/workflow/compiler_yaml_checkout.go— emits the OTLP telemetry steps (mask + credentials check) in the agent job.pkg/workflow/safe_update_enforcement.go—GH_AW_DEFAULT_OTLP_HEADERSregistered as compiler-internal so safe-update doesn't flag every compile as introducing a new secret.pkg/cli/env_command.go—default_otlp_endpointbinding forgh aw env.open-telemetry.mdx; table rows and precedence notes incompiler-enterprise-environment-controls.md..lock.ymlfiles regenerated.Review notes
mask_otlp_headers.shstep.network.allowedthemselves. This is documented but worth a look.Run: https://github.com/github/gh-aw/actions/runs/33081464762