Skip to content

Refactor createProviderServer into composable HTTP/WebSocket handlers#3702

Merged
lpcox merged 3 commits into
mainfrom
copilot/refactor-extract-request-handlers
May 25, 2026
Merged

Refactor createProviderServer into composable HTTP/WebSocket handlers#3702
lpcox merged 3 commits into
mainfrom
copilot/refactor-extract-request-handlers

Conversation

Copy link
Copy Markdown
Contributor

Copilot AI commented May 25, 2026

containers/api-proxy/server.js had a large createProviderServer function combining routing, health/reflect endpoints, disabled-provider responses, rate limiting, proxy delegation, and WebSocket upgrade logic. This refactor separates those concerns into focused handlers while preserving existing behavior and call sites.

  • What changed

    • Extracted HTTP handler factories:
      • createHealthCheckHandler(adapter)
      • createReflectHandler()
      • createDisabledProviderHandler(adapter)
      • createProxyHandler(adapter)
    • Extracted WebSocket upgrade factory:
      • createWebSocketUpgradeHandler(adapter)
    • Reduced createProviderServer(adapter) to routing/composition only:
      • management endpoint short-circuit
      • /health and /reflect routing
      • disabled-adapter guard
      • proxy fallback
      • server.on('upgrade', ...) wiring via extracted upgrade handler
  • Behavioral intent

    • Preserve all existing response shapes/status codes and routing order.
    • Keep adapter-driven behavior unchanged (auth headers, URL transforms, body transforms, target/base-path resolution, unconfigured responses).
  • Illustrative structure

    function createProviderServer(adapter) {
      const handleHealthCheck = createHealthCheckHandler(adapter);
      const handleReflect = createReflectHandler();
      const handleDisabledProvider = createDisabledProviderHandler(adapter);
      const handleProxy = createProxyHandler(adapter);
    
      const server = http.createServer((req, res) => {
        if (adapter.isManagementPort && handleManagementEndpoint(req, res)) return;
        if (req.url === '/health' && req.method === 'GET') return handleHealthCheck(req, res);
        if (req.url === '/reflect' && req.method === 'GET') return handleReflect(req, res);
        if (!adapter.isEnabled()) return handleDisabledProvider(req, res);
        return handleProxy(req, res);
      });
    
      server.on('upgrade', createWebSocketUpgradeHandler(adapter));
      return server;
    }

Copilot AI changed the title [WIP] Refactor: Extract request handlers from createProviderServer function Refactor createProviderServer into composable HTTP/WebSocket handlers May 25, 2026
Copilot finished work on behalf of lpcox May 25, 2026 03:17
Copilot AI requested a review from lpcox May 25, 2026 03:17
@lpcox lpcox marked this pull request as ready for review May 25, 2026 03:19
Copilot AI review requested due to automatic review settings May 25, 2026 03:19
@github-actions
Copy link
Copy Markdown
Contributor

github-actions Bot commented May 25, 2026

✅ Coverage Check Passed

Overall Coverage

Metric Base PR Delta
Lines 96.36% 96.43% 📈 +0.07%
Statements 96.21% 96.28% 📈 +0.07%
Functions 97.96% 97.96% ➡️ +0.00%
Branches 90.35% 90.39% 📈 +0.04%
📁 Per-file Coverage Changes (1 files)
File Lines (Before → After) Statements (Before → After)
src/config-writer.ts 83.0% → 85.6% (+2.54%) 83.0% → 85.6% (+2.54%)

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

@github-actions

This comment has been minimized.

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

Refactors containers/api-proxy/server.js by splitting the previously large createProviderServer implementation into smaller, composable HTTP handlers (health/reflect/disabled/proxy) plus a WebSocket upgrade handler, with createProviderServer reduced to routing/composition.

