Add --skip-observability-permissions to setup all - #501
Krishnadheeraj (DheerajPannala) wants to merge 1 commit into
Conversation
Blueprint agents that export telemetry through the app-only S2S endpoint (microsoft/Agent365-nodejs#290, microsoft/Agent365-Samples#339) are authorized by their agent registration, so the Observability API OtelWrite permission, and the admin consent it needs, is unnecessary for them. - New opt-in `setup all --skip-observability-permissions` omits Observability API from the permission specs (inheritable permissions, app role grants, batch consent) and from the per-resource and combined admin consent URLs. Defaults are unchanged: the published SDKs still export to the non-S2S endpoint by default. - The flag fails fast for AI Teammate agents and with authMode s2s/both, since OtelWrite is the only app role those modes grant. A contradicting --authmode flag is rejected before bootstrap signs in. - With the flag, a failed agent registration is an error (exit 1), because registration is then the agent's only Observability authorization. - Fix: `setup all --agent-registration-only` exited 0 when registration failed. - Dry run plan, setup summary, CHANGELOG, and docs updated. Co-authored-by: Copilot <[email protected]> Copilot-Session: 5cbf5f6b-cc40-4b7e-a591-65848db73a12
github-actions
Bot
added
the
documentation
Dependency Review✅ No vulnerabilities or license issues or OpenSSF Scorecard issues found.Scanned FilesNone |
There was a problem hiding this comment.
Copilot review overview
🔵 Needs a closer look
Unresolved validation, failure-handling, persisted-consent, custom-permission, and route-documentation issues remain.
Review effort: Lite
Findings: None
What changed in this PR
Adds --skip-observability-permissions for blueprint setup and fixes registration-only failure handling.
Changes:
- Adds permission, consent, validation, dry-run, and summary handling.
- Updates registration failure severity and related tests.
- Updates documentation and changelog.
| File | Description |
|---|---|
src/Tests/Microsoft.Agents.A365.DevTools.Cli.Tests/Helpers/SetupHelpersDisplaySetupSummaryTests.cs |
Tests summary behavior. |
src/Tests/Microsoft.Agents.A365.DevTools.Cli.Tests/Commands/SetupSubcommands/PermissionSpecsTests.cs |
Tests permission specifications. |
src/Tests/Microsoft.Agents.A365.DevTools.Cli.Tests/Commands/SetupCommandTests.cs |
Tests validation and dry-run behavior. |
src/Tests/Microsoft.Agents.A365.DevTools.Cli.Tests/Commands/NonDwBlueprintSetupOrchestratorExecuteTests.cs |
Tests registration failure handling. |
src/Tests/Microsoft.Agents.A365.DevTools.Cli.Tests/Commands/AllSubcommandTests.cs |
Tests permission and consent wiring. |
src/Microsoft.Agents.A365.DevTools.Cli/Commands/SetupSubcommands/SetupResults.cs |
Tracks skipped permissions. |
src/Microsoft.Agents.A365.DevTools.Cli/Commands/SetupSubcommands/SetupHelpers.cs |
Handles permissions and consent URLs. |
src/Microsoft.Agents.A365.DevTools.Cli/Commands/SetupSubcommands/SetupContext.cs |
Stores setup options. |
src/Microsoft.Agents.A365.DevTools.Cli/Commands/SetupSubcommands/README.md |
Documents the option. |
src/Microsoft.Agents.A365.DevTools.Cli/Commands/SetupSubcommands/NonDwBlueprintSetupOrchestrator.cs |
Applies skip behavior and failure handling. |
src/Microsoft.Agents.A365.DevTools.Cli/Commands/SetupSubcommands/AllSubcommand.cs |
Adds and validates the CLI option. |
docs/agent365-guided-setup/a365-observability-instructions.md |
Updates observability guidance. |
CHANGELOG.md |
Records the feature and fix. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Rick Brighenti (rbrighenti)
left a comment
Rick Brighenti (rbrighenti)
left a comment
There was a problem hiding this comment.
Requesting changes. The flag is well scoped and the incompatible-combination guards look right, but two gaps undercut the contract the docs promise ("setup exits with code 1 if registration fails"). Details inline. Both need a regression test.
|
|
||
| // Registration is the sole purpose of --agent-registration-only and, with OtelWrite skipped, | ||
| // the agent's only Observability authorization, so its failure must fail setup. | ||
| var registrationRequired = skipIdentityAndPermissions || ctx.SkipObservabilityPermissions; |
There was a problem hiding this comment.
registrationRequired covers the create-failure path, but the inconclusive-verification branch below (AgentRegistrationExistsAsync returns null, "retaining stored value") still sets registrationAlreadyExisted = true and setup exits 0. With --skip-observability-permissions registration is the agent's only authorization, so an auth or transient failure there should be an error rather than a pass. Please treat the null case as an error when registrationRequired is true, and add a test where the check returns null.
|
|
||
| var consentResourceNames = PopulateAdminConsentUrls(ctx.Config, mcpResourceAppId, mcpScopes, isM365, mcpScopesByAudience, mcpAudienceDisplayNames); | ||
| var includeObservability = !ctx.SkipObservabilityPermissions; | ||
| var consentResourceNames = PopulateAdminConsentUrls(ctx.Config, mcpResourceAppId, mcpScopes, isM365, mcpScopesByAudience, mcpAudienceDisplayNames, includeObservability); |
There was a problem hiding this comment.
PopulateAdminConsentUrls only upserts into ResourceConsents, so if an earlier run without the flag saved an Observability consent entry, it survives this run and is still surfaced to the admin. When includeObservability is false the Observability entry should be removed. A test that seeds the generated config with an existing Observability consent would cover it.
Summary
Adds an opt-in
a365 setup all --skip-observability-permissionsfor blueprint agents that export telemetry through the app-only S2S endpoint, and fixessetup all --agent-registration-onlyexiting 0 when registration fails.Why
With microsoft/Agent365-nodejs#290 and microsoft/Agent365-Samples#339, agents export telemetry with an app-only token to the S2S endpoint (
/observabilityService/tenants/{tenantId}/otlp/agents/{agentId}/traces). That endpoint authorizes registered agent instances without theAgent365.Observability.OtelWriterole: an end-to-end run with a roleless app-only token from a DevTools-registered agent returned HTTP 200 and the spans arrived. Todaysetup allalways requests OtelWrite as both a delegated scope and an app role, and both need admin consent. This flag lets those agents skip them.Defaults are unchanged, because the published SDKs still export to the non-S2S endpoint by default, and that endpoint does need OtelWrite.
Changes
--skip-observability-permissionsremoves Observability API from:s2s/both, whether set by flag or ina365.config.json, because OtelWrite is the only app role those modes grant. A contradicting--authmodeflag is rejected before bootstrap signs in or writes config.--agent-registration-onlynow exits 1 when registration fails. The summary already pointed to "see errors", but the failure was recorded as a warning.Commands/SetupSubcommands/README.md, and the guided-setup observability doc.The flag does not revoke permissions granted by earlier runs. Stale-permission cleanup already protects Observability API.
Testing
1.1.223+bacea97843), then checkedsetup all --help, the dry run, and the rejected combinations. The rejected combinations write no files.setup all --skip-observability-permissionsagainst a tenant, because it creates Entra objects. Roleless S2S access was verified on the/otlproute above; the legacy S2S route and sovereign clouds were not verified.Out of scope / follow-ups
create-instance) still grants OtelWrite.a365 setup permissions botstill configures Observability API.