Fix #460: skip redundant agent-identity S2S grant when role is inherited

When inheritable permissions are configured (allAllowed) and the blueprint SP
holds the app roles, the agent identity inherits them automatically — the same
basis the OBO/delegated path already relies on. The direct grant on the agent
identity SP was therefore redundant and, when the CLI token cannot write app
roles, produced a spurious "Action Required" PowerShell block on an otherwise
successful Global Admin run.

- Skip the per-identity grant (and the PowerShell prompt) when the blueprint
  grant and inheritance both succeeded; report the inherited grant as Granted.
- When a direct grant is still needed (developer / non-inherited path), retry
  via az rest before falling back to PowerShell, matching the blueprint grant.
- Correct stale README/CHANGELOG claim that Phase 2a/2b are always skipped for
  non-DW agents (untrue since #421).
- Add review-time checks for comment-essay and CHANGELOG crispness
  (pr-code-reviewer #30/#31, review-pr SKILL) and codify both in CLAUDE.md and
  copilot-instructions.

Verified live: setup completes cleanly with no PowerShell block, and
query-entra reports the Observability app role granted on the blueprint SP with
effective inheritance OK.

Author: sellakumaran

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

@@ -1122,6 +1122,26 @@ When a string property represents a discrete set of values (e.g., `"obo"`, `"s2s
   ```
 - **Real example (PR #406, Comment 1)**: `isShowSecret = args.Contains("--show-secret")` in `Program.cs` would not skip the Graph preflight when the user passed `--show-secret=true`, accidentally triggering an online call on an offline-only command.
 
+### 30. Comment Essay — Multi-Line Rationale Block Where One Line Suffices
+
+Comments added in the diff must be crisp. A comment states *why* in one or two lines; it does not retell the change, restate what the code already says, or narrate design history (that belongs in the commit message / PR description).
+
+- **Pattern to catch**: any newly-added comment block — `//` inline or `///` XML `<summary>` — running more than ~2 lines of prose rationale; comments that paraphrase the next statement; commit-message-style narration ("Previously we did X, now we do Y because...") in source.
+- **Severity**: `low` — not a runtime defect, but the repo standard. Flag every instance so it does not accumulate.
+- **Check**: for each added comment, ask "does this say *why* in one line?" If it explains mechanism the code already shows, or runs to a paragraph, it is an essay. Trivial mechanical edits (a log-level change, a blank-line separator, a `catch { throw; }`) get **zero or one** line, never a rationale block.
+- **Fix**: cut to a single-line *why*; move the long-form reasoning to the commit message or PR body. Keep an issue/PR reference (`(issue #460)`) but drop the surrounding paragraph.
+- **Real example (issue #460)**: a 7-line `<summary>` and a 6-line inline `// Issue #460: ...` block both restated the inheritance rationale already captured in the commit; each was cut to one line plus the issue reference.
+
+### 31. CHANGELOG Entry Not Release-Note-Ready
+
+`CHANGELOG.md` `[Unreleased]` feeds straight into the nuget.org release notes, so each entry must be **one crisp sentence about the user-visible change** — readable by a package consumer who has never seen the code.
+
+- **Pattern to catch**: a new/edited `CHANGELOG.md` entry that names internal classes/methods (`BatchPermissionsOrchestrator`, `Phase 2a`), explains implementation mechanism (`retries via az rest`, `POST /appRoleAssignments`), restates rationale, or runs to multiple sentences of background.
+- **Severity**: `low`.
+- **Check**: read each CHANGELOG line in the diff. Would a consumer who only runs the CLI understand the *behavior* change from it, with no code knowledge? If it leans on internals or reads like a design note, it fails. Also verify the entry does not contradict another entry in the same `[Unreleased]` block (stale claims left behind by the change).
+- **Fix**: rewrite to the user-facing outcome in one sentence; keep the `(#NNN)` reference. Drop class/method names and mechanism. Correct any sibling entry the change makes stale.
+- **Real example (issue #460)**: a two-sentence entry naming `az rest` / PowerShell internals was cut to one outcome-focused sentence; a stale sibling line ("Phase 2a/2b always skipped for non-DW agents") that the change contradicted was corrected at the same time.
+
 ## Example Invocation
 
 When you receive a request like "Review PR #253", you should:

.claude/skills/review-pr/SKILL.md

@@ -63,6 +63,8 @@ This skill enforces the following principles:
 - **Function reuse**: Encourages reusing functions across commands
 - **No special characters**: Avoids emojis in logs/output (Windows compatibility)
 - **Self-documenting code**: Prefers clear code over excessive comments
+- **Crisp comments (pr-code-reviewer #30)**: Flags added comments that run past 1-2 lines, restate the code, or narrate design history — a comment says *why* in one line; long-form reasoning belongs in the commit/PR.
+- **Release-note-ready CHANGELOG (pr-code-reviewer #31)**: Flags `CHANGELOG.md` entries that name internals, explain mechanism, or run multiple sentences — each entry is one crisp consumer-facing sentence (it ships verbatim to nuget.org release notes).
 - **Minimal changes**: Makes only necessary changes to solve the problem
 
 ### Testing Standards

.github/copilot-instructions.md

@@ -100,6 +100,13 @@
 - Keep user-facing messages clear and professional
 - Follow client-facing help text conventions
 
+### Comments
+- Comments are crisp: state *why* in one or two lines, never an essay. A `//` or `///` `<summary>` that runs to a paragraph, restates the code, or narrates the change ("previously X, now Y because...") belongs in the commit message / PR, not in source.
+- Keep an issue/PR reference (`(issue #460)`) but drop the surrounding narration. Trivial mechanical edits get zero or one comment line.
+
+### CHANGELOG
+- `CHANGELOG.md` `[Unreleased]` ships verbatim to nuget.org release notes. Each entry is one crisp consumer-facing sentence — no class/method names, no implementation mechanism, no multi-sentence rationale. Keep the `(#NNN)` reference; fix any sibling entry the change makes stale.
+
 ### Code Review Mindset
 - Be cautious about deleting code; avoid `git restore` without review
 - Do not create unnecessary documentation files

CHANGELOG.md

@@ -37,7 +37,7 @@ Agents provisioned before this release need `Agent365.Observability.OtelWrite` g
   - `obo` (default): principal-scoped delegated grants (`consentType: "Principal"`); no Global Administrator required.
   - `s2s`: application role assignments on the agent identity SP; attempted programmatically, falls back to printed PowerShell instructions if the caller lacks Global Administrator.
   - `both`: applies both OBO delegated grants and S2S app role assignments.
-  - Inheritable permissions (Phase 2a) and AllPrincipals grants (Phase 2b) are always skipped for non-DW agents regardless of `authMode`, to avoid requiring a Global Administrator role.
+  - Non-DW agents stamp inheritable permissions and S2S grants on the blueprint (Global Administrator only); the agent identity inherits them, so a per-identity grant runs only when inheritance is not in force.
   - `authMode` can be persisted in `a365.config.json` to apply on every run without the flag.
 - `--project-path <path>` option on `develop list-configured`, `develop add-mcp-servers`, and `develop remove-mcp-servers` — specify the manifest location without requiring `a365.config.json`.
 - `setup requirements` runs without `a365.config.json` — system checks (PowerShell modules, Frontier enrollment) always run; client app checks run when a config file or Azure CLI session is available.
@@ -58,6 +58,7 @@ Agents provisioned before this release need `Agent365.Observability.OtelWrite` g
 - `a365 develop get-token --device-code` — forces device code auth for Microsoft Graph scopes the Windows WAM broker rejects (e.g. Exchange `MailboxSettings.ReadWrite`, `ExchangeMessageTrace.Read.All`).
 
 ### Fixed
+- `setup all --authmode s2s` no longer prints spurious "Action Required" PowerShell steps when the agent identity already inherits its app roles from the blueprint, and now retries the grant automatically before falling back to manual steps (#460).
 - `a365 develop get-token` now falls back to device code when the Windows WAM broker rejects Exchange Graph scopes with `ApiContractViolation`, instead of failing with an opaque MSAL error.
 - `setup blueprint` now configures the blueprint's inheritable Microsoft Graph permissions even when the signed-in user is not a Global Administrator, no longer aborts with a misleading "Failed to configure inheritable permissions" error when the tenant-wide consent grant cannot be made programmatically, and ends with a setup summary whose Action Required block surfaces the admin-consent URL for non-admins to hand off (#452).
 - Messaging endpoint registration and removal now retry on transient network errors instead of failing on a momentary DNS or connection blip.

CLAUDE.md

@@ -111,6 +111,27 @@ src/Microsoft.Agents.A365.DevTools.Cli/
 - All `IDisposable` objects must be disposed (especially `HttpResponseMessage`)
 - Cross-platform compatibility required (Windows, macOS, Linux)
 
+### Comments
+Comments are crisp: state *why* in one line, not *what* the code already shows. Do not write essays.
+- A code comment (`//` or `///` `<summary>`) is **one or two lines**. If you need a paragraph of rationale, it belongs in the commit message or PR description, not in source.
+- Keep an issue/PR reference (`(issue #460)`) but drop the surrounding narration.
+- Never narrate the change ("previously we did X, now Y because...") — that is commit-message content.
+- Trivial mechanical edits (log-level change, blank-line separator, `catch { throw; }`) get zero or one line, never a rationale block.
+
+```csharp
+// Bad: essay restating the change and its history
+// Issue #460: when inheritable permissions were configured with kind=allAllowed (covering both
+// scopes and roles) AND the blueprint SP was granted the app roles, the agent identity inherits
+// them automatically — the same basis the OBO branch relies on. A direct grant then only adds a
+// duplicate row and a spurious prompt, so we skip it and report the inherited grant as Granted.
+
+// Good: one-line why + reference
+// Skip the direct grant when the role is already inherited from the blueprint (issue #460).
+```
+
+### CHANGELOG entries
+`CHANGELOG.md` `[Unreleased]` ships verbatim to nuget.org release notes. Each entry is **one crisp consumer-facing sentence** about the user-visible change — no class/method names, no implementation mechanism, no multi-sentence rationale. Keep the `(#NNN)` reference. When a change makes a sibling entry stale, fix it in the same edit.
+
 ### Input Validation
 User-controlled input that reaches file system operations must be validated before use. This applies to CLI arguments, config values read from disk, and any value whose origin is outside this process.
 

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

@@ -526,7 +526,7 @@ private static async Task ExecuteAgentIdentityAndRegistrationAsync(
 
                 // S2S and Both: app role assignments (requires Global Admin; falls back to PowerShell instructions).
                 if (ctx.IsS2sMode || ctx.IsBothMode)
-                    await GrantOrInstructAgentIdentityAppPermissionsAsync(ctx, specs);
+                    await GrantAgentIdentityS2SPermissionsAsync(ctx, specs);
             }
         }
 
@@ -655,10 +655,39 @@ private static async Task ExecuteAgentIdentityAndRegistrationAsync(
         }
     }
 
+    /// <summary>
+    /// Step 5a (S2S/Both): grants the agent identity's app roles, or skips the grant when they are
+    /// already inherited from the blueprint (issue #460, see <see cref="AgentIdentityInheritsBlueprintAppRoles"/>).
+    /// </summary>
+    internal static async Task GrantAgentIdentityS2SPermissionsAsync(
+        SetupContext ctx,
+        List<ResourcePermissionSpec> specs)
+    {
+        var hasS2sSpecs = specs.Any(s => s.AppRoleScopes is { Length: > 0 });
+        if (hasS2sSpecs && AgentIdentityInheritsBlueprintAppRoles(ctx.Results))
+        {
+            ctx.Logger.LogDebug("Agent identity inherits S2S app roles from the blueprint; skipping redundant direct grant.");
+            ctx.Results.AgentIdentityS2SOutcome = Models.GrantOutcome.Granted;
+            return;
+        }
+
+        await GrantOrInstructAgentIdentityAppPermissionsAsync(ctx, specs);
+    }
+
+    /// <summary>
+    /// True when the agent identity already holds the blueprint's S2S app roles via inheritance —
+    /// inheritable permissions configured (allAllowed) and the blueprint SP granted the roles — so a
+    /// direct grant is redundant (issue #460). Both conditions are required.
+    /// </summary>
+    internal static bool AgentIdentityInheritsBlueprintAppRoles(SetupResults results) =>
+        results.BatchPermissionsPhase2Completed
+        && results.BlueprintS2SOutcome == Models.GrantOutcome.Granted;
+
     /// <summary>
     /// Attempts to grant app role assignments on the agent identity SP for S2S access.
-    /// Requires Agent ID Administrator, Application Administrator, or Global Administrator. When the signed-in user lacks
-    /// one of those roles, prints PowerShell instructions covering only the app permission section.
+    /// Requires Agent ID Administrator, Application Administrator, or Global Administrator. When the
+    /// programmatic Graph path fails, retries via <c>az rest</c> against the operator's existing az
+    /// session before falling back to PowerShell instructions covering only the app permission section.
     /// </summary>
     internal static async Task GrantOrInstructAgentIdentityAppPermissionsAsync(
         SetupContext ctx,
@@ -712,6 +741,20 @@ internal static async Task GrantOrInstructAgentIdentityAppPermissionsAsync(
             return;
         }
 
+        // Issue #460: the Graph path failed (CLI token lacks AppRoleAssignment.ReadWrite.All). Retry
+        // via az rest before PowerShell — a GA's az token carries it via the directory role. Mirrors
+        // the blueprint SP fallback in BatchPermissionsOrchestrator.
+        ctx.Logger.LogDebug("S2S app role assignments on the agent identity could not be completed via the Graph API; falling back to az rest.");
+        var (attempted, succeeded) = await AzRestS2SRunner.TryRunAsync(
+            ctx.Executor, agentIdentitySpObjectId, failedSpecs, ctx.Logger, ctx.CancellationToken);
+        if (attempted && succeeded)
+        {
+            using (ctx.Logger.Indent())
+                ctx.Logger.LogInformation("S2S app role assignments granted to agent identity.");
+            ctx.Results.AgentIdentityS2SOutcome = Models.GrantOutcome.Granted;
+            return;
+        }
+
         // Non-admin fallback: print PowerShell instructions for only the failed resources.
         ctx.Results.AgentIdentityS2SOutcome = Models.GrantOutcome.Failed;
         ctx.Logger.LogInformation("");

src/Microsoft.Agents.A365.DevTools.Cli/Commands/SetupSubcommands/README.md

@@ -21,7 +21,7 @@ This folder contains the workflow components for the `a365 setup` command. The s
 | **SetupHelpers** | `SetupHelpers.cs` | Shared helper methods; `EnsureResourcePermissionsAsync` used by standalone callers and `CopilotStudioSubcommand` |
 | **SetupResults** | `SetupResults.cs` | Result models for setup operations |
 | **SetupContext** | `SetupContext.cs` | Context bundle threaded through orchestrator steps; exposes `AuthMode`, `IsOboMode`, `IsS2sMode`, `IsBothMode` |
-| **NonDwBlueprintSetupOrchestrator** | `NonDwBlueprintSetupOrchestrator.cs` | Blueprint-based non-DW setup flow; skips Phase 2a/2b (inheritable permissions); gates agent identity grants by `authMode` |
+| **NonDwBlueprintSetupOrchestrator** | `NonDwBlueprintSetupOrchestrator.cs` | Blueprint-based non-DW setup flow; stamps inheritable permissions + S2S grants on the blueprint, then gates agent identity grants by `authMode` (skipping the per-identity S2S grant when the role is already inherited) |
 
 ---
 
@@ -74,12 +74,12 @@ The `--authmode` option controls how the agent identity service principal is gra
 | Value | Behaviour |
 |-------|-----------|
 | `obo` (default) | Principal-scoped delegated grants (`consentType: "Principal"`) on the agent identity SP — no Global Admin required |
-| `s2s` | Application role assignments on the agent identity SP — attempted programmatically; PowerShell instructions printed as fallback if the caller lacks Global Admin |
+| `s2s` | Application role assignments — when the blueprint already holds the roles and inheritable permissions are configured (`allAllowed`), the agent identity inherits them and no direct grant is made; otherwise the grant is attempted on the agent identity SP programmatically, then via `az rest`, with PowerShell instructions printed only if both fail |
 | `both` | Both OBO delegated grants and S2S app role assignments |
 
 `authMode` may also be persisted in `a365.config.json` so it takes effect on every run without the flag.
 
-Phase 2a (inheritable permissions on the blueprint) and Phase 2b (AllPrincipals grants) are **always skipped** for non-DW agents regardless of `authMode`, to avoid requiring a Global Admin role.
+Non-DW agents stamp the same permission spec set on the blueprint (inheritable permissions with `kind=allAllowed`, plus S2S app-role grants on the blueprint SP when the caller is a Global Admin). Because inheritance covers both scopes and roles, the agent identity inherits the blueprint's grants automatically — so the per-identity S2S grant (issue #460) is skipped when the blueprint grant and inheritance both succeeded, and the delegated (OBO) path never issues a per-identity grant at all.
 
 ```bash
 # Use OBO grants (default)