Skip to content

Improve agent setup: clarify S2S and consent messaging#404

Merged
sellakumaran merged 5 commits intomainfrom
users/sellak/issue-402-fixes
May 5, 2026
Merged

Improve agent setup: clarify S2S and consent messaging#404
sellakumaran merged 5 commits intomainfrom
users/sellak/issue-402-fixes

Conversation

@sellakumaran
Copy link
Copy Markdown
Contributor

  • Only log "Bot API permissions configured successfully" if S2S app role assignment succeeds; otherwise, show a warning with retry instructions.
  • Return value now reflects both consent and S2S assignment success to avoid false positives.
  • Update consent-required message to clarify that a Global Administrator must grant tenant-wide consent, improving accuracy and user guidance.

Closes #402

Copilot AI review requested due to automatic review settings May 5, 2026 17:07
@sellakumaran sellakumaran requested review from a team as code owners May 5, 2026 17:07
@github-actions
Copy link
Copy Markdown

github-actions Bot commented May 5, 2026

⚠️ Deprecation Warning: The deny-licenses option is deprecated for possible removal in the next major release. For more information, see issue 997.

Dependency Review

✅ No vulnerabilities or license issues or OpenSSF Scorecard issues found.

Scanned Files

None

@github-actions github-actions Bot added the documentation Improvements or additions to documentation label May 5, 2026
Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Improves a365 setup permissions bot operator guidance by tightening success/warning messaging around S2S app-role assignment outcomes and clarifying the “admin consent required” wording, with corresponding release notes in CHANGELOG.md.

Changes:

  • Update MSAL consent-required warning to state that a Global Administrator must grant tenant-wide consent.
  • Adjust setup permissions bot completion messaging and return value to account for S2S app-role assignment failure.
  • Document the behavior change in CHANGELOG.md.

Reviewed changes

Copilot reviewed 3 out of 3 changed files in this pull request and generated 4 comments.

File Description
src/Microsoft.Agents.A365.DevTools.Cli/Services/MsalBrowserCredential.cs Refines consent-required warning text to better match the actual failure condition.
src/Microsoft.Agents.A365.DevTools.Cli/Commands/SetupSubcommands/PermissionsSubcommand.cs Attempts to gate the “configured successfully” message and boolean return on S2S assignment success.
CHANGELOG.md Adds release notes describing the updated messaging behavior.

Comment thread CHANGELOG.md Outdated
sellakumaran added a commit that referenced this pull request May 5, 2026
- Only log "Bot API permissions configured successfully" if S2S app role assignment succeeds; otherwise, show a warning with retry instructions.
- Return value now reflects both consent and S2S assignment success to avoid false positives.
- Update consent-required message to clarify that a Global Administrator must grant tenant-wide consent, improving accuracy and user guidance.
@sellakumaran sellakumaran force-pushed the users/sellak/issue-402-fixes branch from caf441c to b7460a4 Compare May 5, 2026 19:53
Copilot AI review requested due to automatic review settings May 5, 2026 19:53
@sellakumaran sellakumaran enabled auto-merge (squash) May 5, 2026 19:53
Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 4 out of 4 changed files in this pull request and generated 2 comments.

Comment thread src/Microsoft.Agents.A365.DevTools.Cli/Services/MsalBrowserCredential.cs Outdated
sellakumaran and others added 2 commits May 5, 2026 13:10
…dministrator

Application Administrator and Agent ID Administrator can also grant
tenant-wide admin consent — the previous wording incorrectly implied
only Global Administrators could do so.

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…l safety

Reorders the logging conditional so the non-admin path (consentGranted=false)
is checked before the S2S state, preventing a null S2SAppRoleGranted from
routing to the S2S-failure warning instead of the consent-required message.

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Copilot AI review requested due to automatic review settings May 5, 2026 20:55
Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 4 out of 4 changed files in this pull request and generated 3 comments.

Comment thread CHANGELOG.md
- CHANGELOG: broaden S2S entry from Observability-specific to any S2S failure
- CHANGELOG: correct consent message to match actual code ("An administrator" not "A Global Administrator")
- Rename WhenConsentPending tests to WhenGraphAuthFails — null GraphGetAsync drives the phase1Result=null (auth-failed) path, not the non-admin consent-pending path

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
@sellakumaran sellakumaran merged commit 25970fb into main May 5, 2026
9 checks passed
@sellakumaran sellakumaran deleted the users/sellak/issue-402-fixes branch May 5, 2026 21:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

documentation Improvements or additions to documentation

Projects

None yet

Development

Successfully merging this pull request may close these issues.

setup permissions bot silently drops two of three S2S app-role assignments

4 participants