feat(routing): add bounded router and metered classifier path - #8918
Conversation
Co-authored-by: lpcox <15877973+lpcox@users.noreply.github.com>
Co-authored-by: lpcox <15877973+lpcox@users.noreply.github.com>
Co-authored-by: lpcox <15877973+lpcox@users.noreply.github.com>
There was a problem hiding this comment.
Copilot review overview
🟡 Changes recommended
Router retries miss common transport errors, SSE observer exceptions can crash the proxy, and the provider executor lacks direct coverage.
Get a fresh assessment by requesting another Copilot review.
Review effort: Balanced
Findings: 1
Open (3)
What changed in this PR
Adds bounded router communication and a metered Copilot classifier path using the existing API proxy.
Changes:
- Adds router clients, retry planning, and classifier request handling.
- Preserves proxy guards, cancellation, accounting, and trusted request context.
- Extends token telemetry with routing purpose and SSE observation.
| File | Description |
|---|---|
body-handler.js |
Suppresses steering for routing requests. |
proxy-guards.js |
Exposes guard checks and suppresses classifier diagnostics. |
proxy-request.js |
Preserves structured transform errors. |
routing-classifier.js |
Builds and validates bounded classifier requests. |
routing-classifier.test.js |
Tests classifier construction and extraction. |
routing-planner.js |
Adds deadline-aware retry orchestration. |
routing-planner.test.js |
Tests retry and cancellation behavior. |
routing-provider-executor.js |
Executes classification through the shared proxy. |
routing-router-client.js |
Adds bounded HTTP router transport. |
routing-router-client.test.js |
Tests router endpoints and failures. |
token-budget-log.js |
Adjusts classifier budget accounting. |
token-persistence.js |
Persists request purpose. |
token-tracker-http.js |
Propagates purpose and SSE observations. |
upstream-http.js |
Propagates classifier cancellation upstream. |
upstream-response.js |
Disables retries and diagnostics for classifiers. |
upstream-token.js |
Threads routing metadata through accounting. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
|
|
||
| const dataLines = parseSseDataLines(complete); | ||
| for (const line of dataLines) { | ||
| if (typeof onSseData === 'function') onSseData(line); |
There was a problem hiding this comment.
The hook isn't external input. Only trusted in-process code sets req.awfRouting, which comes in the next issue, and nothing sets it in this PR. Wrapping it in a try/catch would silently drop a terminal SSE failure and let a routed stream finish as a success, which is the opposite of failing closed. The contract is that onSseData must never throw. Its implementation in the next issue parses inside a try/catch and reports through recordFailure, which guards its own logging and exits 78 if it can't publish. That requirement is now written into the next issue. If the hook did throw, the proxy would exit uncleanly, and the host treats that as a routing failure (78), so a crash still fails closed. I'd leave this as is.
|
|
||
| const PLANNER_ATTEMPT_TIMEOUT_MS = 5_000; | ||
| const PLANNER_MAX_ATTEMPTS = 3; | ||
| const RETRYABLE_ERROR_CODES = new Set(['ECONNREFUSED', 'ECONNRESET', 'EHOSTUNREACH', 'ENETUNREACH', 'ETIMEDOUT']); |
There was a problem hiding this comment.
Agreed for EPIPE and EAI_AGAIN. EPIPE is the router closing the socket mid-write, like ECONNRESET, and EAI_AGAIN is a temporary failure in Docker's embedded DNS. Please add both, with cases in the isRetryablePlannerError tests.
I'd keep ENOTFOUND terminal. The router is reached through a fixed alias on an internal network, and the proxy only starts after the router's health check passes, so a missing name means misconfiguration. Retrying it would spend the deadline and report a configuration error as router_unavailable.
| return { ...result, terminal: { code, detail: `Classifier execution was rejected with ${code}` } }; | ||
| } | ||
|
|
||
| function createRoutingProviderExecutor({ getCopilotAdapter, proxyRequest, checkRateLimit, getGuardChecks = getCurrentGuardChecks }) { |
There was a problem hiding this comment.
Agreed. This is the main coverage the PR drops compared with the issue. Please add containers/api-proxy/routing-provider-executor.test.js. The cases below pass against this PR's executor as written, and they cover each point in the comment.
'use strict';
const { createRoutingProviderExecutor, MAX_CLASSIFIER_RESPONSE_BYTES } = require('./routing-provider-executor');
function makeAdapter() {
return {
name: 'copilot',
isEnabled: () => true,
getRoutingProviderIdentity: () => 'github-copilot',
getTargetHost: () => 'api.githubcopilot.com',
getAuthHeaders: jest.fn(() => ({ Authorization: 'Bearer secret' })),
getBasePath: () => '',
getRequestSigner: () => null,
getTargetScheme: () => 'https',
};
}
function makeRequest() {
return {
purpose: 'routing_classification',
path: '/responses',
body: { model: 'gpt-5.4', instructions: 'Classify only.', input: [{ role: 'user', content: 'Fix this.' }], tools: [], stream: false },
};
}
function respond(statusCode, body) {
return (_req, res) => {
res.writeHead(statusCode, { 'Content-Type': 'application/json' });
res.end(body);
};
}
describe('createRoutingProviderExecutor', () => {
it('calls the existing proxy path with trusted in-memory context and no purpose header', async () => {
const proxyRequest = jest.fn((req, res) => {
expect(req.headers).not.toHaveProperty('x-awf-purpose');
expect(req.awfRequestContext).toMatchObject({ purpose: 'routing_classification' });
respond(200, '{"output_text":"{}"}')(req, res);
});
const executor = createRoutingProviderExecutor({ getCopilotAdapter: makeAdapter, proxyRequest, checkRateLimit: jest.fn(() => false) });
const result = await executor.execute(makeRequest());
expect(result.statusCode).toBe(200);
expect(proxyRequest).toHaveBeenCalledWith(
expect.anything(), expect.anything(), 'api.githubcopilot.com', { Authorization: 'Bearer secret' },
'copilot', '', null, null, 'https', // the null body transform disables alias resolution and fallback
);
});
it('uses the existing rate limiter before entering the proxy path', async () => {
const proxyRequest = jest.fn();
const executor = createRoutingProviderExecutor({
getCopilotAdapter: makeAdapter,
proxyRequest,
checkRateLimit: (_req, res) => {
res.writeHead(429, { 'Content-Type': 'application/json' });
res.end(JSON.stringify({ error: { type: 'rate_limit_error' } }));
return true;
},
});
await expect(executor.execute(makeRequest())).resolves.toMatchObject({ statusCode: 429, terminal: { code: 'rate_limit_error' } });
expect(proxyRequest).not.toHaveBeenCalled();
});
it('returns native guard failures as terminal and provider availability failures as retryable', async () => {
const run = proxyRequest => createRoutingProviderExecutor({
getCopilotAdapter: makeAdapter, proxyRequest, checkRateLimit: jest.fn(() => false),
}).execute(makeRequest());
await expect(run(respond(403, JSON.stringify({ error: { type: 'model_policy_violation', message: 'raw policy detail' } }))))
.resolves.toMatchObject({ terminal: { code: 'model_policy_violation', detail: 'Classifier execution was rejected with model_policy_violation' } });
await expect(run(respond(503, 'service unavailable'))).resolves.toMatchObject({ availabilityFailure: true });
await expect(run(respond(400, JSON.stringify({ error: { type: 'invalid_request_error', message: 'the requested model is not supported' } }))))
.resolves.toMatchObject({ availabilityFailure: true });
});
it('aborts an in-flight classifier call and forwards the signal', async () => {
let upstream;
const executor = createRoutingProviderExecutor({
getCopilotAdapter: makeAdapter,
proxyRequest: (req, res) => { upstream = { req, res }; },
checkRateLimit: jest.fn(() => false),
});
const controller = new AbortController();
const pending = executor.execute(makeRequest(), { signal: controller.signal });
expect(upstream.req.awfRequestContext.signal).toBe(controller.signal);
controller.abort();
await expect(pending).rejects.toMatchObject({ code: 'routing_cancelled' });
expect(upstream.res.destroyed).toBe(true);
});
it('rejects a classifier response larger than its bound', async () => {
const executor = createRoutingProviderExecutor({
getCopilotAdapter: makeAdapter,
proxyRequest: (_req, res) => {
res.writeHead(200, { 'Content-Type': 'application/json' });
res.end(Buffer.alloc(MAX_CLASSIFIER_RESPONSE_BYTES + 1));
},
checkRateLimit: jest.fn(() => false),
});
await expect(executor.execute(makeRequest())).rejects.toMatchObject({ code: 'routing_contract_error' });
});
it('refuses an untrusted purpose or a non-native Copilot target before any provider call', async () => {
const proxyRequest = jest.fn();
const adapter = makeAdapter();
const executor = createRoutingProviderExecutor({ getCopilotAdapter: () => adapter, proxyRequest, checkRateLimit: jest.fn(() => false) });
await expect(executor.execute({ ...makeRequest(), purpose: 'agent' })).rejects.toMatchObject({ code: 'routing_configuration_error' });
adapter.getRoutingProviderIdentity = () => null;
await expect(executor.execute(makeRequest())).rejects.toMatchObject({ code: 'provider_unavailable' });
expect(proxyRequest).not.toHaveBeenCalled();
});
});The issue also listed tests for the shared request path. These are the most important:
body-handler: steering is skipped for a request with therouting_classificationpurpose and for a request carryingreq.awfRouting, and still runs for an ordinary request.upstream-http: the trusted context's signal is passed as the upstream requestsignal, so cancelling stops the paid call.upstream-response: a classifier's 400 response is not buffered and retried with a fallback model.upstream-tokenandtoken-tracker-http: a classifier record persistspurpose: 'routing_classification', and an ordinary record has nopurposekey.
Please also add the purpose entry to schemas/token-usage.schema.json.
|
Thanks for the feedback. I think the review landed on a clear follow-up plan:
Recommended next steps for this PR:
This keeps the scope tight and addresses the concrete review feedback without broadening the PR beyond the routing/classifier path. |
|
@copilot execute the follow-up plan described here #8918 (comment) |
Co-authored-by: lpcox <15877973+lpcox@users.noreply.github.com>
Executed the follow-up plan in
Full api-proxy suite passes (1816 tests). |
|
✅ Copilot review passed with no inline comments. @copilot Add the |
|
@rabeckett make sure the updates look ok |
|
❌ Smoke Copilot BYOK AOAI (api-key) reports failed. AOAI BYOK (api-key) mode investigation needed...
|
|
Chroot tests passed! Smoke Chroot - All security and functionality tests succeeded.
|
|
🔌 Smoke Services — All services reachable! ✅
|
|
✨ The prophecy is fulfilled... Smoke Codex has completed its mystical journey. The stars align. 🌟 Warning Firewall blocked 12 domainsThe following domains were blocked by the firewall during workflow execution:
[!TIP] tools:
github:
mode: gh-proxySee GitHub Tools for more information on To allow these domains, add them to the network:
allowed:
- defaults
- "ab.chatgpt.com"
- "accounts.google.com"
- "api.github.com"
- "clients2.google.com"
- "collector.github.com"
- "contentautofill.googleapis.com"
- "github.com"
- "github.githubassets.com"
- "msfeed25.pkgs.visualstudio.com"
- "update.googleapis.com"
- "www.google.com"
- "www.gstatic.com"See Network Configuration for more information.
|
|
❌ Smoke Copilot BYOK AOAI (Entra) reports failed. AOAI BYOK (Entra) mode investigation needed...
|
|
✅ Smoke Copilot BYOK completed. Copilot BYOK mode operational. 🔓
|
|
❌ Smoke Gemini reports failed. Facets need polishing...
|
|
✅ Security Guard completed successfully! Security review of PR #8918 complete. No security vulnerabilities found. All new routing infrastructure properly validates inputs, isolates internal traffic from user-facing requests, enforces timeouts, maintains token tracking accuracy, and propagates errors safely. Existing guard controls remain intact.
|
|
📰 VERDICT: Smoke Copilot has concluded. All systems operational. This is a developing story. 🎤
|
|
📡 Smoke OTel Tracing completed. All tracing scenarios validated. ✅ Warning Firewall blocked 2 domainsThe following domains were blocked by the firewall during workflow execution:
To allow these domains, add them to the network:
allowed:
- defaults
- "o205451.ingest.us.sentry.io"
- "registry.npmjs.org"See Network Configuration for more information.
|
|
✅ Smoke Claude passed Warning Firewall blocked 1 domainThe following domain was blocked by the firewall during workflow execution:
To allow these domains, add them to the network:
allowed:
- defaults
- "api.anthropic.com"See Network Configuration for more information.
|
|
Smoke Cloud Hypervisor completed. Cloud Hypervisor + Copilot passed. Warning Firewall blocked 2 domainsThe following domains were blocked by the firewall during workflow execution:
To allow these domains, add them to the network:
allowed:
- defaults
- "example.com"
- "github.com"See Network Configuration for more information.
|
|
✅ Build Test Suite completed successfully! Warning Firewall blocked 8 domainsThe following domains were blocked by the firewall during workflow execution:
To allow these domains, add them to the network:
allowed:
- defaults
- "api.nuget.org"
- "bun.sh"
- "dc.services.visualstudio.com"
- "deno.land"
- "dl.deno.land"
- "github.com"
- "releaseassets.githubusercontent.com"
- "repo.maven.apache.org"See Network Configuration for more information.
|
✅ Smoke Test: Copilot BYOK (Direct) ModeStatus: PASS Test Results:
Running in direct BYOK mode with real key held by sidecar, placeholder injected into agent.
|
|
Smoke Test: Copilot Engine
Overall: PASS
|
Smoke Test: Cloud Hypervisor + Copilot
All checks passed. Warning Firewall blocked 2 domainsThe following domains were blocked by the firewall during workflow execution:
To allow these domains, add them to the network:
allowed:
- defaults
- "example.com"
- "github.com"See Network Configuration for more information.
|
|
EGRESS_RESULT allow=pass deny=pass ✅ Allowed domain (github.com) reachable — HTTP 200 Overall: PASS Warning Firewall blocked 2 domainsThe following domains were blocked by the firewall during workflow execution:
[!TIP] tools:
github:
mode: gh-proxySee GitHub Tools for more information on To allow these domains, add them to the network:
allowed:
- defaults
- "api.github.com"
- "example.com"See Network Configuration for more information.
|
github-actions
Bot
added
the
smoke-copilot-network-isolation
|
Smoke Test: Services Connectivity
Overall: PASS
|
Smoke Test: Claude Engine Validation
Overall result: PASS Warning Firewall blocked 1 domainThe following domain was blocked by the firewall during workflow execution:
To allow these domains, add them to the network:
allowed:
- defaults
- "api.anthropic.com"See Network Configuration for more information.
|
Chroot Version Comparison
Overall: FAILED — Node.js version mismatch between host and chroot environment (host
|
|
Reviewed merged PRs:
GitHub review/details: ✅ Warning Firewall blocked 12 domainsThe following domains were blocked by the firewall during workflow execution:
[!TIP] tools:
github:
mode: gh-proxySee GitHub Tools for more information on To allow these domains, add them to the network:
allowed:
- defaults
- "ab.chatgpt.com"
- "accounts.google.com"
- "api.github.com"
- "clients2.google.com"
- "collector.github.com"
- "contentautofill.googleapis.com"
- "github.com"
- "github.githubassets.com"
- "msfeed25.pkgs.visualstudio.com"
- "update.googleapis.com"
- "www.google.com"
- "www.gstatic.com"See Network Configuration for more information.
|
Smoke Test: API Proxy OpenTelemetry Tracing — Results
Overall: PASS — all implemented OTEL integration points (module init, span creation, GenAI usage attributes, env propagation, token-tracker hook) verified working. Span export to a live OTLP endpoint is untested in this environment as expected (no collector configured), and the code degrades gracefully without errors. Warning Firewall blocked 2 domainsThe following domains were blocked by the firewall during workflow execution:
To allow these domains, add them to the network:
allowed:
- defaults
- "o205451.ingest.us.sentry.io"
- "registry.npmjs.org"See Network Configuration for more information.
|
🏗️ Build Test Suite Results
Overall: 8/8 ecosystems passed — PASS Note: The default Maven local repository ( All ecosystems cloned and built/tested successfully with no firewall-related network blocking observed. Warning Firewall blocked 8 domainsThe following domains were blocked by the firewall during workflow execution:
To allow these domains, add them to the network:
allowed:
- defaults
- "api.nuget.org"
- "bun.sh"
- "dc.services.visualstudio.com"
- "deno.land"
- "dl.deno.land"
- "github.com"
- "releaseassets.githubusercontent.com"
- "repo.maven.apache.org"See Network Configuration for more information.
|
Adds the inert routing transport and classifier execution path needed before route selection is wired into runs. Router and provider inference calls are bounded, trusted, and metered without creating a second credentialed upstream path.
Router transport
Classifier execution
responsesandchat/completionsrequests.proxyRequest, preserving existing credentials, headers, guards, rate limits, and accounting.Trusted internal context