feat: non-admin setup flow, a365 setup admin command, and cleanup 403 fixes

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

@@ -131,9 +131,8 @@ For each changed file, analyze:
    - Are error messages user-friendly?
 
 4. **Resource Management**
-   - Are IDisposable objects disposed?
-   - Are connections/streams closed?
-   - Any potential memory leaks?
+   - Are IDisposable objects disposed? Are connections/streams closed? Any potential memory leaks?
+   - **IMPORTANT**: For every `var x = await SomeMethod(...)` in the diff, use `Read` to look up the method's return type in the source file. If the return type implements `IDisposable`, flag missing `using` as a `high` severity `resource_leak`. Do NOT rely on the diff alone — the return type is almost never in the diff.
 
 5. **Null Safety**
    - Potential null reference exceptions?
@@ -410,6 +409,119 @@ Differentiate between:
   - Runs on Linux runners (cross-platform not required)
   - Tests strongly recommended but not blocking
 
+## C#-Specific Anti-Patterns (Check These in Every Review)
+
+These patterns have caused real bugs and Copilot review comments in this repo. Always scan new/changed code for them.
+
+### 1. Wrong Scope Constant for Operation
+When a method acquires a token with a specific scope, verify the scope constant matches the operation.
+- **Pattern to catch**: `DeleteXxx` method using `ReadWriteAllScope` instead of `DeleteRestoreAllScope`
+- **Severity**: `high` — causes deterministic 403s for the operation
+- **Check**: Read the constant used and compare to the method name + docs describing what permission is needed
+
+### 2. Null-Only Guard on Nullable String Variables
+`== null` is insufficient for string values returned from JSON/APIs — empty string is also invalid.
+- **Pattern to catch**: `if (existingId == null)` where `existingId` came from a JSON parse or API response
+- **Severity**: `high` — empty string generates malformed URLs (e.g., `.../oauth2PermissionGrants/`)
+- **Fix**: Always use `string.IsNullOrWhiteSpace(existingId)` for Guard checks on strings used in URLs
+
+### 3. Unused Tuple Return Elements
+Multi-element tuples where one element is always `null` at all return sites.
+- **Pattern to catch**: `Task<(bool x, string? y, string? z)>` where every `return` statement ends with `, null)`
+- **Severity**: `medium` — API noise, confusing callers, harder to understand contract
+- **Fix**: Remove the unused element from the return type and all callers
+
+### 4. Misleading Log Message Scope
+Log messages that claim to cover "all configured resources" when only a subset is handled.
+- **Pattern to catch**: `"covers all configured resources"` in a consent/grant flow that only builds URLs for one resource type (e.g., Microsoft Graph only)
+- **Severity**: `medium` — misleads operators troubleshooting why non-Graph resources aren't consented
+- **Fix**: Qualify the message: `"covers Microsoft Graph delegated scopes only"`
+
+### 5. CancellationToken.None in Long-Running Operations
+Hardcoded `CancellationToken.None` in handler body for long-running async calls (infrastructure provisioning, permission grants, etc.).
+- **Pattern to catch**: `SetHandler(async (opt1, opt2, ...) => { ... SomethingAsync(..., CancellationToken.None) ... }, opt1, opt2, ...)`
+- **Severity**: `medium` — Ctrl+C cannot cancel long-running operations; partial state may be applied
+- **Fix**: Use `InvocationContext`:
+  ```csharp
+  command.SetHandler(async (InvocationContext context) =>
+  {
+      var opt1 = context.ParseResult.GetValueForOption(opt1Option);
+      var ct = context.GetCancellationToken();
+      await SomethingAsync(..., ct);
+  });
+  ```
+
+### 6. Duplicate Logic Using Different Execution Mechanisms
+Two separate implementations of the same operation using different execution paths (e.g., `ProcessStartInfo` vs. `CommandExecutor`).
+- **Pattern to catch**: Static helper method running `az account show` via `Process.Start` when an instance method in a sibling service does the same via `CommandExecutor`
+- **Severity**: `medium` — divergence risk; one gets fixes/improvements the other doesn't; different testability
+- **Fix**: Extract to a shared static helper in `Services/Helpers/` and delegate from both callers
+
+### 7. `Task.Delay` Without CancellationToken
+`Task.Delay` called without a CancellationToken inside a handler that receives one — makes the wait non-cancellable, blocking Ctrl+C and accumulating if the step is retried.
+- **Pattern to catch**: `await Task.Delay(N)` inside a method/handler that has a `ct` or `cancellationToken` parameter in scope
+- **Severity**: `medium` — Ctrl+C stalls during the delay; can compound if the delay is in a loop
+- **Fix**: `await Task.Delay(N, ct);`
+
+### 8. `Process.WaitForExitAsync` With Unread Redirected Stderr
+`RedirectStandardError = true` combined with reading only stdout — if the process writes enough to stderr the pipe buffer fills and it deadlocks waiting for the reader.
+- **Pattern to catch**: `ProcessStartInfo` with `RedirectStandardError = true` where only `StandardOutput.ReadToEndAsync()` is awaited before `WaitForExitAsync()`
+- **Severity**: `high` — deterministic deadlock when the subprocess writes >4 KB to stderr
+- **Fix**: Read both streams concurrently before waiting:
+  ```csharp
+  var outputTask = process.StandardOutput.ReadToEndAsync();
+  var errorTask  = process.StandardError.ReadToEndAsync();
+  await Task.WhenAll(outputTask, errorTask);
+  await process.WaitForExitAsync();
+  var output = outputTask.Result;
+  ```
+
+### 9. `Environment.Exit` Instead of `ExceptionHandler.ExitWithCleanup`
+Direct `Environment.Exit(N)` calls skip the repo's output-flush / console-state-reset logic in `ExceptionHandler.ExitWithCleanup`.
+- **Pattern to catch**: `Environment.Exit(1)` (or any exit code) in CLI command handlers or exception catch blocks
+- **Severity**: `medium` — console may be left in a dirty state (partial progress output not flushed, ANSI reset not sent)
+- **Fix**: Replace with `ExceptionHandler.ExitWithCleanup(1);`
+
+### 10. Bearer Token Embedded in Process Command-Line Arguments
+Injecting a raw Bearer token as a CLI argument (e.g., `az rest --headers "Authorization=Bearer {token}"`).
+- **Pattern to catch**: String interpolation of a token into `az rest --headers` argument passed to `ExecuteAsync`
+- **Severity**: `high` (security) — process command-line arguments are visible to all local users via OS process listing, crash dumps, and audit logs
+- **Fix**: Use in-process HTTP (`GraphApiService` / `HttpClient`) or pass token via stdin/temp file with restricted permissions
+
+### 11. Test Classes Creating Real `GraphApiService` Without Cache Warmup
+Test classes that construct real (non-substitute) `GraphApiService` or `AgentBlueprintService` instances without pre-warming the `AzCliHelper` process-level token cache. `EnsureGraphHeadersAsync` calls `AzCliHelper.AcquireAzCliTokenAsync` as its FIRST step — if the cache is cold, it spawns a real `az account get-access-token` subprocess (~20s per test class instance). This makes the test suite take minutes instead of seconds.
+
+A related dead-code smell: mocking `CommandExecutor.ExecuteAsync` to return `"fake-token"` for `get-access-token` calls looks correct but is never reached — the subprocess fires before the executor fallback is attempted.
+
+- **Pattern to catch** (any of the following in test code):
+  1. `new GraphApiService(logger, executor, handler)` or `new AgentBlueprintService(...)` without `AzCliHelper.WarmAzCliTokenCache(...)` in the test class constructor
+  2. `executor.ExecuteAsync(...).Returns(...)` matching `"get-access-token"` in a class that also constructs real `GraphApiService` instances — confirms the executor mock is dead code
+  3. Missing `loginHintResolver: () => Task.FromResult<string?>(null)` parameter when constructing `GraphApiService` in tests (bypasses the `az account show` subprocess)
+- **Severity**: `high` — causes ~20s per test *instance* (xUnit creates one instance per test method); a 10-test class goes from <1s to 200s
+- **Check**: For every `new GraphApiService(` or `new AgentBlueprintService(` in a test file, verify the test class constructor contains:
+  ```csharp
+  AzCliHelper.WarmAzCliTokenCache("https://graph.microsoft.com/", "<tenantId>", "fake-graph-token");
+  ```
+  where `<tenantId>` matches all tenant ID strings used in that class's test methods.
+- **Fix**:
+  ```csharp
+  // In test class constructor — warm for every tenantId string used in this class:
+  public MyServiceTests()
+  {
+      AzCliHelper.WarmAzCliTokenCache("https://graph.microsoft.com/", "tenant-123", "fake-graph-token");
+      // Also pass loginHintResolver to bypass az account show:
+      // new GraphApiService(logger, executor, handler, loginHintResolver: () => Task.FromResult<string?>(null))
+  }
+  ```
+- **Note**: `GraphApiServiceTokenCacheTests` is the intentional exception — it owns the cache and manages `AzCliTokenAcquirerOverride` explicitly via setUp/tearDown.
+
+**MANDATORY REPORTING RULE**: Whenever the diff contains any test file (`.Tests.cs`), you MUST emit a named finding for this check — even if no violation is found. The finding must appear in the review output with one of three statuses:
+  - **`high` severity** if a violation is found (missing warmup, dead executor mock, etc.)
+  - **`info` — FIXED** if the PR is fixing a prior violation (warmup added to previously-cold classes) — list each class fixed and its measured or estimated speedup
+  - **`info` — PASS** if all test classes with real service instances already have warmup in their constructors
+
+Do NOT silently omit this check. The rule exists because silent omission is how the regression in `da6f750` went undetected.
+
 ## Example Invocation
 
 When you receive a request like "Review PR #253", you should:

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

