Use S2S-only OBS export and app-only hosting token resolvers - #290
Krishnadheeraj (DheerajPannala) wants to merge 3 commits into
Conversation
Always route OBS to observabilityService, retain the legacy endpoint option as ignored compatibility state, and replace delegated hosting-cache exchange with an explicit app-only resolver. Co-authored-by: Copilot <[email protected]>
There was a problem hiding this comment.
🟡 Changes recommended
There are a couple of actionable review findings (type-only imports to avoid runtime dependencies and a brittle/slow cache-capacity test) that should be addressed before approval.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
This PR updates the observability (OBS) export pathing and hosting token cache contract to enforce S2S-only export routing and require app-only token acquisition, aligning exporter behavior, tests, and documentation with the new authentication model.
Changes:
- Route all OBS exports (batch + per-request) to
/observabilityServiceand deprecate/ignoreuseS2SEndpoint(even whenfalse). - Change hosting
AgenticTokenCache.RefreshObservabilityTokento require an app-only resolver; legacyTurnContext/Authorizationoverload now throws and token acquisition failures propagate. - Update docs/changelog and adjust tests to cover the new endpoint and token acquisition/expiry behaviors.
File summaries
| File | Description |
|---|---|
| tests/observability/extension/hosting/agentic-token-cache.test.ts | Reworked tests for app-only OBS resolver flow, error propagation, expiry/TTL, and cache behavior. |
| tests/observability/core/agent365-exporter.test.ts | Updated expectations to /observabilityService and added coverage for legacy flag behavior and no OBO fallback. |
| packages/agents-a365-observability/src/tracing/exporter/Agent365ExporterOptions.ts | Documented app-only token requirement; deprecated/ignored useS2SEndpoint with updated default. |
| packages/agents-a365-observability/src/tracing/exporter/Agent365Exporter.ts | Removed endpoint switching and always targets /observabilityService. |
| packages/agents-a365-observability/src/index.ts | Re-exported TokenResolver type for consumers. |
| packages/agents-a365-observability/README.md | Documented S2S-only routing and app-only token requirements; migration guidance for hosting cache. |
| packages/agents-a365-observability-hosting/src/index.ts | Exported ObservabilityTokenResolver type. |
| packages/agents-a365-observability-hosting/src/caching/AgenticTokenCache.ts | Introduced app-only resolver overload; legacy overload throws; improved failure surfacing and expiry metadata handling. |
| packages/agents-a365-observability-hosting/docs/design.md | Updated design guidance for app-only OBS token acquisition and the new cache API. |
| CHANGELOG.md | Documented breaking changes for S2S-only export routing and hosting cache resolver requirement. |
Review details
- Files reviewed: 10/10 changed files
- Comments generated: 2
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Jason-R-Lien
left a comment
Jason-R-Lien
left a comment
There was a problem hiding this comment.
Full first-pass panel review
Verdict: Needs work (high risk). The S2S-only route is a security-sensitive public authentication-contract change. Batch export and the hosting cache now have an app-only resolver path, but per-request export still ignores that resolver and forwards the existing OTel-context token to the new S2S endpoint. That leaves a supported mode unable to satisfy the contract and can drop all per-request telemetry with 401/403 responses.
Blocking
packages/agents-a365-observability/src/tracing/exporter/Agent365Exporter.ts:192- Per-request export is routed to/observabilityService, but the token branch at lines 210-212 still always usesgetExportToken().ObservabilityBuilder.createPerRequestProcessor()also does not propagate a configuredtokenResolver, and the changed test explicitly proves an arbitrary context token is forwarded. Existing AI Teammate/OBO callers therefore keep sending delegated tokens that the PR says S2S rejects. Wire an app-only resolver into per-request export (or introduce a distinctly contracted app-only context), update the public token-context migration guidance, and add a regression that distinguishes delegated from app-only acquisition.
Existing unresolved review items (not duplicated)
- Copilot: use type-only imports in
AgenticTokenCache.ts. - Copilot: avoid hard-coding and iterating through 10,001 cache entries in the eviction test.
Trade-off
Removing the delegated/OBO fallback is the correct security direction; the fix should preserve that invariant rather than restore the old route. The missing piece is an app-only token source for every export mode.
Persona roll-up
- Security: Blocking token-source/trust-boundary mismatch in per-request mode; no secret exposure or authorization fallback added elsewhere.
- Privacy: No new collection, retention, or tenant-mixing path; the loss-of-user-attribution caveat is documented.
- Performance: Retries and cache lifetime remain bounded; the expensive eviction test is already covered by an existing thread.
- Customer service: The changelog describes the break, but per-request consumers lack a working migration path.
- Business / COGS: No material storage, egress, cardinality, or provisioning increase.
- Senior engineer: The changed test verifies routing but not the new authentication invariant, allowing a production telemetry outage to pass.
- Architect: Batch/cache and per-request modes now implement different credential contracts behind one public exporter API.
Feedback ledger
No repository-specific ledger exists yet. The two existing Copilot findings were suppressed from new inline comments to avoid duplication.
Approval gate
Not approved: one blocking finding remains, two prior review threads are unresolved, and the branch is behind main. All exact-head CI checks currently pass, but green CI does not exercise the delegated-token counterfactual described above.
| const servicePrefix = this.options.useS2SEndpoint ? '/observabilityService' : '/observability'; | ||
| const endpointRelativePath = `${servicePrefix}/tenants/${encodeURIComponent(tenantId)}/otlp/agents/${encodeURIComponent(agentId)}/traces`; | ||
| // OBS routing is independent of the agent's workload authentication flow. | ||
| const endpointRelativePath = `/observabilityService/tenants/${encodeURIComponent(tenantId)}/otlp/agents/${encodeURIComponent(agentId)}/traces`; |
There was a problem hiding this comment.
[blocking] Security / Senior Engineer / Architect: This now forces per-request exports onto the S2S route, but the per-request branch still ignores options.tokenResolver and sends getExportToken() from the existing OTel context. ObservabilityBuilder.createPerRequestProcessor() likewise does not pass the configured resolver, and the changed test proves only that an arbitrary tok-from-context is forwarded. Existing AI Teammate/OBO callers will therefore send delegated tokens that this PR says S2S rejects, causing all per-request telemetry to fail with 401/403. Please source an app-only resolver in per-request mode (or use a separately contracted app-only context), update the public migration docs, and add a delegated-vs-app-only regression without restoring the OBO fallback.
There was a problem hiding this comment.
Fixed in 78deb5d.
- Batch and per-request export now share
ObservabilityBuilder.createExporterOptions(), so per-request mode receives the same configured app-onlytokenResolver(builder method takes precedence overexporterOptions.tokenResolver). Agent365Exporterno longer readsgetExportToken(); a token placed inrunWithExportTokenis never used as the OBS credential.- An enabled exporter without a resolver now fails at configuration, and the error names the fix (
withTokenResolver(...)). Empty tokens or acquisition failures fail the export without sending a request. 401/403/404 stay on the S2S route; no OBO fallback was restored. - New regressions distinguish delegated from app-only acquisition: a delegated JWT in request context is ignored while the resolver's roleless app token is sent, exercised through the real builder -> per-request processor -> exporter path, including concurrent identities and resolver precedence.
- README, CHANGELOG and the hosting design doc now describe the per-request migration and note that resolvers should cache (the exporter calls them per export batch).
Use the configured OBS resolver for batch and per-request export without ambient-token or OBO-route fallback. Fail missing-token exports explicitly, add delegated-context counterfactuals and builder integration coverage, declare the tooling axios dependency, and update migration guidance and cache tests.
Review response amendments:
- Startup error now names the fix ("Per-request export now requires withTokenResolver(...)") and points at AgenticTokenCache for caching.
- README and CHANGELOG document that resolvers must cache; the exporter invokes the resolver on every batch and per identity group.
- Defensive resolver guard in exportGroup now comments that it only catches post-construction mutation; the constructor is the primary check.
Co-authored-by: Copilot <[email protected]>
Copilot-Session: 5cbf5f6b-cc40-4b7e-a591-65848db73a12
Bring in main's dependency security overrides. Align the new tooling axios dependency with main's axios override (^1.16.0, resolved 1.20.0) and record it with the override specifier in pnpm-lock.yaml, matching how main records hono. Co-authored-by: Copilot <[email protected]> Copilot-Session: 5cbf5f6b-cc40-4b7e-a591-65848db73a12
Review feedback addressedPushed 78deb5d (fixes) and 65ffbcc (merge of Changes
Breaking change: per-request users who relied on Validation
|
There was a problem hiding this comment.
Copilot review overview
🟡 Changes recommended
Critical resolver wiring and migration documentation findings remain unresolved.
Get a fresh assessment by requesting another Copilot review.
Review effort: Lite
Findings: 2
Open (2)
Resolved since last review (2)
Files not reviewed (1)
- pnpm-lock.yaml: Generated file
| Batch and per-request exports both call the configured `tokenResolver` with the | ||
| exporting agent and tenant IDs. Configure it with `withTokenResolver(...)` or | ||
| `exporterOptions.tokenResolver`; the explicit builder method takes precedence. |
| The configured app-only OBS resolver is required in both batch and per-request | ||
| modes. The builder merges resolver options consistently, with `withTokenResolver` | ||
| taking precedence over `exporterOptions.tokenResolver`. Request context is retained | ||
| for tracing, but its token is not consumed by `Agent365Exporter`. See the | ||
| [per-request migration guide](../README.md#migrating-per-request-authentication). |
Summary
/observabilityService, including batch and per-request exports. RetainuseS2SEndpointfor compatibility but ignore it, even when false; never fall back to/observability.AgenticTokenCache.RefreshObservabilityToken. The legacy user-authorization overload now fails explicitly instead of acquiring a delegated OBS token.Compatibility
This changes OBS routing and the hosting cache's authentication contract. Callers must supply an app-only OBS token for the exporting agent and tenant; a delegated
scptoken cannot authenticate the S2S route. Endpoint selection does not mint or convert tokens.Validation