Handle PR comments

Harden consent URLs, fix resource leaks, improve tests

- Replace hardcoded OAuth2 `state` in admin consent URLs with random GUIDs for CSRF protection; centralize URL construction in `SetupHelpers.BuildAdminConsentUrl`
- Dispose overwritten `JsonDocument` in `FederatedCredentialService` to prevent resource leaks
- Improve retry logic to propagate cancellation immediately on user-initiated cancel (Ctrl+C)
- Remove unused CLI option variable (`verbose`) to avoid dead code
- Enhance tests: assert random state in consent URLs and add `because:` documentation to clarify test requirements

Author: sellakumaran

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

@@ -133,6 +133,7 @@ For each changed file, analyze:
 4. **Resource Management**
    - 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.
+   - **IMPORTANT**: Also scan for `var x = await A(...); if (...) { ... } else { x = await B(...); }` — the first `IDisposable` value is silently leaked when the else-branch overwrites `x`. See Anti-Pattern #13.
 
 5. **Null Safety**
    - Potential null reference exceptions?
@@ -515,6 +516,65 @@ A related dead-code smell: mocking `CommandExecutor.ExecuteAsync` to return `"fa
   ```
 - **Note**: `GraphApiServiceTokenCacheTests` is the intentional exception — it owns the cache and manages `AzCliTokenAcquirerOverride` explicitly via setUp/tearDown.
 
+### 12. Retry Loop Catches `TaskCanceledException` Without Early Exit
+A catch block that handles `TaskCanceledException` (or `OperationCanceledException`) alongside transient errors and retries all of them equally — so a user pressing Ctrl+C burns through all retry attempts before propagating.
+- **Pattern to catch**: `catch (Exception ex) when (ex is HttpRequestException or TaskCanceledException)` (or `OperationCanceledException`) inside a retry loop, with no check of `cancellationToken.IsCancellationRequested` before the retry delay
+- **Severity**: `high` — Ctrl+C appears to hang for the full retry window; partial state may continue to be applied
+- **Fix**:
+  ```csharp
+  catch (Exception ex) when (ex is HttpRequestException or TaskCanceledException)
+  {
+      if (ex is TaskCanceledException && cancellationToken.IsCancellationRequested)
+          throw; // propagate immediately — do not retry
+      // ... retry logic ...
+  }
+  ```
+
+### 13. `IDisposable` Variable Overwritten in Else-Branch Without Prior Disposal
+A variable holding an `IDisposable` is overwritten in an else/fallback branch without first disposing the value assigned in the if-branch.
+- **Pattern to catch**: `var doc = await Primary(...); if (doc != null && ...) { use doc } else { doc = await Fallback(...); }` where the first `doc` is not disposed before reassignment
+- **Severity**: `high` — the primary result leaks on every code path that falls into the else-branch; in high-frequency callers this accumulates
+- **Fix**: Dispose explicitly before overwriting, or restructure with separate `using` scopes:
+  ```csharp
+  var primaryDoc = await Primary(...);
+  JsonDocument? doc;
+  if (primaryDoc != null && ...)
+  {
+      doc = primaryDoc;
+  }
+  else
+  {
+      primaryDoc?.Dispose();
+      doc = await Fallback(...);
+  }
+  ```
+- **Check**: In the diff, for every pattern `var x = ...; if (...) { ... } else { x = ...; }` where the type is `IDisposable`, verify the original value is disposed in the else-branch.
+
+### 14. CLI Option Value Read from `ParseResult` But Never Used in Handler
+An option is wired up and parsed but the variable holding its value is never referenced in the handler body — the flag appears in `--help` output but silently has no effect.
+- **Pattern to catch**: `var verbose = context.ParseResult.GetValueForOption(verboseOption);` (or any option) with no subsequent reference to `verbose` in the handler lambda
+- **Severity**: `medium` — misleads users who pass `--verbose` expecting more output
+- **Fix**: Either wire the variable into logging configuration (e.g., adjust log level) or remove the `GetValueForOption` call. Keeping the option declaration is acceptable so it appears in help — just don't claim to read a value you discard.
+
+### 15. Hardcoded OAuth2 `state` Parameter
+A fixed string (e.g., `"xyz123"`, `"state"`, `"abc"`) used as the OAuth2 `state` parameter in a consent/authorization URL.
+- **Pattern to catch**: `$"&state=xyz123"` or any literal string in an OAuth2 URL `state=` segment
+- **Severity**: `medium` — the `state` parameter is designed to be a random nonce for CSRF protection; a hardcoded value eliminates that protection. Even when the URL is only displayed (not automatically followed), it sets a bad precedent and will fail audits.
+- **Fix**: Generate a random nonce per URL construction:
+  ```csharp
+  $"&state={Guid.NewGuid():N}"
+  ```
+
+### 16. Test Assertion Flipped Without `because:` Documenting the Requirement Change
+An assertion is changed from one expected value to another (e.g., `BeFalse()` → `BeTrue()`, `Be("old")` → `Be("new")`) without a `because:` string explaining what requirement changed.
+- **Pattern to catch**: `result.Should().BeTrue()` / `result.Should().BeFalse()` / `result.Should().Be(...)` in the diff (added lines) with no `because:` argument, especially when the surrounding context shows the original assertion had a different expected value
+- **Severity**: `medium` — a flipped assertion with no `because:` is indistinguishable from an implementation-tracking change (test updated to match code, not to match the requirement); the next reader cannot know if the behavior change was intentional
+- **Fix**: Add `because:` to document the invariant:
+  ```csharp
+  result.Should().BeTrue(
+      because: "McpServersMetadata.Read.All is always included even when the manifest is missing, so the method proceeds and returns true");
+  ```
+
 **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

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

