Skip to content

chore: normalize otel auth#32864

Merged
pelikhan merged 16 commits into
mainfrom
simplify-otel
May 17, 2026
Merged

chore: normalize otel auth#32864
pelikhan merged 16 commits into
mainfrom
simplify-otel

Conversation

@mnkiefer
Copy link
Copy Markdown
Collaborator

@mnkiefer mnkiefer commented May 17, 2026

  • Add automatic rewriting of the Authorization header to x-sentry-auth for Sentry endpoints, ensuring correct authentication, while leaving the header unchanged for Grafana and other backends.
  • Remove the now-unnecessary shared/otel.md import and update documentation and tests to reflect the new behavior.

@mnkiefer mnkiefer self-assigned this May 17, 2026
@mnkiefer mnkiefer marked this pull request as ready for review May 17, 2026 17:47
Copilot AI review requested due to automatic review settings May 17, 2026 17:47
@mnkiefer
Copy link
Copy Markdown
Collaborator Author

@copilot Recompile workflows

@github-actions

This comment has been minimized.

@github-actions

This comment has been minimized.

@github-actions
Copy link
Copy Markdown
Contributor

🧪 Test Quality Sentinel Report

Test Quality Score: 100/100

Excellent — all new tests enforce behavioral contracts with error/edge case coverage.

Metric Value
New/modified tests analyzed 5
✅ Design tests (behavioral contracts) 5 (100%)
⚠️ Implementation tests (low value) 0 (0%)
Tests with error/edge cases 5 (100%)
Duplicate test clusters 0
Test inflation detected No (1.48:1 ratio)
🚨 Coding-guideline violations None

Test Classification Details

View all 5 tests
Test File Classification Issues Detected
TestNormalizeOTLPHeadersForEndpoint / rewrites Authorization header for sentry URL pkg/workflow/observability_otlp_test.go:865 ✅ Design None
TestNormalizeOTLPHeadersForEndpoint / rewrites Authorization header for sentry endpoint expression pkg/workflow/observability_otlp_test.go:874 ✅ Design None
TestNormalizeOTLPHeadersForEndpoint / preserves Authorization header for grafana endpoint pkg/workflow/observability_otlp_test.go:883 ✅ Design None
TestCollectAllOTLPEndpoints / array form: sentry endpoint rewrites Authorization while grafana keeps it pkg/workflow/observability_otlp_test.go:1099 ✅ Design None
TestInjectOTLPConfig_MultipleEndpoints / rewrites sentry auth header without changing grafana auth header pkg/workflow/observability_otlp_test.go:1302 ✅ Design None

Language Support

