Merge remote-tracking branch 'origin/main' into users/sellak/issue-408-secret-null-macos

# Conflicts:
#	CHANGELOG.md

Author: sellakumaran

.claude/agents/pr-code-reviewer.md

@@ -215,6 +215,56 @@ For each changed file, analyze:
    - **Fix pattern**: replace `context.ExitCode = 1;` with `context.ExitCode = ex.ExitCode;` so the exit code is always authoritative from the exception definition
    - **Real example (PR #406, Comments 5 & 6)**: `ConfigFileNotFoundException.ExitCode => 2`, but two catch blocks in `AllSubcommand.cs` hardcoded `context.ExitCode = 1`. Scripts expecting exit code 2 on configuration errors received 1 instead.
 
+   **E. Static scope array emptied or scopes removed → verify callers and document intent**
+   When the diff sets a static `string[]` scope array to `[]` or removes entries from it (e.g., `RequiredPermissionGrantScopes`, `RequiredS2SGrantScopes`):
+   - Run `Grep` for every reference to the array across non-test source files
+   - For each call site that passes the array as `scopes` to `EnsureGraphHeadersAsync` or equivalent:
+     - Read the `hasScopes = scopes?.Any() == true` guard logic — confirm the empty-array fallback path still acquires a valid token via `GetGraphAccessTokenAsync` (the standard `AuthenticationService` path)
+     - Confirm the operations that previously needed those scopes are still covered by `RequiredClientAppPermissions` or by a PowerShell fallback
+   - Check that the XML doc on the constant explains WHY it is empty and names the fallback behavior
+   - Severity: **MEDIUM** if XML doc doesn't explain empty state; **HIGH** if fallback path would fail in practice
+
+   **F. Runtime `Contains()` on a static compile-time array → verify element IS in the array**
+   When the diff adds `SomeStaticArray.Contains(SomeConstant)` as a guard condition:
+   - Read `SomeStaticArray`'s initializer — confirm whether `SomeConstant` is actually present
+   - If `SomeConstant` is intentionally absent (disabled feature, future path), the condition is always `false`:
+     - Verify the `else` branch handles the always-false case correctly and critical functionality is not silently skipped
+     - Require a comment that makes the always-false state explicit AND states the consequence (e.g., "this means agent user cleanup is always skipped until create-instance is re-enabled")
+   - Severity: **MEDIUM** if consequence is limited scope; **HIGH** if critical functionality (e.g., cleanup of real resources) is silently disabled with no user-visible warning
+   - **Real example (PR #409)**: `RequiredClientAppPermissions.Contains(AgentIdUserReadWriteAllScope)` is always `false` because that scope is intentionally absent — agent user queries and cleanup always skip silently
+
+   **G. XML/inline comments claiming permission scope semantics → verify against actual scope requirements**
+   When the diff adds or modifies `///` XML comments (especially in test files) that describe what a permission scope covers (e.g., "umbrella — includes SP creation", "covers X, Y, Z"):
+   - Cross-check the claim against `RequiredClientAppPermissions` and the PR's own changes
+   - Specifically: if the PR adds a *separate* scope for an operation (e.g., `AgentIdentityBlueprintPrincipal.Create` for SP creation), any comment claiming that operation is "covered by the umbrella" is now wrong
+   - Flag **MEDIUM** for inaccurate scope coverage claims in comments — these mislead future reviewers and generate follow-up PR comments
+   - **Real example (PR #409)**: XML comment said `AgentIdentityBlueprint.ReadWrite.All` "includes SP creation" but the same PR requires `AgentIdentityBlueprintPrincipal.Create` separately for SP creation
+
+   **H. JWT claim decoded → verify the token was issued by the app registration that has the claim configured**
+   When the diff adds code that decodes a JWT and reads a claim that is configured *per app registration*
+   (e.g., `wids`, `roles`, `groups`, any custom optional claim from Entra Token Configuration):
+   - Trace backward from the decoder to the token-acquisition call. Identify which clientId issues the token.
+   - If acquisition goes through `AuthenticationService.GetAccessTokenAsync(...)` without an explicit
+     `clientId` argument, the token is from the PowerShell well-known clientId
+     (`AuthenticationConstants.PowershellClientId`) — **not** the custom client app. Optional claims
+     configured on the custom client app's manifest will NOT be in this token.
+   - The correct path for tokens that must carry custom-app optional claims is
+     `IMicrosoftGraphTokenProvider.GetMgGraphAccessTokenAsync(..., clientAppId: CustomClientAppId, ...)`.
+   - Flag **HIGH** when the decoder relies on a claim that the issuing app doesn't have configured.
+     The bug is silent: the decoder returns "claim absent → Unknown / DoesNotHaveRole", and the caller
+     proceeds with the conservative path (e.g., admin treated as non-admin → admin URL printed even
+     for actual admins). Functional but degraded UX, easy to miss in passing tests because mocks
+     usually return a hand-crafted JWT regardless of which auth path is used.
+   - Also apply this rule to **empty static scope arrays** (related to Rule E): when an empty
+     `IEnumerable<string>` is passed to a helper that routes "no scopes" through the legacy
+     `AuthenticationService` path, the token will be from the PowerShell clientId. If any downstream
+     code reads custom-app optional claims, the routing is wrong.
+   - **Real example (PR #409)**: `CheckDirectoryRoleAsync` decoded `wids` from a token acquired via
+     `GetGraphAccessTokenAsync` → `AuthenticationService` → PowerShell clientId. `wids` was
+     configured on the custom client app only, so the JWT never carried it and the method always
+     returned `Unknown`. Fix: route through `_tokenProvider` with `CustomClientAppId` and a minimal
+     scope (`User.Read`).
+
    **D2. Cross-method "catch-returns-null / caller-sets-ExitCode" pattern — same exit-code problem, harder to spot**
    Rule 9-D catches *inline* catch blocks. This variant spans two methods and is easy to miss:
    ```csharp

.github/workflows/ci.yml

@@ -13,6 +13,7 @@ jobs:
   dotnet-sdk:
     name: .NET SDK
     runs-on: ubuntu-latest
+    timeout-minutes: 20
     permissions:
       contents: write
     defaults:
@@ -52,7 +53,7 @@ jobs:
         run: dotnet build tests.proj --no-restore --configuration Release
 
       - name: Run tests
-        run: dotnet test tests.proj --no-build --configuration Release --verbosity normal
+        run: dotnet test tests.proj --no-build --configuration Release --verbosity normal --blame-hang --blame-hang-timeout 5min
 
       - name: Pack NuGet packages
         id: pack

.gitignore

@@ -1,5 +1,6 @@
 # Internal working documents
 docs/plans/
+docs/min-permissions/
 docs/Permissions-Review.md
 docs/Testing.md
 scripts/skills/

CHANGELOG.md

@@ -42,9 +42,16 @@ Agents provisioned before this release need `Agent365.Observability.OtelWrite` g
 - Messaging endpoint row added to `a365 setup all` summary output, with "registered"/"reused"/"skipped (non-M365)"/"manual config required"/"failed" states. When registration can't complete, the summary surfaces an "Action Required" entry with the Teams Developer Portal URL so the user knows exactly what to do next.
 - Defensive fallback when the server rejects the new request with a known contract-mismatch signature — the CLI logs `"Automated messaging endpoint registration is not available for this tenant yet. You'll need to configure it manually."` and directs the user to the Teams Developer Portal. Same user-facing path is reused when registration fails because the signed-in user is not a blueprint owner.
 
+### Changed
+- Agent identity creation now uses Blueprint app-only credentials (`AgentIdentity.CreateAsManager`, auto-granted to all Blueprint apps). The custom CLI app no longer requires `AgentIdentity.Create.All` or `DelegatedPermissionGrant.ReadWrite.All`. Administrators can remove these permissions from the CLI app registration. See [Custom client app registration](https://learn.microsoft.com/microsoft-agent-365/developer/custom-client-app-registration) for the updated permission list.
+- `setup all` now retries agent identity creation and blueprint token acquisition with exponential back-off (delay doubles up to a 60-second cap; agent identity retries up to 5 times, blueprint token up to 12 times — worst case is several minutes per call when Entra replication lag is severe) when Entra replication lag causes transient 401/AADSTS errors on fresh blueprint setups. Retry progress is logged at `Debug` level only.
+
 ### Fixed
 - `setup blueprint` / `setup all` on macOS: `agentBlueprintClientSecret` was written to the wrong file when symlinks were present in the working directory path. `Environment.CurrentDirectory` (getcwd, symlink-resolved) diverged from `config.DirectoryName` (unresolved), causing the secret save to target a different file than `LoadAsync` reads back — resulting in `agentBlueprintClientSecret: null` on subsequent runs. Fixed by threading the resolved `generatedConfigPath` explicitly through `CreateBlueprintClientSecretAsync`.
 - CLI log file now captures `[DBG]` messages by default. Previously `SetMinimumLevel` was applied globally, preventing Debug-level messages from reaching the file logger even though `FileLoggerProvider` was configured to accept Trace and above.
+- `setup all` with `--authmode obo --aiteammate` no longer exits with an error. `obo` is the default for AI Teammate agents and is accepted with a warning; `--authmode s2s` and `--authmode both` remain incompatible with `--aiteammate`.
+- `setup all` summary now shows **Messaging endpoint — not configured** (with a link to the Teams Developer Portal to register the endpoint manually) instead of **failed — see Action Required** when the messaging endpoint URL is absent from config. Real failures (bad URL, contract mismatch) continue to show the error path.
+- `setup blueprint` no longer silently loses the blueprint client secret on re-runs when `agentBlueprintClientSecret` was previously `null` in `a365.generated.config.json`. Null dynamic properties are now omitted from the generated config file instead of being written as explicit nulls (fixes macOS secret-loss regression introduced in the issue #408 fix).
 - Error messages for commands run without required configuration no longer expose internal file paths. `setup all`, `cleanup`, and `create-instance` without `--agent-name` now show actionable guidance with the exact command to run. `develop addpermissions` and `develop gettoken` without `--app-id` now prompt for the application ID directly.
 - `setup all` no longer inherits stale resource IDs when the user switches tenants between runs (`az logout` + `az login` to a different tenant). The CLI detects the tenant change before loading configuration, silently backs up files from the previous run, and prompts the user to re-run with `--agent-name` for a clean setup in the new tenant.
 - `setup permissions bot` no longer emits "Bot API permissions configured successfully" when any S2S app-role assignment fails; shows a warning with retry instructions instead.
@@ -53,6 +60,7 @@ Agents provisioned before this release need `Agent365.Observability.OtelWrite` g
 - `setup all` now skips agent registration with a clear warning when the agent identity ID is not available, instead of silently sending an invalid request. Retry with `a365 setup all --agent-registration-only` once the identity is ready.
 - `setup permissions bot` now returns a non-zero exit code when an S2S app role assignment fails, so callers and scripts can detect the failure.
 - `setup all --agent-registration-only` reliability fixes: stored IDs are now correctly read in bootstrap (`--agent-name`) mode; falls back to a Graph API lookup when `agenticAppId` is missing; skips identity, permission, and project-settings steps that don't apply.
+- `setup permissions bot` help text and final "Next step" log no longer suggest the non-existent `a365 deploy` command; both now point at `a365 publish` (the actual next command in the workflow).
 
 ### Removed
 - `a365 config` command family (`config init`, `config display`, `config permissions`) — replaced by `a365 setup all --agent-name` and `a365 setup permissions custom`.

src/Microsoft.Agents.A365.DevTools.Cli/Commands/SetupSubcommands/AllSubcommand.cs

@@ -179,9 +179,16 @@ public static Command CreateCommand(
             }
             if (authMode is not null && aiTeammateFlag == true)
             {
-                logger.LogError("--authmode is not supported with --aiteammate — AI Teammate agents automatically use OBO via agent user identity.");
-                context.ExitCode = 1;
-                return;
+                if (authMode == "obo")
+                {
+                    logger.LogWarning("--authmode obo is redundant with --aiteammate — AI Teammate agents always use OBO. Flag ignored.");
+                }
+                else
+                {
+                    logger.LogError("--authmode {AuthMode} is not supported with --aiteammate — AI Teammate agents always use OBO via agent user identity.", authMode);
+                    context.ExitCode = 1;
+                    return;
+                }
             }
 
             // Generate correlation ID at workflow entry point
@@ -782,7 +789,7 @@ internal static async Task ExecuteMessagingEndpointStepAsync(SetupContext ctx)
         {
             ctx.Logger.LogWarning("Messaging endpoint registration skipped: agent blueprint ID is missing (the blueprint step likely failed).");
             ctx.Results.MessagingEndpointResult = Models.EndpointRegistrationResult.Failed;
-            ctx.Results.MessagingEndpointFailureReason = "BlueprintMissing";
+            ctx.Results.MessagingEndpointFailureReason = MessagingEndpointFailureReasons.BlueprintMissing;
             ctx.Results.MessagingEndpoint = ctx.Config.MessagingEndpoint;
             ctx.Results.Warnings.Add("Messaging endpoint: agent blueprint ID is missing, so endpoint registration was not attempted. Resolve the blueprint creation failure first, then re-run 'a365 setup blueprint --endpoint-only --m365'.");
             return;
@@ -816,7 +823,7 @@ internal static async Task ExecuteMessagingEndpointStepAsync(SetupContext ctx)
             // setup should continue; surface the failure in the summary as a warning.
             ctx.Logger.LogWarning("Messaging endpoint registration skipped: {Message}", ex.Message);
             ctx.Results.MessagingEndpointResult = Models.EndpointRegistrationResult.Failed;
-            ctx.Results.MessagingEndpointFailureReason = "Other";
+            ctx.Results.MessagingEndpointFailureReason = MessagingEndpointFailureReasons.Other;
             ctx.Results.MessagingEndpoint = ctx.Config.MessagingEndpoint;
             ctx.Results.Warnings.Add($"Messaging endpoint: {ex.Message}");
         }

src/Microsoft.Agents.A365.DevTools.Cli/Commands/SetupSubcommands/BlueprintSubcommand.cs

@@ -792,7 +792,7 @@ public static async Task<bool> EnsureDelegatedConsentWithRetriesAsync(
         {
             logger.LogError(ex, "Error during delegated consent: {Message}", ex.Message);
             logger.LogError("Common causes:");
-            logger.LogError("  1. Insufficient permissions - You need Application.ReadWrite.All and DelegatedPermissionGrant.ReadWrite.All");
+            logger.LogError("  1. Insufficient permissions - You need AgentIdentityBlueprint.ReadWrite.All consented on your client app");
             logger.LogError("  2. Not a Global Administrator or similar privileged role");
             logger.LogError("  3. Azure CLI authentication expired - Run 'az login' and retry");
             logger.LogError("  4. Network connectivity issues");
@@ -1023,7 +1023,9 @@ public static async Task<bool> EnsureDelegatedConsentWithRetriesAsync(
             // Explicit scopes — NOT .default. Using .default bundles all consented scopes including
             // AgentIdentityBlueprint.*, which Entra rejects for POST /v1.0/servicePrincipals
             // ("backing application must be in the local tenant").
-            // AgentIdentityBlueprintPrincipal.Create is the correct scope per Agent ID team (Kyle Marsh).
+            // AgentIdentityBlueprintPrincipal.Create is required for POST /v1.0/serviceprincipals/graph.agentIdentityBlueprintPrincipal.
+            // AgentIdentityBlueprint.ReadWrite.All is the umbrella for blueprint app operations but does NOT cover
+            // blueprint SP creation — that is a separate resource requiring Principal.Create (confirmed Run 1, 2026-05-08).
             logger.LogDebug("Acquiring blueprint httpClient token — scope: AgentIdentityBlueprintPrincipal.Create, loginHint: {LoginHint}", blueprintLoginHint ?? "(none)");
             var graphToken = await AcquireMsalGraphTokenAsync(tenantId, setupConfig.ClientAppId, logger, ct,
                 scope: AuthenticationConstants.AgentIdentityBlueprintPrincipalCreateScope,
@@ -1514,7 +1516,7 @@ await retryHelper.ExecuteWithRetryAsync(
                 ficError = ficCreateResult?.ErrorMessage
                     ?? "Federated Identity Credential creation failed";
                 logger.LogWarning("[WARN] Federated Identity Credential creation failed - you may need to create it manually in Entra ID");
-                logger.LogWarning("  Ensure the client app has 'AgentIdentityBlueprint.UpdateAuthProperties.All' permission consented.");
+                logger.LogWarning("  Ensure the client app has 'AgentIdentityBlueprint.ReadWrite.All' permission consented.");
             }
         }
         else if (!useManagedIdentity)