Changes:

  • Extracted dedicated HTTP handler factories for /health, /reflect, disabled-provider responses, and proxy delegation.
  • Extracted a WebSocket upgrade handler factory and wired it via server.on('upgrade', ...).
  • Simplified createProviderServer(adapter) to orchestration/routing only (management short-circuit, endpoint routing, disabled guard, proxy fallback).
Show a summary per file
File Description
containers/api-proxy/server.js Breaks createProviderServer into focused handler factories and composes them into the provider server/router.

Copilot's findings

Tip

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

  • Files reviewed: 1/1 changed files
  • Comments generated: 1

Comment thread containers/api-proxy/server.js Outdated
Comment on lines +433 to +439
*
* @param {import('./providers').ProviderAdapter} adapter
* @returns {http.Server}
* @returns {(req: http.IncomingMessage, res: http.ServerResponse) => void}
This was referenced May 25, 2026
@github-actions

This comment has been minimized.

@github-actions

This comment has been minimized.

@github-actions

This comment has been minimized.

@github-actions

This comment has been minimized.

@github-actions

This comment has been minimized.

@github-actions

This comment has been minimized.

@github-actions

This comment has been minimized.

@github-actions

This comment has been minimized.

@lpcox
Copy link
Copy Markdown
Collaborator

lpcox commented May 25, 2026

@copilot address review feedback

Copy link
Copy Markdown
Contributor Author

Copilot AI commented May 25, 2026

@copilot address review feedback

Addressed in commit 20b0d3e. I updated the misplaced JSDoc so createHealthCheckHandler is documented as a handler factory, and moved the server-level provider-agnostic description onto createProviderServer.

Copilot finished work on behalf of lpcox May 25, 2026 03:42
@github-actions
Copy link
Copy Markdown
Contributor

Smoke Test Results

✅ GitHub API check — 2 recent PRs verified
✅ GitHub playwright check — navigation test PASS
✅ File verification — smoke test artifact exists

Overall: PASS — Claude engine validation successful

💥 [THE END] — Illustrated by Smoke Claude

@github-actions
Copy link
Copy Markdown
Contributor

Smoke Test Results

GitHub MCP – PR #3704: "Reduce Documentation Maintainer workflow prompt/tool overhead"
File I/O – File not found at path
GitHub.com connectivity – Missing HTTP status (no pre-step output)

Status: FAIL

@Copilot @lpcox – File test failed, connectivity status unavailable.

📰 BREAKING: Report filed by Smoke Copilot

@github-actions
Copy link
Copy Markdown
Contributor

Smoke Test Results: Copilot BYOK (Offline) Mode

Status: ⚠️ PARTIAL PASS

  • ✅ GitHub MCP: Retrieved PR Reduce Documentation Maintainer workflow prompt/tool overhead #3704 "Reduce Documentation Maintainer workflow prompt/tool overhead"
  • ❌ File write/read: /tmp/smoke-test-file.txt not found
  • ❌ GitHub.com connectivity: HTTP code not provided in template vars
  • ✅ BYOK inference: Successfully responding via api-proxy → api.githubcopilot.com

Note: Running in BYOK offline mode (COPILOT_OFFLINE=true) via api-proxy sidecar

PR Context: @Copilot authored, assigned to @lpcox, @Copilot

🔑 BYOK report filed by Smoke Copilot BYOK

This was referenced May 25, 2026
@github-actions
Copy link
Copy Markdown
Contributor

Smoke test partially complete. Checking connectivity.

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

✅ API Proxy OpenTelemetry Tracing Validation

All scenarios passed successfully:

Scenario 1: Module Loading ✅

  • otel.js loads successfully
  • Exports: startRequestSpan, setTokenAttributes, endSpan, endSpanError, shutdown, isEnabled
  • Graceful degradation shim works when module not available

