From 51d3406120a86f2f43ed1aa3152893f53d9e861f Mon Sep 17 00:00:00 2001 From: Aaron Gonzales Date: Fri, 16 Oct 2026 14:16:54 +0000 Subject: [PATCH] fix: make health preflight outcomes explicit and validated Signed-off-by: Aaron Gonzales --- .github/workflows/health-checks.yml | 12 +- CONTRIBUTING.md | 6 + Makefile | 44 ++----- docs/engineering/health-checks.md | 48 +++++++ scripts/check_health.py | 131 +++++++++++++++++++ tests/test_check_health.py | 189 ++++++++++++++++++++++++++++ 6 files changed, 386 insertions(+), 44 deletions(-) create mode 100644 docs/engineering/health-checks.md create mode 100644 scripts/check_health.py create mode 100644 tests/test_check_health.py diff --git a/.github/workflows/health-checks.yml b/.github/workflows/health-checks.yml index 501e17c..aabbeb5 100644 --- a/.github/workflows/health-checks.yml +++ b/.github/workflows/health-checks.yml @@ -37,7 +37,7 @@ jobs: name: ${{ matrix.provider }} end to end runs-on: ubuntu-latest # A provider outage should not hide the state of the other two. - continue-on-error: true + timeout-minutes: 50 strategy: fail-fast: false matrix: @@ -93,6 +93,7 @@ jobs: make health-check \ PROVIDER_CONFIG="$PROVIDER_CONFIG" \ HEALTH_LOCALE="$HEALTH_LOCALE" \ + HEALTH_SUMMARY_FILE=/tmp/usersim-health-summary.md \ | tee /tmp/health.log - name: Summary @@ -100,16 +101,13 @@ jobs: env: PROVIDER: ${{ matrix.provider }} LOCALE: ${{ matrix.locale }} - SECRET_NAME: ${{ matrix.secret_name }} run: | { echo "### $PROVIDER ($LOCALE)" echo - if grep -q '^SKIP' /tmp/health.log; then - echo "Skipped: repository secret \`$SECRET_NAME\` is not set." + if test -f /tmp/usersim-health-summary.md; then + cat /tmp/usersim-health-summary.md else - echo '```' - tail -20 /tmp/health.log || echo "(no output captured)" - echo '```' + echo "**failure** (setup): health check did not produce an outcome." fi } >> "$GITHUB_STEP_SUMMARY" diff --git a/CONTRIBUTING.md b/CONTRIBUTING.md index df53ede..e951c9c 100644 --- a/CONTRIBUTING.md +++ b/CONTRIBUTING.md @@ -352,3 +352,9 @@ not**. See [SECURITY.md](SECURITY.md) for the private disclosure channels. ## Code of conduct Participation is governed by the [Contributor Covenant](CODE_OF_CONDUCT.md). + +## Live provider checks + +See [Live provider health checks](docs/engineering/health-checks.md) for credential +precedence, local commands, and the pass, skip, and failure outcomes. These checks +make real model requests and are separate from offline contributor validation. diff --git a/Makefile b/Makefile index 7631be6..bb54d64 100644 --- a/Makefile +++ b/Makefile @@ -243,45 +243,15 @@ PROVIDER_CONFIG ?= src/usersim/cli/models_default.toml HEALTH_LOCALE ?= ja_JP HEALTH_PROBE ?= sov_ai_dynamic -# A provider you have no key for is skipped rather than failed. Without this -# the target is red for everyone who has one key, and the scheduled workflow -# is red until every secret is provisioned, which trains people to ignore it. -define REQUIRE_KEYS_PY -import sys -from usersim.cli._models import load_models_config, required_api_key_env_vars -config = sys.argv[1] -missing = sorted(v for v in required_api_key_env_vars(load_models_config(config)) if not __import__("os").environ.get(v)) -if missing: - print(f"SKIP {config}: {', '.join(missing)} not set") - sys.exit(1) -endef -export REQUIRE_KEYS_PY +# Only missing credentials skip; configuration and execution errors fail. +HEALTH_TIMEOUT ?= 900 +HEALTH_SUMMARY_FILE ?= health-check: ## Live one-row simulate + eval + report against a provider - @set -eu; \ - if ! $(UV) run python -c "$$REQUIRE_KEYS_PY" "$(PROVIDER_CONFIG)"; then exit 0; fi; \ - OUT=$$(mktemp -d); \ - trap 'rm -rf "$$OUT"' EXIT; \ - echo "==> simulate ($(HEALTH_LOCALE), $(PROVIDER_CONFIG))"; \ - $(UV) run usersim simulate \ - --locale $(HEALTH_LOCALE) --num-rows 1 --max-turns 2 \ - --probe-mix "$(HEALTH_PROBE)=1.0" \ - --models $(PROVIDER_CONFIG) \ - --out "$$OUT/trajectories"; \ - RUN=$$(basename $$(ls -d "$$OUT"/trajectories/run=* | head -1) | cut -d= -f2); \ - echo "==> eval (run $$RUN)"; \ - $(UV) run usersim eval --run "$$RUN" \ - --trajectories "$$OUT/trajectories" \ - --models $(PROVIDER_CONFIG) \ - --out "$$OUT/evaluations"; \ - echo "==> report (run $$RUN)"; \ - $(UV) run usersim report --run "$$RUN" \ - --trajectories "$$OUT/trajectories" \ - --evaluations "$$OUT/evaluations" \ - --out "$$OUT/report"; \ - test -f "$$OUT/report/run=$$RUN/index.html" \ - || { echo "report produced no index.html"; exit 1; }; \ - echo "✓ simulate, eval and report all completed" + @$(UV) run python scripts/check_health.py \ + --models "$(PROVIDER_CONFIG)" --locale "$(HEALTH_LOCALE)" \ + --probe "$(HEALTH_PROBE)" --timeout "$(HEALTH_TIMEOUT)" \ + $(if $(HEALTH_SUMMARY_FILE),--summary-file "$(HEALTH_SUMMARY_FILE)") check-extensions: ## Install an out-of-tree package and prove all six seams work @set -eu; \ diff --git a/docs/engineering/health-checks.md b/docs/engineering/health-checks.md new file mode 100644 index 0000000..8a09762 --- /dev/null +++ b/docs/engineering/health-checks.md @@ -0,0 +1,48 @@ +# Live provider health checks + +Run `make health-check` to verify that a real provider completes simulation, +evaluation, and HTML reporting. This check makes paid model requests and runs +separately from the offline test suite. + +```bash +make health-check PROVIDER_CONFIG=src/usersim/cli/models_openai.toml HEALTH_LOCALE=ko_KR +``` + +Credentials follow the CLI's precedence: exported environment variables win, +then `.env.local`, then `.env`. The check searches from the working directory +upward, using the nearest directory containing either env file. Keep these +files private and untracked. Custom providers declare the credential variable +name through `api_key` in their model configuration. + +The helper prints a final JSON record with `status`, `stage`, and `detail`: + +| Status | Meaning | Exit status | +| --- | --- | --- | +| `pass` | Simulation, evaluation, and reporting completed; the report exists. | 0 | +| `skip` | Valid configuration needs credentials that are absent or empty. | 0 | +| `failure` | Configuration, dependencies, execution, timeout, or expected outputs failed. | 1 | + +A skip does not establish provider health. Invalid or missing model files and +missing required aliases fail before checking credentials. Model parameters are +validated through the CLI's existing configuration conversion before a missing +credential can produce a skip. For example, zero `timeout` or +`max_parallel_requests` fails without starting a provider client or pipeline. +When a command fails, inspect its output and the reported stage; the helper +does not print credential values or dump subprocess arguments in its final +record. + +Each command has a 900-second timeout. Override it with `HEALTH_TIMEOUT` in +seconds. The check uses one row and at most two turns, with `HEALTH_PROBE` +defaulting to `sov_ai_dynamic`. Temporary run artifacts are removed on exit. +Use the CLI commands directly with explicit output paths when investigating +artifacts from a failed run. + +The scheduled/manual `Health checks` workflow runs each provider independently +with only that provider's secret. Provider failures fail their jobs; missing +credentials appear as explicit skips in the summary. Setup failures without a +helper outcome also appear as failures. Pull requests never receive these +credentials. For a local summary, set `HEALTH_SUMMARY_FILE` to an output path. + +Regression tests in `tests/test_check_health.py` exercise credential precedence, +invalid configuration, skipped checks, command failures, timeouts, and missing +outputs without calling a live provider. diff --git a/scripts/check_health.py b/scripts/check_health.py new file mode 100644 index 0000000..1f210bd --- /dev/null +++ b/scripts/check_health.py @@ -0,0 +1,131 @@ +# SPDX-FileCopyrightText: Copyright (c) 2026 NVIDIA CORPORATION & AFFILIATES. All rights reserved. +# SPDX-License-Identifier: Apache-2.0 + +"""Run the live CLI pipeline, skipping only absent provider credentials.""" + +from __future__ import annotations + +import argparse +import json +import subprocess +import sys +import tempfile +from pathlib import Path + + +def preflight(models: Path) -> list[str]: + # Keep imports within the reported boundary: a broken installation fails, + # rather than masquerading as an unconfigured provider. + import dotenv # noqa: F401 -- required by load_local_env, which tolerates its absence + + from usersim.cli._env import load_local_env, missing_keys + from usersim.cli._models import ( + REQUIRED_EVALUATOR_ALIASES, + REQUIRED_SIMULATOR_ALIASES, + assistant_replays_reasoning, + load_models_config, + require_aliases, + required_api_key_env_vars, + to_model_configs, + ) + + load_local_env() + config = load_models_config(models) + require_aliases( + config, + required=REQUIRED_SIMULATOR_ALIASES + REQUIRED_EVALUATOR_ALIASES, + context="health check", + ) + assistant_replays_reasoning(config, store_reasoning=True) + # Validate model parameters through the same offline conversion as the CLI. + to_model_configs(config) + return sorted(missing_keys(required_api_key_env_vars(config))) + + +def run_stage(stage: str, arguments: list[str], timeout: float) -> None: + print(f"==> {stage}", flush=True) + subprocess.run([sys.executable, "-m", "usersim.cli", stage, *arguments], check=True, timeout=timeout) + + +def pipeline(args: argparse.Namespace, output: Path, outcome: dict[str, str]) -> None: + trajectories = str(output / "trajectories") + evaluations = str(output / "evaluations") + outcome["stage"] = "simulate" + run_stage( + "simulate", + [ + "--locale", + args.locale, + "--num-rows", + "1", + "--max-turns", + "2", + "--probe-mix", + f"{args.probe}=1.0", + "--models", + str(args.models), + "--out", + trajectories, + ], + args.timeout, + ) + runs = sorted(path for path in Path(trajectories).glob("run=*") if path.is_dir()) + if len(runs) != 1: + raise RuntimeError("simulate must produce exactly one run directory") + run = runs[0].name.removeprefix("run=") + outcome["stage"] = "eval" + run_stage( + "eval", + ["--run", run, "--trajectories", trajectories, "--models", str(args.models), "--out", evaluations], + args.timeout, + ) + outcome["stage"] = "report" + run_stage( + "report", + ["--run", run, "--trajectories", trajectories, "--evaluations", evaluations, "--out", str(output / "report")], + args.timeout, + ) + if not (output / "report" / f"run={run}" / "index.html").is_file(): + raise RuntimeError("report produced no index.html") + + +def error_detail(error: Exception) -> str: + # Avoid printing subprocess arguments or credential-bearing exception text. + if isinstance(error, subprocess.CalledProcessError): + return f"command exited with status {error.returncode}" + if isinstance(error, subprocess.TimeoutExpired): + return f"command exceeded {error.timeout:g} seconds" + if isinstance(error, RuntimeError): + return str(error) + return f"{type(error).__name__}; check configuration, dependencies and installation" + + +def main(argv: list[str] | None = None) -> int: + parser = argparse.ArgumentParser(description=__doc__) + parser.add_argument("--models", type=Path, required=True) + parser.add_argument("--locale", default="ja_JP") + parser.add_argument("--probe", default="sov_ai_dynamic") + parser.add_argument("--timeout", type=float, default=900, help="maximum seconds per CLI command (default: 900)") + parser.add_argument("--summary-file", type=Path) + args = parser.parse_args(argv) + outcome = {"status": "failure", "stage": "preflight", "detail": ""} + try: + if args.timeout <= 0 or not args.timeout < float("inf"): + raise ValueError("timeout must be finite and positive") + missing = preflight(args.models) + if missing: + outcome.update(status="skip", detail=f"missing credentials: {', '.join(missing)}") + else: + with tempfile.TemporaryDirectory(prefix="usersim-health-") as temporary: + pipeline(args, Path(temporary), outcome) + outcome.update(status="pass", stage="complete", detail="simulate, eval and report completed") + except Exception as error: + outcome["detail"] = error_detail(error) + print(json.dumps(outcome), flush=True) + if args.summary_file: + args.summary_file.write_text(f"**{outcome['status']}** ({outcome['stage']}): {outcome['detail']}\n") + return int(outcome["status"] == "failure") + + +if __name__ == "__main__": + raise SystemExit(main()) diff --git a/tests/test_check_health.py b/tests/test_check_health.py new file mode 100644 index 0000000..a8dea1b --- /dev/null +++ b/tests/test_check_health.py @@ -0,0 +1,189 @@ +# SPDX-FileCopyrightText: Copyright (c) 2026 NVIDIA CORPORATION & AFFILIATES. All rights reserved. +# SPDX-License-Identifier: Apache-2.0 + +"""Health checks skip only missing credentials and fail on execution errors.""" + +from __future__ import annotations + +import importlib.util +import json +import subprocess +from pathlib import Path + +import pytest + +_SCRIPT = Path(__file__).resolve().parents[1] / "scripts" / "check_health.py" +_spec = importlib.util.spec_from_file_location("check_health", _SCRIPT) +assert _spec and _spec.loader +health = importlib.util.module_from_spec(_spec) +_spec.loader.exec_module(health) + + +@pytest.fixture +def config(tmp_path, monkeypatch): + monkeypatch.chdir(tmp_path) + monkeypatch.delenv("HEALTH_TEST_KEY", raising=False) + path = tmp_path / "models.toml" + path.write_text( + '[[providers]]\nname="health-test"\nendpoint="http://localhost/v1"\napi_key="HEALTH_TEST_KEY"\n' + + "".join( + f'[[models]]\nalias="{alias}"\nmodel="test"\nprovider="health-test"\n' + for alias in ("user_model", "assistant_model", "judge_model", "evaluator_model") + ) + ) + return path + + +def invoke(config, capsys, *args): + code = health.main(["--models", str(config), *args]) + outcome = json.loads(capsys.readouterr().out.splitlines()[-1]) + return code, outcome + + +@pytest.mark.parametrize("content", [None, "[[models]", '[[models]]\nalias="user_model"']) +def test_invalid_config_fails_before_missing_credentials(config, capsys, content): + if content is None: + config.unlink() + else: + config.write_text(content) + code, outcome = invoke(config, capsys) + assert code == 1 + assert outcome["status"] == "failure" + assert outcome["stage"] == "preflight" + + +@pytest.mark.parametrize("alias", ["user_model", "judge_model", "evaluator_model"]) +def test_invalid_replay_setting_is_not_a_credential_skip(config, capsys, alias): + config.write_text(config.read_text().replace(f'alias="{alias}"', f'alias="{alias}"\nreplay_reasoning=true')) + code, outcome = invoke(config, capsys) + assert code == 1 + assert outcome["status"] == "failure" + assert outcome["stage"] == "preflight" + + +@pytest.mark.parametrize( + "alias,parameter", [("assistant_model", "timeout"), ("evaluator_model", "max_parallel_requests")] +) +def test_invalid_model_parameters_fail_before_missing_credentials(config, capsys, monkeypatch, parameter, alias): + def unexpected(*args, **kwargs): + pytest.fail("preflight must not execute the provider pipeline") + + monkeypatch.setattr(health, "pipeline", unexpected) + monkeypatch.setattr(health.subprocess, "run", unexpected) + config.write_text(config.read_text().replace(f'alias="{alias}"', f'alias="{alias}"\n{parameter}=0')) + code, outcome = invoke(config, capsys) + assert code == 1 + assert outcome["status"] == "failure" + assert outcome["stage"] == "preflight" + + +def test_missing_credential_is_explicit_skip(config, capsys, monkeypatch, tmp_path): + def unexpected(*args, **kwargs): + pytest.fail("missing credentials must not invoke the provider") + + monkeypatch.setattr(health.subprocess, "run", unexpected) + summary = tmp_path / "summary.md" + code, outcome = invoke(config, capsys, "--summary-file", str(summary)) + assert code == 0 + assert outcome["status"] == "skip" + assert "HEALTH_TEST_KEY" in outcome["detail"] + assert "skip" in summary.read_text() + + +@pytest.mark.parametrize("exported,expected", [(None, "local-key"), ("shell-key", "shell-key")]) +def test_env_local_precedence_and_complete_pipeline(config, capsys, monkeypatch, tmp_path, exported, expected): + (tmp_path / ".env.local").write_text("HEALTH_TEST_KEY=local-key\n") + (tmp_path / ".env").write_text("HEALTH_TEST_KEY=base-key\n") + if exported: + monkeypatch.setenv("HEALTH_TEST_KEY", exported) + seen = [] + + def command(argv, *, check, timeout): + import os + import sys + + assert argv[:3] == [sys.executable, "-m", "usersim.cli"] + assert os.environ["HEALTH_TEST_KEY"] == expected + assert timeout == 17 + seen.append(argv[3]) + out = Path(argv[argv.index("--out") + 1]) + if argv[3] == "simulate": + (out / "run=test-run").mkdir(parents=True) + elif argv[3] == "report": + report = out / "run=test-run" / "index.html" + report.parent.mkdir(parents=True) + report.write_text("report") + return subprocess.CompletedProcess(argv, 0) + + monkeypatch.setattr(health.subprocess, "run", command) + code, outcome = invoke(config, capsys, "--timeout", "17") + assert code == 0 + assert outcome["status"] == "pass" + assert seen == ["simulate", "eval", "report"] + assert expected not in json.dumps(outcome) + + +@pytest.mark.parametrize( + "stage,error", + [ + ("simulate", "nonzero"), + ("eval", "nonzero"), + ("report", "nonzero"), + ("simulate", "timeout"), + ("simulate", "missing-tool"), + ], +) +def test_command_errors_fail(config, capsys, monkeypatch, stage, error): + monkeypatch.setenv("HEALTH_TEST_KEY", "key") + + def command(argv, **kwargs): + if argv[3] == stage: + if error == "timeout": + raise subprocess.TimeoutExpired(argv, kwargs["timeout"]) + if error == "missing-tool": + raise FileNotFoundError("usersim") + raise subprocess.CalledProcessError(9, argv) + if argv[3] == "simulate": + (Path(argv[argv.index("--out") + 1]) / "run=test-run").mkdir(parents=True) + + monkeypatch.setattr(health.subprocess, "run", command) + code, outcome = invoke(config, capsys) + assert code == 1 + assert outcome["status"] == "failure" + assert outcome["stage"] == stage + + +@pytest.mark.parametrize("stage", ["simulate", "report"]) +def test_successful_commands_must_produce_outputs(config, capsys, monkeypatch, stage): + monkeypatch.setenv("HEALTH_TEST_KEY", "key") + + def command(argv, **kwargs): + if argv[3] == "simulate" and stage != "simulate": + (Path(argv[argv.index("--out") + 1]) / "run=test-run").mkdir(parents=True) + + monkeypatch.setattr(health.subprocess, "run", command) + code, outcome = invoke(config, capsys) + assert code == 1 + assert outcome["status"] == "failure" + assert outcome["stage"] == stage + + +def test_import_failure_is_not_a_skip(config, capsys, monkeypatch): + import sys + + monkeypatch.setitem(sys.modules, "dotenv", None) + code, outcome = invoke(config, capsys) + assert code == 1 + assert outcome["status"] == "failure" + assert "ModuleNotFoundError" in outcome["detail"] + + +def test_workflow_preserves_failures(): + import yaml + + workflow = yaml.safe_load((_SCRIPT.parents[1] / ".github/workflows/health-checks.yml").read_text()) + job = workflow["jobs"]["live"] + assert not job.get("continue-on-error", False) + assert job["strategy"]["fail-fast"] is False + assert all(not step.get("continue-on-error", False) for step in job["steps"]) + assert "pipefail" in workflow["defaults"]["run"]["shell"].split()