diff --git a/.claude/agents/pr-code-reviewer.md b/.claude/agents/pr-code-reviewer.md index cbc5b0e7..bd140508 100644 --- a/.claude/agents/pr-code-reviewer.md +++ b/.claude/agents/pr-code-reviewer.md @@ -661,6 +661,33 @@ When a catch block, comment, or doc string covers multiple distinct error condit Do NOT silently omit this check. The rule exists because silent omission is how the regression in `da6f750` went undetected. +### 22. Extractable Code Duplicated Across Sibling Files or Methods + +When a block of code — a method body, a collection initializer, a sequence of API calls, or a set of constant references — appears verbatim (or near-verbatim, differing only in a single parameter or flag) in two or more sibling classes or methods, it is a maintainability defect. Future changes must be applied to every copy; one copy will eventually be missed. + +- **Pattern to catch**: + - The same logical block (3+ lines, or any block constructing a data structure with domain constants) appears in two or more sibling files in the same namespace or folder + - The copies differ only in a single simple parameter: a boolean flag, an enum value, a string literal, or a single variable binding + - Common manifestations in this codebase: + - Identical `List` or array initializers referencing the same `ConfigConstants.*` values across multiple `*Subcommand.cs` files + - The same sequence of `await service.DoX(); await service.DoY();` calls in parallel command handlers + - Repeated `if (dryRun) { logger.LogInformation(...) }` blocks with the same message template in multiple subcommands +- **Severity**: `medium` — inconsistency risk on every future change to the duplicated logic; flag as `high` if the block contains security-sensitive data (auth scopes, app IDs) +- **Check**: For each substantial block (3+ non-trivial lines) in the diff, use `Grep` to search for the same constant names, method call signatures, or string literals in sibling files. If the same pattern appears in two or more files, assess whether the differing part can be parameterized. +- **Fix**: Extract the shared logic into a shared helper method, extension method, or factory, parameterizing the varying element: + ```csharp + // SetupHelpers.cs — single source of truth + internal static ResourcePermissionSpec[] GetFixedApiPermissionSpecs(bool setInheritable) => [ ... ]; + + // AllSubcommand.cs + specs.AddRange(SetupHelpers.GetFixedApiPermissionSpecs(setInheritable: true)); + + // AdminSubcommand.cs + specs.AddRange(SetupHelpers.GetFixedApiPermissionSpecs(setInheritable: false)); + ``` + +**Real example (from `users/sellak/blueprintScopes`):** `AllSubcommand.cs`, `AdminSubcommand.cs`, and `PermissionsSubcommand.cs` each contained an identical three-entry block for Bot API, Observability API, and Power Platform API. When `Agent365.Observability.OtelWrite` was added, the new scope had to be written in three places — and would have been missed without manual cross-file inspection. Extracted to `SetupHelpers.GetFixedApiPermissionSpecs(bool setInheritable)`. + ## Example Invocation When you receive a request like "Review PR #253", you should: diff --git a/CHANGELOG.md b/CHANGELOG.md index 8028f0e4..fa4d4bc1 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -7,6 +7,9 @@ The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.1.0/). ## [Unreleased] ### Added +- `Agent365.Observability.OtelWrite` scope now granted to all provisioned agent identities on the Observability API alongside `user_impersonation`, enabling agents to write OpenTelemetry data to the Agent 365 observability service +- `ChannelMessage.Read.All` and `ChannelMessage.Send` added to default blueprint Microsoft Graph delegated scopes (`agentIdentityScopes`) +- `Files.ReadWrite.All`, `ChannelMessage.Read.All`, and `ChannelMessage.Send` added to default blueprint Microsoft Graph application scopes (`agentApplicationScopes`) - Server-driven notice system: security advisories and critical upgrade prompts are displayed at startup when a maintainer updates `notices.json`. Notices are suppressed once the user upgrades past the specified `minimumVersion`. Results are cached locally for 4 hours to avoid network calls on every invocation. - `a365 cleanup azure --dry-run` — preview resources that would be deleted without making any changes or requiring Azure authentication - `AppServiceAuthRequirementCheck` — validates App Service deployment token before `a365 deploy` begins, catching revoked grants (AADSTS50173) early diff --git a/src/Microsoft.Agents.A365.DevTools.Cli/Commands/CreateInstanceCommand.cs b/src/Microsoft.Agents.A365.DevTools.Cli/Commands/CreateInstanceCommand.cs index 7bd5e015..4fb574ca 100644 --- a/src/Microsoft.Agents.A365.DevTools.Cli/Commands/CreateInstanceCommand.cs +++ b/src/Microsoft.Agents.A365.DevTools.Cli/Commands/CreateInstanceCommand.cs @@ -175,7 +175,7 @@ public static Command CreateCommand(ILogger logger, IConf instanceConfig.TenantId, agenticAppSpObjectId, observabilityApiResourceSpObjectId, - new[] { "user_impersonation" }); + new[] { "user_impersonation", ConfigConstants.ObservabilityApiOtelWriteScope }); if (!observabilityApiGrantOk) logger.LogWarning("Failed to create/update oauth2PermissionGrant for agent identity to Observability API."); diff --git a/src/Microsoft.Agents.A365.DevTools.Cli/Commands/SetupSubcommands/AdminSubcommand.cs b/src/Microsoft.Agents.A365.DevTools.Cli/Commands/SetupSubcommands/AdminSubcommand.cs index 28990f18..8d2c257e 100644 --- a/src/Microsoft.Agents.A365.DevTools.Cli/Commands/SetupSubcommands/AdminSubcommand.cs +++ b/src/Microsoft.Agents.A365.DevTools.Cli/Commands/SetupSubcommands/AdminSubcommand.cs @@ -173,22 +173,8 @@ await RequirementsSubcommand.RunChecksOrExitAsync( "Agent 365 Tools", mcpScopes, SetInheritable: false), - new ResourcePermissionSpec( - ConfigConstants.MessagingBotApiAppId, - "Messaging Bot API", - new[] { "Authorization.ReadWrite", "user_impersonation" }, - SetInheritable: false), - new ResourcePermissionSpec( - ConfigConstants.ObservabilityApiAppId, - "Observability API", - new[] { "user_impersonation" }, - SetInheritable: false), - new ResourcePermissionSpec( - PowerPlatformConstants.PowerPlatformApiResourceAppId, - "Power Platform API", - new[] { "Connectivity.Connections.Read" }, - SetInheritable: false), }; + specs.AddRange(SetupHelpers.GetFixedApiPermissionSpecs(setInheritable: false)); foreach (var customPerm in setupConfig.CustomBlueprintPermissions ?? new List()) { diff --git a/src/Microsoft.Agents.A365.DevTools.Cli/Commands/SetupSubcommands/AllSubcommand.cs b/src/Microsoft.Agents.A365.DevTools.Cli/Commands/SetupSubcommands/AllSubcommand.cs index 99f4943b..3d901536 100644 --- a/src/Microsoft.Agents.A365.DevTools.Cli/Commands/SetupSubcommands/AllSubcommand.cs +++ b/src/Microsoft.Agents.A365.DevTools.Cli/Commands/SetupSubcommands/AllSubcommand.cs @@ -357,22 +357,8 @@ await PermissionsSubcommand.RemoveStaleCustomPermissionsAsync( "Agent 365 Tools", mcpScopes, SetInheritable: true), - new ResourcePermissionSpec( - ConfigConstants.MessagingBotApiAppId, - "Messaging Bot API", - new[] { "Authorization.ReadWrite", "user_impersonation" }, - SetInheritable: true), - new ResourcePermissionSpec( - ConfigConstants.ObservabilityApiAppId, - "Observability API", - new[] { "user_impersonation" }, - SetInheritable: true), - new ResourcePermissionSpec( - PowerPlatformConstants.PowerPlatformApiResourceAppId, - "Power Platform API", - new[] { "Connectivity.Connections.Read" }, - SetInheritable: true), }; + specs.AddRange(SetupHelpers.GetFixedApiPermissionSpecs(setInheritable: true)); foreach (var customPerm in setupConfig.CustomBlueprintPermissions ?? new List()) { diff --git a/src/Microsoft.Agents.A365.DevTools.Cli/Commands/SetupSubcommands/PermissionsSubcommand.cs b/src/Microsoft.Agents.A365.DevTools.Cli/Commands/SetupSubcommands/PermissionsSubcommand.cs index d8da9e32..bc0ba826 100644 --- a/src/Microsoft.Agents.A365.DevTools.Cli/Commands/SetupSubcommands/PermissionsSubcommand.cs +++ b/src/Microsoft.Agents.A365.DevTools.Cli/Commands/SetupSubcommands/PermissionsSubcommand.cs @@ -211,7 +211,7 @@ private static Command CreateBotSubcommand( logger.LogInformation("Would configure Bot API permissions:"); logger.LogInformation(" - Blueprint: {BlueprintId}", setupConfig.AgentBlueprintId); logger.LogInformation(" - Messaging Bot API: Authorization.ReadWrite, user_impersonation"); - logger.LogInformation(" - Observability API: user_impersonation"); + logger.LogInformation(" - Observability API: user_impersonation, {OtelScope}", ConfigConstants.ObservabilityApiOtelWriteScope); logger.LogInformation(" - Power Platform API: Connectivity.Connections.Read"); return; } @@ -435,24 +435,7 @@ public static async Task ConfigureBotPermissionsAsync( try { - var specs = new List - { - new ResourcePermissionSpec( - ConfigConstants.MessagingBotApiAppId, - "Messaging Bot API", - new[] { "Authorization.ReadWrite", "user_impersonation" }, - SetInheritable: true), - new ResourcePermissionSpec( - ConfigConstants.ObservabilityApiAppId, - "Observability API", - new[] { "user_impersonation" }, - SetInheritable: true), - new ResourcePermissionSpec( - PowerPlatformConstants.PowerPlatformApiResourceAppId, - "Power Platform API", - new[] { "Connectivity.Connections.Read" }, - SetInheritable: true), - }; + var specs = new List(SetupHelpers.GetFixedApiPermissionSpecs(setInheritable: true)); var (_, _, consentGranted, _) = await BatchPermissionsOrchestrator.ConfigureAllPermissionsAsync( graphService, blueprintService, setupConfig, diff --git a/src/Microsoft.Agents.A365.DevTools.Cli/Commands/SetupSubcommands/SetupHelpers.cs b/src/Microsoft.Agents.A365.DevTools.Cli/Commands/SetupSubcommands/SetupHelpers.cs index ef406c8f..3d967ffc 100644 --- a/src/Microsoft.Agents.A365.DevTools.Cli/Commands/SetupSubcommands/SetupHelpers.cs +++ b/src/Microsoft.Agents.A365.DevTools.Cli/Commands/SetupSubcommands/SetupHelpers.cs @@ -16,6 +16,30 @@ namespace Microsoft.Agents.A365.DevTools.Cli.Commands.SetupSubcommands; /// internal static class SetupHelpers { + /// + /// Returns the fixed-scope ResourcePermissionSpecs for the three platform APIs that every + /// agent blueprint requires: Messaging Bot API, Observability API, and Power Platform API. + /// Callers control whether the specs set inheritable permissions on the blueprint. + /// + internal static ResourcePermissionSpec[] GetFixedApiPermissionSpecs(bool setInheritable) => + [ + new ResourcePermissionSpec( + ConfigConstants.MessagingBotApiAppId, + "Messaging Bot API", + new[] { "Authorization.ReadWrite", "user_impersonation" }, + setInheritable), + new ResourcePermissionSpec( + ConfigConstants.ObservabilityApiAppId, + "Observability API", + new[] { "user_impersonation", ConfigConstants.ObservabilityApiOtelWriteScope }, + setInheritable), + new ResourcePermissionSpec( + PowerPlatformConstants.PowerPlatformApiResourceAppId, + "Power Platform API", + new[] { PowerPlatformConstants.PermissionNames.ConnectivityConnectionsRead }, + setInheritable), + ]; + /// /// Display verification URLs after successful setup /// diff --git a/src/Microsoft.Agents.A365.DevTools.Cli/Constants/ConfigConstants.cs b/src/Microsoft.Agents.A365.DevTools.Cli/Constants/ConfigConstants.cs index 3560ad35..c9879ec6 100644 --- a/src/Microsoft.Agents.A365.DevTools.Cli/Constants/ConfigConstants.cs +++ b/src/Microsoft.Agents.A365.DevTools.Cli/Constants/ConfigConstants.cs @@ -78,11 +78,17 @@ public static class ConfigConstants /// /// Observability API scope used for admin consent URL construction. - /// Note: the orchestrator grants "user_impersonation" via OAuth2 permission grants; this - /// scope is the consent-URL-facing name for the same resource. + /// Note: the orchestrator grants "user_impersonation" + ObservabilityApiOtelWriteScope via OAuth2 + /// permission grants; this scope is the consent-URL-facing name for the same resource. /// public const string ObservabilityApiAdminConsentScope = "Maven.ReadWrite.All"; + /// + /// Observability API scope for writing OpenTelemetry data. + /// Granted alongside "user_impersonation" to all provisioned agent identities. + /// + public const string ObservabilityApiOtelWriteScope = "Agent365.Observability.OtelWrite"; + /// /// Production deployment environment /// @@ -106,7 +112,9 @@ public static class ConfigConstants "Chat.Read", "Chat.ReadWrite", "Files.Read.All", - "Sites.Read.All" + "Sites.Read.All", + "ChannelMessage.Read.All", + "ChannelMessage.Send", }; /// @@ -131,7 +139,10 @@ public static class ConfigConstants "Mail.Send", "Chat.ReadWrite", "User.Read.All", - "Sites.Read.All" + "Sites.Read.All", + "Files.ReadWrite.All", + "ChannelMessage.Read.All", + "ChannelMessage.Send", };