You signed in with another tab or window. Reload to refresh your session.You signed out in another tab or window. Reload to refresh your session.You switched accounts on another tab or window. Reload to refresh your session.Dismiss alert
.NET port of microsoft/Agent365-nodejs#290. Agent 365 observability (OBS) export now uses only the app-only S2S route, so customers no longer need to admin-consent the delegated Agent365.Observability.OtelWrite permission for telemetry.
The exporter always uses S2S. The sync and async exporters always post to /observabilityService/tenants/{tenantId}/otlp/agents/{agentId}/traces?api-version=1.
Agent365ExporterOptions.UseS2SEndpoint and Agent365ExporterCore.BuildEndpointPath(string, string, bool) are [Obsolete] and ignored, even when false.
There is no fallback to /observability. HTTP 401, 403 and 404 fail the batch with no retry and no delegated route.
Export authenticates only with the configured app-only resolver.TokenResolver / ContextualTokenResolver must return an app-only OBS token for the exporting agent and tenant.
The resolver is called once per tenant/agent identity group in each export batch, so it should cache tokens per agent and tenant.
A null or empty token, or a resolver exception, fails the batch before any HTTP request is sent.
Hosting cache: an app-only resolver replaces delegated OBO.
AgenticTokenCache now implements IExporterTokenCache<ObservabilityTokenResolver>.
New RefreshObservabilityToken(agentId, tenantId, resolver[, scopes]):
returns the cached token while it is still valid; otherwise it calls the resolver;
tokens without an exp claim expire 1 hour after they are acquired;
on failure it clears the cached token and rethrows.
Entries are keyed by the (agentId, tenantId) pair, so IDs that contain a colon can't collide.
RegisterObservability keeps its first-registration-wins behavior.
Concurrent refreshes for the same agent and tenant are serialized, so callers share one acquisition.
Cleanup (the timer and RemoveExpiredTokens) clears expired token values but keeps resolver registrations. A resolver registered once therefore keeps working after an idle period longer than the token lifetime.
The delegated RegisterObservability(..., AgenticTokenStruct, ...) overload and the AgenticTokenStruct constructor are [Obsolete(error: true)]. A call that bypasses the compiler logs once and never calls ExchangeTokenAsync.
The default OBS scope is /.default.EnvironmentUtils.GetObservabilityAuthenticationScope() now returns api://9b975845-388f-4429-889e-eab1ef63949c/.default, which is what client credentials requires. This matches PROD_OBSERVABILITY_SCOPE in the Node SDK.
Other changes: docs, READMEs and CHANGELOG updated; version 1.1-preview → 2.0-preview.
Unchanged: workload MCP/Graph/OBO authentication and ServiceTokenCache.
Compatibility
This is a breaking change, hence the major version bump:
Delegated/OBO OBS tokens are no longer an export path; the S2S route rejects scp tokens.
Callers of the delegated AgenticTokenStruct registration get a compile error that points them to RefreshObservabilityToken.
Code that sets UseS2SEndpoint still compiles; it gets a CS0618 warning.
The SDK does not validate token claims itself. Resolvers should check that the token is app-only:
accept idtyp=app; when idtyp is absent, accept a non-empty roles array or oid == sub;
reject any scp claim;
check aud and expiry.
This is documented in the Runtime and Hosting READMEs.
Coordinated export configuration (CLAUDE.md)
This PR changes two of the three constants that must stay in sync. All three have been reviewed together:
Previously /observability/... unless UseS2SEndpoint was set
ExportConfigConsistencyTests is updated to the new values. The live check below uses a token for this scope, sent to this host and path.
Validation
Full suite:dotnet test src/Microsoft.Agents.A365.Sdk.sln gives 818 passed / 9 skipped / 827 total. Main's baseline is 791 passed / 9 skipped / 800 total, and no tests were removed.
Build:dotnet build finishes with 0 warnings and 0 errors.
Mutation checks: each check temporarily reverted one behavior and confirmed that a test fails:
S2S route
empty token fails before sending
resolver called once per batch
401/403/404 cause no retry or fallback
cache early return
cache cleared on resolver failure
1-hour boundary for tokens without exp
first registration wins
IDs that contain a colon don't share a cache entry
concurrent refreshes share one acquisition, and a failed refresh doesn't clear another caller's token
cleanup keeps resolver registrations and skips a refresh in flight
a cache-driven refresh doesn't restore a resolver that an explicit refresh replaced
default /.default scope
removed delegated DI contract (compile error)
Live check against the production Agent 365 endpoint, using a registered agent instance in a test tenant:
Token acquisition: blueprint FMI assertion, then the agent's client_credentials grant for the OBS /.default scope. The token has idtyp=app, no app roles and no scp, so no OtelWrite grant is involved.
One invoke_agent activity was exported through Agent365Exporter in three configurations:
Configuration
Result
Default options
POST /observabilityService/tenants/{t}/otlp/agents/{a}/traces?api-version=1 → 200, ExportResult.Success
UseS2SEndpoint = false (deprecated)
Same S2S route → 200, ExportResult.Success
Resolver returns an empty token
ExportResult.Failure, and no HTTP request is sent
TokenResolver backed by AgenticTokenCache.RefreshObservabilityToken(agentId, tenantId, resolver), two exports
Both exports → 200. The resolver was called once and received the default scope api://9b975845-388f-4429-889e-eab1ef63949c/.default
… cleanup
- Serialize AgenticTokenCache.RefreshObservabilityToken per agent and tenant
so concurrent callers share one acquisition and a failed refresh cannot
clear a token another caller cached while it waited.
- RemoveExpiredTokens (and the cleanup timer) now clears expired token values
instead of removing entries, so a resolver registered once keeps working
after an idle period longer than the token lifetime. Entries with a refresh
in flight are skipped.
- IExporterTokenCache.RegisterObservability: whether a repeated registration
replaces the existing one is implementation-specific.
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Copilot-Session: 5cbf5f6b-cc40-4b7e-a591-65848db73a12
GetObservabilityToken snapshots entry.TokenResolver and entry.Scopes before acquiring the per-entry refresh lock. If a concurrent explicit refresh installs a new resolver and token after that snapshot, this stale call later enters RefreshObservabilityToken, writes the old resolver back at lines 190–191, and returns the newly cached token. The next expiry then uses the old resolver, contradicting the documented guarantee that an explicit refresh replaces the resolver used by future refreshes. Have the cache-driven path acquire the entry lock and read the current resolver/scopes under that lock without overwriting them.
Malformed or opaque tokens containing a period reach ReadJwtToken, which can throw SecurityTokenMalformedException; that exception is not derived from ArgumentException, so the refresh fails instead of treating the token as having no readable exp and applying the documented one-hour fallback. Check CanReadToken and catch token-parsing exceptions so all non-JWT token shapes follow the opaque-token path.
…n refresh
- GetObservabilityToken no longer snapshots the resolver before taking the
per-entry refresh lock. Cache-driven refreshes read the current resolver
under the lock and never write it, so a refresh queued behind an explicit
RefreshObservabilityToken cannot restore the resolver it replaced.
- Add regression tests pinning the opaque one-hour fallback for tokens the
JWT handler cannot parse (two segments, invalid base64url, non-JSON,
non-numeric exp).
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Copilot-Session: 5cbf5f6b-cc40-4b7e-a591-65848db73a12
Follow-up on the two "previously missed" findings in Copilot's second review (on d2b9ec8):
Stale resolver snapshot (medium): fixed in 03e642d. A new test queues a cache-driven GetObservabilityToken behind an explicit RefreshObservabilityToken, and it reproduced the problem: the queued refresh wrote the replaced resolver back. Cache-driven refreshes now read the current resolver under the per-entry lock and never overwrite it, so only an explicit refresh replaces it. The test is GetObservabilityToken_QueuedBehindExplicitRefresh_DoesNotRestoreReplacedResolver, and reverting the fix makes it fail.
Malformed tokens bypass the opaque fallback (medium): doesn't reproduce with the library version we ship. With System.IdentityModel.Tokens.Jwt 8.15.0, SecurityTokenMalformedException derives from SecurityTokenArgumentException, which derives from System.ArgumentException. The existing catch (ArgumentException) therefore handles it, and those tokens get the one-hour opaque fallback. I added regression tests that keep that behavior fixed if a future library version changes it. They cover a two-segment token, invalid base64url, non-JSON segments and a non-numeric exp.
Full suite: 817 passed / 9 skipped, with 0 build warnings.
The repository's required C# header format includes a blank line between the license header and the first using; this modified file currently places the using immediately after the header.
Declare scopes or use the documented three-argument overload
This standalone example passes scopes, but the variable is never declared in the snippet, so copying it does not compile. Use the three-argument overload here (which also demonstrates the documented default /.default scope), or define the array in the example.
The resolver-cache and migration snippets passed an undeclared `scopes`
variable. Use the three-argument RefreshObservabilityToken overload, which
passes the default app-only OBS scope.
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Copilot-Session: 5cbf5f6b-cc40-4b7e-a591-65848db73a12
ReadJwtToken reports malformed compact tokens with SecurityTokenMalformedException, which does not derive from ArgumentException. As a result, the new opaque-token fallback throws for cases such as the added opaque.token and not.a.jwt rows instead of applying the one-hour lifetime. Catch SecurityTokenException as well (or preflight with CanReadToken) so malformed tokens take the documented opaque-token path.
Clarify resolver runs once per identity partition
src/Observability/Runtime/README.md:16
A single OpenTelemetry export batch can contain several tenant/agent identities, and the implementation invokes the resolver once for each partition, not once for the whole batch. Update this guidance to avoid underestimating resolver calls.
Document resolver calls per identity group, not per batch
This invocation guarantee is inaccurate for a batch containing multiple tenant/agent identities. Agent365Exporter.Export partitions the batch and ExportBatchCoreAsync invokes the resolver inside the loop over those identity groups, so the resolver may run multiple times for one export batch. Document the actual per-identity-group behavior so consumers can size and cache their resolver correctly.
The resolver is called inside the loop over tenant/agent partitions, so one export batch can invoke it more than once. This design documentation should describe the per-identity-group cardinality rather than promising one call for the entire batch.
Verdict: Needs work · Risk: Medium — this is a breaking public authentication/export contract with one inaccurate resolver-invocation guarantee still open.
Should fix
src/Observability/Runtime/Tracing/Exporters/Agent365ExporterOptions.cs:62 — The public contract says the resolver runs once per export batch, but ExportBatchCoreAsync invokes it inside the tenant/agent partition loop. A mixed-identity batch can therefore invoke the resolver multiple times. Please describe this as once per tenant/agent identity group (and align src/Observability/Runtime/README.md, src/Observability/Runtime/docs/design.md, the CHANGELOG, and PR description). This matters because SDK consumers use this guarantee to size and cache token acquisition. (Customer Service, Performance, Senior Engineer · SemVer/public-contract accuracy, Azure WAF Performance Efficiency)
This exact-head issue was already raised by the structured Copilot review, so I did not add duplicate inline comments.
Persona roll-up
Security: No new finding. S2S-only routing is fail-closed on missing/empty token, plaintext HTTP remains rejected, no secret is logged, and CodeQL is green.
Privacy: No concerns; the change adds no data field, sink, retention, or residency path.
Performance: The per-key refresh lock and post-lock usability check correctly coalesce concurrent acquisition; the remaining documentation must state the real per-partition call cardinality.
Customer Service: Major-version migration, obsolete compatibility members, README/design guidance, and changelog are present; the resolver-call guarantee above remains inaccurate.
Business / COGS: No material storage, egress, cardinality, or CI-cost concern.
Senior Engineer: The prior cache-cleanup, concurrent-failure, stale-resolver, and opaque-token cases have exact-head regression coverage; no additional correctness finding survived source verification.
Architect: Scope, endpoint host, and S2S path are coordinated and snapshot-tested; the Node companion has merged and the remaining companion PRs are open under the documented coordinated-release plan.
Feedback-ledger and review reconciliation
No repository ledger entry required suppression.
Resolved prior cache findings were verified at a863f2bdfb902189ac8652cf8f0ceec52dc8335c.
The repeated malformed-token claim was suppressed: with the pinned IdentityModel version the thrown type derives from ArgumentException, and the exact-head malformed-token regressions pass in CI.
Low-severity header-spacing and standalone-snippet findings remain advisory and do not affect the approval gate.
Approval gate
Not approved: one consequential should-fix public-contract finding remains. All completed substantive checks are green, all review threads are resolved, and no structured review is still in progress. Merge remains subject to branch protection and the documented cross-SDK release coordination.
…ache keys
- The exporter invokes the OBS token resolver once per tenant/agent identity
group in each export batch, not once per batch. Correct the options XML doc,
Runtime README, design doc and CHANGELOG.
- Key AgenticTokenCache entries by an (agentId, tenantId) tuple instead of
"agentId:tenantId", so IDs that contain a colon cannot alias another
identity's cached token.
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Copilot-Session: 5cbf5f6b-cc40-4b7e-a591-65848db73a12
Resolver call count (your should-fix): the resolver is called inside the loop over tenant/agent groups in ExportBatchCoreAsync. The docs now say it runs once per tenant/agent identity group in each export batch, so a batch with several identities calls it several times. I updated the TokenResolver XML doc, Runtime/README.md, Runtime/docs/design.md, the CHANGELOG and the PR description.
Same key fix as your Python finding:AgenticTokenCache now keys entries by an (agentId, tenantId) tuple instead of "agentId:tenantId". The colon form predates this PR, but it had the same aliasing problem you found in Python (Use S2S-only OBS export and app-only hosting token resolvers Agent365-python#275). The regression test RefreshObservabilityToken_SeparatorBearingIds_DoNotShareCacheEntry fails if the string key is restored.
Full suite: 818 passed / 9 skipped, 0 build warnings. On the repeated malformed-token claim: I agree with your suppression, and the regression tests from 03e642d cover it.
The reason will be displayed to describe this comment to others. Learn more.
Re-review complete - approved
The approval gate passes at 90d4d100860f96d201f27415cacff6a193c2ca37.
Author-claimed fixes
Accepted - resolver invocation cardinality (90d4d10).Agent365ExporterOptions.TokenResolver, Runtime/README.md, Runtime/docs/design.md, the changelog, and the live PR summary now consistently state that the resolver runs once per tenant/agent identity group in each export batch. This matches ExportBatchCoreAsync, which resolves inside the identity-group loop. The prior should-fix is closed.
Accepted - separator-bearing cache identities (90d4d10).AgenticTokenCache now uses the tuple (AgentId, TenantId) as its dictionary key instead of concatenating IDs with :, and RefreshObservabilityToken_SeparatorBearingIds_DoNotShareCacheEntry pins the formerly-colliding pairs ("a:b", "c") and ("a", "b:c").
Accepted from the prior response - stale resolver snapshot (03e642d). Cache-driven refresh reads the current resolver under the per-entry lock and no longer restores an older resolver over an explicit replacement; the queued-get regression covers the ordering.
Accepted rebuttal - malformed-token opaque fallback (03e642d). With the pinned IdentityModel version, the malformed-token exception derives from ArgumentException; the exact-head regressions cover two-segment, invalid-base64url, non-JSON, and non-numeric-exp tokens. This is not an open finding.
Accepted from the earlier thread replies - concurrent refresh and cleanup (d2b9ec8). Per-entry locking, the post-lock usability check, and cleanup's preservation of resolver registrations are present with concurrency/lifecycle regressions.
Review evidence
Security: S2S-only authentication remains fail-closed; no new secret, delegated fallback, or isolation issue was introduced by this delta.
Privacy: No new data field, sink, retention, residency, or personal-data flow.
Performance: Tuple lookup remains O(1), and per-key refresh coalescing is unchanged.
Customer Service: The breaking resolver contract and migration guidance now describe the implementation accurately.
Business / COGS: No material storage, egress, cardinality, or paid-dependency change.
Senior Engineer: The delta is limited to contract alignment plus collision-safe cache identity and a focused regression; no new correctness or edge-case finding survived review.
Architect: The coordinated S2S host/scope/path contract is unchanged by this delta. The PR's cross-SDK merge order remains an operator merge gate, not an unresolved defect in this exact head.
All review threads are resolved. The exact-head Copilot structured review reports no findings. All completed substantive checks, including .NET SDK, both C# analyses, CodeQL, dependency review, and CLA, are green. A local targeted rerun could not restore from NuGet because the TLS handshake failed; exact-head CI is therefore the authoritative test result. Merge remains subject to branch protection and the documented coordinated-release order.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Jason-R-Lien
left a comment
Summary
.NET port of microsoft/Agent365-nodejs#290. Agent 365 observability (OBS) export now uses only the app-only S2S route, so customers no longer need to admin-consent the delegated
Agent365.Observability.OtelWritepermission for telemetry./observabilityService/tenants/{tenantId}/otlp/agents/{agentId}/traces?api-version=1.Agent365ExporterOptions.UseS2SEndpointandAgent365ExporterCore.BuildEndpointPath(string, string, bool)are[Obsolete]and ignored, even whenfalse./observability. HTTP 401, 403 and 404 fail the batch with no retry and no delegated route.TokenResolver/ContextualTokenResolvermust return an app-only OBS token for the exporting agent and tenant.AgenticTokenCachenow implementsIExporterTokenCache<ObservabilityTokenResolver>.RefreshObservabilityToken(agentId, tenantId, resolver[, scopes]):expclaim expire 1 hour after they are acquired;(agentId, tenantId)pair, so IDs that contain a colon can't collide.RegisterObservabilitykeeps its first-registration-wins behavior.RemoveExpiredTokens) clears expired token values but keeps resolver registrations. A resolver registered once therefore keeps working after an idle period longer than the token lifetime.RegisterObservability(..., AgenticTokenStruct, ...)overload and theAgenticTokenStructconstructor are[Obsolete(error: true)]. A call that bypasses the compiler logs once and never callsExchangeTokenAsync./.default.EnvironmentUtils.GetObservabilityAuthenticationScope()now returnsapi://9b975845-388f-4429-889e-eab1ef63949c/.default, which is what client credentials requires. This matchesPROD_OBSERVABILITY_SCOPEin the Node SDK.1.1-preview→2.0-preview.ServiceTokenCache.Compatibility
This is a breaking change, hence the major version bump:
scptokens.AgenticTokenStructregistration get a compile error that points them toRefreshObservabilityToken.UseS2SEndpointstill compiles; it gets a CS0618 warning.The SDK does not validate token claims itself. Resolvers should check that the token is app-only:
idtyp=app; whenidtypis absent, accept a non-emptyrolesarray oroid == sub;scpclaim;audand expiry.This is documented in the Runtime and Hosting READMEs.
Coordinated export configuration (CLAUDE.md)
This PR changes two of the three constants that must stay in sync. All three have been reviewed together:
ProdObservabilityScopeapi://9b975845-388f-4429-889e-eab1ef63949c/.default.../Agent365.Observability.OtelWritescopeDefaultEndpointHostagent365.svc.cloud.microsoftBuildEndpointPath()/observabilityService/tenants/{tenantId}/otlp/agents/{agentId}/traces/observability/...unlessUseS2SEndpointwas setExportConfigConsistencyTestsis updated to the new values. The live check below uses a token for this scope, sent to this host and path.Validation
dotnet test src/Microsoft.Agents.A365.Sdk.slngives 818 passed / 9 skipped / 827 total. Main's baseline is 791 passed / 9 skipped / 800 total, and no tests were removed.dotnet buildfinishes with 0 warnings and 0 errors.exp/.defaultscopeclient_credentialsgrant for the OBS/.defaultscope. The token hasidtyp=app, no app roles and noscp, so no OtelWrite grant is involved.invoke_agentactivity was exported throughAgent365Exporterin three configurations:POST /observabilityService/tenants/{t}/otlp/agents/{a}/traces?api-version=1→ 200,ExportResult.SuccessUseS2SEndpoint = false(deprecated)ExportResult.SuccessExportResult.Failure, and no HTTP request is sentTokenResolverbacked byAgenticTokenCache.RefreshObservabilityToken(agentId, tenantId, resolver), two exportsapi://9b975845-388f-4429-889e-eab1ef63949c/.defaultMerge gate
Ship together with the other PRs for this change: microsoft/Agent365-nodejs#290 (Node SDK), microsoft/Agent365-python#275 (Python SDK), microsoft/Agent365-devTools#501 (CLI stops requesting OtelWrite admin consent), microsoft/Agent365-Samples#339 (samples), and microsoft/agent365-skills#84 (skills). Merge the CLI PR last.