@@ -1,7 +1,7 @@
 ---
 name: review-staged
 description: Generate structured code review for staged files (git staged changes) using Claude Code agents. Provides feedback before committing to catch issues early.
-allowed-tools: Bash(git:*), Read, Write
+allowed-tools: Bash(git:*), Bash(dotnet:*), Bash(cd:*), Read, Write
 ---
 
 # Review Staged Files Skill
@@ -27,8 +27,9 @@ Examples:
 4. **Analyzes changes** for security, testing, design patterns, and code quality issues
 5. **Differentiates contexts**: CLI code vs GitHub Actions code (different standards)
 6. **Creates actionable feedback**: Specific refactoring suggestions based on file names and patterns
-7. **Generates structured review document** saved to a markdown file
-8. **Shows summary** of all issues found organized by severity
+7. **Runs the test suite and measures per-test timing** — flags any test taking > 1 second as a performance regression
+8. **Generates structured review document** saved to a markdown file
+9. **Shows summary** of all issues found organized by severity
 
 ## Engineering Review Principles
 
@@ -66,6 +67,15 @@ This skill enforces the same principles as the PR review skill:
 - **CLI reliability**: CLI code without tests is BLOCKING
 - **GitHub Actions tests**: Strongly recommended (HIGH severity) but not blocking
 - **Mock external dependencies**: Proper mocking patterns
