Skip to content

Deduplicate OIDC getToken() across token providers via base-class implementation#3881

Merged
lpcox merged 2 commits into
mainfrom
copilot/fix-duplicate-gettoken-methods
May 27, 2026
Merged

Deduplicate OIDC getToken() across token providers via base-class implementation#3881
lpcox merged 2 commits into
mainfrom
copilot/fix-duplicate-gettoken-methods

Conversation

Copy link
Copy Markdown
Contributor

Copilot AI commented May 26, 2026

Three BaseOidcTokenProvider subclasses carried identical getToken() logic in the token-serving path, creating drift risk for refresh/cache behavior changes. This PR moves that logic into the base class and removes subclass overrides.

  • Shared token retrieval in base class

    • Added getToken() to containers/api-proxy/oidc-token-provider-base.js.
    • Implementation uses _getCachedValue() + _expiresAt, and triggers immediate refresh when stale and no refresh is in flight.
  • Removed duplicated subclass methods

    • Deleted getToken() overrides from:
      • containers/api-proxy/anthropic-oidc-token-provider.js
      • containers/api-proxy/gcp-oidc-token-provider.js
      • containers/api-proxy/oidc-token-provider.js (Azure)
  • Focused base behavior coverage

    • Added containers/api-proxy/oidc-token-provider-base.test.js to validate:
      • cached value returned when unexpired
      • null returned + refresh scheduled when expired
      • no duplicate refresh scheduling when one is already in flight
// containers/api-proxy/oidc-token-provider-base.js
getToken() {
  const now = Math.floor(Date.now() / 1000);
  const cached = this._getCachedValue();
  if (cached && this._expiresAt > now) return cached;
  if (!this._refreshInFlight) this._scheduleRefresh(0);
  return null;
}

Copilot AI changed the title [WIP] Refactor duplicate getToken() method in OIDC token providers Deduplicate OIDC getToken() across token providers via base-class implementation May 26, 2026
Copilot finished work on behalf of lpcox May 26, 2026 22:30
Copilot AI requested a review from lpcox May 26, 2026 22:30
@lpcox lpcox marked this pull request as ready for review May 26, 2026 22:41
Copilot AI review requested due to automatic review settings May 26, 2026 22:41
@github-actions
Copy link
Copy Markdown
Contributor

✅ Coverage Check Passed

Overall Coverage

Metric Base PR Delta
Lines 96.57% 96.62% 📈 +0.05%
Statements 96.44% 96.48% 📈 +0.04%
Functions 98.22% 98.22% ➡️ +0.00%
Branches 90.66% 90.70% 📈 +0.04%
📁 Per-file Coverage Changes (1 files)
File Lines (Before → After) Statements (Before → After)
src/config-writer.ts 89.3% → 90.9% (+1.65%) 89.3% → 90.9% (+1.65%)

Coverage comparison generated by scripts/ci/compare-coverage.ts

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

Deduplicates the identical getToken() implementations across Azure, GCP, and Anthropic OIDC token providers by hoisting the logic into BaseOidcTokenProvider, and adds focused base-class tests.

Changes:

  • Add getToken() to BaseOidcTokenProvider using the existing _getCachedValue()/_expiresAt abstraction.
  • Remove the duplicated getToken() overrides from Azure, GCP, and Anthropic providers.
  • Add oidc-token-provider-base.test.js covering cached, expired, and in-flight refresh cases.
Show a summary per file
File Description
containers/api-proxy/oidc-token-provider-base.js New shared getToken() implementation on the base class.
containers/api-proxy/oidc-token-provider.js Removes Azure subclass getToken() override.
containers/api-proxy/gcp-oidc-token-provider.js Removes GCP subclass getToken() override.
containers/api-proxy/anthropic-oidc-token-provider.js Removes Anthropic subclass getToken() override.
containers/api-proxy/oidc-token-provider-base.test.js Adds base-class tests for cached/expired/in-flight behavior.

Copilot's findings

Tip

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

  • Files reviewed: 5/5 changed files
  • Comments generated: 0

@github-actions
Copy link
Copy Markdown
Contributor

Smoke Test: Claude Engine

  • ✅ GitHub API (recent PRs): PASS
  • ✅ GitHub check (playwright): PASS
  • ✅ File verify: PASS

Result: PASS

💥 [THE END] — Illustrated by Smoke Claude

@github-actions
Copy link
Copy Markdown
Contributor

🔥 Smoke Test: Copilot BYOK (Offline) Mode

Test Result
GitHub MCP (list PRs) ✅ PR #3852 retrieved successfully
GitHub.com connectivity ⚠️ Pre-step vars unexpanded (template not evaluated)
File write/read ⚠️ Pre-step vars unexpanded (template not evaluated)
BYOK inference (api-proxy → api.githubcopilot.com) ✅ Responding via BYOK path

Running in BYOK offline mode (COPILOT_OFFLINE=true) via api-proxy → api.githubcopilot.com.

PR author: @Copilot — Assignees: @lpcox, @Copilot

Overall: PARTIAL — BYOK inference ✅, pre-step smoke data vars unexpanded (workflow template issue)

🔑 BYOK report filed by Smoke Copilot BYOK

@github-actions
Copy link
Copy Markdown
Contributor

🔬 Smoke Test Results

Test Status
GitHub MCP connectivity
GitHub.com HTTP ⚠️ (pre-step data unavailable — template vars unexpanded)
File write/read ⚠️ (pre-step data unavailable — template vars unexpanded)

