Skip to content

refactor(api-proxy): decompose handleUpstreamResponse into focused helpers#5421

Merged
lpcox merged 5 commits into
mainfrom
copilot/refactor-handle-upstream-response
Jun 23, 2026
Merged

refactor(api-proxy): decompose handleUpstreamResponse into focused helpers#5421
lpcox merged 5 commits into
mainfrom
copilot/refactor-handle-upstream-response

Conversation

Copilot AI commented Jun 22, 2026

Copy link
Copy Markdown
Contributor

handleUpstreamResponse in upstream-response.js mixed three unrelated concerns in 138 lines, making the control flow hard to follow and the retry logic untestable in isolation.

Changes

  • handle400WithRetry(proxyRes, requestHeaders, responseBody, opts) — extracts the entire shouldBuffer400 branch: deprecated-header retry (a), transient model-not-supported catalogue retry (b), and model_unavailable diagnostic (c). Returns true when a retry was triggered.

  • setupTokenTracking(proxyRes, body, opts) — extracts request-body model parsing and trackTokenUsage wiring with OTEL span callbacks (onUsage, onSpanEnd).

  • handleUpstreamResponse reduced from 138 → ~50 lines; now just orchestrates the error handler, shouldBuffer400 decision, and delegates to the two helpers.

Both helpers are inner functions of createUpstreamResponseHandlers, so they share the factory closure without any new public API surface.

// Before: 138-line monolith
function handleUpstreamResponse(proxyRes, requestHeaders, { ... }) {
  // error handler + shouldBuffer400 check +
  // 70 lines of retry branches +
  // 20 lines of token-tracking wiring
}

// After: ~50-line coordinator
function handleUpstreamResponse(proxyRes, requestHeaders, { ... }) {
  // error handler (unchanged)
  // shouldBuffer400 check (unchanged)
  if (shouldBuffer400) {
    proxyRes.on('end', () => handle400WithRetry(proxyRes, requestHeaders, responseBody, { ... }));
    return;
  }
  // normal path
  setupTokenTracking(proxyRes, body, { ... });
}

…d setupTokenTracking

Extract two focused helpers from handleUpstreamResponse in upstream-response.js:

- handle400WithRetry: handles all three 400 retry/diagnostic branches
  (a) deprecated beta-header retry for anthropic/copilot
  (b) transient model-not-supported catalogue retry (copilot, up to MAX_MODEL_NOT_SUPPORTED_RETRIES)
  (c) model_unavailable diagnostic when retries are exhausted

- setupTokenTracking: parses requestModel from the request body and wires
  trackTokenUsage with OTEL span callbacks (onUsage, onSpanEnd)

handleUpstreamResponse is reduced from 138 lines to ~50 lines. No behaviour
changes; all 1277 existing tests continue to pass.

Closes #5401
Copilot AI changed the title [WIP] Refactor handleUpstreamResponse for clearer responsibilities refactor(api-proxy): decompose handleUpstreamResponse into focused helpers Jun 22, 2026
Copilot AI requested a review from lpcox June 22, 2026 22:59
Copilot finished work on behalf of lpcox June 22, 2026 22:59
@lpcox lpcox marked this pull request as ready for review June 23, 2026 13:52
Copilot AI review requested due to automatic review settings June 23, 2026 13:52

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

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 refactors the api-proxy upstream response handling to make handleUpstreamResponse a smaller coordinator by extracting the 400-response retry/forwarding logic and the token-tracking wiring into focused helper functions within createUpstreamResponseHandlers.

Changes:

  • Extracts buffered-400 handling (deprecated-header retry, transient model-not-supported retry, and model_unavailable diagnostic + forwarding) into handle400WithRetry(...).
  • Extracts request-body model parsing and trackTokenUsage + OTEL callbacks into setupTokenTracking(...).
  • Simplifies handleUpstreamResponse(...) to orchestrate buffering/streaming flow and delegate to the helpers.
Show a summary per file
File Description
containers/api-proxy/upstream-response.js Decomposes handleUpstreamResponse into two inner helper functions for buffered-400 retry/forwarding and token-usage tracking wiring.

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: 3

