Use S2S-only OBS with isolated app-token providers across samples - #339
Open
Krishnadheeraj (DheerajPannala) wants to merge 8 commits into
Open
Krishnadheeraj (DheerajPannala) wants to merge 8 commits into
Krishnadheeraj (DheerajPannala) wants to merge 8 commits into
Conversation
Configure S2S OBS across sample languages, add isolated blueprint-to-agent application-token providers, preserve workload OBO, and cover routing and authentication failure paths. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Krishnadheeraj (DheerajPannala)
requested a review
from a team
as a code owner
September 9, 2026 20:56
Dependency ReviewThe following issues were found:
License Issues.github/workflows/ci-observability.yml
python/crewai/sample_agent/pyproject.toml
OpenSSF ScorecardScorecard details
Scanned Files
|
Copilot started reviewing on behalf of
Krishnadheeraj (DheerajPannala)
September 9, 2026 20:56
View session
![github-code-quality[bot]](https://avatars.githubusercontent.com/u/9919?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
🟡 Changes recommended
The new OBS-only token helper has an opaque error message (“not a ******”) across multiple samples and the .NET managed-identity assertion scope is likely incorrect without the /.default suffix.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
This PR standardizes Agent 365 observability (OBS) export across the repository to use S2S-only ingestion (/observabilityService), and introduces isolated app-only token providers for interactive samples so OBS authentication is kept separate from business MCP/Graph/OBO authentication.
Changes:
- Adds/updates sample-local OBS app-token resolvers (blueprint FMI → agent client_credentials) and wires them into Node.js, Python, and .NET sample observability configuration.
- Removes legacy “per-request export” and delegated-token fallback paths, and hardens token/expiry/identity validation behavior in samples and tests.
- Updates READMEs,
.env/appsettings templates, and adds regression coverage for route selection and configuration.
File summaries
| File | Description |
|---|---|
| README.md | Documents S2S-only OBS routing, token requirements, and validation commands |
| tests/observability/test_s2s_export.py | Adds mocked HTTP regression test ensuring no legacy route fallback |
| tests/e2e/Agent365.E2E.Tests.csproj | Links shared .NET OBS token provider + fixture sources into E2E test project |
| python/docs/design.md | Updates Python design guidance to use OBS-only token service + exporter options |
| python/openai/sample-agent/README.md | Documents dedicated OBS credentials and flow for OpenAI Python sample |
| python/openai/sample-agent/pyproject.toml | Raises observability core minimum to >= 1.0.0 |
| python/openai/sample-agent/host_agent_server.py | Removes delegated token exchange/caching for OBS |
| python/openai/sample-agent/docs/design.md | Updates design to use exporter_options with S2S + OBS-only resolver |
| python/openai/sample-agent/agent.py | Switches to OBS-only token resolver + exporter_options S2S configuration |
| python/openai/sample-agent/AGENT-CODE-WALKTHROUGH.md | Updates walkthrough to use exporter_options + OBS-only resolver |
| python/openai/sample-agent/.env.template | Adds AGENT365_OBS_* settings; removes legacy KAIRO flag |
| python/openai/sample-agent/observability_token_service.py | Adds sample-local OBS-only token acquisition/cache helper |
| python/observability-with-otlp/README.md | Adds optional A365 S2S export section + prerequisites |
| python/observability-with-otlp/pyproject.toml | Pins observability core minimum to >= 1.0.0 |
| python/observability-with-otlp/main.py | Uses exporter_options with S2S + OBS-only resolver; stamps AgentDetails IDs |
| python/observability-with-otlp/.env.template | Adds AGENT365_OBS_* settings for optional exporter |
| python/observability-with-otlp/observability_token_service.py | Adds sample-local OBS-only token acquisition/cache helper |
| python/observability-with-langgraph/README.md | Documents optional S2S export + removes stub-token narrative |
| python/observability-with-langgraph/pyproject.toml | Pins observability core minimum to >= 1.0.0 |
| python/observability-with-langgraph/main.py | Updates to exporter_options S2S + OBS-only resolver and newer scope APIs |
| python/observability-with-langgraph/.env.template | Adds AGENT365_OBS_* settings for optional exporter |
| python/observability-with-langgraph/observability_token_service.py | Adds sample-local OBS-only token acquisition/cache helper |
| python/observability-with-azure-monitor/README.md | Documents optional S2S export + removes stub-token narrative |
| python/observability-with-azure-monitor/pyproject.toml | Pins observability core minimum to >= 1.0.0 |
| python/observability-with-azure-monitor/main.py | Uses exporter_options S2S + adds explicit baggage context for standalone demo |
| python/observability-with-azure-monitor/.env.template | Adds AGENT365_OBS_* settings for optional exporter |
| python/observability-with-azure-monitor/observability_token_service.py | Adds sample-local OBS-only token acquisition/cache helper |
| python/google-adk/sample-agent/README.md | Documents OBS S2S auth + clarifies app-id vs agent-user attribution |
| python/google-adk/sample-agent/pyproject.toml | Raises observability core minimum to >= 1.0.0 |
| python/google-adk/sample-agent/main.py | Uses exporter_options S2S + OBS-only resolver |
| python/google-adk/sample-agent/agent.py | Uses agentic_app_id (or OBS env) instead of agent-user ID for OBS baggage |
| python/google-adk/sample-agent/.env.template | Adds AGENT365_OBS_* settings |
| python/google-adk/sample-agent/observability_token_service.py | Adds sample-local OBS-only token acquisition/cache helper |
| python/crewai/sample_agent/start_with_generic_host.py | Switches to exporter_options S2S + OBS-only resolver |
| python/crewai/sample_agent/README.md | Documents shared OBS-only resolver for both bootstraps |
| python/crewai/sample_agent/pyproject.toml | Raises observability core minimum to >= 1.0.0 |
| python/crewai/sample_agent/host_agent_server.py | Removes delegated token exchange/caching; wires exporter_options S2S + OBS-only resolver |
| python/crewai/sample_agent/.env.template | Adds AGENT365_OBS_* settings |
| python/crewai/sample_agent/observability_token_service.py | Adds sample-local OBS-only token acquisition/cache helper |
| python/claude/sample-agent/README.md | Documents dedicated OBS credentials and flow |
| python/claude/sample-agent/pyproject.toml | Raises observability core minimum to >= 1.0.0 |
| python/claude/sample-agent/observability_config.py | Uses exporter_options S2S + OBS-only resolver |
| python/claude/sample-agent/host_agent_server.py | Removes delegated token exchange/caching for OBS |
| python/claude/sample-agent/.env.template | Adds AGENT365_OBS_* settings |
| python/claude/sample-agent/observability_token_service.py | Adds sample-local OBS-only token acquisition/cache helper |
| python/agent-framework/sample-agent/README.md | Documents required OBS-only credentials when distro export is enabled |
| python/agent-framework/sample-agent/host_agent_server.py | Uses distro S2S + OBS-only resolver; removes delegated token exchange/caching |
| python/agent-framework/sample-agent/agent.py | Removes legacy cached-token resolver block from agent |
| python/agent-framework/sample-agent/AGENT-CODE-WALKTHROUGH.md | Updates observability section to distro S2S + OBS-only resolver |
| python/agent-framework/sample-agent/.env.template | Adds required AGENT365_OBS_* settings |
| python/agent-framework/sample-agent/observability_token_service.py | Adds sample-local OBS-only token acquisition/cache helper |
| python/autonomous/github-trending/observability_token_service.py | Hardens OBS token acquisition expiry validation and cache behavior |
| python/autonomous/github-trending/main.py | Fails closed when OBS token missing/expired (no empty-token fallback) |
| nodejs/docs/design.md | Updates Node.js design guidance for S2S + OBS-only token resolver patterns |
| nodejs/openai/sample-agent/src/otel.ts | Adds early OBS bootstrap: S2S enabled + token resolver + per-request export guard |
| nodejs/openai/sample-agent/src/observability-token-service.ts | Adds OBS-only blueprint FMI → agent token resolver with strict validation |
| nodejs/openai/sample-agent/src/index.ts | Imports ./otel first to ensure early OBS configuration |
| nodejs/openai/sample-agent/src/client.ts | Removes in-module manager setup; scopes now use turn context identities |
| nodejs/openai/sample-agent/src/agent.ts | Removes delegated token preloading; stamps agent/tenant baggage explicitly |
| nodejs/openai/sample-agent/README.md | Documents OBS-only app auth + legacy service route behavior |
| nodejs/openai/sample-agent/docs/design.md | Updates design docs to reference otel.ts bootstrap and resolver |
| nodejs/openai/sample-agent/AGENT-CODE-WALKTHROUGH.md | Updates walkthrough for new bootstrap + scope signature changes |
| nodejs/openai/sample-agent/.env.template | Adds AGENT365_OBS_* settings; removes custom resolver toggle |
| nodejs/langchain/sample-agent/src/observability-token-service.ts | Adds OBS-only token resolver helper |
| nodejs/langchain/sample-agent/src/index.ts | Distro S2S enabled + durable delivery replay disabled + OBS-only resolver |
| nodejs/langchain/sample-agent/src/client.ts | Uses turnContext/env tenant+agent IDs for attribution |
| nodejs/langchain/sample-agent/src/agent.ts | Removes delegated token preload logic |
| nodejs/langchain/sample-agent/README.md | Documents OBS-only app auth and S2S exporter behavior |
| nodejs/langchain/sample-agent/package.json | Bumps @microsoft/opentelemetry to ^1.4.0 |
| nodejs/langchain/sample-agent/docs/design.md | Updates design docs package references for distro usage |
| nodejs/langchain/sample-agent/Agent-Code-Walkthrough.md | Updates walkthrough imports and scope signature |
| nodejs/langchain/sample-agent/.env.example | Adds AGENT365_OBS_* settings; removes legacy resolver toggle |
| nodejs/vercel-sdk/sample-agent/src/otel.ts | Adds early OBS bootstrap for legacy SDK family + per-request export guard |
| nodejs/vercel-sdk/sample-agent/src/index.ts | Imports ./otel first |
| nodejs/vercel-sdk/sample-agent/src/client.ts | Uses turnContext identities and user details for inference scopes |
| nodejs/vercel-sdk/sample-agent/src/agent.ts | Passes turnContext into client factory for correct attribution |
| nodejs/vercel-sdk/sample-agent/README.md | Documents OBS-only app auth and legacy route selection |
| nodejs/vercel-sdk/sample-agent/docs/design.md | Pins observability package version reference to preview.125 |
| nodejs/vercel-sdk/sample-agent/.env.example | Adds AGENT365_OBS_* settings |
| nodejs/perplexity/sample-agent/src/otel.ts | Adds early OBS bootstrap for legacy SDK family + per-request export guard |
| nodejs/perplexity/sample-agent/src/index.ts | Imports ./otel first |
| nodejs/perplexity/sample-agent/README.md | Documents pinned legacy SDK family + OBS-only app auth |
| nodejs/perplexity/sample-agent/package.json | Pins preview.115 dependencies, adds Node >=22 engines, adds @opentelemetry/core |
| nodejs/perplexity/sample-agent/docs/design.md | Documents otel.ts bootstrap + OBS-only app auth settings |
| nodejs/perplexity/sample-agent/.env.template | Adds AGENT365_OBS_* settings and removes legacy flags |
| nodejs/devin/sample-agent/src/utils.ts | Adds caller details + normalizes tenant/agent ID sourcing for OBS |
| nodejs/devin/sample-agent/src/otel.ts | Adds early OBS bootstrap + per-request export guard |
| nodejs/devin/sample-agent/src/observability-token-service.ts | Adds OBS-only token resolver helper |
| nodejs/devin/sample-agent/src/index.ts | Imports ./otel first; keeps shutdown hook for ObservabilityManager |
| nodejs/devin/sample-agent/src/agent.ts | Removes in-constructor OBS init; improves scope disposal/error recording |
| nodejs/devin/sample-agent/README.md | Documents pinned legacy SDK family + OBS-only app auth |
| nodejs/devin/sample-agent/package.json | Pins preview.115 deps, adds @opentelemetry/core, updates deps |
| nodejs/devin/sample-agent/docs/design.md | Documents otel.ts bootstrap + pinned SDK family |
| nodejs/devin/sample-agent/.env.example | Adds AGENT365_OBS_* settings; removes legacy flags |
| nodejs/copilot-studio/sample-agent/src/otel.ts | Adds early OBS bootstrap + per-request export guard |
| nodejs/copilot-studio/sample-agent/src/index.ts | Imports ./otel first |
| nodejs/copilot-studio/sample-agent/src/client.ts | Refactors scope creation to include baggage + explicit disposal/error recording |
| nodejs/copilot-studio/sample-agent/src/agent.ts | Removes delegated token preload; builds baggage explicitly with IDs |
| nodejs/copilot-studio/sample-agent/README.md | Documents pinned legacy SDK family + OBS-only app auth |
| nodejs/copilot-studio/sample-agent/package.json | Pins preview.115 deps, adds Node >=22 engines, adds @opentelemetry/core |
| nodejs/copilot-studio/sample-agent/.env.template | Adds AGENT365_OBS_* settings; removes legacy resolver toggle |
| nodejs/claude/sample-agent/src/otel.ts | Enables distro S2S + uses OBS-only resolver |
| nodejs/claude/sample-agent/src/client.ts | Removes blueprint secret from subprocess env; uses turnContext/env IDs for scopes |
| nodejs/claude/sample-agent/README.md | Documents OBS-only app auth and distro configuration |
| nodejs/claude/sample-agent/docs/design.md | Updates distro snippet to include S2S + token resolver |
| nodejs/claude/sample-agent/.env.template | Adds AGENT365_OBS_* settings |
| nodejs/autonomous/github-trending/src/observability-token-service.ts | Requires real expiry for cached OBS tokens; sanitizes error bodies |
| nodejs/autonomous/github-trending/src/index.ts | Fails closed if OBS token missing/expired; removes empty-token fallback |
| dotnet/shared/Observability/ObservabilityAppTokenFactory.cs | Adds managed-identity assertion wiring for shared OBS-only token provider |
| dotnet/w365-computer-use/sample-agent/W365ComputerUseSample.csproj | Adds Azure.Identity alias and links shared Observability sources |
| dotnet/w365-computer-use/sample-agent/Telemetry/ObservabilityServiceCollectionExtensions.cs | Uses S2S endpoint + injects dedicated OBS token resolver |
| dotnet/w365-computer-use/sample-agent/Telemetry/A365OtelWrapper.cs | Removes delegated token registration/caching path for OBS |
| dotnet/w365-computer-use/sample-agent/README.md | Documents Agent365Observability config + deployment guidance |
| dotnet/w365-computer-use/sample-agent/Program.cs | Wires shared OBS-only app token provider into OpenTelemetry config |
| dotnet/w365-computer-use/sample-agent/appsettings.json | Adds BlueprintClientId/Secret + managed identity knobs |
| dotnet/w365-computer-use/sample-agent/Agent/MyAgent.cs | Removes exporter token cache dependencies from agent constructor/calls |
| dotnet/semantic-kernel/sample-agent/SemanticKernelSampleAgent.csproj | Adds Azure.Identity alias and links shared Observability sources |
| dotnet/semantic-kernel/sample-agent/README.md | Documents dedicated OBS-only credentials and S2S configuration |
| dotnet/semantic-kernel/sample-agent/Program.cs | Injects shared OBS-only resolver and enables S2S on exporter |
| dotnet/semantic-kernel/sample-agent/appsettings.json | Adds Agent365Observability configuration template |
| dotnet/docs/design.md | Updates .NET design docs for shared OBS-only provider + S2S-only exporter |
| dotnet/autonomous/github-trending/sample-agent/Program.cs | Fails closed if OBS token missing/expired (no empty-token fallback) |
| dotnet/agent-framework/sample-agent/README.md | Documents dedicated OBS-only credentials and deployment considerations |
| dotnet/agent-framework/sample-agent/Program.cs | Injects shared OBS-only resolver and enables S2S on exporter |
| dotnet/agent-framework/sample-agent/appsettings.json | Replaces legacy client ID/secret fields with OBS-only settings |
| dotnet/agent-framework/sample-agent/AgentFrameworkSampleAgent.csproj | Adds Azure.Identity alias and links shared Observability sources |
| dotnet/agent-framework/sample-agent/Agent/MyAgent.cs | Removes delegated token registration for OBS; keeps business auth separate |
| agent-platforms/salesforce/apex-observability/README.md | Marks endpoint-selection flag deprecated; OBS always uses S2S path |
| agent-platforms/salesforce/apex-observability/force-app/main/default/objects/A365_Observability_Config__mdt/fields/UseS2SEndpoint__c.field-meta.xml | Deprecates legacy route flag metadata and labeling |
| agent-platforms/salesforce/apex-observability/force-app/main/default/classes/A365TelemetryTest.cls | Adds tests asserting no legacy route fallback (including on 401) |
| agent-platforms/salesforce/apex-observability/force-app/main/default/classes/A365ObsConfig.cls | Forces S2S route selection regardless of deprecated flag |
Review details
- Files reviewed: 141/141 changed files
- Comments generated: 9
- 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 (medium risk). The cross-language S2S implementation consistently isolates observability credentials from business MCP/Graph/OBO credentials, validates the returned app token, bounds refreshes, and fails closed. One new regression test does not actually protect the isolation invariant it claims to verify.
Should fix
tests/observability/node-app-token.test.cjs:212-213- The business-auth regression reads itsbeforevalue fromgit show HEAD:<file>and compares it with the same clean-checkout file, so it is tautological in CI. Assert the expected call arguments directly, execute the calls with mocks, or compare against a real base fixture so a PR that changesaddToolServersToAgent/exchangeTokenfails the test.
Existing review reconciliation
Twelve exact-head bot threads remain unresolved, so the approval gate cannot pass even apart from the new finding. I did not duplicate them: eight are the same diagnostic-rendering comment, and the remaining four cover a broad catch, LINQ style, fixture-path hardening, and the managed-identity request resource. The last claim does not survive source verification: Azure Identity's managed-identity path converts a single TokenRequestContext value to a resource, accepting api://AzureADTokenExchange unchanged and stripping /.default when supplied (Azure.Core ScopeUtilities.ScopesToResource).
Trade-off
Keeping a static structural guard is useful for these samples, but it must be independent of the commit under test; otherwise the test adds maintenance cost without protecting the business-auth boundary.
Persona roll-up
- Security: No new credential crossover, delegated-token fallback, token logging, or fail-open path found; the two-step tokens are audience/role/type/expiry validated.
- Privacy: No new user identity collection, cross-tenant cache reuse, or sensitive diagnostic disclosure found.
- Performance: Refreshes are cached, single-flight, timeout-bounded, and do not return stale tokens after failure.
- Customer service: The migration guidance is broad, but the advertised Node business-auth regression is ineffective.
- Business / COGS: Token exchanges occur only on refresh; no material storage, egress, or cardinality increase found.
- Senior engineer: One should-fix test defect; the .NET and Python focused tests otherwise exercise failure and cache behavior well.
- Architect: The S2S endpoint and app-only resolver contract is consistent across supported sample families; no additional design break found.
Feedback ledger
No repository-specific ledger existed, so no prior dismissal suppressed a source-valid finding. Existing PR threads were reconciled and not duplicated.
Approval gate
Not approved: one should-fix finding remains and all 12 prior review threads are unresolved. The PR is open at a6f88cf310557d9609688cb12eff6c2b702dd711; all 41 exact-head checks are complete and successful.
Preserve workload OBO while accepting explicitly app-only roleless OBS tokens. Add independent business-auth contract guards, narrow .NET acquisition failures, harden fixture paths, and reuse the managed-identity exchange scope. Align OpenAI dependencies and update registration and authentication guidance. Review response amendments: - Roleless tokens now also accepted when oid==sub (with no scp), because Entra may not emit idtyp=app for the OBS resource in every tenant. Delegated tokens have oid != sub, so this stays app-only. - Restore the .NET "never leak secrets" guarantee: sanitize CryptographicException, IOException, and any unexpected non-programming exception; still propagate InvalidOperation/NullReference/Argument/KeyNotFound/Overflow as programming failures. - Widen the OpenAI tracing smoke fixture timeout to tolerate cold-cache require() resolution. Follow-ups (session files/pr-followups.md): - Bump sample SDK once #290 publishes and remove the per-request-export startup rejection. - Live-verify roleless acceptance against a second tenant and an OBO-authorized agent. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 5cbf5f6b-cc40-4b7e-a591-65848db73a12
Author
Review feedback addressedPushed e9206cb. Changes
Validation
Known gaps
|
Jason-R-Lien
left a comment
Jason-R-Lien
left a comment
There was a problem hiding this comment.
Re-review of a6f88cf..e9206cb
Verdict: Needs work (medium risk). The new roleless app-token checks are fail-closed across .NET, Node, and Python, and all exact-head CI checks are successful. Approval is still blocked by one existing test-coverage finding and one new documentation-contract finding.
Author-claimed fixes
- Business-auth guard — partially accepted. The
git show HEADtautology is gone:business-auth-contracts.jsonis an independent oracle, and the mutation cases cover handler, turn context, workload token/scope, and call removal. However, no workflow invokestests/observability/node-app-token.test.cjs; the author also lists CI wiring as a known gap. The original regression-protection finding therefore remains open until this guard runs in CI. - Roleless OBS token validation — accepted in source. All three providers reject any
scp, explicit non-app/nullidtyp, malformed roles, wrong tenant/client/audience, and invalid lifetime; absent/empty roles requireidtyp=appor, whenidtypis absent,oid == sub. The new focused tests exercise both acceptance and rejection branches. - .NET exception handling — accepted. Expected HTTP, acquisition, cryptographic, I/O, timeout, parsing, shape, and lifetime failures are sanitized without inner exceptions; caller cancellation and the enumerated programming errors propagate.
- Fixture-path hardening — accepted.
Fixture()rejects rooted, traversal, drive-relative, UNC/device, ADS, separator, whitespace, and trailing-dot/space inputs beforePath.Combine; the new genericPath.Combinealert does not survive source verification. - Managed-identity exchange scope — accepted. The factory now reuses
ExchangeScope(api://AzureADTokenExchange/.default). - Masked “bearer token” comments — accepted as display-only false positives. The raw source already contains the actionable
bearer tokenwording in every helper copy. - OpenAI runtime alignment — accepted. The package now declares
@openai/agents ^0.7.0andopenai ^6.27.0, with a runtime-resolution/instrumentation regression in the Node test. - Documentation — rejected in part. Registration/service-policy guidance is improved, but the newly added roleless-token wording excludes the implemented
oid == subfallback in the root and two .NET sample READMEs; see the inline finding.
Approval blockers
- Existing
tests/observability/node-app-token.test.cjsbusiness-auth finding: independent oracle fixed, but the guard is not wired into CI. - New auth-contract documentation inconsistency at
README.md:64(alsodotnet/agent-framework/sample-agent/README.md:163anddotnet/semantic-kernel/sample-agent/README.md:49). - Live review threads remain unresolved. The new LINQ advisories are non-consequential style suggestions, and the new
Path.Combineadvisory is rebutted by the preceding bare-filename validator; they are not counted as substantive findings, but the Step 7 thread-state gate is not satisfied.
Persona roll-up
- Security / Privacy: App-only/delegated separation, tenant-agent binding, audience/lifetime validation, sanitization, and no-secret diagnostics are source-correct; no new privacy flow found.
- Performance / COGS: Refresh remains cached, single-flight, and bounded; no hot-path or material cost regression found.
- Customer service: One operator-facing authentication contract is internally contradictory.
- Senior engineer / Architect: The original guard now has a real oracle but no automated execution; no additional architecture or correctness defect found.
No repository-specific feedback ledger exists, so no finding was suppressed. Merge remains subject to branch protection.
Align the roleless OBS token guidance in the root, .NET, Node.js and Python docs with the implemented providers: absent or empty roles are accepted with idtyp=app, or with absent idtyp when a nonempty oid equals sub. Tokens without idtyp still work with valid nonempty roles. The .NET autonomous README no longer implies that the service requires idtyp=app. Also use Select projections for the five claim loops and Path.Join for the validated fixture filename in ObservabilityAppTokenTests. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 5cbf5f6b-cc40-4b7e-a591-65848db73a12
…orkflow Run the reviewed business MCP/OBO contract and mutation checks from tests/observability/node-app-token.test.cjs after the OpenAI sample build, so they execute in CI. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 5cbf5f6b-cc40-4b7e-a591-65848db73a12
Jason-R-Lien
left a comment
Jason-R-Lien
left a comment
There was a problem hiding this comment.
Re-review of e9206cb..f85bf6c
Verdict: Needs work (medium risk); no new findings. The documentation-contract inconsistency is fixed and the quality-only test refactors preserve behavior. One existing should-fix remains: the business-auth regression has an independent fixture oracle, but no workflow invokes tests/observability/node-app-token.test.cjs, so the isolation invariant is still not protected in CI.
Author-response reconciliation
- Business-auth guard — partially accepted, still open. The
git show HEADtautology was fixed ine9206cb; the reviewed fixture and mutation controls are source-valid. The author's stated CI-wiring gap remains atf85bf6c: no workflow references the Node observability test or its fixture. - Roleless-token validation and provider behavior — accepted. The .NET, Node, and Python providers and focused tests support
idtyp=appor absentidtypwith nonemptyoid == sub, while retaining fail-closed delegated, identity, audience, tenant, and lifetime checks. - Documentation fix — accepted. The root, .NET, Node, and Python documentation now describes both accepted roleless forms and no longer contradicts the providers.
- .NET exception sanitization — accepted. Expected acquisition/parsing/transport failures are sanitized; caller cancellation and the documented programming errors propagate.
- Explicit claim-validation loop rebuttal — accepted. The loop validates every present
appid/azpclaim and avoids the invalidTryGetPropertypredicate suggested by the advisory. - Fixture-path hardening and
Path.Joinfollow-up — accepted. Bare-filename validation precedes the join, and the join change does not widen accepted input. - Managed-identity exchange scope — accepted. The factory reuses
ExchangeScopewith/.default. - Eight masked “bearer token” replies — accepted as display-only false positives. Raw source contains the full actionable wording.
- OpenAI runtime alignment — accepted. The declared Agents/OpenAI versions and runtime-resolution regression remain in place.
- Five LINQ
Selectfixes — accepted. Each loop now projects claims before iteration with no semantic change; the exact-head focused token suite passes.
Approval gate
- Blocked: one consequential review thread remains unresolved because the business-auth guard is not run by CI.
- Clean: all exact-head GitHub checks are completed and successful; no new correctness, security/privacy, performance/COGS, customer-service, or architecture finding was introduced by this delta.
Merge remains subject to branch protection.
Jason-R-Lien
left a comment
Jason-R-Lien
left a comment
There was a problem hiding this comment.
Same-head PR-content re-review
Verdict: Needs work (medium risk); no new findings. The head remains f85bf6c, and the current title/description matches content hash 5e8dacb…, which was already reviewed. The updated operator guidance does not contradict the implementation, but the prior CI-coverage blocker remains.
Author-response reconciliation
- Business-auth guard — partially accepted, still open. The independent fixture oracle and mutation controls fixed the original
git show HEADtautology. Source at this head still has no workflow reference totests/observability/node-app-token.test.cjs, matching the author's stated CI-wiring gap. - Roleless-token behavior and documentation — accepted. Provider tests and active docs consistently allow explicit
idtyp=appor absentidtypwith nonemptyoid == sub, while retaining delegated, identity, tenant, audience, and lifetime rejection. - .NET exception sanitization, claim-validation-loop rebuttal, fixture-path hardening/
Path.Join, and managed-identity exchange scope — accepted. The previously verified source remains unchanged. - Masked “bearer token” replies — accepted as display-only false positives. Raw source contains the full wording.
- OpenAI runtime alignment and five LINQ
Selectfixes — accepted. The verified implementation is unchanged.
Approval gate
- Blocked: the one source-valid business-auth regression is not run by CI; its thread remains unresolved consistently with that missing acceptance criterion.
- Clean: all 40 exact-head checks are completed and successful, and no later author response, submitted review, issue comment, or source change introduces another finding.
Merge remains subject to branch protection.
Author
|
Jason-R-Lien, the remaining blocker is addressed in 3bac1b1: the business-auth guard ( |
Jason-R-Lien
previously approved these changes
Sep 28, 2026
Jason-R-Lien
left a comment
Jason-R-Lien
left a comment
There was a problem hiding this comment.
Re-review of f85bf6c..3bac1b1
Approved. The only delta wires the existing independent business-auth fixture/mutation guard into the required Node.js OpenAI workflow. The step runs from the repository root, selects all 24 business-auth contract cases, and completed successfully on both Node 18 and Node 20. No new findings.
Author-response reconciliation
- Business-auth oracle and CI coverage — accepted.
e9206cbreplaced thegit show HEADcomparison with reviewed fixtures and mutation controls;3bac1b1now invokes that unchanged guard from CI. This fully closes the prior should-fix. - Roleless-token contract and documentation — accepted. The fail-closed
idtyp=app/ absent-idtypwithoid == subbehavior and matching documentation remain present at head. - .NET exception handling, fixture-path validation, exchange scope, OpenAI runtime alignment, LINQ projections, and
Path.Joinrefactor — accepted. The previously cited source and regression coverage remain unchanged at head. - Explicit
appid/azpclaim loop — rebuttal accepted. The loop intentionally validates every present claim and avoids a redundant lookup; no correctness issue remains. - Eight masked Python error-message comments — rebuttal accepted. The source contains the explicit
not a bearer tokenwording; the masked rendering was not a source defect.
All 20 review threads are resolved, all 40 exact-head checks completed successfully, and there are no open blocking or should-fix findings. Merge remains subject to branch protection.
dbezic (dbezic)
previously approved these changes
Sep 28, 2026
Rick Brighenti (rbrighenti)
requested changes
Sep 28, 2026
Rick Brighenti (rbrighenti)
left a comment
Rick Brighenti (rbrighenti)
left a comment
Contributor
There was a problem hiding this comment.
Requesting changes, mainly because of the cross-PR rollout; see the [Blocking] inline comment on copilot-studio otel.ts.
The app-only token providers look solid:
- token validation is strict
- failures fail closed, with no stale-token or delegated fallback
- business MCP/OBO auth stays separate from OBS auth
I ran everything locally at 3bac1b1 and it passed:
- all 8 Node samples build, and 426/426 Node OBS tests pass
- the 4 .NET samples build, and 183/183
ObservabilityAppTokenTestspass - 3183/3183 Python
tests/observabilitytests pass (on Windows only withPYTHONUTF8=1; see the inline comment on encoding)
All referenced package versions are published, and nothing depends on the unreleased SDK.
Cross-PR consistency
- (a) CLI and the legacy route. microsoft/Agent365-devTools#501's "registered blueprint agents need no OtelWrite" was validated on the
/otlproute. Five Node samples here export to the legacy non-/otlproute (inline on copilot-studiootel.ts). - (b) Credential model and SDK family. microsoft/agent365-skills#84's resolver reuses the hosting connection for each turn's agent identity and scaffolds the
@microsoft/opentelemetrydistro. These samples use a static single-instanceAGENT365_OBS_*config with a duplicate secret, and pin three samples to the legacypreview.115packages. - (c) AI Teammates. The CLI and Skills PRs keep the OtelWrite app-role step for AI Teammates, but the sample docs say not to add OtelWrite.
- (d) Roleless token contract. The hosting design doc in microsoft/Agent365-nodejs#290 says roleless tokens need explicit
idtyp=app. The providers here also accept a token with noidtypwhenoid == sub. Could we settle on one documented contract across both repos? - (e) After the SDK release. When microsoft/Agent365-nodejs#290 ships, the preview-pinned samples will still use the non-
/otlproute; they don't pick up the new behavior.
Items not tied to a changed line
- [Should-fix] The unused
token-cache.ts/token_cache.pymodules (the delegated OBS cache pattern this PR retires) are still innodejs/{devin,langchain,openai}andpython/{agent-framework,claude,crewai,openai}..github/workflows/python-claude-sample.ymlstill compiles and importstoken_cache. Could we delete them? - [Should-fix] Step 4 of the message flow in
python/openai/sample-agent/docs/design.mdstill says "Token exchange for observability…cache_agentic_token()". - [Nit]
python/agent-framework/sample-agent/agent.pystill has the old "Microsoft. All rights reserved." header. It was fixed inpython/openaibut not here. - [Question] The OpenAI Node sample bumps
@openai/agents0.1→0.7 andopenai4→6 and dropsoverrides. Devin, Perplexity and Copilot Studio also get scope and dispose rewrites. The dispose fixes look good, and the OpenAI bump is needed for dedupe. Could the description call these changes out? Its validation counts also look stale: I see 426 Node and 3183 Python tests.
Align the manager-based Node samples on the Agent365 1.0.0 package family so S2S export uses the /otlp route with the isolated app-token resolver. Update scope calls for the 1.0.0 observability API and extend route/version regression coverage. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 5cbf5f6b-cc40-4b7e-a591-65848db73a12
Move Node observability guidance into configuration sections, document the static single-instance provider limitation, AI Teammate OtelWrite caveat, and sovereign-cloud limitation. Remove unused delegated token-cache helpers and stale preview/legacy-route documentation. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 5cbf5f6b-cc40-4b7e-a591-65848db73a12
Gate .NET and Python observability token providers behind exporter enablement, make .NET helpers sample-local, remove retired Python delegated caches, and add offline observability CI coverage. Update root and sample documentation for the /otlp route finding, static single-instance provider limitation, AI Teammate OtelWrite caveat, roleless app-token contract, and sovereign-cloud limitations. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 5cbf5f6b-cc40-4b7e-a591-65848db73a12
…core pin - Drop the engines blocks this PR added to copilot-studio and perplexity so engine changes stay out of this PR. - Restore .NET test packages from the repository's default feeds in CI. - Keep the @opentelemetry/core pin: the 1.0.0 exporter requires it without declaring it, so the READMEs now say why. - Keep internal service-policy detail out of the public route note. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 5cbf5f6b-cc40-4b7e-a591-65848db73a12
Author
|
Thanks, Rick, and thanks Jason and Dominik for the approvals. I pushed 0f3aefe, 00cec4e, 8ff1e7f and 065753c, and replied on every inline thread. Blocking item: the five Node samples that exported to the legacy route now use Items not tied to a changed line
Cross-PR consistency
Also found while integrating: CI: all 41 checks pass on 065753c, including the new Observability Offline Tests jobs. |
Krishnadheeraj (DheerajPannala)
requested a review
from Rick Brighenti (rbrighenti)
September 28, 2026 17:17
Jason-R-Lien
left a comment
Jason-R-Lien
left a comment
There was a problem hiding this comment.
Re-review of 3bac1b1..065753c
Comment — no new code findings, but the approval gate is still blocked. I reviewed the four-commit, 90-file remediation delta and refreshed the complete review record.
Author-response reconciliation
- Legacy Node route and SDK migration — accepted. Copilot Studio, Devin, OpenAI, Perplexity, and Vercel now pin the published Agent 365 1.0.0 family, retain
useS2SEndpoint = true, and document the/otlproute. The exact-head Node E2E/build checks that exist are green, and the new source-contract tests pin the package family and route. - .NET and Python disabled-export startup — accepted. All three .NET entry points use
CreateIfEnabled; disabled placeholder configurations, enabled-invalid configurations, and invalid flags have focused tests. Python Agent Framework no longer forcesenabled=True, and its bootstrap path is covered. - Self-contained .NET samples — accepted. Each sample now owns identical
ObservabilityAppTokenFactory.csandObservabilityAppTokenProvider.cscopies; project links todotnet/sharedare gone, and the regression suite compares the copies. - Single-instance credential model — accepted as a documented sample limitation. The root guide,
docs/observability-s2s.md, and sample READMEs explicitly bound the examples to one tenant/instance, a separate blueprint credential, no Node/Python managed identity, and direct multi-instance deployments to a per-agent/per-tenant hosting-connection design. - AI Teammate and sovereign-cloud guidance — accepted. The docs preserve the OtelWrite setup step for AI Teammates and state that the hard-coded public-cloud authority requires provider changes for sovereign clouds. Companion PRs
Agent365-devTools#501andagent365-skills#84carry the same AI Teammate boundary. - Roleless token cross-repo contract — accepted.
Agent365-nodejs#290atceea486documents the sameidtyp=app/ nonemptyroles/ absent-idtypwithoid == subcontract and rejects anyscp. - UTF-8 and offline CI coverage — accepted. Every
tests/observabilityread_textcall is explicit UTF-8; the required orchestrator now runs the Python suite on Windows andObservabilityAppTokenTestson Ubuntu, and both exact-head jobs passed. The author’s deferred full Node suite and three additional sample builds do not invalidate the requested Python/.NET minimum, but remain manual coverage. - Cleanup claims — accepted. The retired Node/Python token-cache files and stale imports/docs are gone, the Agent Framework Python header is corrected, the stale LangChain comment is removed, Node boolean parsing accepts
on, unused hosting dependencies are removed where claimed, the .NET instance-ID placeholder is corrected, and refresh lead time is aligned to 60 seconds. - Node engine response — accepted as scoped. The temporary Copilot Studio/Perplexity engine edits were removed; LangChain already resolved
@microsoft/opentelemetry1.4.0 from its prior unlocked^1.0.0range, so the Node-version mismatch was not introduced by this delta. - PR description update — accepted. The live body now calls out the Node 1.0.0/OpenAI dependency and scope/dispose migrations and reports the current 433 Node, 3185 Python, and 197 .NET validation counts.
- Prior author responses remain accepted. The independent business-auth fixture/mutation oracle and required CI invocation, roleless-token docs, .NET exception/path/exchange fixes, runtime alignment, LINQ/
Path.Joinchanges, intentionalappid/azploop, and explicit Python bearer-token wording remain source-valid at head. - Three new
Wheresuggestions — rejected as non-actionable duplicates. They target byte-identical sample-local copies of the same intentionalappid/azploop already reviewed: the loop validates every present claim with one lookup; the suggested rewrite adds another lookup without changing correctness.
Approval gate
All 42 exact-head checks are complete and successful, with zero new blocking or should-fix findings. However, GitHub currently reports 39 review threads, 19 unresolved, including the human change-request threads and the three duplicate Where threads. Step 7 requires every prior thread to be resolved after source verification, so I am leaving a COMMENT rather than re-approving. Once the thread owners resolve/close those discussions, the source and CI evidence support approval; merge remains subject to branch protection.
Jason-R-Lien
left a comment
Jason-R-Lien
left a comment
There was a problem hiding this comment.
Metadata-only re-review at 065753c
Comment — no new findings; approval remains blocked only by thread state. The title/description update is consistent with the reviewed source and closes the prior metadata mismatch.
Author-response reconciliation
- Rollout and dependency guidance — accepted. The live body now states the coordinated SDK 2.0.0 rollout, identifies
Agent365-nodejs#290,Agent365-devTools#501, andagent365-skills#84, and records the follow-up from Node 1.0.0 to 2.0.0. This matches the previously verified/otlpmigration and companion-PR contracts. - Migration and validation claims — accepted. The body now names the OpenAI dependency upgrades, the 1.0.0 scope/dispose rewrites, the retained
@opentelemetry/core@2.1.0workaround, and the current 433 Node / 3185 Python / 197 .NET results. All 42 exact-head checks are complete and successful. - Legacy-route fix — accepted. The five affected Node samples use the published 1.0.0 observability family and
/otlp; source-contract tests pin that route/package family, consistent with the author's reported 200/otlpand 401 legacy-route probe. - Startup gating and sample isolation — accepted. The .NET samples use
CreateIfEnabled, Python Agent Framework no longer forces export on, and each .NET sample owns identical local provider/factory copies. Focused regression coverage exercises disabled placeholders and enabled-invalid configurations. - Credential and environment boundaries — accepted as documented limitations. The docs state one configured instance/tenant, a separate blueprint credential, no Node/Python managed identity, the per-agent/per-tenant design for multi-instance hosts, the AI Teammate OtelWrite requirement, and the public-cloud authority limitation.
- Roleless-token contract — accepted. The providers and
Agent365-nodejs#290align onidtyp=app, or absentidtypwith nonemptyrolesoroid == sub, while rejectingscpand otheridtypvalues. - Cleanup and coverage claims — accepted. Retired token caches/import checks are removed, the Python design flow and header are corrected, UTF-8 reads are explicit, Node boolean parsing includes
on, unused hosting dependencies are removed, and required Windows Python / Ubuntu .NET offline CI is wired and green. - Node engine response — accepted as scoped. Temporary engine edits were removed; LangChain's unlocked pre-existing range already resolved to the Node-22-requiring telemetry package, so that compatibility mismatch was not introduced here.
- Earlier fixes remain accepted. The independent business-auth fixture/mutation oracle and required CI invocation, roleless-token docs, .NET exception/path/exchange fixes, runtime alignment,
Path.Join/LINQ cleanups, intentionalappid/azpvalidation loop, and explicit Python bearer-token wording remain source-valid at this unchanged head. - Rejected author claims: none. The three
Wheresuggestions were reviewer-generated, not author claims; they were correctly left unchanged because they duplicate the already adjudicatedappid/azploop suggestion without improving correctness.
Approval gate
GitHub currently reports 39 review threads, 16 unresolved. Those are the human change-request threads whose fixes/rationales are accepted above; the three duplicate Where threads have now been resolved. The gate nevertheless requires every prior thread to be resolved, so I am leaving a COMMENT rather than re-approving. Once the remaining thread owners resolve/close those discussions, the source, metadata, and CI evidence support approval; merge remains subject to branch protection.
Author
|
I've resolved the 16 remaining review threads. Each has a reply pointing to the commit with the fix, and the fixes were verified in the review at 065753c. Jason-R-Lien, this should clear the thread gate from your last review. |
Jason-R-Lien
approved these changes
Sep 28, 2026
Jason-R-Lien
left a comment
Jason-R-Lien
left a comment
There was a problem hiding this comment.
Same-head author-response re-review at 065753c
Approved. The author's latest claim is accepted: GitHub now reports all 39 review threads resolved. The head and PR-content hash are unchanged from the prior evidence pass, where the source, updated requirements, and all author-claimed fixes were verified.
Author-response reconciliation
- Thread-gate claim — accepted. A fresh GraphQL query reports 39 total threads, 0 unresolved.
- Previously claimed source fixes — accepted. The
/otlpand Node 1.0.0 migration, disabled-export startup gating, sample-local .NET providers, UTF-8 handling, offline CI, cleanup, documentation boundaries, roleless-token contract, business-auth guard, and current PR description remain accepted from the prior exact-head source review. - Intentional
appid/azploop rebuttal — accepted. The three duplicateWheresuggestions are resolved and do not identify a correctness change. - Rejected author claims — none.
Approval gate
The PR is open at 065753c; there are zero open blocking or should-fix findings, all review threads are resolved, and all 42 exact-head checks are complete and successful. No later substantive structured-review finding was posted. Merge remains subject to branch protection.
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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
Summary
/otlproute throughout the Node.js, Python, .NET and Salesforce samples.client_credentialswithfmi_path, then agent-instanceclient_credentialswith the exchange assertion. Business MCP/Graph/OBO authentication stays separate.docs/observability-s2s.md) and regression coverage. Salesforce ignores its deprecated endpoint-selection flag.Dependency and code changes to note
0.1.0-preview.115/^0.1.0-preview.125to the 1.0.0 packages, whose S2S exporter posts to the/otlproute. Copilot Studio, Devin and Perplexity move to the 1.0.0 scope APIs (Request,AgentDetails.tenantId,UserDetails,InvokeAgentScopeDetails, renamed baggage methods), with rewritten scope and dispose handling.@openai/agents^0.1→^0.7andopenai^4→^6, needed to dedupe with the 1.0.0 extensions; theoverridesblock is dropped.@microsoft/opentelemetryfloor^1.4.0, the first release withdurableDelivery(which the sample disables). Main's^1.0.0already resolves to 1.4.0 on a fresh install.@opentelemetry/core@2.1.0stays an explicit dependency in copilot-studio, devin and perplexity because the 1.0.0 exporter imports it without declaring it.@microsoft/agents-a365-observability-hostingdependency (copilot-studio, vercel-sdk) and the retired delegatedtoken-cachemodules (Node devin/langchain/openai; Python agent-framework/claude/crewai/openai).Observability/in each sample);dotnet/shared/Observabilityis gone.ci-observability.yml: Pythontests/observabilityon Windows and .NETObservabilityAppTokenTestson Ubuntu.Compatibility and setup
AGENT365_OBS_*settings. .NET uses dedicatedAgent365Observabilityconfiguration and supports blueprint secrets or managed identity. The checked-in templates leave export disabled, and providers are created and validated only when export is enabled.login.microsoftonline.com; sovereign clouds need provider changes.a365 setup all --aiteammateprints. AI Teammate S2S export without it hasn't been validated.Validation
ObservabilityAppTokenTests. The Python suite passes on Windows withoutPYTHONUTF8./otlproute returned 200, and the legacy non-/otlproute returned 401 (AuthenticationSchemeNotSupported).