Skip to content

Commit 1c81c99

Browse files
AarononthewebAaronontheweb
committed
fix(providers): address Copilot review — disposal, dedupe, validation, sync path
Four bug fixes from the GitHub Copilot pull-request review: 1. OAuthDeviceFlowService.PostFormAsync leaked HttpRequestMessage (and its FormUrlEncodedContent). Callers also did not dispose the response. In the device-flow polling loop both grew connection-handle pressure. Now wraps the request in `using`, makes the helper async, and `using`s the response at every call site. 2. CopilotTokenExchanger.GetTokenAsync allowed N concurrent calls to ExchangeAsync for the same OAuth token when a burst of chat requests arrived inside the 2-minute refresh buffer. Adds a per-key SemaphoreSlim with double-checked locking — the first caller refreshes, the rest wait and read the new cache entry. Cuts the worst-case fan-out to a single /copilot_internal/v2/token request per refresh. 3. ExchangeAsync trusted the deserialized token payload. System.Text.Json does not enforce required-ness on positional record parameters, so a {} response would yield Token=null/ExpiresAt=0 and we'd cache a useless Bearer. Validates non-empty Token and positive ExpiresAt before caching; failure throws an InvalidOperationException with the offending endpoint URL in the message. 4. CopilotRequestPolicy.Process performed sync-over-async via GetAwaiter().GetResult() on an HTTP call. Deadlock risk under a sync context, blocks the calling thread on network I/O. The OpenAI SDK uses ProcessAsync for chat completions; the sync overload was only present because PipelinePolicy declares it. Now throws NotSupportedException with a clear message directing callers to the async pipeline. Adds a regression test.
1 parent 01ecfce commit 1c81c99

4 files changed

Lines changed: 94 additions & 18 deletions

File tree

‎src/Netclaw.Daemon.Tests/Providers/GitHubCopilot/CopilotRequestPolicyTests.cs‎

Lines changed: 21 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -96,6 +96,27 @@ public async Task ProcessAsync_OverwritesPreviousAuthorizationHeader()
9696
Assert.Equal("Bearer copilot-real", auth);
9797
}
9898

99+
[Fact]
100+
public void Process_Synchronous_ThrowsNotSupported()
101+
{
102+
// The sync pipeline path would require blocking on async token exchange.
103+
// The OpenAI SDK uses the async pipeline for chat completions; the sync
104+
// overload is only hit by misconfigured callers and we fail loudly.
105+
var policy = new CopilotRequestPolicy(
106+
ExchangerReturning("copilot-bearer"), OAuthEntry());
107+
108+
var clientPipeline = ClientPipeline.Create(new ClientPipelineOptions());
109+
using var message = clientPipeline.CreateMessage();
110+
message.Request.Method = "POST";
111+
message.Request.Uri = new Uri("https://api.githubcopilot.com/chat/completions");
112+
113+
IReadOnlyList<PipelinePolicy> pipeline = [policy, new TerminalCapturingPolicy(() => { })];
114+
115+
var ex = Assert.Throws<NotSupportedException>(() =>
116+
policy.Process(message, pipeline, 0));
117+
Assert.Contains("async", ex.Message, StringComparison.OrdinalIgnoreCase);
118+
}
119+
99120
/// <summary>
100121
/// No-op terminal policy that records whether the previous policy invoked
101122
/// the chain. Used so ProcessNextAsync has somewhere to land without

‎src/Netclaw.Providers/GitHubCopilot/CopilotRequestPolicy.cs‎

