Skip to content

Fix: make bare adk telemetry exit 0 on Click >= 8.2 - #53

Open
AmaadMartin wants to merge 1 commit into
mainfrom
fix/telemetry-cli-bare-group-exit-code
Open

Fix: make bare adk telemetry exit 0 on Click >= 8.2#53
AmaadMartin wants to merge 1 commit into
mainfrom
fix/telemetry-cli-bare-group-exit-code

Conversation

@AmaadMartin

@AmaadMartin AmaadMartin commented Jul 30, 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):

No public issue is filed for this; it is a CI failure on main.

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

Problem: tests/unittests/cli/utils/test_cli_tools_click.py::test_telemetry_cli_commands is the only failing test in tests/unittests on main, and it fails on every Python version in the Unit Tests matrix.

telemetry is declared as a plain Click group (src/google/adk/cli/cli_tools_click.py:293). A click.Group built that way gets no_args_is_help=True. On Click 8.1.x a bare adk telemetry printed the group help and exited 0; from Click 8.2.0 onwards the same invocation raises NoArgsIsHelpError (a UsageError subclass) — it still prints the full help text but exits 2.

pyproject.toml pins click>=8.1.8,<9 and the repo has no lockfile, so uv sync --extra test in CI resolves the newest compatible Click (8.4.2 today). The test was authored against a locally resolved 8.1.x, so it passed then and fails now:

>   assert result.exit_code == 0
E   assert 2 == 0
E    +  where 2 = <Result SystemExit(2)>.exit_code

Measured exit code of bare adk telemetry against the unchanged group, on this checkout:

click exit code
8.1.8 0
8.2.0 2
8.3.0 2
8.4.2 2

This is a user-visible regression, not just a test artifact: with a current Click, a plain help request returns a failure status to the shell.

Solution: Declare the group with invoke_without_command=True and echo the help from the callback:

@main.group("telemetry", invoke_without_command=True)
@click.pass_context
def telemetry(ctx: click.Context) -> None:
  """Manage telemetry settings."""
  if ctx.invoked_subcommand is None:
    click.echo(ctx.get_help())

Group.__init__ sets no_args_is_help = not invoke_without_command, so this disables the implicit usage error and the callback returns normally — exit 0 on every Click version the pin allows, restoring the 8.1.x behaviour rather than baking in 8.2+ behaviour. adk telemetry <unknown> still exits 2 (a genuine usage error), and status / enable / disable are untouched.

Alternatives rejected: changing the assertion to == 2 (or in (0, 2)) bakes in Click ≥ 8.2 while pyproject.toml still permits 8.1.8, and leaves the user-visible regression in place; raising the click floor forces a dependency bump on downstream users to work around a test expectation and still leaves adk telemetry exiting 2. No dependency pin was changed.

The usage line legitimately becomes [COMMAND] [ARGS]... (COMMAND is now optional). No user-facing docs in this repository mention adk telemetry, so no doc update is needed.

Collision check (required by the contribution workflow): gh pr list --repo AmaadMartin/adk-python --state open --limit 100 returned 50 open PRs; 17 of them touch cli_tools_click.py, so each was diffed and grepped for invoke_without_command, no_args_is_help, and test_telemetry_cli_commands. Zero hits — no open PR lands or overlaps this change, so this branches 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:

  • I have added or updated unit tests for my change.
  • All unit tests pass locally.
pytest tests/unittests/cli/utils/test_cli_tools_click.py -q   # 52 passed
pytest tests/unittests/cli -q                                 # 564 passed, 5 skipped

The change adds two executable lines. Both are covered and both branches are exercised, measured with coverage run --branch --source=google.adk.cli.cli_tools_click: for the patched region, missing_lines is empty and neither branch of if ctx.invoked_subcommand is None appears in missing_branches100% line and branch coverage of the new code. The None branch comes from runner.invoke(main, ["telemetry"]); the not-None branch from the five status / enable / disable invocations that follow.

I strengthened the bare-invocation assertions (the group docstring plus all three subcommand names) so the test proves the group help was printed, rather than any output merely containing "Usage:". The pre-existing assert result.exit_code == 0 was deliberately not relaxed — that assertion is what pins the fix. No test was skipped, weakened, or deleted.

Proof the tests can fail — each new/changed assertion was run against mutated source and observed to fail:

# Mutation Result
A Revert the production hunk to the original plain @main.group("telemetry") FAILS: assert result.exit_code == 0E assert 2 == 0
B Keep invoke_without_command=True but replace click.echo(ctx.get_help()) with pass FAILS: E AssertionError: assert 'Usage:' in ''
C Replace click.echo(ctx.get_help()) with click.echo("Usage: adk telemetry") — output contains "Usage:" but is not the group help FAILS on a new assertion: E AssertionError: assert 'Manage telemetry settings.' in 'Usage: adk telemetry\n'