Tests analyzed:

  • 🐹 Go (*_test.go): 5 tests — unit (//go:build !integration)

Verdict

Check passed. 0% of new tests are implementation tests (threshold: 30%). All new tests directly verify observable behavior of the normalizeOTLPHeadersForEndpoint function and the end-to-end injectOTLPConfig pipeline.


📖 Understanding Test Classifications

Design Tests (High Value) verify what the system does:

  • Assert on observable outputs, return values, or state changes
  • Cover error paths and boundary conditions
  • Would catch a behavioral regression if deleted
  • Remain valid even after internal refactoring

Implementation Tests (Low Value) verify how the system does it:

  • Assert on internal function calls (mocking internals)
  • Only test the happy path with typical inputs
  • Break during legitimate refactoring even when behavior is correct
  • Give false assurance: they pass even when the system is wrong

Goal: Shift toward tests that describe the system's behavioral contract — the promises it makes to its users and collaborators.

🧪 Test quality analysis by Test Quality Sentinel · ● 5.6M ·

Copy link
Copy Markdown
Contributor

@github-actions github-actions Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

✅ Test Quality Sentinel: 100/100. Test quality is excellent — 0% of new tests are implementation tests (threshold: 30%). All 5 new tests are behavioral design tests with error/edge case coverage.

Co-authored-by: mnkiefer <8320933+mnkiefer@users.noreply.github.com>
Copy link
Copy Markdown
Contributor

@github-actions github-actions Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🧠 Reviewed using Matt Pocock's skills by Matt Pocock Skills Reviewer · ● 4.8M

Comment thread pkg/workflow/observability_otlp_test.go
Comment thread pkg/workflow/observability_otlp.go
Comment thread pkg/workflow/observability_otlp.go
Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR normalizes OTLP auth handling so Sentry endpoints use x-sentry-auth while other backends continue using Authorization.

Changes:

  • Adds endpoint-aware OTLP header normalization and Sentry header rewriting.
  • Updates OTLP tests and workflow documentation for the new auth secret names.
  • Removes direct shared/otel.md imports in favor of shared/otlp.md.
Show a summary per file
File Description
pkg/workflow/observability_otlp.go Adds endpoint-aware OTLP header normalization and Sentry rewrite detection.
pkg/workflow/observability_otlp_test.go Adds tests for Sentry/Grafana header behavior and multi-endpoint injection.
.github/workflows/smoke-otel-backends.md Updates required OTEL secret names.
.github/workflows/shared/otlp.md Documents Sentry/Grafana auth secret usage.
.github/workflows/shared/otel.md Removes the legacy wrapper import.
.github/workflows/daily-skill-optimizer.md Removes redundant shared/otel.md import.
.github/workflows/daily-model-inventory.md Removes redundant shared/otel.md import.
.github/workflows/daily-hippo-learn.md Removes redundant shared/otel.md import.
.github/workflows/daily-caveman-optimizer.md Removes redundant shared/otel.md import.
.github/workflows/daily-cache-strategy-analyzer.md Removes redundant shared/otel.md import.
.github/workflows/daily-aw-cross-repo-compile-check.md Removes redundant shared/otel.md import.
.github/workflows/daily-astrostylelite-markdown-spellcheck.md Removes redundant shared/otel.md import.

Copilot's findings

Tip

Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comments suppressed due to low confidence (1)

.github/workflows/shared/otel.md:1

  • Deleting this runtime import target leaves several checked-in lock workflows still referencing .github/workflows/shared/otel.md (for example daily-skill-optimizer.lock.yml:251 and daily-model-inventory.lock.yml:254). Runtime imports are non-optional and processRuntimeImport throws when the file is missing, so those workflows will fail until the corresponding .lock.yml files are regenerated or the compatibility shim is kept.
  • Files reviewed: 241/241 changed files
  • Comments generated: 4

Comment thread pkg/workflow/observability_otlp.go Outdated
Comment thread .github/workflows/shared/otlp.md Outdated
Comment thread pkg/workflow/observability_otlp_test.go
Comment thread .github/workflows/smoke-otel-backends.md
…n header rewrite

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
@github-actions
Copy link
Copy Markdown
Contributor

Commit pushed: 9c6b769

🏗️ ADR gate enforced by Design Decision Gate 🏗️ · ● 5.4M

@github-actions
Copy link
Copy Markdown
Contributor

🏗️ Design Decision Gate — ADR Required

This PR makes significant changes to core business logic (+124 net additions in pkg/workflow/ and shared OTLP plumbing) but does not have a linked Architecture Decision Record (ADR).

AI has analyzed the PR diff and generated a draft ADR to help you get started:

📄 Draft ADR: docs/adr/32864-endpoint-aware-sentry-authorization-header-rewrite.md

The decision identified from the diff is: endpoint-aware rewrite of the Authorization header to x-sentry-auth for Sentry OTLP endpoints, with substring-based detection on the endpoint URL (covering both resolved hostnames and ${{ secrets.* }} expressions), plus removal of shared/otel.md in favor of a single shared/otlp.md import.

What to do next

  1. Review the draft ADR committed to your branch — it was generated from the PR diff and follows the Michael Nygard template (Context, Decision, Alternatives, Consequences) with a normative RFC 2119 section.
  2. Complete the missing sections — verify the rationale, refine the alternatives, and confirm the listed consequences. In particular, double-check the Negative consequences around the substring-sentry heuristic — they are real trade-offs that future readers should know about.
  3. Commit the finalized ADR to docs/adr/ on your branch.
  4. Reference the ADR in this PR body by adding a line such as:

    ADR: ADR-32864: Endpoint-Aware Sentry Authorization Header Rewrite

Once an ADR is linked in the PR body, this gate will re-run and verify the implementation matches the decision.

Why ADRs Matter

"AI made me procrastinate on key design decisions. Because refactoring was cheap, I could always say 'I'll deal with this later.' Deferring decisions corroded my ability to think clearly."

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 Reference

An ADR must contain these four sections to be considered complete:

  • Context — What is the problem? What forces are at play?
  • Decision — What did you decide? Why?
  • Alternatives Considered — What else could have been done?
  • Consequences — What are the trade-offs (positive and negative)?

All ADRs are stored in docs/adr/ as Markdown files numbered by PR number (e.g., 32864-*.md for PR #32864).

References:

🔒 This PR cannot merge until an ADR is linked in the PR body.

🏗️ ADR gate enforced by Design Decision Gate 🏗️ · ● 5.4M ·

Copy link
Copy Markdown
Contributor

Copilot AI commented May 17, 2026

@copilot Recompile workflows

Addressed in 52b56ff.

Co-authored-by: mnkiefer <8320933+mnkiefer@users.noreply.github.com>
@mnkiefer
Copy link
Copy Markdown
Collaborator Author

@copilot Address github-actions review comments.

@github-actions
Copy link
Copy Markdown
Contributor

@copilot review all comments and address the unresolved review feedback.
Please refresh the branch and rerun checks so this can move forward.

Generated by 👨‍🍳 PR Sous Chef ·

Copilot AI and others added 5 commits May 17, 2026 18:15
Co-authored-by: mnkiefer <8320933+mnkiefer@users.noreply.github.com>
Co-authored-by: mnkiefer <8320933+mnkiefer@users.noreply.github.com>
Co-authored-by: mnkiefer <8320933+mnkiefer@users.noreply.github.com>
Co-authored-by: mnkiefer <8320933+mnkiefer@users.noreply.github.com>
Co-authored-by: mnkiefer <8320933+mnkiefer@users.noreply.github.com>
Copilot AI and others added 4 commits May 17, 2026 18:21
Co-authored-by: mnkiefer <8320933+mnkiefer@users.noreply.github.com>
Co-authored-by: mnkiefer <8320933+mnkiefer@users.noreply.github.com>
Co-authored-by: mnkiefer <8320933+mnkiefer@users.noreply.github.com>
@mnkiefer
Copy link
Copy Markdown
Collaborator Author

@copilot Recompile workflows

Co-authored-by: mnkiefer <8320933+mnkiefer@users.noreply.github.com>
Copy link
Copy Markdown
Contributor

Copilot AI commented May 17, 2026

@copilot Address github-actions review comments.

Fixed in 5625e55.

@github-actions

This comment has been minimized.

Co-authored-by: mnkiefer <8320933+mnkiefer@users.noreply.github.com>
Copy link
Copy Markdown
Contributor

Copilot AI commented May 17, 2026

@copilot Recompile workflows

Re-ran gh aw compile; it produced no diff, so the lock files are already up to date on 620896d.

@github-actions
Copy link
Copy Markdown
Contributor

✅ smoke-ci: safeoutputs CLI comment + comment-memory run (25999340882)

Generated by 🧪 Smoke CI for issue #32864 ·

@pelikhan pelikhan merged commit 2784e93 into main May 17, 2026
39 checks passed
@pelikhan pelikhan deleted the simplify-otel branch May 17, 2026 19:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants