Improve error handling, logging, and Python validation#21
Merged
Conversation
Refined exception handling across the application: - Replaced generic `catch` blocks with specific exceptions. - Introduced `DeployAppPythonCompileException` for Python syntax errors. - Added a new error code `DeploymentAppCompileFailed`. Enhanced user feedback and logging: - Updated error message formatting to align with Azure CLI style. - Improved deployment error handling with detailed troubleshooting steps. - Added deployment summaries for both success and failure cases. Integrated Python syntax validation: - Added `python -m py_compile` checks in `PythonBuilder` to catch syntax errors early. Performed code cleanup and refactoring: - Removed redundant code and comments. - Reorganized `using` directives for better structure.
tmlsousa
approved these changes
Nov 16, 2025
sellakumaran
added a commit
that referenced
this pull request
Feb 27, 2026
Refined exception handling across the application: - Replaced generic `catch` blocks with specific exceptions. - Introduced `DeployAppPythonCompileException` for Python syntax errors. - Added a new error code `DeploymentAppCompileFailed`. Enhanced user feedback and logging: - Updated error message formatting to align with Azure CLI style. - Improved deployment error handling with detailed troubleshooting steps. - Added deployment summaries for both success and failure cases. Integrated Python syntax validation: - Added `python -m py_compile` checks in `PythonBuilder` to catch syntax errors early. Performed code cleanup and refactoring: - Removed redundant code and comments. - Reorganized `using` directives for better structure.
sellakumaran
added a commit
that referenced
this pull request
Mar 8, 2026
…aph, tests) Exit codes (#7, #8/#9): - Set Environment.ExitCode = 1 in ValidateDeploymentPrerequisitesAsync before each null return so callers exit non-zero on config/Web App validation failure - Replace deploy-mcp guard `return` with ExceptionHandler.ExitWithCleanup(1) for AgentBlueprintId, AgenticAppId, and TenantId missing-config cases Log severity (#15, #16, #17): - LogCheckWarning: LogInformation -> LogWarning - LogCheckFailure: all three LogInformation -> LogError - ExecuteCheckWithLoggingAsync warning path: log ErrorMessage ?? Details so the primary warning message is no longer silently dropped skip-graph regressions (#21, #22): - Guard RunChecksOrExitAsync(MOS checks) behind if (!skipGraph) - Guard clientAppId null check behind !skipGraph in PublishCommand Unused parameter (#14): - Remove IPrerequisiteRunner from BlueprintSubcommand.CreateCommand signature - Update SetupCommand.cs call site and BlueprintSubcommandTests accordingly InfrastructureRequirementCheck (#5, #6): - Add I1/I2/I3/I1V2/I2V2/I3V2 (Isolated) SKUs to validation error message - Wrap CheckAsync with ExecuteCheckWithLoggingAsync so [PASS]/[FAIL] is printed PrerequisiteRunner warning message (#3): - Log ErrorMessage ?? Details, log even when both are empty IsCaeError gap (#18): - Add InvalidAuthenticationToken to IsCaeError in ClientAppValidator Stale comment (#10): - Update ValidateDeploymentPrerequisitesAsync doc to remove "environment" Tests (#19, #20): - Add AppServiceAuthRequirementCheckTests (success, failure, metadata, null guard) - Add MosPrerequisitesRequirementCheckTests (exception->failure, metadata, null guards) - Update FrontierPreviewRequirementCheckTests: [WARN] now at LogWarning not LogInformation Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
sellakumaran
added a commit
that referenced
this pull request
Mar 10, 2026
…siteRunner (#106) (#312) * feat: unify prerequisite validation via IRequirementCheck + IPrerequisiteRunner (#106) Commands now declare prerequisites using IRequirementCheck and fail early with actionable messages before any side effects occur. Phase 1 - pure reorganization (zero behavioral change): - Add AzureAuthRequirementCheck and InfrastructureRequirementCheck adapters - Add IPrerequisiteRunner / PrerequisiteRunner to run checks in order - Route AllSubcommand, BlueprintSubcommand, InfrastructureSubcommand, and DeployCommand through the shared runner instead of ad-hoc validators - Delete dead code: ISubCommand.ValidateAsync, IAzureValidator/AzureValidator - Make AzureAuthValidator.ValidateAuthenticationAsync virtual for testability Phase 2 - minimal early-fail additions: - cleanup azure: auth check before preview display - deploy mcp: explicit early guards for agentBlueprintId and agenticAppId before any Graph/network calls Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> * fix: three CLI polish fixes - ConfigFileNotFoundException now extends Agent365Exception so missing config errors surface as clean user messages (no stack trace) on all commands, not just those with local catch blocks. Removes ad-hoc FileNotFoundException catches in CleanupCommand and CreateInstanceCommand. - config init: expand relative/dot deployment paths to absolute before saving so the stored value is portable across directories. Update help text to clarify relative paths are accepted. - config init: drop platform-specific parenthetical from 'Allow public client flows' log message -- the setting is required on all platforms. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> * Polish CLI output: reduce noise, fix ordering, add TraceId - Move "Running all setup steps..." to after requirements check output - Remove redundant "Agent 365 Setup" header (user already knows the command) - Change CorrelationId log to LogDebug for setup all and blueprint; surface as TraceId inline on the action line ("Running all setup steps... (TraceId: ...)") so it is always captured in setup.log as [INF] and visible on console - Demote PlatformDetector internal logs to LogDebug; single "Detected project platform: X" line remains as the user-facing output - Add AzureAuthRequirementCheck to GetConfigRequirementChecks so Azure auth appears in requirements output for all setup subcommands - Remove redundant mid-execution auth gate from BlueprintSubcommand that caused duplicate [PASS] Azure Authentication output - Fix RequirementCheck base class: use LogInformation for all check result lines to avoid WARNING:/ERROR: prefix doubling from logger formatter - Collapse verbose requirements summary to single line: "Requirements: X passed, Y warnings, Z failed" - Update tests to match new message text and log level assertions Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> * feat: add fail-early requirement checks to remaining commands Extends fail-early validation to setup infrastructure, setup permissions, setup copilot-studio, cleanup azure, deploy, and publish commands. Each command now runs targeted IRequirementCheck-based pre-flight checks with formatted [PASS]/[FAIL] output before executing destructive or slow operations, surfacing auth and config failures immediately. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> * refactor: structured requirement check composition + fix CAE token revocation UX Phase 1 (zero behavioral change): - Add GetBaseChecks() to SetupCommand and CleanupCommand for explicit check composition - Add GetChecks() to each setup subcommand so check lists are co-located with their command - Add RunChecksOrExitAsync() helper to RequirementsSubcommand to eliminate four-line boilerplate - Guard all requirement check calls with if (!dryRun) to avoid spurious network calls - Update RequirementsSubcommandTests to use public API after making internal helpers private Fix CAE token revocation UX: - Add ClientAppValidationException.TokenRevoked() factory for clear re-auth guidance - Detect server-side CAE token revocation in GetClientAppInfoAsync and throw TokenRevoked instead of returning null (which was misreported as "app not found") - Pass suppressErrorLogging: true to all az CLI calls in ClientAppValidator so raw error output no longer leaks to console before the formatted [FAIL] message - Update ClientAppValidatorTests mocks to match suppressErrorLogging parameter Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> * fix: suppress raw subprocess output leaking before structured check results AzureAuthValidator: add suppressErrorLogging to az account show call to prevent CommandExecutor from printing raw stderr before [FAIL] output. Remove verbose LogError/LogInformation guidance blocks — the validator returns bool only; issue/resolution messaging belongs in the check layer. PowerShellModulesRequirementCheck: downgrade auto-install progress from LogInformation/LogWarning to LogDebug so they don't print before [PASS]. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> * feat: add cleanup azure --dry-run, AppService/MOS checks, update docs - Add `--dry-run` flag to `a365 cleanup azure`: previews resources that would be deleted without requiring Azure auth or making any changes - Add `AppServiceAuthRequirementCheck`: validates App Service deployment token before `a365 deploy`, catching AADSTS50173 token revocation early - Add `MosPrerequisitesRequirementCheck`: validates MOS service principals before `a365 publish` proceeds, converting SetupValidationException to structured failure output - Wire new checks into DeployCommand and PublishCommand via RunChecksOrExitAsync, replacing ad-hoc inline validation - Add `GetChecks(AzureAuthValidator)` to InfrastructureSubcommand for explicit check composition - Add `GetAppServiceTokenAsync` to AzureAuthValidator - Update CLI design.md: add Requirements/ to project structure and document the IRequirementCheck prerequisite validation pattern - Update CHANGELOG.md with user-visible additions Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> * fix: address PR #312 review comments (exit codes, log levels, skip-graph, tests) Exit codes (#7, #8/#9): - Set Environment.ExitCode = 1 in ValidateDeploymentPrerequisitesAsync before each null return so callers exit non-zero on config/Web App validation failure - Replace deploy-mcp guard `return` with ExceptionHandler.ExitWithCleanup(1) for AgentBlueprintId, AgenticAppId, and TenantId missing-config cases Log severity (#15, #16, #17): - LogCheckWarning: LogInformation -> LogWarning - LogCheckFailure: all three LogInformation -> LogError - ExecuteCheckWithLoggingAsync warning path: log ErrorMessage ?? Details so the primary warning message is no longer silently dropped skip-graph regressions (#21, #22): - Guard RunChecksOrExitAsync(MOS checks) behind if (!skipGraph) - Guard clientAppId null check behind !skipGraph in PublishCommand Unused parameter (#14): - Remove IPrerequisiteRunner from BlueprintSubcommand.CreateCommand signature - Update SetupCommand.cs call site and BlueprintSubcommandTests accordingly InfrastructureRequirementCheck (#5, #6): - Add I1/I2/I3/I1V2/I2V2/I3V2 (Isolated) SKUs to validation error message - Wrap CheckAsync with ExecuteCheckWithLoggingAsync so [PASS]/[FAIL] is printed PrerequisiteRunner warning message (#3): - Log ErrorMessage ?? Details, log even when both are empty IsCaeError gap (#18): - Add InvalidAuthenticationToken to IsCaeError in ClientAppValidator Stale comment (#10): - Update ValidateDeploymentPrerequisitesAsync doc to remove "environment" Tests (#19, #20): - Add AppServiceAuthRequirementCheckTests (success, failure, metadata, null guard) - Add MosPrerequisitesRequirementCheckTests (exception->failure, metadata, null guards) - Update FrontierPreviewRequirementCheckTests: [WARN] now at LogWarning not LogInformation Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> --------- Co-authored-by: Claude Sonnet 4.6 <noreply@anthropic.com>
sellakumaran
added a commit
that referenced
this pull request
Mar 24, 2026
Anti-pattern #19: Unreachable catch clause — when the inner method already handles the exception before propagating, the outer catch is dead code and risks a double-attempt on the same fallback (the AuthenticationService belt-and-suspenders catch that slipped through review-staged). Anti-pattern #20: MsalServiceException.ErrorCode used when AADSTS code expected — ErrorCode is the OAuth code ("access_denied"), the AADSTS code (e.g. AADSTS53003) is only in ex.Message; using ErrorCode in log messages produces misleading diagnostics. Anti-pattern #21: Log message / comment covers fewer cases than the code handles — when a when-clause matches two error codes but the log only names one, operators are misled during triage; also catches "bypasses" / "not subject to" claims in comments that are not universally true. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
sellakumaran
added a commit
that referenced
this pull request
Mar 25, 2026
…ocks browser/WAM auth (#294) (#323) * fix: automatic device code fallback when Conditional Access Policy blocks browser/WAM auth (#294) When AADSTS53003 (Conditional Access Policy) or AADSTS53000 (device compliance policy) blocks interactive browser or WAM authentication, the CLI now automatically falls back to device code flow instead of failing with no recovery path. Covers all 6 browser auth locations: - MsalBrowserCredential (primary fix, covers AuthenticationService, InteractiveGraphAuthService, MicrosoftGraphTokenProvider MSAL path, and BlueprintSubcommand) - MicrosoftGraphTokenProvider PowerShell path (retries Connect-MgGraph with -UseDeviceCode) - AuthenticationService belt-and-suspenders catch for future custom credential implementations Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> * fix: address PR #323 Copilot review comments for CAP auth fallback - Remove belt-and-suspenders CAP catch from AuthenticationService (C1/C2): unreachable in production and risked double device code attempt - Fix doc comment in AuthenticationConstants: device code flow may still be affected by CAP policies, not "bypasses" them (C3) - Fix log message in MicrosoftGraphTokenProvider to cover both AADSTS53003 and AADSTS53000 (Conditional Access and device compliance) (C4) - Fix ErrorCode placeholder in MsalBrowserCredential to log the AADSTS code (extracted from message) instead of the OAuth "access_denied" code (C5) - Fix comment in MsalBrowserCredential: "may still be affected" instead of "not subject to these policies" (C6) - Remove 3 tests that covered the removed belt-and-suspenders catch; keep the device code error surface test (still valid) Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> * feat(review): add anti-patterns #19-21 from PR #323 Copilot findings Anti-pattern #19: Unreachable catch clause — when the inner method already handles the exception before propagating, the outer catch is dead code and risks a double-attempt on the same fallback (the AuthenticationService belt-and-suspenders catch that slipped through review-staged). Anti-pattern #20: MsalServiceException.ErrorCode used when AADSTS code expected — ErrorCode is the OAuth code ("access_denied"), the AADSTS code (e.g. AADSTS53003) is only in ex.Message; using ErrorCode in log messages produces misleading diagnostics. Anti-pattern #21: Log message / comment covers fewer cases than the code handles — when a when-clause matches two error codes but the log only names one, operators are misled during triage; also catches "bypasses" / "not subject to" claims in comments that are not universally true. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> --------- Co-authored-by: Claude Sonnet 4.6 <noreply@anthropic.com>
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
2 participants
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.
Refined exception handling across the application:
catchblocks with specific exceptions.DeployAppPythonCompileExceptionfor Python syntax errors.DeploymentAppCompileFailed.Enhanced user feedback and logging:
Integrated Python syntax validation:
python -m py_compilechecks inPythonBuilderto catch syntax errors early.Performed code cleanup and refactoring:
usingdirectives for better structure.Previously python compilation errors were not caught early in a365 deploy app.
Now we see this:
[11:17:04 INF] [2/7] Building Python application...
[11:17:04 INF] Building Python project...
[11:17:05 ERR] Command failed with exit code 1: Sorry: IndentationError: unexpected indent (host_agent_server.py, line 289)
[11:17:05 ERR] Python syntax error in C:\A365-Ignite-Demo-Python\sample_agent\host_agent_server.py:
Sorry: IndentationError: unexpected indent (host_agent_server.py, line 289)
ERROR: py_compile failure
Python syntax error in C:\A365-Ignite-Demo-Python\sample_agent\host_agent_server.py:
Sorry: IndentationError: unexpected indent (host_agent_server.py, line 289)
To resolve this issue:
Error code: DEPLOYMENT_APP_COMPILE_FAILED
[11:17:05 ERR] Operation failed. ErrorCode=DEPLOYMENT_APP_COMPILE_FAILED, IssueDescription=py_compile failure