[AppService] Fix #34005: az webapp ssh: Report SSH session failures instead of exiting 0 - #34006
Open
Aditya Pujara (a0x1ab) wants to merge 9 commits into
Open
Conversation
…lures instead of exiting 0 * Initial plan * [AppService] `az webapp ssh`: Report SSH session failures instead of exiting 0 Co-authored-by: a0x1ab <59631311+a0x1ab@users.noreply.github.com> --------- Co-authored-by: copilot-swe-agent[bot] <198982749+Copilot@users.noreply.github.com> Co-authored-by: a0x1ab <59631311+a0x1ab@users.noreply.github.com>
microsoft-github-policy-service
Bot
requested review from
Yu Chen (jsntcy),
ZelinWang (wangzelin007),
Julie Zhu (yanzhudd) and
Yong Zhang (yonzhan)
August 27, 2026 20:18
microsoft-github-policy-service
Bot
added
Auto-Assign
|
Azure Pipelines: There may be pipelines that require an authorized user to comment /azp run to run. |
Contributor
Live test skipped⏭️ Skipping the live test for this revision because no changed test file was found ( The live-test pipeline runs only the test files a PR changes, so there is nothing to execute for this commit. A skipped live test is not a passing test result. The Agent review separately checks whether the affected command module includes focused regression tests or updated recordings. If a test file is changed in a later commit, the live test will run automatically. |
Contributor
There was a problem hiding this comment.
Pull request overview
This PR adjusts the AppService az webapp ssh implementation so SSH session failures are surfaced to the user and cause a non-zero CLI exit, instead of being silently swallowed and returning success.
Changes:
- Update
_start_ssh_sessionto raise aCLIErrorwhen the interactive SSH session fails. - Update
create_tunnel_and_sessionto capture exceptions from the SSH session thread and re-raise them on the main thread.
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Comment on lines
11419
to
11420
| while s.is_alive() and t.is_alive(): | ||
| time.sleep(5) |
Collaborator
|
AppService |
![x-engineering-agent[bot]](https://avatars.githubusercontent.com/in/4205534?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Upstream CI
azdev-style
- Result: failure
- Relevance: Uncertain
- Evidence: The check completed without a concise diagnostic.
- Suggested fix: Open the linked log and confirm whether the first actionable error touches the PR diff before changing code.
- Verify: Re-run
azdev-styleand confirm it passes on the new head.
azdev-style
- Result: failure
- Relevance: Uncertain
- Evidence: The check completed without a concise diagnostic.
- Suggested fix: Open the linked log and confirm whether the first actionable error touches the PR diff before changing code.
- Verify: Re-run
azdev-styleand confirm it passes on the new head.
azdev-style
- Result: failure
- Relevance: Uncertain
- Evidence: The check completed without a concise diagnostic.
- Suggested fix: Open the linked log and confirm whether the first actionable error touches the PR diff before changing code.
- Verify: Re-run
azdev-styleand confirm it passes on the new head.
azdev-style
- Result: failure
- Relevance: Uncertain
- Evidence: The check completed without a concise diagnostic.
- Suggested fix: Open the linked log and confirm whether the first actionable error touches the PR diff before changing code.
- Verify: Re-run
azdev-styleand confirm it passes on the new head.
azdev-style
- Result: failure
- Relevance: Uncertain
- Evidence: The check completed without a concise diagnostic.
- Suggested fix: Open the linked log and confirm whether the first actionable error touches the PR diff before changing code.
- Verify: Re-run
azdev-styleand confirm it passes on the new head.
Test validation
- Live test: Skipped: no runnable changed test files were found.
- Regression coverage: Gap detected for
appservice: production behavior changed without a focused test or recording change.
No additional evidence-confirmed semantic findings beyond the coverage gap noted above.
Risk assessment
43/100 · Medium · High confidence
The Medium rating is driven by public CLI behavior, failure-handling behavior, no changed regression test.
- Change scope: 1 changed file, 16 changed lines (
+13/-3), including 1 production file. - Affected components:
appservice - Risk drivers: public CLI behavior (+18); failure-handling behavior (+12); no changed regression test (+10)
- Regression evidence: No changed regression test was detected for the production changes, increasing risk.
- Confidence: High because changed-line patches were available for every production file.
- Required review: Owning-squad review is recommended for
appservicebefore merge.
Contributor
|
Started a Copilot task using |
Co-authored-by: a0x1ab <59631311+a0x1ab@users.noreply.github.com>
Co-authored-by: a0x1ab <59631311+a0x1ab@users.noreply.github.com>
Contributor
Live test results —
|
Contributor
There was a problem hiding this comment.
Upstream CI
azdev-style
- Result: failure
- Relevance: Uncertain
- Evidence: The check completed without a concise diagnostic.
- Suggested fix: Open the linked log and confirm whether the first actionable error touches the PR diff before changing code.
- Verify: Re-run
azdev-styleand confirm it passes on the new head.
Azure.azure-cli
- Result: failure
- Included checks:
Azure.azure-cli (Check CLI Style) - Failure details:
- Check CLI Style / Bash (Uncertain)
- Evidence: Running pylint on modules...
Pylint: PASSED
ERROR: ************* Module azure.cli.command_modules.appservice.custom
src/azure-cli/azure/cli/command_modules/appservice/custom.py:11423:8: E0702: Raising NoneType while only classes or instances are allowed (raising-bad-type)
- Evidence: Running pylint on modules...
- Check CLI Style / Bash (Uncertain)
Your code has been rated at 10.00/10
ERROR: Pylint: FAILED
Running flake8 on modules...
Flake8: PASSED
- Next action: Address the first actionable diagnostic quoted from the task log; do not infer a source-code cause from the aggregate build status.
- Verify: Re-run Check CLI Style / Bash and confirm the quoted diagnostic is gone.
Test validation
- Live test: Passed. Workflow run
- Regression coverage: Present for
appservice: 1 focused test file(s) changed.
No review-skill findings were confirmed from the changed-line evidence.
Summary: CI is failing because of a real pylint error introduced by this PR: raising-bad-type at src/azure-cli/azure/cli/command_modules/appservice/custom.py:11423 (raising None instead of an exception class/instance). Please fix the raise statement so it raises a proper exception (e.g. a CLIError/AzCLIError subclass or Exception instance) instead of None, then re-run azdev style locally to confirm before pushing.
Contributor
|
Started a Copilot task using |
Co-authored-by: a0x1ab <59631311+a0x1ab@users.noreply.github.com>
Contributor
Live test results —
|
Contributor
There was a problem hiding this comment.
Summary
All checks passed:
- CI: 50/50 checks passed, 0 failed, 0 pending.
- Live test: Dispatched
live-test.ymlon the changed test file (test_webapp_commands_thru_mock.py); conclusionsuccess. - Regression coverage: No gap detected — the production change in
appservice/custom.pyis accompanied by new/updated tests intest_webapp_commands_thru_mock.py. - Review-skill findings: None confirmed. The change replaces a silently-swallowed SSH session exception (
logger.info(ex)) with a raisedCLIError, and propagates the SSH thread's exception back to the caller ofcreate_tunnel_and_sessioninstead of exiting 0 on failure. New unit tests cover the raised-error path, connection-close-on-failure path, and the error-propagation path through the calling function.
Risk assessment
28/100 · Medium · High confidence
The Medium rating is driven by public CLI behavior, failure-handling behavior.
- Change scope: 2 changed files, 111 changed lines (
+107/-4), including 1 production file. - Affected components:
appservice - Risk drivers: public CLI behavior (+18); failure-handling behavior (+12)
- Regression evidence: Changed regression tests are included, reducing risk.
- Confidence: High because changed-line patches were available for every production file.
- Required review: Owning-squad review is recommended for
appservicebefore merge.
Co-authored-by: a0x1ab <59631311+a0x1ab@users.noreply.github.com>
Contributor
Live test results —
|
Contributor
Live test results —
|
Member
Author
|
From Yu Chen (@jsntcy): @Azure/azure-client-tools-agent |
Contributor
|
Started a Copilot task using |
Co-authored-by: a0x1ab <59631311+a0x1ab@users.noreply.github.com>
Contributor
Live test results —
|
Contributor
There was a problem hiding this comment.
Automated review summary
CI (50/50 checks) and the live-test dispatch both passed for this PR, but a review-skill scope-consistency check found unrelated bundled changes that must be resolved before merge.
Scope-consistency finding
- Scope-consistency (Medium) —
src/azure-cli/requirements.py3.Darwin.txt,src/azure-cli/requirements.py3.Linux.txt,src/azure-cli/requirements.py3.windows.txt,doc/quoting-issues-with-powershell.md: this PR is titled and scoped as a fix foraz webapp sshfailure reporting (custom.py+ its test file), but it also bundles two unrelated commits: (1)chore: upgrade invoke from 2.2.0 to 3.0.3bumping theinvokedependency pin in all threerequirements.py3.*.txtfiles, and (2)doc: add section on closing parenthesis ) being stripped in PowerShell, adding an unrelated quoting-issues doc section. Neither change relates to SSH session failure reporting, the stated issue (#34005), or the PR title/History Notes.
Remediation: Split theinvokedependency bump and the PowerShell quoting-issues doc section into separate, independently-titled PRs (or drop them from this branch if they were picked up unintentionally), so this PR only contains the SSH failure-reporting fix and its test.
Verify: Confirm the final diff for this PR touches onlysrc/azure-cli/azure/cli/command_modules/appservice/custom.pyand itstest_webapp_commands_thru_mock.pytest file (plus any required History Notes for the stated fix).
Risk assessment
29/100 · Medium · High confidence
The Medium rating is driven by public CLI behavior, failure-handling behavior.
- Change scope: 6 changed files, 149 changed lines (
+142/-7), including 1 production file. - Affected components:
appservice - Risk drivers: public CLI behavior (+18); failure-handling behavior (+12)
- Regression evidence: Changed regression tests are included, reducing risk.
- Confidence: High because changed-line patches were available for every production file.
- Required review: Owning-squad review is recommended for
appservicebefore merge.
Contributor
|
Started a Copilot task using |
…ction Reverts: - invoke 3.0.3 → 2.2.0 in requirements.py3.*.txt (unrelated to SSH fix) - doc section on PowerShell closing parenthesis (unrelated to SSH fix) Co-authored-by: a0x1ab <59631311+a0x1ab@users.noreply.github.com>
Contributor
Live test results —
|
Contributor
Live test results —
|
Contributor
There was a problem hiding this comment.
Automated review summary
Result: Pass
- CI: All 50 checks passed (0 failed, 0 pending).
- Live test:
azdev testrun against the changed test file completed successfully. - Regression coverage: Production change in
src/azure-cli/azure/cli/command_modules/appservice/custom.pyis covered by new/updated tests intest_webapp_commands_thru_mock.py(no coverage gap detected). - Review skills: No confirmed findings from release-artifact, test-strength, scope-consistency or domain-edge-case checks. The change is scoped to the SSH-session failure path in
create_tunnel_and_session/_start_ssh_session, matches the stated fix (az webapp sshnow raisesCLIErrorand reports failures instead of exiting 0), and the added tests cover the exception-propagation and connection-cleanup behavior with negative-path assertions.
No blocking issues found.
Member
Author
|
@Azure/azure-client-tools-agent Re-run the full validation and automated review for the current PR state. Do not change code unless you find a real issue. |
Contributor
|
Started a Copilot task using |
Contributor
There was a problem hiding this comment.
Validation passed ✅
All CI checks passed, live tests completed successfully, and no deterministic review-skill findings were identified.
Test validation
- Live test: Passed. Workflow run
- Regression coverage: Present for
appservice: 1 focused test file(s) changed.
Risk assessment
28/100 · Medium · High confidence
The Medium rating is driven by public CLI behavior, failure-handling behavior.
- Change scope: 2 changed files, 111 changed lines (
+107/-4), including 1 production file. - Affected components:
appservice - Risk drivers: public CLI behavior (+18); failure-handling behavior (+12)
- Regression evidence: Changed regression tests are included, reducing risk.
- Confidence: High because changed-line patches were available for every production file.
- Required review: Owning-squad review is recommended for
appservicebefore merge.
Aditya Pujara (a0x1ab)
added
X Engineering Agent Reviewed
Contributor
|
🔔 Routing this PR to @Azure/act-observability-squad. |
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.
🤖 PR Validation — ️✔️ All clear
Description
Fixes #34005.
_start_ssh_sessionto raiseCLIErrorinstead of swallowing SSH session exceptions silentlycreate_tunnel_and_sessionto capture and re-raise exceptions from the SSH session thread so the CLI exits non-zero