Comment thread containers/api-proxy/upstream-response.js Outdated
Comment thread containers/api-proxy/upstream-response.js Outdated
Comment thread containers/api-proxy/upstream-response.js
lpcox and others added 2 commits June 23, 2026 07:51
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
@github-actions

Copy link
Copy Markdown
Contributor

✅ Copilot review passed with no inline comments.

@copilot Add the ready-for-aw label to this PR to trigger agentic CI smoke tests.

Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
@github-actions

github-actions Bot commented Jun 23, 2026

Copy link
Copy Markdown
Contributor

Chroot tests passed! Smoke Chroot - All security and functionality tests succeeded.

@github-actions

github-actions Bot commented Jun 23, 2026

Copy link
Copy Markdown
Contributor

Smoke Copilot BYOK completed. Copilot BYOK mode operational. 🔓

@github-actions

github-actions Bot commented Jun 23, 2026

Copy link
Copy Markdown
Contributor

📡 Smoke OTel Tracing completed. All tracing scenarios validated. ✅

@github-actions

github-actions Bot commented Jun 23, 2026

Copy link
Copy Markdown
Contributor

🔌 Smoke Services — All services reachable! ✅

@github-actions

github-actions Bot commented Jun 23, 2026

Copy link
Copy Markdown
Contributor

Smoke Claude passed

@github-actions

github-actions Bot commented Jun 23, 2026

Copy link
Copy Markdown
Contributor

Build Test Suite completed successfully!

@github-actions

github-actions Bot commented Jun 23, 2026

Copy link
Copy Markdown
Contributor

✨ The prophecy is fulfilled... Smoke Codex has completed its mystical journey. The stars align. 🌟

@github-actions

github-actions Bot commented Jun 23, 2026

Copy link
Copy Markdown
Contributor

Smoke Gemini completed. All facets verified. 💎

Gemini Smoke Test started

@github-actions

github-actions Bot commented Jun 23, 2026

Copy link
Copy Markdown
Contributor

🔑 Smoke Copilot PAT PAT auth validated. All systems operational. ✅

@github-actions

github-actions Bot commented Jun 23, 2026

Copy link
Copy Markdown
Contributor

Smoke Copilot BYOK AOAI (api-key) completed. Copilot AOAI BYOK (api-key) mode operational. 🔓

@github-actions

github-actions Bot commented Jun 23, 2026

Copy link
Copy Markdown
Contributor

Smoke Copilot BYOK AOAI (Entra) reports failed. AOAI BYOK (Entra) mode investigation needed...

@github-actions

Copy link
Copy Markdown
Contributor

✅ Coverage Check Passed

Overall Coverage

Metric Base PR Delta
Lines 98.01% 98.05% 📈 +0.04%
Statements 97.95% 97.98% 📈 +0.03%
Functions 99.51% 99.51% ➡️ +0.00%
Branches 93.68% 93.72% 📈 +0.04%
📁 Per-file Coverage Changes (1 files)
File Lines (Before → After) Statements (Before → After)
src/workdir-setup.ts 92.7% → 94.5% (+1.82%) 92.7% → 94.5% (+1.82%)

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

@github-actions

github-actions Bot commented Jun 23, 2026

Copy link
Copy Markdown
Contributor

📰 VERDICT: Smoke Copilot has concluded. All systems operational. This is a developing story. 🎤

@github-actions

Copy link
Copy Markdown
Contributor

🔐 Smoke Test: Copilot BYOK (Direct) Mode

Status: PASS

  • GitHub MCP Testing — Successfully listed merged PRs
  • GitHub.com Connectivity — HTTP 200
  • File Write/Read — Created and verified test file
  • BYOK Inference — Running in direct BYOK mode (COPILOT_PROVIDER_API_KEY via api-proxy → api.githubcopilot.com)

Inference requests routed through api-proxy sidecar with real credentials held server-side; agent sees placeholder only.

cc: @lpcox @Copilot

🔑 BYOK report filed by Smoke Copilot BYOK

@github-actions

Copy link
Copy Markdown
Contributor

Smoke Test: Claude Engine Validation

  • API status: ✅ PASS
  • gh check: ✅ PASS
  • File status: ✅ PASS

Overall result: PASS

