Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
61 changes: 14 additions & 47 deletions agents/Aevatar.GAgents.ChannelRuntime/AgentBuilderTool.cs
Original file line number Diff line number Diff line change
Expand Up @@ -233,51 +233,30 @@ private async Task<string> CreateDailyReportAgentAsync(
? SkillRunnerDefaults.GenerateActorId()
: args.Str("agent_id")!.Trim();

// Diagnostic (#417 follow-up, PR for production 403 root-cause hunt): log the resolved
// service-id list and the exact payload we're about to send to NyxID `POST /api-keys`.
// The api-key request body is otherwise opaque in production; without this we can't tell
// whether the deployed binary actually carries the `allow_all_services=false` field, or
// whether `requiredServiceIds` resolved to per-user `UserService.id`s vs catalog ids.
var apiKeyPayload = BuildCreateApiKeyPayload(agentId, requiredServiceIds.value!);
_logger?.LogInformation(
"Diagnostic[#417]: minting agent api-key. agentId={AgentId} requiredSlugs={Slugs} resolvedServiceIds={Ids} payload={Payload}",
agentId,
string.Join(",", templateSpec!.RequiredServiceSlugs),
string.Join(",", requiredServiceIds.value!),
apiKeyPayload);

var createKeyResponse = await nyxClient.CreateApiKeyAsync(token, apiKeyPayload, ct);
var createKeyResponse = await nyxClient.CreateApiKeyAsync(
token,
BuildCreateApiKeyPayload(agentId, requiredServiceIds.value!),
ct);

if (IsErrorPayload(createKeyResponse))
return createKeyResponse;

if (!TryParseApiKeyCreateResponse(createKeyResponse, out var apiKeyId, out var apiKeyValue, out var apiKeyError))
return JsonSerializer.Serialize(new { error = apiKeyError });

_logger?.LogInformation(
"Diagnostic[#417]: agent api-key created. agentId={AgentId} apiKeyId={ApiKeyId}",
agentId,
apiKeyId);

// Issue aevatarAI/aevatar#411 / #417 follow-up: catch in-flight GitHub OAuth revocation.
// The earlier `BuildGitHubAuthorizationResponseAsync` check covers the "no provider token
// at all" case; this preflight only earns its keep when the OAuth grant was revoked or
// had its scopes downgraded between connect-time and create-time.
// Issue aevatarAI/aevatar#411 / #417 follow-up: catch in-flight GitHub-side issues.
// The earlier `BuildGitHubAuthorizationResponseAsync` check covers the "no provider
// token at all" case; this preflight catches misconfigurations that only surface at
// request time (the original case under #421 was a missing `User-Agent` header that
// GitHub rejects with 403; OAuth grant revocation is the other one).
//
// Codex review (PR #418 r3141846175): even though the api-key itself is correctly
// configured under #417, we still revoke it on preflight failure. Reason: each retry of
// `/daily` mints a *new* api-key before the preflight runs, so without revoke the user's
// NyxID account accumulates one orphan proxy-scoped key per failed retry until they
// re-authorize GitHub. The revoke is a best-effort cleanup, not a safety claim about the
// key's correctness.
// PR #418 review r3141846175: revoke the freshly-minted key on preflight failure so
// each `/daily` retry doesn't leave another orphan proxy-scoped key behind in the
// user's NyxID account. The revoke is best-effort cleanup, not a safety claim about
// the key's correctness.
var preflight = await PreflightGitHubProxyAsync(nyxClient, apiKeyValue!, providerSlug, ct);
if (preflight is not null)
{
_logger?.LogWarning(
"Diagnostic[#417]: preflight failed; revoking new api-key. agentId={AgentId} apiKeyId={ApiKeyId} preflightResponse={Response}",
agentId,
apiKeyId,
preflight);
await BestEffortRevokeApiKeyAsync(nyxClient, token, apiKeyId!, "github_preflight_failed", ct);
return preflight;
}
Expand Down Expand Up @@ -1778,18 +1757,6 @@ private static string NormalizeScopeId(string? value) =>
extraHeaders: null,
ct);