+- **Test performance — measured by running, not just static analysis**: The review ALWAYS runs the full test suite and reports per-test timing. Any test method taking **> 1 second** is flagged as a performance regression (HIGH severity). The finding must include:
+  - The slow test class and method name(s) with their measured time
+  - The root cause (cold `AzCliHelper` token cache, missing `WarmAzCliTokenCache` call, real subprocess not mocked, etc.)
+  - The fix (warmup call pattern, `loginHintResolver` injection, etc.)
+  - Expected time after fix
+
+  If all tests complete in < 1 second each: emit an **INFO — PASS** finding with the total suite time.
+
+  **Do not skip the test run.** Static code analysis alone missed the regression in `da6f750`; only measurement catches it reliably.
 
 ### Security
 - **No hardcoded secrets**: Use environment variables or Azure Key Vault
@@ -101,7 +111,19 @@ The skill uses **Claude Code directly** for semantic code analysis (same as revi
 4. Claude Code gets staged changes: `git diff --staged`
 5. Claude Code performs semantic analysis using its own capabilities
 6. Claude Code identifies specific issues with line numbers and code references
-7. Claude Code writes markdown file to `.codereviews/claude-staged-<timestamp>.md`
+7. **Claude Code runs the full test suite with per-test timing:**
+   ```bash
+   cd src && dotnet test tests.proj --configuration Release --logger "console;verbosity=normal" 2>&1
+   ```
+   Parse the output for lines matching `[X s]` or `[X,XXX ms]` patterns. Extract test class name, method name, and duration. Flag any test method taking **> 1 second**. Group findings by test class and include the measured times in the review.
+8. Claude Code writes markdown file to `.codereviews/claude-staged-<timestamp>.md`
+
+**Test timing output format** (from `dotnet test --logger "console;verbosity=normal"`):
+```
+  Passed SomeTests.Method_Scenario_ExpectedResult [< 1 ms]
+  Passed OtherTests.Method_Slow [22 s]
+```
+Any line showing `[X s]` where X ≥ 1 is a slow test. Report all such tests in a dedicated finding.
 
 **Key Advantages**:
 - ✅ No API key required - uses Claude Code's existing authentication

.github/copilot-instructions.md

@@ -36,6 +36,8 @@
 - Focus on quality over quantity of tests
 - Add regression tests for bug fixes
 - Tests should verify CLI reliability
+- **Tests must assert requirements, not implementation** — when a test is changed to match new code behavior (rather than to reflect a changed requirement), that is a red flag. A test that silently tracks whatever the code does provides no regression protection. If a test needs to be updated, explicitly document the requirement the new assertion encodes (use `because:` in FluentAssertions). If you cannot articulate a requirement reason, the test change should be questioned.
+- **FluentAssertions `because:` is mandatory for non-obvious assertions** — any assertion on a URL structure, encoding format, security-sensitive behavior, or protocol requirement must include a `because:` clause explaining the invariant being enforced.
 - **Dispose IDisposable objects properly**:
   - `HttpResponseMessage` objects created in tests must be disposed
   - Even in mock/test handlers, follow proper disposal patterns
@@ -114,7 +116,18 @@
   - Check if it's a legacy reference that needs to be updated
 - **Files to check**: All `.cs`, `.csx` files in the repository
 
-### Rule 2: Verify Copyright Headers
+### Rule 2: Flag Tests Changed to Match Implementation
+- **Description**: When a PR or staged change modifies a test assertion to match new code behavior, treat it as a high-priority review flag — not a routine update.
+- **The anti-pattern**: A test previously asserted `X`. Code changed, so the test was updated to assert `not X` (or a different value of `X`) without documenting *why the requirement changed*.
+- **Why it matters**: Tests that chase implementation provide zero regression protection. They give false confidence — all tests green, but the regression was in the test suite, not just the code. This is how silent regressions reach production.
+- **Action**: For every test assertion change in the diff:
+  1. Ask: "Did the *requirement* change, or just the implementation?"
+  2. If the requirement changed: the PR must include a comment or `because:` clause stating the new requirement.
+  3. If only the implementation changed: the test assertion should not need to change. Flag as **HIGH** if a test is weakened (e.g., `Contain` → `NotContain`, `Equal("x")` → `NotBeNull()`).
+  4. If the assertion is on a security-sensitive, protocol-level, or external-API contract (OAuth URLs, HTTP headers, encoding format): flag as **CRITICAL** — require explicit documented justification.
+- **Example of the failure mode** (from project history): Consent URL tests asserted `redirect_uri=` was present. When URL encoding was changed, tests were updated to match. No one asked whether `redirect_uri` was still required by the AAD protocol. The regression (`AADSTS500113`) reached the user before any test caught it.
+
+### Rule 3: Verify Copyright Headers
 - **Description**: Ensure all C# files have proper Microsoft copyright headers
 - **Action**: If a `.cs` file is missing a copyright header:
   - Add the Microsoft copyright header at the top of the file

.gitignore

@@ -1,3 +1,6 @@
+# Internal working documents
+docs/plans/
+
 ## A streamlined .gitignore for modern .NET projects
 ## including temporary files, build results, and
 ## files generated by popular .NET tools. If you are

CHANGELOG.md

@@ -10,10 +10,14 @@ The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.1.0/).
 - 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