Lines changed: 11 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -21,10 +21,17 @@ internal sealed class CopilotRequestPolicy(CopilotTokenExchanger exchanger, Prov
2121
public override void Process(
2222
PipelineMessage message, IReadOnlyList<PipelinePolicy> pipeline, int currentIndex)
2323
{
24-
var token = exchanger.GetTokenAsync(entry, message.CancellationToken)
25-
.GetAwaiter().GetResult();
26-
Apply(message, token);
27-
ProcessNext(message, pipeline, currentIndex);
24+
// The OpenAI SDK's chat-completions path always invokes ProcessAsync.
25+
// The sync overload exists on PipelinePolicy because the abstract
26+
// contract requires it, but in practice it's only hit by callers
27+
// that misconfigure their client. We refuse to block on an async
28+
// network call (deadlock risk under a sync context, synchronous I/O
29+
// on the calling thread) and fail loudly per the no-silent-fallback
30+
// rule in CLAUDE.md.
31+
throw new NotSupportedException(
32+
"CopilotRequestPolicy requires the async pipeline. Token exchange " +
33+
"is an async network call; use the OpenAI SDK's async chat methods " +
34+
"(e.g. ChatClient.CompleteChatAsync) rather than the synchronous overloads.");
2835
}
2936

3037
public override async ValueTask ProcessAsync(

‎src/Netclaw.Providers/GitHubCopilot/CopilotTokenExchanger.cs‎

Lines changed: 56 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -39,6 +39,13 @@ public sealed class CopilotTokenExchanger(HttpClient httpClient, TimeProvider? t
3939
private readonly TimeProvider time = timeProvider ?? TimeProvider.System;
4040
private readonly ConcurrentDictionary<string, CachedToken> cache = new();
4141

42+
// Per-OAuth-token semaphore so a burst of chat requests arriving inside
43+
// the refresh window doesn't fan out into N concurrent calls to
44+
// /copilot_internal/v2/token (GitHub rate-limits aggressively). The first
45+
// caller refreshes; the others wait, then read the new cache entry under
46+
// the lock.
47+
private readonly ConcurrentDictionary<string, SemaphoreSlim> refreshLocks = new();
48+
4249
/// <summary>
4350
/// Returns a valid Copilot API token for the OAuth credential carried by
4451
/// <paramref name="entry"/>, fetching from <c>/copilot_internal/v2/token</c>
@@ -54,17 +61,39 @@ public async Task<string> GetTokenAsync(
5461
"GitHub OAuth access token (re-run 'netclaw provider add <name> github-copilot --auth oauth-device')");
5562

5663
var cacheKey = HashKey(oauthToken.Value);
57-
var now = time.GetUtcNow();
5864

59-
if (cache.TryGetValue(cacheKey, out var cached)
60-
&& cached.ExpiresAt - RefreshBuffer > now)
61-
{
65+
if (TryGetFresh(cacheKey, out var cached))
6266
return cached.Token;
67+
68+
var sem = refreshLocks.GetOrAdd(cacheKey, _ => new SemaphoreSlim(1, 1));
69+
await sem.WaitAsync(ct);
70+
try
71+
{
72+
// Re-check under the lock — a previous caller may have refreshed
73+
// while we were waiting.
74+
if (TryGetFresh(cacheKey, out cached))
75+
return cached.Token;
76+
77+
var fresh = await ExchangeAsync(oauthToken.Value, ct);
78+
cache[cacheKey] = fresh;
79+
return fresh.Token;
80+
}
81+
finally
82+
{
83+
sem.Release();
84+
}
85+
}
86+
87+
private bool TryGetFresh(string cacheKey, out CachedToken cached)
88+
{
89+
if (cache.TryGetValue(cacheKey, out cached!)
90+
&& cached.ExpiresAt - RefreshBuffer > time.GetUtcNow())
91+
{
92+
return true;
6393
}
6494

65-
var fresh = await ExchangeAsync(oauthToken.Value, ct);
66-
cache[cacheKey] = fresh;
67-
return fresh.Token;
95+
cached = default!;
96+
return false;
6897
}
6998

7099
private async Task<CachedToken> ExchangeAsync(string oauthToken, CancellationToken ct)
@@ -94,7 +123,26 @@ private async Task<CachedToken> ExchangeAsync(string oauthToken, CancellationTok
94123

95124
var parsed = JsonSerializer.Deserialize<TokenResponse>(body)
96125
?? throw new InvalidOperationException(
97-
"Empty token response from /copilot_internal/v2/token.");
126+
$"Empty token response from {TokenEndpoint}.");
127+
128+
// System.Text.Json doesn't enforce required-ness on positional record
129+
// parameters by default, so a {} response would deserialize to
130+
// Token=null/ExpiresAt=0 and we'd cache a useless Bearer. Validate
131+
// here so the failure surfaces at the exchange boundary, not later
132+
// when the chat call returns 401.
133+
if (string.IsNullOrWhiteSpace(parsed.Token))
134+
{
135+
throw new InvalidOperationException(
136+
$"GitHub Copilot token exchange at {TokenEndpoint} returned a "
137+
+ $"payload with no 'token' field: {Truncate(body)}");
138+
}
139+
140+
if (parsed.ExpiresAt <= 0)
141+
{
142+
throw new InvalidOperationException(
143+
$"GitHub Copilot token exchange at {TokenEndpoint} returned an "
144+
+ $"invalid 'expires_at' value ({parsed.ExpiresAt}).");
145+
}
98146

99147
return new CachedToken(
100148
parsed.Token,

‎src/Netclaw.Providers/OAuth/OAuthDeviceFlowService.cs‎

Lines changed: 6 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -104,7 +104,7 @@ public OAuthDeviceFlowService(HttpClient httpClient, TimeProvider? timeProvider
104104
public async Task<DeviceAuthorizationResponse> StartDeviceAuthorizationAsync(
105105
OAuthDeviceFlowConfig config, CancellationToken ct = default)
106106
{
107-
var response = await PostFormAsync(
107+
using var response = await PostFormAsync(
108108
config.DeviceAuthorizationEndpoint,
109109
BuildDeviceAuthParams(config),
110110
ct);
@@ -143,7 +143,7 @@ public async Task<OAuthDeviceFlowResult> PollForTokenAsync(
143143
["client_id"] = config.ClientId
144144
};
145145

146-
var response = await PostFormAsync(config.TokenEndpoint, tokenParams, ct);
146+
using var response = await PostFormAsync(config.TokenEndpoint, tokenParams, ct);
147147

148148
var json = await response.Content.ReadAsStringAsync(ct);
149149
using var doc = JsonDocument.Parse(json);
@@ -213,7 +213,7 @@ public async Task<OAuthDeviceFlowResult> PollForTokenAsync(
213213
["refresh_token"] = refreshToken.Value
214214
};
215215

216-
var response = await PostFormAsync(tokenEndpoint, tokenParams, ct);
216+
using var response = await PostFormAsync(tokenEndpoint, tokenParams, ct);
217217

218218
if (!response.IsSuccessStatusCode)
219219
{
@@ -261,17 +261,17 @@ private OAuthDeviceFlowResult ParseTokenResponse(JsonElement root)
261261
// GitHub's OAuth endpoints return application/x-www-form-urlencoded by default
262262
// and only switch to JSON when the request explicitly asks for it. Other providers
263263
// (OpenAI Codex) already return JSON, so the header is a safe no-op.
264-
private Task<HttpResponseMessage> PostFormAsync(
264+
private async Task<HttpResponseMessage> PostFormAsync(
265265
string url,
266266
IEnumerable<KeyValuePair<string, string>> form,
267267
CancellationToken ct)
268268
{
269-
var request = new HttpRequestMessage(HttpMethod.Post, url)
269+
using var request = new HttpRequestMessage(HttpMethod.Post, url)
270270
{
271271
Content = new FormUrlEncodedContent(form),
272272
};
273273
request.Headers.Accept.Add(new MediaTypeWithQualityHeaderValue("application/json"));
274-
return _httpClient.SendAsync(request, ct);
274+
return await _httpClient.SendAsync(request, ct);
275275
}
276276

277277
private static List<KeyValuePair<string, string>> BuildDeviceAuthParams(OAuthDeviceFlowConfig config)

0 commit comments

Comments
 (0)