Improve blueprint app creation, secret flow#288
Open
sellakumaran wants to merge 1 commit intomainfrom
Open
Conversation
- Add mandatory "Verification Rules" to pr-code-reviewer.md to prevent false positives and require concrete, actionable, and idiomatic review feedback. - Refactor Graph token acquisition to always use MSAL interactive auth (never Azure CLI), ensuring client secret creation works for any user with a properly configured custom client app. - Enhance fallback logic for app registration: retry without sponsors first, then without owners only as a last resort, preserving ownership when possible and logging clear warnings. - Improve error handling and user guidance for manual client secret creation, including explicit permission requirements, documentation links, and step-by-step instructions. - Ensure HTTP responses are disposed after use. - Add regression tests for Issue #279 to verify correct logging, guidance, and that Azure CLI tokens are never used for secret creation. - Overall, increases reliability, user support, and test coverage for blueprint registration and secret management. Fix MSAL token usage, error handling, and test reliability - Switch BlueprintSubcommand to MSAL token for Graph API (fixes #279) - Add robust error handling for app registration creation (retry sponsors/owners removal) - Improve client secret creation logging and manual guidance - Refactor tests for thread safety and sequential execution - Prevent blocking prompts in PublishCommand for non-interactive environments - Add mandatory verification rules to pr-code-reviewer.md
Dependency Review✅ No vulnerabilities or license issues or OpenSSF Scorecard issues found.Scanned FilesNone |
Contributor
There was a problem hiding this comment.
Pull request overview
This pull request fixes Issue #279 by refactoring Graph token acquisition to always use MSAL interactive authentication instead of Azure CLI tokens, ensuring client secret creation works for any user with a properly configured custom client app. The PR also enhances app registration fallback logic to preserve ownership when possible, improves error handling with clear user guidance for manual secret creation, ensures proper disposal of HTTP responses, adds comprehensive regression tests, and introduces verification rules for the PR review process.
Changes:
- Refactored
CreateBlueprintClientSecretAsyncto use MSAL tokens (AcquireMsalGraphTokenAsync) instead of Azure CLI tokens, fixing the core Issue #279 bug where non-admin users couldn't create client secrets - Implemented cascading fallback strategy for app registration creation: retry without sponsors first, then without owners as a last resort, to preserve ownership whenever possible
- Enhanced error handling with detailed permission requirements, step-by-step manual secret creation instructions, and documentation links for users who need to create secrets manually
- Improved test reliability by adding parallelization controls for tests that modify global state (Console.In) and thread-safe logging with ConcurrentBag
- Added Console.IsInputRedirected check to prevent PublishCommand from blocking in non-interactive environments
- Added four regression tests to verify correct logging, error guidance, and that Azure CLI tokens are never used for secret creation
Reviewed changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| src/Tests/Microsoft.Agents.A365.DevTools.Cli.Tests/Services/Agent365ConfigServiceTests.cs | Added Collection attribute to join existing ConfigTests collection for proper test isolation |
| src/Tests/Microsoft.Agents.A365.DevTools.Cli.Tests/Commands/PublishCommandTests.cs | Added collection definition with parallelization disabled, StringReader pre-answer for Console prompts, and Dispose pattern to restore Console.In |
| src/Tests/Microsoft.Agents.A365.DevTools.Cli.Tests/Commands/MockToolingServerSubcommandTests.cs | Refactored TestLogger to use ConcurrentBag for thread-safe logging, updated tests to use Task.Factory.StartNew with LongRunning option and polling pattern |
| src/Tests/Microsoft.Agents.A365.DevTools.Cli.Tests/Commands/BlueprintSubcommandTests.cs | Added four regression tests for Issue #279 to verify permission guidance logging, config field guidance, re-run instructions, and that Azure CLI tokens are not used |
| src/Microsoft.Agents.A365.DevTools.Cli/Commands/SetupSubcommands/BlueprintSubcommand.cs | Replaced GetTokenFromGraphClient with AcquireMsalGraphTokenAsync, implemented cascading fallback for app registration (sponsors → owners), added HTTP response disposal, enhanced error messages with detailed manual secret creation steps |
| src/Microsoft.Agents.A365.DevTools.Cli/Commands/PublishCommand.cs | Added Console.IsInputRedirected check to skip interactive prompts in non-interactive environments (CI/CD) |
| .claude/agents/pr-code-reviewer.md | Added mandatory verification rules to prevent false positives: require quoted evidence for mismatch claims, acknowledge behavioral differences in replacement suggestions, use idiomatic .NET patterns, and require verifiable evidence for blocking/high severity issues |
src/Tests/Microsoft.Agents.A365.DevTools.Cli.Tests/Commands/PublishCommandTests.cs
Show resolved
Hide resolved
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fix MSAL token usage, error handling, and test reliability
Closes #279