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

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

@@ -1122,6 +1122,38 @@ 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.
+
+**MANDATORY REPORTING RULE**: Whenever the diff adds or modifies any comment (`//` or `///` `<summary>`), you MUST emit a named finding for this check with one of two statuses: **`low` per instance** if essays are found (list every `file:line`), or **`info` — PASS** if all added comments are crisp. Do NOT silently omit it. A `low`-severity comment sweep is exactly what a multi-finding fan-out drops as noise — which is how 9 `<summary>` essays shipped past review in PR #461 and were caught only by Copilot. The check is cheap and per-instance; report it every time.
+
+### 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.
+
+### 32. Test Relies on a Shared Mock-Builder Default Instead of an Explicit Stub
+
+When a test reaches an assertion through a call it does not stub — depending instead on a shared mock/helper builder's *default* return (e.g. `BuildMockExecutor`, `BuildS2SGrantTestContext`) — the test is silently coupled to that helper's internals. A later change to the default flips the test's meaning with no local signal, and the test no longer documents what it actually exercises.
+
+- **Pattern to catch**: a test whose asserted path depends on an un-stubbed call that a *sibling* test in the same file stubs explicitly; a comment like "relies on the default ... behavior"; or an assertion that only holds because of an implicit helper return.
+- **Severity**: `medium` — non-deterministic coupling; the test can pass for the wrong reason after an unrelated helper edit.
+- **Check**: for each new/changed test, list the calls the asserted path depends on. Confirm each is stubbed in *this* test body — not merely defaulted by the builder, and not merely stubbed in a neighbouring test. If a sibling stubs the same call explicitly and this one does not, flag the inconsistency.
+- **Fix**: stub every call the assertion depends on explicitly in the test body, mirroring the sibling that already does so.
+- **Real example (PR #461, Copilot)**: `..._GraphAndAzRestBothFail_...` relied on `BuildMockExecutor`'s default for the initial `GET appRoleAssignments` while its sibling `..._GraphFailsAzRestSucceeds_...` stubbed it explicitly. The GET was added explicitly so the fallback's failure point is deterministic.
+
 ## 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
@@ -99,7 +101,7 @@ You can edit this file to:
 The skill uses **Claude Code directly** for semantic code analysis (inspired by Agent365-dotnet). No separate API key required!
 
 **Generate mode** (default):
-1. Claude Code reads `.claude/agents/pr-code-reviewer.md` for review process guidelines
+1. Claude Code reads `.claude/agents/pr-code-reviewer.md` for review process guidelines. Read the **working-tree (PR) version** of this file and of `.github/copilot-instructions.md` and `CLAUDE.md` — not the base-branch copy. When the PR under review *adds or changes a review rule* (as PR #461 did with rules #30/#31), the new rule must be applied to that same PR in the same run; reading the base copy would skip it.
 2. Claude Code reads `.github/copilot-instructions.md` for coding standards
 3. Claude Code fetches PR details: `gh pr view <number> --json ...`
 4. Claude Code analyzes actual code changes: `gh pr diff <number>`

.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);
             }
         }
 
@@ -656,9 +656,33 @@ private static async Task ExecuteAgentIdentityAndRegistrationAsync(
     }
 
     /// <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.
+    /// Grants the agent identity's S2S app roles, or skips when already inherited from the blueprint (issue #460).
+    /// </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 inherits the blueprint's S2S app roles, making a direct grant redundant (issue #460).
+    /// </summary>
+    internal static bool AgentIdentityInheritsBlueprintAppRoles(SetupResults results) =>
+        results.BatchPermissionsPhase2Completed
+        && results.BlueprintS2SOutcome == Models.GrantOutcome.Granted;
+
+    /// <summary>
+    /// Grants S2S app role assignments on the agent identity SP; requires Agent ID, Application, or Global Administrator.
+    /// Falls back to az rest, then PowerShell instructions, when the Graph path lacks permission (issue #460).
     /// </summary>
     internal static async Task GrantOrInstructAgentIdentityAppPermissionsAsync(
         SetupContext ctx,
@@ -712,6 +736,18 @@ internal static async Task GrantOrInstructAgentIdentityAppPermissionsAsync(
             return;
         }
 
+        // Issue #460: Graph token lacks AppRoleAssignment.ReadWrite.All; retry via az rest (a GA's az token carries it) before PowerShell.
+        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)