Generated by Smoke Claude for issue #5421 · 36.7 AIC · ⊞ 3.1K ·

@github-actions

Copy link
Copy Markdown
Contributor

Smoke Test Results — Auth mode: PAT (COPILOT_GITHUB_TOKEN)

Test Result
GitHub MCP connectivity ✅ PR list returned successfully
github.com HTTP status ✅ HTTP 200
File write/read ❌ Pre-step outputs not interpolated (template vars unexpanded)

Overall: FAIL ⚠️ (pre-step data unavailable — workflow expression substitution did not occur)

cc @lpcox @Copilot

🔑 PAT report filed by Smoke Copilot PAT

@github-actions

Copy link
Copy Markdown
Contributor

Reviewed: Remove unused ParsedDomainList export from domain-matchers
Reviewed: Remove unused ParsedDomain from domain-patterns
✅ GitHub PR queries
✅ Playwright title check
✅ File write/read check
✅ Discussion comment
✅ Build (npm ci && npm run build)
Overall status: 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: API Proxy OpenTelemetry Tracing

Scenario Result Notes
1. Module Loading otel.js loads cleanly; exports: startRequestSpan, setTokenAttributes, setBudgetAttributes, endSpan, endSpanError, shutdown, isEnabled + internal helpers
2. Test Suite 39/39 tests passed (otel.test.js, 1.809s)
3. Env Var Forwarding src/services/api-proxy-service-config.ts forwards GH_AW_OTLP_ENDPOINTS, GITHUB_AW_OTEL_TRACE_ID, GITHUB_AW_OTEL_PARENT_SPAN_ID to the api-proxy container
4. Token Tracker Integration onUsage and onSpanEnd callbacks present in token-tracker-http.js (lines 283, 324, 350)
5. OTEL Diagnostics Graceful degradation confirmed: falls back to FileSpanExporter/var/log/api-proxy/otel.jsonl when no OTLP endpoint is configured

All 5 scenarios passed. OTEL tracing integration is fully functional.

📡 OTel tracing validated by Smoke OTel Tracing

@github-actions

Copy link
Copy Markdown
Contributor

🔍 Chroot Smoke Test Results

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

Result: ❌ Tests did not pass — Python and Node.js versions differ between host and chroot environments.

Tested by Smoke Chroot

@github-actions

Copy link
Copy Markdown
Contributor

Smoke Test Results

PR: refactor(api-proxy): decompose handleUpstreamResponse into focused helpers
Author: @Copilot | Assignees: @lpcox, @Copilot

Test Result
GitHub MCP connectivity
GitHub.com HTTP connectivity ❌ (pre-step data unavailable)
File write/read ❌ (pre-step data unavailable)

Overall: FAIL — Pre-step output variables were not substituted; HTTP and File test results could not be verified.

📰 BREAKING: Report filed by Smoke Copilot

@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 passed ✅ PASS
Node.js execa all passed ✅ PASS
Node.js p-limit all 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 #5421 · 34 AIC · ⊞ 7.7K ·

@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

Smoke Test: GitHub Actions Services Connectivity

Check Result
Redis PING ❌ timeout (no PONG)
PostgreSQL pg_isready ❌ no response
PostgreSQL SELECT 1 ❌ timeout

Overall: FAILhost.docker.internal service containers are not reachable from this runner.

🔌 Service connectivity validated by Smoke Services

@github-actions

Copy link
Copy Markdown
Contributor

@Copilot @lpcox

  • GitHub MCP tool: ✅
  • GitHub.com HTTP: ✅
  • Agent file I/O: ✅
  • Direct BYOK inference: ✅

PASS: Running in direct BYOK mode (COPILOT_PROVIDER_API_KEY + COPILOT_PROVIDER_BASE_URL) via api-proxy → Azure OpenAI (Foundry, o4-mini-aw) #aw_smoke

🔑 BYOK (AOAI api-key) report filed by Smoke Copilot BYOK AOAI (api-key)

@lpcox lpcox merged commit fd072c9 into main Jun 23, 2026
85 of 88 checks passed
@lpcox lpcox deleted the copilot/refactor-handle-upstream-response branch June 23, 2026 15:33
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.

3 participants