Mutation C is the one that justifies the added assertions: the pre-existing "Usage:" check alone survives it.

Click-version independence (the whole point of the change) — pytest tests/unittests/cli/utils/test_cli_tools_click.py -q on each version permitted by click>=8.1.8,<9:

click before after
8.1.8 52 passed 52 passed
8.2.0 2 failed, 50 passed 1 failed, 51 passed
8.3.0 1 failed, 51 passed 52 passed
8.4.2 1 failed, 51 passed 52 passed

On 8.2.0 exactly one failure remains, test_cli_deploy_agent_engine_otel_to_cloud_success. It is pre-existing and unrelated — it fails identically on unmodified main with click 8.2.0 and passes on 8.1.8, 8.3.0 and 8.4.2, so it is a defect specific to that one Click release and out of scope here. It has been filed separately rather than bundled into this fix.

Lint and type checks on the changed files, matching the versions pinned in .pre-commit-config.yaml:

pyink==25.12.0 --check   # 2 files would be left unchanged
ruff==0.15.17 check      # All checks passed!
isort==8.0.1 --check-only # clean

Mypy was run the same way the Mypy Check job compares baseline to PR (mypy ... | grep error: | sed 's/:\([0-9]\+\):/::/g' | sort, then comm -13): 93 pre-existing errors in this file before the change, 93 after, 0 new errors.

Manual End-to-End (E2E) Tests:

Run against the real installed adk entry point with no mocks, with HOME pointed at a temporary directory so the consent file is isolated:

export TEST_HOME=$(mktemp -d)
run() { HOME="$TEST_HOME" adk "$@" </dev/null; echo "exit=$?"; }

run telemetry          # group help, exit=0   (was exit=2 before this change)
run telemetry --help   # group help, exit=0
run telemetry status   # "Telemetry collection is not configured (defaults to OFF)." exit=0
run telemetry enable   # "Telemetry collection has been enabled."  exit=0
run telemetry status   # "Telemetry collection is enabled."        exit=0
run telemetry disable  # "Telemetry collection has been disabled." exit=0
run telemetry status   # "Telemetry collection is disabled."       exit=0
run telemetry bogus    # "Error: No such command 'bogus'."         exit=2
adk --help             # unchanged, exit=0
rm -rf "$TEST_HOME"

All of the above were observed with those exact outputs and exit codes, and the enable/disable round trip really wrote $TEST_HOME/.adk/config.json.

CI result on this PR

All test jobs pass — including the Unit Tests matrix this change exists to repair:

Job Result
Unit Tests (Python 3.10 / 3.11 / 3.12 / 3.13 / 3.14) pass (5/5)
Mypy Check (Python 3.10 / 3.11 / 3.12 / 3.13) pass (4/4)
A2A v0.3 Tests (Python 3.10 – 3.14) pass (5/5)
Pre-commit Linter fail — pre-existing, unrelated

The Pre-commit Linter failure is not caused by this diff. Every hook passes; the sole failing hook is update-constraints, which dies with ./scripts/update_constraints.sh: line 103: uv: command not found because the lint job never installs uv. That hook is scoped to files: ^(pyproject\.toml|constraints-.*\.txt)$ — neither of which this PR touches — and only executes at all because the CI action invokes pre-commit run --all-files. The identical failure reproduces on other open PRs with unrelated diffs.

Locally, pre-commit run --files src/google/adk/cli/cli_tools_click.py tests/unittests/cli/utils/test_cli_tools_click.py passes every hook, with update-constraints correctly reported as (no files to check) Skipped.

That lint-job defect is already being fixed by a separate open PR that moves the hook to stages: [manual], so it is deliberately not duplicated here.

Checklist

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

A plain `click.Group` gets `no_args_is_help=True`. Click 8.1.x printed the
group help and exited 0 for a bare invocation; Click 8.2.0 and later raise a
usage error instead, still printing the help but exiting 2.

`pyproject.toml` allows `click>=8.1.8,<9` and the repo has no lockfile, so CI
resolves the newest Click and `adk telemetry` returns a non-zero status for
what is really a help request. That also broke
`test_telemetry_cli_commands`, the single failing test in `tests/unittests`.

Declare the group with `invoke_without_command=True` and echo the help from
the callback, which is exit-code 0 on every Click version the project allows.
`adk telemetry <unknown>` still exits 2, and the three subcommands are
untouched.

Also strengthen the bare-invocation assertions so they pin the actual group
help (docstring plus the three subcommand names) rather than any output
containing "Usage:".
This was referenced Jul 30, 2026
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