From fe8384aa9d1e93347d3b6e4d8439b6463dbda4f7 Mon Sep 17 00:00:00 2001 From: Gaurav karmakar Date: Sat, 15 Aug 2026 23:47:33 +0530 Subject: [PATCH] fix(wait): stop sharing exit code 2 with Click's usage errors `run wait` exits to tell a script what happened -- 0 everything passed, 1 something failed, and it used 2 for a timeout. But Click already exits 2 for any usage error: an unknown flag, a missing argument, an out-of-range --interval. All three verified against the built command. So the one thing the feature promises -- "gate on the outcome without parsing anything" -- is the thing that breaks. A script written as sp run wait $ID case $? in 0) merge ;; 1) block ;; 2) sleep 600; retry ;; esac reads a mistyped --timeout as "still running" and retries forever a command that never made a single API call. Nothing crashes and nothing is printed; the script just takes the wrong branch quietly, which is the failure mode exit codes exist to prevent. Timeout is now 9. The README's table stopped at 8 and skipped 2 already, which is why that gap was there; both are now stated explicitly so the next command does not rediscover this. Also: always render a collection. payload = rows[0] if len(rows) == 1 else {'data': rows} made the output shape depend on how many ids were passed, so `sp run wait $IDS | jq '.data[]'` worked until the list happened to hold one run. Every other list in the CLI is {'data': [...]} whether it has one row or a hundred. And a note in the docstring that a `pass` status can come from a run that recorded results for a fraction of its samples -- the API derives it from the rows that exist, not the ones expected -- so a green wait is not proof of coverage. Production run 9360 reports `pass` on 1 of 237. --- AGENTS.md | 6 +++++- README.md | 8 ++++++++ sp_cli/commands/run.py | 25 ++++++++++++++++--------- sp_cli/constants.py | 9 +++++++++ tests/test_cli.py | 42 ++++++++++++++++++++++++++++++++++++++++-- 5 files changed, 78 insertions(+), 12 deletions(-) diff --git a/AGENTS.md b/AGENTS.md index b65b0f4..655b38b 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -68,6 +68,8 @@ sp run infra-errors # VM / checkout / build / storage problems sp run artifacts # binary, coredump, stdout, outputs sp queue # what is running right now sp run progress # live status of one run +sp run wait [ ...] # block until they finish; 0 all passed, + # 1 something failed, 9 timed out sp run ls --pr # every run for one pull request sp run ls --created-after 2026-08-09T00:00:00Z # recent activity ``` @@ -131,7 +133,9 @@ sp run ls --limit 5 | jq -r '.data[] | "\(.run_id) \(.platform) \(.status) ``` Exit codes to branch on: `0` ok, `3` unreachable, `4` not found, `5` validation, -`6` auth, `7` rate limited, `8` conflict. +`6` auth, `7` rate limited, `8` conflict, `9` `run wait` timed out. **`2` means +you got the command wrong** — an unknown flag, a missing argument, a value out +of range — so treat it as a bug in your invocation, never as an outcome. Progress spinners, retry notices, and colour go to **stderr** and are suppressed when stdout is not a terminal, so JSON on stdout is always parseable. diff --git a/README.md b/README.md index 5689da4..5ebeac6 100644 --- a/README.md +++ b/README.md @@ -134,6 +134,7 @@ sp health # API + dependency health sp queue # queue depth and running jobs sp run ls # list CI runs sp run ls --pr 2309 # ... just one pull request's runs +sp run wait 9412 9413 # block until they finish; exit code is the verdict sp run create --commit --platform linux --repository owner/repo sp sample ls / show / details # media samples sp regression ls / show # regression-test definitions @@ -275,6 +276,13 @@ Scripts and agents can branch on the exit status: | 6 | authentication / authorization failure | | 7 | rate limited | | 8 | conflict (e.g. deleting a test that has results) | +| 9 | `run wait` gave up before every run finished | + +`run wait` additionally exits 1 when a run it waited for failed or was +canceled. Note what is *not* in this table: **2**, which Click returns for any +usage error — an unknown flag, a missing argument, an out-of-range value. Never +give 2 a meaning of your own, or a mistyped flag becomes indistinguishable from +a real outcome. ## Development diff --git a/sp_cli/commands/run.py b/sp_cli/commands/run.py index 03a9fd3..7c55654 100644 --- a/sp_cli/commands/run.py +++ b/sp_cli/commands/run.py @@ -11,11 +11,11 @@ from sp_cli.compare import compare_runs, coverage_warnings from sp_cli.constants import (ARTIFACT_TYPES, CANCEL_REASON_MIN_LENGTH, COMMIT_SHA_LENGTH, ERROR_GROUP_BY, - ERROR_SEVERITIES, ERROR_TYPES, INFRA_ERROR_TYPES, - LOG_CONTAINS_MAX_LENGTH, LOG_LEVELS, LOG_SOURCES, - MAX_OFFSET, MAX_PAGE_LIMIT, - MAX_REGRESSION_TEST_IDS, PLATFORMS, - PR_SCAN_DEFAULT, PR_SCAN_MAX, + ERROR_SEVERITIES, ERROR_TYPES, EXIT_TIMEOUT, + INFRA_ERROR_TYPES, LOG_CONTAINS_MAX_LENGTH, + LOG_LEVELS, LOG_SOURCES, MAX_OFFSET, + MAX_PAGE_LIMIT, MAX_REGRESSION_TEST_IDS, + PLATFORMS, PR_SCAN_DEFAULT, PR_SCAN_MAX, RUN_PENDING_STATUSES, RUN_STATUSES, RUN_UNSUCCESSFUL_STATUSES, SAMPLE_STATUSES, WAIT_INTERVAL_DEFAULT, WAIT_INTERVAL_MAX, @@ -777,8 +777,13 @@ def run_wait(ctx: click.Context, run_ids: Tuple[int, ...], interval: int, stays pipeable. Exits 0 only if every run finished successfully; a failed or canceled run - exits 1 and a timeout exits 2, which lets a script gate on the result + exits 1 and a timeout exits 9, which lets a script gate on the result without parsing the output. + + A run whose status is `pass` exits 0 even if it recorded results for only a + fraction of its samples, because the API derives that status from the rows + that exist rather than from the ones expected. Check `sp run summary`'s + skipped_count before treating a green wait as full coverage. """ client = ctx.obj['client'] output = ctx.obj['output'] @@ -806,7 +811,7 @@ def run_wait(ctx: click.Context, run_ids: Tuple[int, ...], interval: int, click.echo(f'timed out after {timeout}s; still pending: ' f'{", ".join(str(r) for r in pending)}', err=True) _render_wait(ctx, run_ids, finished, output) - raise SystemExit(2) + raise SystemExit(EXIT_TIMEOUT) if not quiet: click.echo(f'waiting on {", ".join(str(r) for r in pending)} ' f'({interval}s)', err=True) @@ -834,5 +839,7 @@ def _render_wait(ctx: click.Context, run_ids: Tuple[int, ...], :type output: str """ rows = [finished[run_id] for run_id in dict.fromkeys(run_ids) if run_id in finished] - payload: Any = rows[0] if len(rows) == 1 else {'data': rows} - render(payload, output, ctx.obj.get('color', False)) + # Always a collection, even for one run. The shape must not depend on how + # many ids the caller passed, or `sp run wait $IDS | jq '.data[]'` works + # until the list happens to contain a single run. + render({'data': rows, 'total': len(rows)}, output, ctx.obj.get('color', False)) diff --git a/sp_cli/constants.py b/sp_cli/constants.py index f4c8fb1..80eb94c 100644 --- a/sp_cli/constants.py +++ b/sp_cli/constants.py @@ -1,3 +1,4 @@ + """Enumerations accepted by the platform API, mirrored so the CLI can reject bad input locally. Each tuple matches a validator in the merged ``mod_api`` blueprint. Keeping them @@ -29,6 +30,14 @@ #: exits non-zero on these so a script can gate on it. RUN_UNSUCCESSFUL_STATUSES = ('fail', 'canceled', 'error') +#: ``sp run wait`` exits with this when the deadline passes before every run +#: finishes. Deliberately **not** 2: Click exits 2 on any usage error -- an +#: unknown flag, a missing argument, an out-of-range ``--interval`` -- so a +#: script branching on 2 would read a typo as "still running" and retry a +#: command that never executed. 1 through 8 are taken by ``ApiError.exit_code`` +#: and Click, so the first free code is 9. +EXIT_TIMEOUT = 9 + #: ``sp run wait`` polling bounds, in seconds. WAIT_INTERVAL_DEFAULT = 30 WAIT_INTERVAL_MIN = 5 diff --git a/tests/test_cli.py b/tests/test_cli.py index 5e0df43..a3c9de2 100644 --- a/tests/test_cli.py +++ b/tests/test_cli.py @@ -7,6 +7,7 @@ from click.testing import CliRunner from sp_cli.client import ApiError +from sp_cli.constants import EXIT_TIMEOUT from sp_cli.main import cli from tests import SESSION_SANDBOX # noqa: F401 @@ -1621,15 +1622,52 @@ def test_wait_treats_unknown_status_as_terminal(self, mock_get): @mock.patch('sp_cli.commands.run.time.monotonic') @mock.patch('sp_cli.client.ApiClient.get') def test_wait_times_out(self, mock_get, mock_clock): - """Passing the deadline exits 2 and reports what was still pending.""" + """Passing the deadline exits 9 and reports what was still pending.""" mock_get.return_value = {'run_id': 9476, 'status': 'queued'} # Start, then a reading past the deadline on the first check. mock_clock.side_effect = [0, 10_000, 10_000] result = self.runner.invoke(cli, ['run', 'wait', '9476', '--timeout', '60']) - self.assertEqual(result.exit_code, 2) + self.assertEqual(result.exit_code, EXIT_TIMEOUT) self.assertIn('timed out', result.stderr) + @mock.patch('sp_cli.client.ApiClient.get') + def test_a_timeout_is_distinguishable_from_a_usage_error(self, mock_get): + """The reason the timeout code is not 2. + + Click exits 2 on any usage error. Sharing that code would make a + mistyped flag indistinguishable from "the runs are still going", so a + script branching on it would sleep and retry a command that never ran. + """ + for argv in (['run', 'wait'], # no run ids + ['run', 'wait', '--nosuchflag'], # unknown option + ['run', 'wait', '9476', '--interval', '1']): # below the minimum + result = self.runner.invoke(cli, argv) + self.assertEqual(result.exit_code, 2, argv) + self.assertNotEqual(result.exit_code, EXIT_TIMEOUT, argv) + mock_get.assert_not_called() + + @mock.patch('sp_cli.client.ApiClient.get') + def test_one_run_is_rendered_as_a_collection_like_every_other_list(self, mock_get): + """The payload shape must not depend on how many ids were passed. + + Returning a bare record for a single run breaks + `sp run wait $IDS | jq '.data[]'` exactly when the list happens to hold + one run -- the same command, working or not by coincidence. + """ + mock_get.return_value = {'run_id': 9476, 'status': 'pass'} + single = json.loads(self.runner.invoke(cli, ['run', 'wait', '9476']).stdout) + + mock_get.side_effect = [{'run_id': 9476, 'status': 'pass'}, + {'run_id': 9477, 'status': 'pass'}] + several = json.loads( + self.runner.invoke(cli, ['run', 'wait', '9476', '9477']).stdout) + + self.assertEqual(single['total'], 1) + self.assertEqual(several['total'], 2) + for payload in (single, several): + self.assertIsInstance(payload['data'], list) + @mock.patch('sp_cli.client.ApiClient.get') def test_wait_surfaces_api_errors(self, mock_get): """An API failure stops the wait rather than retrying forever."""