PR: Deduplicate OIDC getToken() across token providers via base-class implementation
Author: @Copilot | Assignees: @lpcox, @Copilot

Overall: PARTIAL — MCP ✅, but steps.smoke-data.outputs.* variables were not expanded in the workflow prompt, so HTTP and file tests could not be verified.

📰 BREAKING: Report filed by Smoke Copilot

@github-actions
Copy link
Copy Markdown
Contributor

Smoke Test: API Proxy OpenTelemetry Tracing

Scenario Result Notes
✅ Module Loading Pass otel.js loads successfully; exports: startRequestSpan, setTokenAttributes, endSpan, endSpanError, shutdown, isEnabled + internal helpers
✅ Test Suite Pass 33/33 tests passed in otel.test.js (span creation, token attributes, OTLP export, file fallback, error handling)
✅ Env Var Forwarding Pass api-proxy-service.ts forwards OTEL_EXPORTER_OTLP_ENDPOINT, OTEL_EXPORTER_OTLP_HEADERS, GITHUB_AW_OTEL_TRACE_ID, GITHUB_AW_OTEL_PARENT_SPAN_ID, OTEL_SERVICE_NAME
✅ Token Tracker Integration Pass onUsage callback exists in token-tracker-http.js (line 237) as the OTEL hook point
✅ OTEL Diagnostics Pass FileSpanExporter provides graceful local fallback (/var/log/api-proxy/otel.jsonl) when no OTLP endpoint configured

All 5 scenarios pass. OTEL tracing integration is fully operational.

📡 OTel tracing validated by Smoke OTel Tracing

@github-actions github-actions Bot mentioned this pull request May 26, 2026
@github-actions
Copy link
Copy Markdown
Contributor

#3852 [awf] ARC/DinD chroot: auto-stage runner binary and critical /etc files for split-filesystem Docker hosts | #3851 api-proxy: add Anthropic Workload Identity Federation provider

  1. GitHub PR review: ✅
  2. safeinputs-gh query: ✅
  3. Playwright title check: ✅
  4. File write/read: ✅
  5. Bash file check: ✅
  6. Discussion interaction: ✅
  7. Build AWF: ✅
    Overall: PASS

Warning

Firewall blocked 1 domain

The following domain was blocked by the firewall during workflow execution:

  • registry.npmjs.org

To allow these domains, add them to the network.allowed list in your workflow frontmatter:

network:
  allowed:
    - defaults
    - "registry.npmjs.org"

See Network Configuration for more information.

🔮 The oracle has spoken through Smoke Codex

@github-actions
Copy link
Copy Markdown
Contributor

Smoke test result: FAIL

Warning

Firewall blocked 1 domain

The following domain was blocked by the firewall during workflow execution:

  • localhost

To allow these domains, add them to the network.allowed list in your workflow frontmatter:

network:
  allowed:
    - defaults
    - "localhost"

See Network Configuration for more information.

💎 Faceted by Smoke Gemini

@github-actions
Copy link
Copy Markdown
Contributor

Chroot Runtime Version Comparison

Runtime Host Version Chroot Version Match?
Python Python 3.12.13 Python 3.12.3 ❌ NO
Node.js v24.15.0 v22.22.3 ❌ NO
Go go1.22.12 go1.22.12 ✅ YES

Result: ❌ Not all runtimes match — Python and Node.js versions differ between host and chroot.

Tested by Smoke Chroot

@github-actions
Copy link
Copy Markdown
Contributor

🏗️ Build Test Suite Results

Ecosystem Project Build/Install Tests Status
Bun elysia 1/1 passed ✅ PASS
Bun hono 1/1 passed ✅ PASS
C++ fmt N/A ✅ PASS
C++ json N/A ✅ PASS
Deno oak N/A 1/1 passed ✅ PASS
Deno std N/A 1/1 passed ✅ PASS
.NET hello-world N/A ✅ PASS
.NET json-parse N/A ✅ PASS
Go color 1/1 passed ✅ PASS
Go env 1/1 passed ✅ PASS
Go uuid 1/1 passed ✅ PASS
Java gson 1/1 passed ✅ PASS
Java caffeine 1/1 passed ✅ PASS
Node.js clsx passed ✅ PASS
Node.js execa passed ✅ PASS
Node.js p-limit passed ✅ PASS
Rust fd 1/1 passed ✅ PASS
Rust zoxide 1/1 passed ✅ PASS

Overall: 8/8 ecosystems passed — ✅ PASS

Note (Java): Maven's default local repo (~/.m2/repository) was owned by root, so tests were run with -Dmaven.repo.local=/tmp/gh-aw/agent/m2/repository to work around the permissions issue. Both projects compiled and tested successfully.

Generated by Build Test Suite for issue #3881 · sonnet46 955K ·

@github-actions
Copy link
Copy Markdown
Contributor

Smoke Test Results: FAIL

Check Result
Redis PING ❌ Timeout (no response)
PostgreSQL pg_isready ❌ No response on port 5432
PostgreSQL SELECT 1 ❌ Not attempted (pg_isready failed)

Overall: FAILhost.docker.internal is not reachable from this environment. Service containers appear to be unavailable or the host DNS alias is not configured.

🔌 Service connectivity validated by Smoke Services

@lpcox lpcox merged commit b2a5d12 into main May 27, 2026
67 of 71 checks passed
@lpcox lpcox deleted the copilot/fix-duplicate-gettoken-methods branch May 27, 2026 00:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[Duplicate Code] getToken() method duplicated across three OIDC token provider subclasses

3 participants