@@ -101,7 +101,6 @@ public static Command CreateCommand(
         command.SetHandler(async (System.CommandLine.Invocation.InvocationContext context) =>
         {
             var config = context.ParseResult.GetValueForOption(configOption)!;
-            var verbose = context.ParseResult.GetValueForOption(verboseOption);
             var dryRun = context.ParseResult.GetValueForOption(dryRunOption);
             var skipInfrastructure = context.ParseResult.GetValueForOption(skipInfrastructureOption);
             var skipRequirements = context.ParseResult.GetValueForOption(skipRequirementsOption);

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

@@ -462,13 +462,7 @@ private static async Task ConfigureOauth2GrantsAsync(
             return (true, null);
         }
 
-        var allScopesEscaped = Uri.EscapeDataString(string.Join(' ', graphScopes));
-        var consentUrl =
-            $"https://login.microsoftonline.com/{tenantId}/v2.0/adminconsent" +
-            $"?client_id={blueprintAppId}" +
-            $"&scope={allScopesEscaped}" +
-            $"&redirect_uri=https://entra.microsoft.com/TokenAuthorize" +
-            $"&state=xyz123";
+        var consentUrl = SetupHelpers.BuildAdminConsentUrl(tenantId, blueprintAppId, graphScopes);
 
         // Check if consent already exists for ALL resolved resources (Phase 2 programmatic grants satisfy this check).
         // Only skip browser consent if every resource has its consent in place.

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

@@ -1459,8 +1459,9 @@ private static List<string> GetApplicationScopes(Models.Agent365Config setupConf
             }
         }
 
-        var applicationScopesJoined = string.Join(' ', applicationScopes);
-        var consentUrlGraph = $"https://login.microsoftonline.com/{tenantId}/v2.0/adminconsent?client_id={appId}&scope={Uri.EscapeDataString(applicationScopesJoined)}&redirect_uri=https://entra.microsoft.com/TokenAuthorize&state=xyz123";
+        var consentUrlGraph = SetupHelpers.BuildAdminConsentUrl(
+            tenantId, appId,
+            applicationScopes.Select(s => $"{AuthenticationConstants.MicrosoftGraphResourceUri}/{s}"));
 
         if (consentAlreadyExists)
         {

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

@@ -249,6 +249,19 @@ internal static List<string> PopulateAdminConsentUrls(
         return populated;
     }
 
+    /// <summary>
+    /// Builds a single /v2.0/adminconsent URL from fully-qualified scope URIs.
+    /// All callers must pass fully-qualified scopes (e.g. "https://graph.microsoft.com/User.Read").
+    /// Each scope is individually Uri.EscapeDataString-encoded and joined with %20.
+    /// A random GUID state parameter is generated for CSRF protection.
+    /// </summary>
+    internal static string BuildAdminConsentUrl(string tenantId, string clientId, IEnumerable<string> fullyQualifiedScopes)
+    {
+        var scopeParam = string.Join("%20", fullyQualifiedScopes.Select(Uri.EscapeDataString));
+        var redirectEncoded = Uri.EscapeDataString(AuthenticationConstants.BlueprintConsentRedirectUri);
+        return $"https://login.microsoftonline.com/{tenantId}/v2.0/adminconsent?client_id={clientId}&scope={scopeParam}&redirect_uri={redirectEncoded}&state={Guid.NewGuid():N}";
+    }
+
     /// <summary>
     /// Builds per-resource admin consent URLs for all five required resources.
     /// Graph and MCP scopes are taken from config; Bot API, Observability, and Power Platform
@@ -261,19 +274,9 @@ internal static List<string> PopulateAdminConsentUrls(
         IEnumerable<string> mcpScopes)
     {
         var urls = new List<(string, string)>();
-        const string loginBase = "https://login.microsoftonline.com";
 
         static string Build(string tenant, string client, string resourceUri, IEnumerable<string> scopes)
-        {
-            // /v2.0/adminconsent requires scope values in the form "<resourceUri>/<scopeName>".
-            // Each full scope token is Uri.EscapeDataString-encoded and joined with %20 (space).
-            // redirect_uri must be present and match a URI accepted by AAD for this endpoint.
-            // Omitting redirect_uri causes AADSTS500113. BlueprintConsentRedirectUri is the
-            // standard Entra Portal consent redirect URI accepted by AAD for admin consent flows.
-            var scopeParam = string.Join("%20", scopes.Select(s => Uri.EscapeDataString($"{resourceUri}/{s}")));
-            var redirectEncoded = Uri.EscapeDataString(AuthenticationConstants.BlueprintConsentRedirectUri);
-            return $"{loginBase}/{tenant}/v2.0/adminconsent?client_id={client}&scope={scopeParam}&redirect_uri={redirectEncoded}";
-        }
+            => BuildAdminConsentUrl(tenant, client, scopes.Select(s => $"{resourceUri}/{s}"));
 
         var graphScopeList = graphScopes.ToList();
         if (graphScopeList.Count > 0)
@@ -301,23 +304,15 @@ internal static string BuildCombinedConsentUrl(
         IEnumerable<string> graphScopes,
         IEnumerable<string> mcpScopes)
     {
-        const string loginBase = "https://login.microsoftonline.com";
-
         var allScopes = new List<string>();
-
         foreach (var s in graphScopes)
-            allScopes.Add(Uri.EscapeDataString($"{AuthenticationConstants.MicrosoftGraphResourceUri}/{s}"));
+            allScopes.Add($"{AuthenticationConstants.MicrosoftGraphResourceUri}/{s}");
         foreach (var s in mcpScopes)
-            allScopes.Add(Uri.EscapeDataString($"{McpConstants.Agent365ToolsIdentifierUri}/{s}"));
-        allScopes.Add(Uri.EscapeDataString($"{ConfigConstants.MessagingBotApiIdentifierUri}/{ConfigConstants.MessagingBotApiAdminConsentScope}"));
-        allScopes.Add(Uri.EscapeDataString($"{ConfigConstants.ObservabilityApiIdentifierUri}/{ConfigConstants.ObservabilityApiAdminConsentScope}"));
-        allScopes.Add(Uri.EscapeDataString($"{PowerPlatformConstants.PowerPlatformApiIdentifierUri}/{PowerPlatformConstants.PermissionNames.ConnectivityConnectionsRead}"));
-
-        // Each scope token is Uri.EscapeDataString-encoded and joined with %20 (space).
-        // redirect_uri must be present — omitting it causes AADSTS500113.
-        var scopeParam = string.Join("%20", allScopes);
-        var redirectEncoded = Uri.EscapeDataString(AuthenticationConstants.BlueprintConsentRedirectUri);
-        return $"{loginBase}/{tenantId}/v2.0/adminconsent?client_id={blueprintClientId}&scope={scopeParam}&redirect_uri={redirectEncoded}";
+            allScopes.Add($"{McpConstants.Agent365ToolsIdentifierUri}/{s}");
+        allScopes.Add($"{ConfigConstants.MessagingBotApiIdentifierUri}/{ConfigConstants.MessagingBotApiAdminConsentScope}");
+        allScopes.Add($"{ConfigConstants.ObservabilityApiIdentifierUri}/{ConfigConstants.ObservabilityApiAdminConsentScope}");
+        allScopes.Add($"{PowerPlatformConstants.PowerPlatformApiIdentifierUri}/{PowerPlatformConstants.PermissionNames.ConnectivityConnectionsRead}");
+        return BuildAdminConsentUrl(tenantId, blueprintClientId, allScopes);
     }
 
     /// <summary>

src/Microsoft.Agents.A365.DevTools.Cli/Services/FederatedCredentialService.cs

@@ -49,20 +49,21 @@ public async Task<List<FederatedCredentialInfo>> GetFederatedCredentialsAsync(
             _logger.LogDebug("Retrieving federated credentials for blueprint: {ObjectId}", blueprintObjectId);
 
             // Try standard endpoint first
-            var doc = await _graphApiService.GraphGetAsync(
+            var primaryDoc = await _graphApiService.GraphGetAsync(
                 tenantId,
                 $"/beta/applications/{blueprintObjectId}/federatedIdentityCredentials",
                 cancellationToken,
                 scopes: [AuthenticationConstants.ApplicationReadWriteAllScope]);
 
-            // If standard endpoint returns data with credentials, use it
-            if (doc != null && doc.RootElement.TryGetProperty("value", out var valueCheck) && valueCheck.GetArrayLength() > 0)
+            JsonDocument? doc;
+            if (primaryDoc != null && primaryDoc.RootElement.TryGetProperty("value", out var valueCheck) && valueCheck.GetArrayLength() > 0)
             {
                 _logger.LogDebug("Standard endpoint returned {Count} credential(s)", valueCheck.GetArrayLength());
+                doc = primaryDoc;
             }
-            // If standard endpoint returns empty or null, try Agent Blueprint-specific endpoint
             else
             {
+                primaryDoc?.Dispose();
                 _logger.LogDebug("Standard endpoint returned no credentials or failed, trying Agent Blueprint fallback endpoint");
                 doc = await _graphApiService.GraphGetAsync(
                     tenantId,

src/Microsoft.Agents.A365.DevTools.Cli/Services/Helpers/RetryHelper.cs

@@ -66,6 +66,9 @@ public async Task<T> ExecuteWithRetryAsync<T>(
             }
             catch (Exception ex) when (ex is HttpRequestException or TaskCanceledException)
             {
+                if (ex is TaskCanceledException && cancellationToken.IsCancellationRequested)
+                    throw;
+
                 lastException = ex;
                 _logger.LogWarning("Exception: {Message}", ex.Message);
 
@@ -172,6 +175,9 @@ public async Task<T> ExecuteWithRetryAsync<T>(
             }
             catch (Exception ex) when (ex is HttpRequestException or TaskCanceledException)
             {
+                if (ex is TaskCanceledException && cancellationToken.IsCancellationRequested)
+                    throw;
+
                 lastException = ex;
                 _logger.LogWarning("Exception: {Message}", ex.Message);
 

src/Tests/Microsoft.Agents.A365.DevTools.Cli.Tests/Commands/BatchPermissionsOrchestratorTests.cs

@@ -132,5 +132,11 @@ await BatchPermissionsOrchestrator.ConfigureAllPermissionsAsync(
         consentUrl.Should().NotBeNullOrWhiteSpace("non-admin must always receive a consent URL for the tenant admin");
         consentUrl.Should().Contain("tenant-123", "consent URL must be scoped to the correct tenant");
         consentUrl.Should().Contain("blueprint-app-id", "consent URL must reference the blueprint application");
+
+        // state parameter must be a random GUID (not the old hardcoded "xyz123")
+        var stateMatch = System.Text.RegularExpressions.Regex.Match(consentUrl!, @"[?&]state=([^&]+)");
+        stateMatch.Success.Should().BeTrue(because: "consent URL must include a state parameter for CSRF protection");
+        Guid.TryParse(stateMatch.Groups[1].Value, out _).Should().BeTrue(
+            because: "state parameter must be a random GUID, not a hardcoded value like 'xyz123'");
     }
 }

src/Tests/Microsoft.Agents.A365.DevTools.Cli.Tests/Commands/PermissionsSubcommandTests.cs

@@ -460,9 +460,8 @@ public async Task ConfigureMcpPermissionsAsync_WithMissingManifest_ShouldHandleG
             config,
             false);
 
-        // Assert - McpServersMetadata.Read.All is always included even when the manifest is missing,
-        // so the method proceeds and returns true (pending admin consent) rather than false.
-        result.Should().BeTrue();
+        result.Should().BeTrue(
+            because: "McpServersMetadata.Read.All is always included even when the ToolingManifest is missing, so the method proceeds to configure permissions and returns true (pending admin consent)");
     }
 
     #endregion