// Diagnostic (#417 follow-up): log the *raw* probe response. The parsed JSON envelope
// we return as `proxy_body` only carries the inner Lark/GitHub `body` string when
// SendAsync wraps non-2xx; the unparsed `probe` is the only place we see what NyxID
// actually returned. Distinguishes:
// - `{"error":true,"status":403,"error_code":9000,"message":"API key does not have access..."}` → NyxID `ApiKeyScopeForbidden` (our payload still wrong)
// - `{"error":true,"status":403,"body":"{\"message\":\"Bad credentials\",...}"}` → GitHub upstream 403 (OAuth grant revoked)
// - other shapes → unclassified
_logger?.LogInformation(
"Diagnostic[#417]: GitHub preflight probe response. providerSlug={ProviderSlug} probe={Probe}",
nyxProviderSlug,
string.IsNullOrWhiteSpace(probe) ? "<empty>" : probe);

if (string.IsNullOrWhiteSpace(probe))
return null;

Expand Down Expand Up @@ -1833,7 +1800,7 @@ private static string NormalizeScopeId(string? value) =>
detail = string.IsNullOrWhiteSpace(detail) ? "GitHub proxy returned 401/403 for the new agent API key." : detail,
http_status = status,
proxy_body = string.IsNullOrWhiteSpace(body) ? null : body,
hint = "GitHub returned 401/403 to NyxID's stored OAuth token. The token was likely revoked at GitHub or had its scopes downgraded after the provider was connected. Re-authorize the GitHub provider at NyxID, then run /daily again.",
hint = "GitHub returned 401/403 through the NyxID proxy. Common causes: (a) the OAuth grant for GitHub was revoked at github.com/settings/applications or its scopes were downgraded — re-authorize the GitHub provider at NyxID; (b) the request reached GitHub without a User-Agent header (NyxIdApiClient now sends a default; if you see this, check that the deployed binary includes that fix). The agent will not produce a useful daily report until proxy access succeeds.",
Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This changed hint currently breaks the existing ChannelRuntime test suite: ExecuteAsync_CreateAgent_DailyReport_FailsClosed_When_GithubProxyDeniedForNewKey still asserts the hint contains Re-authorize, but the new text contains lowercase re-authorize. I ran dotnet test test/Aevatar.GAgents.ChannelRuntime.Tests/Aevatar.GAgents.ChannelRuntime.Tests.csproj --nologo at 2140fcb052f6ce9c67980abd1d16e8127c8204ce; result was Failed: 1, Passed: 420, Total: 421. Please update either the assertion or the message before merge.