+- `a365 setup admin` — new command for Global Administrators to complete tenant-wide AllPrincipals OAuth2 permission grants after `a365 setup all` has been run by an Agent ID Admin
 ### Changed
 - `a365 publish` updates manifest IDs, creates `manifest.zip`, and prints concise upload instructions for Microsoft 365 Admin Center (Agents > All agents > Upload custom agent). Interactive prompts only occur in interactive terminals; redirect stdin to suppress them in scripts.
 
 ### Fixed
+- `a365 cleanup` blueprint deletion now succeeds for Global Administrators even when the blueprint was created by a different user
+- `a365 setup all` no longer times out for non-admin users — the CLI immediately surfaces a consent URL to share with an administrator instead of waiting for a browser prompt
+- `a365 setup all` requests admin consent once for all resources instead of prompting once per resource
 - macOS/Linux: device code fallback when browser authentication is unavailable (#309)
 - Linux: MSAL fallback when PowerShell `Connect-MgGraph` fails in non-TTY environments (#309)
 - Admin consent polling no longer times out after 180s — blueprint service principal now resolved with correct MSAL token (#309)

scripts/cli/install-cli.sh

@@ -84,13 +84,18 @@ if dotnet tool uninstall -g Microsoft.Agents.A365.DevTools.Cli 2>/dev/null; then
     sleep 1
 else
     echo "Could not uninstall existing CLI (may not be installed or locked)."
-    # Try to clear the tool directory manually if locked
+    # Try to clear the tool directory and shim manually (handles ghost/orphaned installs)
     TOOL_PATH="$HOME/.dotnet/tools/.store/microsoft.agents.a365.devtools.cli"
     if [ -d "$TOOL_PATH" ]; then
         echo "Attempting to clear locked tool directory..."
         rm -rf "$TOOL_PATH" 2>/dev/null || true
         sleep 1
     fi
+    # Remove orphaned shim that blocks reinstall even when tool is not registered
+    SHIM="$HOME/.dotnet/tools/a365"
+    for ext in "" ".exe"; do
+        [ -f "${SHIM}${ext}" ] && rm -f "${SHIM}${ext}" 2>/dev/null && echo "Removed orphaned shim: ${SHIM}${ext}" || true
+    done
 fi
 
 # Install with specific version from local source

src/Microsoft.Agents.A365.DevTools.Cli/Commands/DevelopCommand.cs

@@ -145,7 +145,10 @@ private static async Task<bool> CallDiscoverToolServersAsync(IConfigService conf
 
                 logger.LogInformation("Environment: {Environment}, Audience: {Audience}", config.Environment, audience);
 
-                authToken = await authService.GetAccessTokenAsync(audience);
+                // Resolve az CLI login hint so WAM targets the correct account instead of
+                // defaulting to the first cached MSAL account (which may be stale).
+                var loginHint = await Services.Helpers.AzCliHelper.ResolveLoginHintAsync();
+                authToken = await authService.GetAccessTokenAsync(audience, userId: loginHint);
 
                 if (string.IsNullOrWhiteSpace(authToken))
                 {