[AppService] Fix #34005: az webapp ssh: Report SSH session failures instead of exiting 0 - #34006
Open
Aditya Pujara (a0x1ab) wants to merge 4 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>
azure-client-tools-agent
Bot
added
the
Azure Client Tools Agent Requested
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 |
![azure-client-tools-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.
azure-client-tools-agent
Bot
added
the
azure-client-tools-agent
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.
azure-client-tools-agent
Bot
removed
the
Azure Client Tools Agent Requested
Member
Author
|
@azure-client-tools-agent You should actually have updated the package |
Aditya Pujara (a0x1ab)
added
Azure Client Tools Agent Requested
Contributor
There was a problem hiding this comment.
Automated review summary
CI: ✅ All 59 checks passed (Azure DevOps validation, style/lint, title/content gate).
Live test: ✅ azdev test --live passed for the changed test file
test_webapp_commands_thru_mock.py (workflow run reused from a prior
dispatch on the current head SHA).
Regression coverage: ✅ Production change in
src/azure-cli/azure/cli/command_modules/appservice/custom.py is covered by
new/updated tests in test_webapp_commands_thru_mock.py — no coverage gap.
Review-skill findings: none confirmed. Evidence reviewed:
- The fix correctly propagates
_start_ssh_sessionexceptions (previously
swallowed vialogger.info(ex)) as aCLIErrorand surfaces them from
create_tunnel_and_sessionvia a captured-exception holder, replacing the
silent-exit-0 behavior described in the linked issue. - The SSH background thread still always closes the connection in a
finallyblock, and the main thread still waits for both the tunnel and
SSH threads before re-raising, so timing/synchronization behavior is
unchanged for the success path. - New unit tests cover:
_start_ssh_sessionraisingCLIErroron run
failure, connection always closed on failure, and the failure propagating
up throughcreate_tunnel_and_session— includes the required
exception-propagation negative-path coverage for this skill. - Title/description scope match the single changed component (
appservice)
and the stated behavior change; no unrelated changes detected.
Recommendation: No blocking issues. Ready for App Service squad review.
azure-client-tools-agent
Bot
added
azure-client-tools-agent
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