nyx_provider_slug = nyxProviderSlug,
});
}
Expand Down
27 changes: 27 additions & 0 deletions src/Aevatar.AI.ToolProviders.NyxId/NyxIdApiClient.cs
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,19 @@ public sealed record NyxIdChannelRelayReplyResult(
/// <summary>HTTP client for calling NyxID REST API endpoints.</summary>
public sealed class NyxIdApiClient
{
/// <summary>
/// Default <c>User-Agent</c> injected on every call to <see cref="ProxyRequestAsync"/>
/// when the caller does not specify one in <c>extraHeaders</c>. GitHub's REST API rejects
/// requests without a <c>User-Agent</c> with HTTP 403 ("Request forbidden by administrative
/// rules") — see https://docs.github.com/en/rest/overview/resources-in-the-rest-api#user-agent-required.
/// .NET's <c>HttpClient</c> does not set one by default; NyxID proxies the client's headers
/// through to GitHub, so the absence at the .NET layer manifests as a GitHub 403 in
/// production. CLI tools written against <c>reqwest</c> (e.g. <c>nyxid proxy request</c>)
/// happen to send <c>reqwest/x.y</c> as their default and so never hit this.
/// </summary>
public const string DefaultProxyUserAgent = "aevatar-agent-builder";
private const string UserAgentHeaderName = "User-Agent";

private readonly HttpClient _http;
private readonly NyxIdToolOptions _options;
private readonly ILogger _logger;
Expand Down Expand Up @@ -131,12 +144,26 @@ public async Task<string> ProxyRequestAsync(
using var request = new HttpRequestMessage(httpMethod, url);
request.Headers.Authorization = new AuthenticationHeaderValue("Bearer", token);

var callerSpecifiedUserAgent = false;
if (extraHeaders != null)
{
foreach (var (key, value) in extraHeaders)
{
request.Headers.TryAddWithoutValidation(key, value);
if (string.Equals(key, UserAgentHeaderName, StringComparison.OrdinalIgnoreCase))
callerSpecifiedUserAgent = true;
}
}

// GitHub-required User-Agent (#417 follow-up). NyxID proxies whatever the .NET client
// sends, and HttpClient sends none by default, so without this every GitHub call lands
// as 403 "Request forbidden by administrative rules". Inject a default for *all* proxy
// targets — non-GitHub services don't care about UA either way, and pinning it at the
// proxy boundary means SkillRunner / agent-builder / preflight all benefit without
// every call site remembering to pass it.
if (!callerSpecifiedUserAgent)
request.Headers.TryAddWithoutValidation(UserAgentHeaderName, DefaultProxyUserAgent);

if (!string.IsNullOrEmpty(body) && httpMethod != HttpMethod.Get && httpMethod != HttpMethod.Head)
{
request.Content = new StringContent(body, Encoding.UTF8, "application/json");
Expand Down
60 changes: 60 additions & 0 deletions test/Aevatar.AI.Tests/NyxIdApiClientCoverageTests.cs
Original file line number Diff line number Diff line change
Expand Up @@ -175,6 +175,66 @@ public async Task UpdateChannelRelayTextReplyAsync_ShouldRejectEmptyText()
result.Should().BeEquivalentTo(new NyxIdChannelRelayReplyResult(false, Detail: "missing_reply_text"));
}

[Fact]
public async Task ProxyRequestAsync_ShouldInjectDefaultUserAgent_WhenCallerOmitsIt()
{
// #417 follow-up: GitHub's REST API rejects requests without a `User-Agent` header
// with a 403 "Request forbidden by administrative rules". .NET's `HttpClient` doesn't
// send one by default, and NyxID proxies whatever the .NET client sends — so without
// this default, every agent-builder GitHub call lands as a spurious 403 (root cause of
// production /daily failures captured under PR #420 diagnostic logs).
var handler = new CaptureHandler(new HttpResponseMessage(HttpStatusCode.OK)
{
Content = new StringContent("{}", Encoding.UTF8, "application/json"),
});
var client = new NyxIdApiClient(
new NyxIdToolOptions { BaseUrl = "https://nyx.example.com" },
new HttpClient(handler),
NullLogger<NyxIdApiClient>.Instance);

await client.ProxyRequestAsync(
"token",
"api-github",
"/rate_limit",
"GET",
body: null,
extraHeaders: null,
CancellationToken.None);

handler.LastRequest.Should().NotBeNull();
var ua = handler.LastRequest!.Headers.UserAgent.ToString();
ua.Should().Be(NyxIdApiClient.DefaultProxyUserAgent);
}

[Fact]
public async Task ProxyRequestAsync_ShouldHonorCallerSuppliedUserAgent()
{
// The default is only injected when the caller doesn't specify one — agent code that
// wants to identify as a different UA (e.g. SkillRunner with a per-template label)
// should win over the boundary default.
var handler = new CaptureHandler(new HttpResponseMessage(HttpStatusCode.OK)
{
Content = new StringContent("{}", Encoding.UTF8, "application/json"),
});
var client = new NyxIdApiClient(
new NyxIdToolOptions { BaseUrl = "https://nyx.example.com" },
new HttpClient(handler),
NullLogger<NyxIdApiClient>.Instance);

await client.ProxyRequestAsync(
"token",
"api-github",
"/rate_limit",
"GET",
body: null,
extraHeaders: new Dictionary<string, string> { ["User-Agent"] = "custom-skill-runner/1.2.3" },
CancellationToken.None);

handler.LastRequest.Should().NotBeNull();
var ua = handler.LastRequest!.Headers.UserAgent.ToString();
ua.Should().Be("custom-skill-runner/1.2.3");
}

[Fact]
public void TryParseErrorEnvelope_ShouldHandleEmptyInvalidAndStructuredResponses()
{
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -434,7 +434,10 @@ public async Task ExecuteAsync_CreateAgent_DailyReport_FailsClosed_When_GithubPr
doc.RootElement.GetProperty("http_status").GetInt32().Should().Be(403);
// The hint should point users at re-authorizing the GitHub provider at NyxID, not
// at api-key bindings (which used to be the misdiagnosis under #411 — see #417).
doc.RootElement.GetProperty("hint").GetString().Should().Contain("Re-authorize");
// Match case-insensitively so future hint copy edits (capitalization, punctuation)
// don't require flipping this assertion in lockstep — the *intent* of the assertion
// is "hint mentions re-authorization", not "hint matches one specific prefix".
doc.RootElement.GetProperty("hint").GetString()!.ToLowerInvariant().Should().Contain("re-authorize");

// The actor must NOT receive InitializeSkillRunnerCommand — preflight aborts
// BEFORE the actor is invoked so we don't leave a broken agent in the catalog.
Expand Down
Loading