Skip to content

Fix: exit non-zero when an adk deploy subcommand fails - #212

Open
AmaadMartin wants to merge 3 commits into
mainfrom
fix/deploy-commands-exit-non-zero-on-failure
Open

Fix: exit non-zero when an adk deploy subcommand fails#212
AmaadMartin wants to merge 3 commits into
mainfrom
fix/deploy-commands-exit-non-zero-on-failure

Conversation

@AmaadMartin

@AmaadMartin AmaadMartin commented Aug 10, 2026

Copy link
Copy Markdown
Owner

Please ensure you have read the contribution guide before creating a pull request.

Link to Issue or Description of Change

  1. Link to an existing issue (if applicable):

  2. Or, if no issue exists, describe the change:

Problem: All three adk deploy subcommands print Deploy failed: <reason> in red and then return normally, so the process exits 0. adk deploy cloud_run ... && echo deployed prints deployed after a deploy that never happened, and a CI stage gated on $? passes. The broad except Exception also swallows the gcloud CalledProcessError, the gcloud argument-conflict ClickException, and the agent_engine mutually-exclusive-flag UsageError.

Solution: Each handler adds one line, sys.exit(1), after the existing message. cli_tools_click.py:1053-1059 already pairs a red secho with sys.exit(1) in a callback that holds no click context, so this needs no @click.pass_context plumbing and keeps the message text and colour byte-for-byte. No traceback is printed, and --help output is unchanged for all three subcommands.

This is an intentional behaviour change. A pipeline that continued past a failed deploy now stops there.

cli_migrate_session has the same defect. It is deliberately out of scope: adk migrate session is a different command, and its behaviour change deserves its own review and its own test. I filed it as separate work.

Collision check: I listed all 211 open pull requests on the fork and read the diff of every one that touches cli_tools_click.py or cli_deploy.py (#183, #194, #17, #158, #62, #64, #91, #109, #124, #120, #72, #70, #197). None changes the deploy exception handlers, so this PR is branched from main.

Testing Plan

Please describe the tests that you ran to verify your changes. This is required for all PRs that are not small documentation or typo fixes.

Unit Tests:
[x] I have added or updated unit tests for my change.
[x] All unit tests pass locally.

pytest tests/unittests/cli -q -> 775 passed, 5 skipped, 4 xfailed.

I wrote the tests first and ran them against the unfixed code. All six assertions failed with assert 0 != 0. I then deleted each sys.exit(1) one at a time and confirmed only the matching tests fail:

Mutation Failing tests
drop sys.exit(1) in cli_deploy_cloud_run ..._cloud_run_failure, ..._cloud_run_gcloud_failure_exits_non_zero, ..._cloud_run_gcloud_arg_conflict
drop sys.exit(1) in cli_deploy_agent_engine ..._agent_engine_failure, ..._agent_engine_conflicting_import_flags_exit_non_zero, ..._agent_engine_failure_exit_status_of_real_process
drop sys.exit(1) in cli_deploy_gke ..._gke_failure

Coverage of the new code is 100%: a coverage run over the deploy tests marks all three sys.exit(1) lines as executed, and the success tests cover the non-exception branch.

One existing test changes: test_cli_deploy_cloud_run_failure asserted exit_code == 0. That assertion encoded the bug, so it is now != 0.

Manual End-to-End (E2E) Tests:
test_cli_deploy_agent_engine_failure_exit_status_of_real_process runs the real CLI as a subprocess, because CliRunner only reports a simulated exit code. It skips where the interpreter cannot be spawned. The same commands by hand, with no cloud project and no mocks:

$ adk deploy agent_engine --validate-agent-import --skip-agent-import-validation ./my_agent; echo "exit=$?"
Deploy failed: Do not pass both --validate-agent-import and --skip-agent-import-validation.
exit=1

$ adk deploy agent_engine --validate-agent-import --skip-agent-import-validation ./my_agent && echo "deployed"
Deploy failed: Do not pass both --validate-agent-import and --skip-agent-import-validation.

deployed is printed before this change and is not printed after it. No traceback appears. adk deploy cloud_run --help, adk deploy agent_engine --help and adk deploy gke --help still exit 0, and I diffed their output against main: it is identical.

CI on this branch: Unit Tests, Mypy Check and A2A v0.3 Tests pass on Python 3.10 through 3.14. The Pre-commit Linter job fails on its update-constraints hook, which regenerates the constraints-3.*.txt snapshot date. That failure is unrelated to this change and reproduces on every open pull request here, including #209, #210 and #211. Every other hook, ruff, isort and pyink included, passes.

Locally I also ran mypy on cli_tools_click.py before and after: 93 errors both times, the same set, no new error.

Checklist

[x] I have read the CONTRIBUTING.md document.
[x] I have performed a self-review of my own code.
[x] I have commented my code, particularly in hard-to-understand areas.
[x] I have added tests that prove my fix is effective or that my feature works.
[x] New and existing unit tests pass locally with my changes.

Amaad Martin added 3 commits August 10, 2026 16:44
All three `adk deploy` subcommands printed `Deploy failed: <reason>` and
then returned normally, so the process exited 0. A shell chain such as
`adk deploy cloud_run ... && echo deployed` therefore continued past a
deploy that never happened, and a CI stage gated on `$?` passed.

Each handler now calls `ctx.exit(1)` after the existing message.
`agent_engine` and `gke` take the click context through
`@click.pass_context`; `cloud_run` already had it. The message text, its
colour, and the `--help` output do not change.
`cli_deploy_agent_engine` and `cli_deploy_gke` now take the click context,
which click injects and never declares as an option. The cloud_run check
already ignored `ctx` for the same reason. Every real option still has to
match a function parameter.
…ntext

`ctx.exit(1)` obliged `cli_deploy_agent_engine` and `cli_deploy_gke` to
take a click context they had no other use for, and that injected `ctx`
parameter in turn forced an exemption into the option/parameter parity
tests. `sys.exit(1)` needs neither. It already follows a red `secho` this
way in `cli_test`, and it keeps the message and colour unchanged.

This also reverts the parity-test exemption, so those tests match main
again.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant