Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
27 changes: 27 additions & 0 deletions .claude/agents/pr-code-reviewer.md
Original file line number Diff line number Diff line change
Expand Up @@ -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<ResourcePermissionSpec>` 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:
Expand Down
3 changes: 3 additions & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -175,7 +175,7 @@ public static Command CreateCommand(ILogger<CreateInstanceCommand> 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.");
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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<CustomResourcePermission>())
{
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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<CustomResourcePermission>())
{
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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;
}
Expand Down Expand Up @@ -435,24 +435,7 @@ public static async Task<bool> ConfigureBotPermissionsAsync(

try
{
var specs = new List<ResourcePermissionSpec>
{
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<ResourcePermissionSpec>(SetupHelpers.GetFixedApiPermissionSpecs(setInheritable: true));

var (_, _, consentGranted, _) = await BatchPermissionsOrchestrator.ConfigureAllPermissionsAsync(
graphService, blueprintService, setupConfig,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,30 @@ namespace Microsoft.Agents.A365.DevTools.Cli.Commands.SetupSubcommands;
/// </summary>
internal static class SetupHelpers
{
/// <summary>
/// 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.
/// </summary>
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),
];

/// <summary>
/// Display verification URLs after successful setup
/// </summary>
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -78,11 +78,17 @@ public static class ConfigConstants

/// <summary>
/// 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.
/// </summary>
public const string ObservabilityApiAdminConsentScope = "Maven.ReadWrite.All";

/// <summary>
/// Observability API scope for writing OpenTelemetry data.
/// Granted alongside "user_impersonation" to all provisioned agent identities.
/// </summary>
public const string ObservabilityApiOtelWriteScope = "Agent365.Observability.OtelWrite";

/// <summary>
/// Production deployment environment
/// </summary>
Expand All @@ -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",
};

/// <summary>
Expand All @@ -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",
};


Expand Down
Loading