Scenario 2: Test Suite ✅

  • 32 tests passed in containers/api-proxy/otel.test.js
  • Coverage: span creation, token attributes, parent context propagation, OTLP export, file fallback
  • Key validations:
    • GenAI semantic conventions (gen_ai.usage.*) correctly applied
    • Parent trace linkage via GITHUB_AW_OTEL_TRACE_ID / GITHUB_AW_OTEL_PARENT_SPAN_ID
    • ProxyAwareOtlpExporter routes through Squid proxy
    • FileSpanExporter fallback when OTLP endpoint not configured

Scenario 3: Env Var Forwarding ✅

  • src/services/api-proxy-service.ts correctly forwards OTEL env vars:
    • OTEL_EXPORTER_OTLP_ENDPOINT (lines 128-129)
    • OTEL_EXPORTER_OTLP_HEADERS (lines 131-132)
    • OTEL_SERVICE_NAME (line 134, default: "awf-api-proxy")
    • GITHUB_AW_OTEL_TRACE_ID (lines 135-136)
    • GITHUB_AW_OTEL_PARENT_SPAN_ID (lines 138-139)

Scenario 4: Token Tracker Integration ✅

  • onUsage callback exists in token-tracker-http.js (line 62, 237-239)
  • Integration in proxy-request.js (line 647-648):
    onUsage: (normalizedUsage, model) => {
      otel.setTokenAttributes(span, { provider, model, normalizedUsage, streaming });
    }
  • Span lifecycle correctly managed: start → setTokenAttributes → endSpan/endSpanError

Scenario 5: OTEL Diagnostics ✅

  • Spans exported to /tmp/gh-aw/otel.jsonl during workflow run
  • Sample span includes GenAI attributes and parent trace linkage
  • TraceID: 2753c2620edcfbf93175f0b3a0a763f9, correctly linked to workflow parent span

Summary: All OTEL tracing integration points validated. The api-proxy correctly:

  1. Initializes OTEL module with proxy-aware OTLP export
  2. Creates spans with GenAI semantic conventions
  3. Propagates parent context from workflow traces
  4. Exports spans via Squid proxy or fallback to file
  5. Gracefully degrades when OTEL not configured

No issues found

📡 OTel tracing validated by Smoke OTel Tracing

@github-actions
Copy link
Copy Markdown
Contributor

Smoke test Codex:
PRs: Reduce Documentation Maintainer workflow prompt/tool overhead; Split api-proxy-config tests by validation concern
✅ GitHub PR review
❌ safeinputs-gh query
✅ Playwright title
❌ Tavily search
✅ File/bash
❌ Discussion query/comment
✅ Build
Overall: FAIL

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

GitHub Actions Services Connectivity: ❌ FAIL

Redis: ❌ Connection timeout
PostgreSQL pg_isready: ❌ Connection timeout (no response)
PostgreSQL SELECT 1: ❌ Not tested (pg_isready failed)

Result: All service connectivity checks failed. Services running on host are not reachable via host.docker.internal.

🔌 Service connectivity validated by Smoke Services

@github-actions
Copy link
Copy Markdown
Contributor

Chroot Version Comparison Test Results

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

Overall Result: Tests FAILED - Not all runtime versions match between host and chroot environments.

Summary

  • Python: Minor version mismatch (3.12.13 vs 3.12.3)
  • Node.js: Major version mismatch (v24 vs v22)
  • Go: Versions match ✅

The chroot environment is not using the same runtime versions as the host, which may cause compatibility issues.

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 All tests passed ✅ PASS
Node.js execa All tests passed ✅ PASS
Node.js p-limit All tests passed ✅ PASS
Rust fd 1/1 passed ✅ PASS
Rust zoxide 1/1 passed ✅ PASS

Overall: 8/8 ecosystems passed — ✅ PASS

Generated by Build Test Suite for issue #3702 · ● 11.5M ·

@lpcox lpcox merged commit ba32f63 into main May 25, 2026
68 of 72 checks passed
@lpcox lpcox deleted the copilot/refactor-extract-request-handlers branch May 25, 2026 04:00
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.

[Refactoring] Extract request handlers from createProviderServer function

3 participants