diff --git a/.github/workflows/cicd.yaml b/.github/workflows/cicd.yaml index 4ae76fb9..de331c94 100644 --- a/.github/workflows/cicd.yaml +++ b/.github/workflows/cicd.yaml @@ -65,6 +65,44 @@ jobs: # This is associated with the "sdk-test-prod" user, credentials on 1password GROUNDLIGHT_API_TOKEN: ${{ secrets.GROUNDLIGHT_API_TOKEN_PROD }} + # Check that the committed `generated/` tree is what `make generate` produces. This gets its own + # job because it is the only test that needs node and java (for openapi-generator-cli) and the + # only one that needs no API token. REQUIRE_OPENAPI_GENERATOR makes the openapi-generator half + # fail rather than skip, so a missing toolchain here can't quietly turn the check into a no-op -- + # this is the one place it reliably runs. + test-codegen: + runs-on: ubuntu-latest + steps: + - name: get code + uses: actions/checkout@v4 + - name: install python + uses: actions/setup-python@v4 + with: + python-version: ${{ env.PYTHON_VERSION }} + - name: install poetry + uses: snok/install-poetry@v1 + with: + version: ${{ env.POETRY_VERSION }} + - name: Setup npm + uses: actions/setup-node@v3 + with: + # Not 21 like the docs jobs: openapi-generator-cli 2.x require()s proxy-agent, which is + # ESM-only, so it dies with ERR_REQUIRE_ESM on any node without require(esm) support. + # Verified 2026-08-04: 18.19.1 and 21.7.3 fail, 20.19.0 and 22.14.0 work. + node-version: 22 + cache: npm + - name: install java (openapi-generator-cli runs a jar) + uses: actions/setup-java@v4 + with: + distribution: temurin + java-version: "17" + - name: install dependencies and the SDK generator + run: make install-generator + - name: check generated/ is up to date + env: + REQUIRE_OPENAPI_GENERATOR: "1" + run: make test-codegen + # Check that the docs build. (No broken links, etc.) test-docs: runs-on: ubuntu-latest diff --git a/.gitignore b/.gitignore index 6be72b11..c5018146 100644 --- a/.gitignore +++ b/.gitignore @@ -162,6 +162,11 @@ cython_debug/ node_modules/ +# Scratch trees written by test_codegen/ while re-running the SDK generators. They live inside +# the repo because datamodel-codegen and black find our black settings by walking up from the +# files they write; the tests clean them up unless a run is killed part-way. +codegen-scratch-*/ + *.swp **/.python-version diff --git a/DEVELOPING.md b/DEVELOPING.md index 43604682..4fc46997 100644 --- a/DEVELOPING.md +++ b/DEVELOPING.md @@ -55,7 +55,9 @@ it will run them, add the changes, and then ask you to try committing again with The auto-generated SDK code is in the [generated/](generated) directory. Most of the time, you won't need to generate code. But if the API specification changes, you may need to generate SDK code. To re-generate the client code, you'll need to [install npm](https://github.com/nvm-sh/nvm#intro) -first. Then you can install the code generator by running: +first, plus a Java runtime (openapi-generator-cli runs a jar). Use node 20.19+ or 22+ — the +generator CLI fails with `ERR_REQUIRE_ESM` on older node, and on node 21. Then you can install the +code generator by running: ```shell make install-generator @@ -67,6 +69,18 @@ Then you can generate the code by running: make generate ``` +Never hand-edit anything under `generated/` — always change the spec (or the generator +invocation) and re-run `make generate`. `make test` enforces this with the checks in +[test_codegen/](test_codegen), which re-run the generators and diff the result against what is +committed; you can run just those checks, without an API token, with: + +```shell +make test-codegen +``` + +The openapi-generator half of that check needs node and java, so it skips (with a reason) if you +have not run `make install-generator`. It always runs in CI, in the `test-codegen` job. + ### Linters Linters help us find issues before runtime. We're currently using: diff --git a/Makefile b/Makefile index 6c5331f3..f52cd38c 100644 --- a/Makefile +++ b/Makefile @@ -1,4 +1,4 @@ -.PHONY: apidocs docs-comprehensive generate html install install-dev install-extras install-generator install-lint install-pre-commit test test-4edge test-integ test-local help +.PHONY: apidocs docs-comprehensive generate html install install-dev install-extras install-generator install-lint install-pre-commit test test-4edge test-codegen test-integ test-local help help: ## Print all targets with their descriptions @grep -E '^[a-zA-Z_-]+:.*$$' $(MAKEFILE_LIST) | awk 'BEGIN {FS = ":.*?## "}; {if (NF == 1) {printf "\033[36m%-30s\033[0m %s\n", $$1, ""} else {printf "\033[36m%-30s\033[0m %s\n", $$1, $$2}}' @@ -46,7 +46,14 @@ PROFILING_ARGS = \ --durations 25 \ --durations-min 0.1 -test: install ## Run tests against the prod API (needs GROUNDLIGHT_API_TOKEN) +# Checks that `generated/` is what `make generate` would produce. Kept out of `test/` (and given +# its own target) because everything under `test/` needs a GROUNDLIGHT_API_TOKEN to even collect, +# and this needs no API access. TEST_ARGS is deliberately not passed through: a filter meant for +# the real suite would match nothing here and fail the run. +test-codegen: install ## Check that generated/ matches what `make generate` produces (no API token needed) + ${PYTEST} test_codegen + +test: install test-codegen ## Run tests against the prod API (needs GROUNDLIGHT_API_TOKEN) ${PYTEST} ${PROFILING_ARGS} ${TEST_ARGS} ${CLOUD_FILTERS} test test-4edge: install ## Run tests against the prod API via the edge-endpoint (needs GROUNDLIGHT_API_TOKEN) @@ -68,7 +75,7 @@ test-docs-integ: install-extras ## Run the example code and tests in our docs a GROUNDLIGHT_ENDPOINT="https://api.integ.groundlight.ai/" ${PYTEST} --markdown-docs ${TEST_ARGS} docs README.md # Adjust which paths we lint -LINT_PATHS="src test bin samples" +LINT_PATHS="src test test_codegen bin samples" lint: install-lint ## Run linter to check formatting and style ./code-quality/lint ${LINT_PATHS} diff --git a/generated/groundlight_openapi_client/model/api_token.py b/generated/groundlight_openapi_client/model/api_token.py index e967be45..dfd9f69d 100644 --- a/generated/groundlight_openapi_client/model/api_token.py +++ b/generated/groundlight_openapi_client/model/api_token.py @@ -140,7 +140,7 @@ def _from_openapi_data(cls, name, raw_key_snippet, created_at, last_used_at, *ar name (str): An nickname for the API token. This name must be unique for this user. raw_key_snippet (str): Since we're storing hashed keys, it can be useful to see the raw prefix snippet of the token. created_at (datetime): When was this token created? - last_used_at (datetime): The most recent time this API token was used. (Helpful for detecting suspicious activity). + last_used_at (datetime, none_type): The most recent time this API token was used. (Helpful for detecting suspicious activity). Null if the token has never been used. Keyword Args: _check_type (bool): if True, values for parameters in openapi_types @@ -174,6 +174,7 @@ def _from_openapi_data(cls, name, raw_key_snippet, created_at, last_used_at, *ar through its discriminator because we passed in _visited_composed_classes = (Animal,) expires_at (datetime, none_type): When does this token expire? If Null, the token never expires.. [optional] # noqa: E501 + token_ttl (int, none_type): Identity token lifetime policy in whole seconds. Null means tokens minted under this identity never expire (no rotation).. [optional] # noqa: E501 """ _check_type = kwargs.pop("_check_type", True) @@ -265,6 +266,7 @@ def __init__(self, name, *args, **kwargs): # noqa: E501 through its discriminator because we passed in _visited_composed_classes = (Animal,) expires_at (datetime, none_type): When does this token expire? If Null, the token never expires.. [optional] # noqa: E501 + token_ttl (int, none_type): Identity token lifetime policy in whole seconds. Null means tokens minted under this identity never expire (no rotation).. [optional] # noqa: E501 """ _check_type = kwargs.pop("_check_type", True) diff --git a/generated/groundlight_openapi_client/model/api_token_create_response.py b/generated/groundlight_openapi_client/model/api_token_create_response.py index 17acd71c..b9270a1a 100644 --- a/generated/groundlight_openapi_client/model/api_token_create_response.py +++ b/generated/groundlight_openapi_client/model/api_token_create_response.py @@ -145,7 +145,7 @@ def _from_openapi_data( name (str): An nickname for the API token. This name must be unique for this user. raw_key_snippet (str): Since we're storing hashed keys, it can be useful to see the raw prefix snippet of the token. created_at (datetime): When was this token created? - last_used_at (datetime): The most recent time this API token was used. (Helpful for detecting suspicious activity). + last_used_at (datetime, none_type): The most recent time this API token was used. (Helpful for detecting suspicious activity). Null if the token has never been used. raw_key (str): The full API token secret. Returned only once, when the token is created. Keyword Args: @@ -180,6 +180,7 @@ def _from_openapi_data( through its discriminator because we passed in _visited_composed_classes = (Animal,) expires_at (datetime, none_type): When does this token expire? If Null, the token never expires.. [optional] # noqa: E501 + token_ttl (int, none_type): Identity token lifetime policy in whole seconds. Null means tokens minted under this identity never expire (no rotation).. [optional] # noqa: E501 """ _check_type = kwargs.pop("_check_type", True) @@ -272,6 +273,7 @@ def __init__(self, name, *args, **kwargs): # noqa: E501 through its discriminator because we passed in _visited_composed_classes = (Animal,) expires_at (datetime, none_type): When does this token expire? If Null, the token never expires.. [optional] # noqa: E501 + token_ttl (int, none_type): Identity token lifetime policy in whole seconds. Null means tokens minted under this identity never expire (no rotation).. [optional] # noqa: E501 """ _check_type = kwargs.pop("_check_type", True) diff --git a/generated/model.py b/generated/model.py index 4659a06e..6d86a74e 100644 --- a/generated/model.py +++ b/generated/model.py @@ -1,6 +1,6 @@ # generated by datamodel-codegen: # filename: public-api.yaml -# timestamp: 2026-07-10T01:01:06+00:00 +# timestamp: 2026-08-03T21:13:28+00:00 from __future__ import annotations @@ -38,14 +38,20 @@ class ApiToken(BaseModel): created_at: datetime = Field(..., description="When was this token created?") last_used_at: Optional[datetime] = Field( ..., - description="The most recent time this API token was used. (Helpful for detecting suspicious activity). Null if the token has never been used.", + description=( + "The most recent time this API token was used. (Helpful for detecting suspicious activity). Null if the" + " token has never been used." + ), ) expires_at: Optional[datetime] = Field( None, description="When does this token expire? If Null, the token never expires." ) token_ttl: Optional[int] = Field( None, - description="Identity token lifetime policy in whole seconds. Null means tokens minted under this identity never expire (no rotation).", + description=( + "Identity token lifetime policy in whole seconds. Null means tokens minted under this identity never expire" + " (no rotation)." + ), ) @@ -64,14 +70,20 @@ class ApiTokenCreateResponse(BaseModel): created_at: datetime = Field(..., description="When was this token created?") last_used_at: Optional[datetime] = Field( ..., - description="The most recent time this API token was used. (Helpful for detecting suspicious activity). Null if the token has never been used.", + description=( + "The most recent time this API token was used. (Helpful for detecting suspicious activity). Null if the" + " token has never been used." + ), ) expires_at: Optional[datetime] = Field( None, description="When does this token expire? If Null, the token never expires." ) token_ttl: Optional[int] = Field( None, - description="Identity token lifetime policy in whole seconds. Null means tokens minted under this identity never expire (no rotation).", + description=( + "Identity token lifetime policy in whole seconds. Null means tokens minted under this identity never expire" + " (no rotation)." + ), ) raw_key: str = Field(..., description="The full API token secret. Returned only once, when the token is created.") diff --git a/test_codegen/test_generated_matches_generator.py b/test_codegen/test_generated_matches_generator.py new file mode 100644 index 00000000..3fe0c107 --- /dev/null +++ b/test_codegen/test_generated_matches_generator.py @@ -0,0 +1,240 @@ +"""Checks that the committed `generated/` tree is exactly what `make generate` produces. + +`generated/` is excluded from linting (see `LINT_PATHS` in the Makefile), so nothing in CI +notices when generated code is hand-edited instead of regenerated. Nothing is usually +functionally wrong when that happens -- the cost is that the next person to run +`make generate` gets a large unexplained diff mixed into their own PR. These tests re-run +the generators into a scratch tree and diff the result against what is committed. + +They live outside `test/` on purpose: `test/conftest.py` constructs a `Groundlight()` client +in `pytest_configure`, so everything under `test/` needs a GROUNDLIGHT_API_TOKEN before +collection even starts. These checks need no API access, and putting them where a token is +required would mean they never ran. +""" + +import difflib +import os +import re +import shlex +import shutil +import subprocess +import sys +import tempfile +from pathlib import Path +from typing import Dict, Iterator, List, Optional + +import pytest + +REPO_ROOT = Path(__file__).resolve().parent.parent +GENERATED_DIR = REPO_ROOT / "generated" +MAKEFILE = REPO_ROOT / "Makefile" +OPENAPI_GENERATOR_CLI = REPO_ROOT / "node_modules" / ".bin" / "openapi-generator-cli" + +# Set this (CI does) to turn "openapi-generator-cli is not available here" from a skip into a +# failure. A check that quietly skips everywhere is worse than no check, because it reads as +# coverage. +REQUIRE_GENERATOR_ENV_VAR = "REQUIRE_OPENAPI_GENERATOR" + +FIX_HINT = "Run `make generate` and commit the result, rather than hand-editing `generated/`." + +# datamodel-codegen stamps the time of the run into its file header, so this one line +# legitimately differs on every run and has to be normalized away. +CODEGEN_TIMESTAMP_RE = re.compile(r"^#\s+timestamp:.*$", re.MULTILINE) + +# Failure messages carry the real diff, but a whole-tree regeneration diff can be enormous. +MAX_DIFF_LINES = 400 + + +def _generate_recipe_commands() -> List[str]: + """The shell commands in the Makefile's `generate` target, with continuations joined. + + These are parsed out of the Makefile rather than duplicated here so that changing how the + SDK is generated cannot leave these tests silently checking the old way of generating it. + """ + lines = MAKEFILE.read_text().splitlines() + starts = [i for i, line in enumerate(lines) if re.match(r"^generate:", line)] + assert len(starts) == 1, f"expected exactly one `generate:` target in {MAKEFILE}, found {len(starts)}" + + commands: List[str] = [] + for line in lines[starts[0] + 1 :]: + if line.startswith("#"): # a comment column-0 inside a recipe does not end it + continue + if not line.startswith("\t"): # first non-recipe line ends the recipe + break + body = line.lstrip("\t").strip() + if commands and commands[-1].endswith("\\"): + commands[-1] = f"{commands[-1][:-1].strip()} {body}" + else: + commands.append(body) + assert commands, f"found no commands in the `generate` target of {MAKEFILE}" + return commands + + +def _recipe_command(tool: str) -> List[str]: + """The `make generate` command line that runs `tool`, split into argv.""" + matches = [command for command in _generate_recipe_commands() if tool in command] + assert len(matches) == 1, f"expected exactly one `{tool}` command in `make generate`, found {matches}" + return shlex.split(matches[0]) + + +def _retarget(command: List[str], old: str, new: str) -> List[str]: + """Point a generator command at the scratch tree instead of the committed one.""" + assert old in command, f"expected `{old}` as an argument in `make generate`'s command: {command}" + return [new if argument == old else argument for argument in command] + + +def _venv_command(command: List[str]) -> List[str]: + """Rewrite `poetry run ...` to call `` from the environment running these tests. + + We are already inside the poetry environment, so this runs the same versions `make generate` + would while avoiding a nested `poetry run`. + """ + if command[:2] == ["poetry", "run"]: + command = command[2:] + executable = Path(sys.executable).parent / command[0] + if not executable.exists(): + found = shutil.which(command[0]) + assert found, f"`{command[0]}` is not installed in this environment -- run `make install`" + executable = Path(found) + return [str(executable), *command[1:]] + + +def _run(command: List[str], what: str) -> None: + result = subprocess.run(command, cwd=REPO_ROOT, capture_output=True, text=True, check=False) + if result.returncode != 0: + pytest.fail( + f"Could not run {what} (exit {result.returncode}), so we cannot tell whether `generated/` is" + f" up to date.\ncommand: {shlex.join(command)}\nstdout:\n{result.stdout}\nstderr:\n{result.stderr}" + ) + + +def _run_black(paths: List[Path]) -> None: + """Format `paths` the way `make generate`'s final `black .` would.""" + if not paths: + return + command = _venv_command(_recipe_command("black")) + assert command[-1] == ".", f"expected `make generate` to end with `black .`, got {command}" + # Files are passed explicitly instead of handing black the scratch directory: black skips + # gitignored paths when it walks a directory, and the scratch directory is gitignored. + _run([*command[:-1], *(str(path) for path in paths)], "black") + + +@pytest.fixture(name="scratch_dir") +def fixture_scratch_dir() -> Iterator[Path]: + """A scratch directory for generator output, inside the repo. + + It has to be inside the repo: both datamodel-codegen (which formats its own output) and + black find the project's `[tool.black]` settings by walking up from the paths they write, so + generating into /tmp silently produces black's default 88-column style and every comparison + below fails for the wrong reason. + """ + with tempfile.TemporaryDirectory(dir=REPO_ROOT, prefix="codegen-scratch-") as scratch: + yield Path(scratch) + + +def _normalize(source: str) -> str: + return CODEGEN_TIMESTAMP_RE.sub("# timestamp: ", source) + + +def _file_diff(relative_path: str, committed: str, regenerated: str) -> List[str]: + return list( + difflib.unified_diff( + committed.splitlines(), + regenerated.splitlines(), + fromfile=f"generated/{relative_path} (committed)", + tofile=f"generated/{relative_path} (freshly generated)", + lineterm="", + ) + ) + + +def _truncated(diff_lines: List[str]) -> str: + if len(diff_lines) <= MAX_DIFF_LINES: + return "\n".join(diff_lines) + hidden = len(diff_lines) - MAX_DIFF_LINES + return "\n".join([*diff_lines[:MAX_DIFF_LINES], f"... {hidden} more diff lines suppressed ..."]) + + +def _tree_contents(root: Path) -> Dict[str, str]: + return { + path.relative_to(root).as_posix(): path.read_text(errors="replace") + for path in root.rglob("*") + if path.is_file() and "__pycache__" not in path.parts + } + + +def test_generated_model_matches_datamodel_codegen(scratch_dir: Path) -> None: + """`generated/model.py` must be exactly what `make generate`'s datamodel-codegen call writes. + + This half of the generator is pure Python and already a project dependency, so it always runs. + """ + regenerated = scratch_dir / "model.py" + command = _retarget(_venv_command(_recipe_command("datamodel-codegen")), "generated/model.py", str(regenerated)) + _run(command, "datamodel-codegen") + _run_black([regenerated]) + + committed_text = _normalize((GENERATED_DIR / "model.py").read_text()) + regenerated_text = _normalize(regenerated.read_text()) + if committed_text != regenerated_text: + diff = _truncated(_file_diff("model.py", committed_text, regenerated_text)) + pytest.fail( + "generated/model.py is not what datamodel-codegen produces -- it looks hand-edited." + f" {FIX_HINT}\n(The `timestamp:` header line is normalized out, so it is not the" + f" cause of this diff.)\n\n{diff}" + ) + + +def _openapi_generator_unavailable_reason() -> Optional[str]: + """Why the openapi-generator half cannot run here, or None if it can.""" + if not shutil.which("node"): + return "node is not installed, and openapi-generator-cli needs it" + if not OPENAPI_GENERATOR_CLI.exists(): + return f"{OPENAPI_GENERATOR_CLI.relative_to(REPO_ROOT)} is missing -- run `make install-generator`" + if not shutil.which("java"): + return "openapi-generator-cli runs a Java jar, and `java` is not on PATH" + return None + + +def test_generated_client_matches_openapi_generator(scratch_dir: Path) -> None: + """Everything openapi-generator owns under `generated/` must match a fresh run. + + Skipped when node/java/the generator CLI are unavailable, since neither is guaranteed on a + contributor's machine -- but CI sets REQUIRE_OPENAPI_GENERATOR so that it cannot skip there. + """ + reason = _openapi_generator_unavailable_reason() + if reason: + if os.environ.get(REQUIRE_GENERATOR_ENV_VAR): + pytest.fail(f"{REQUIRE_GENERATOR_ENV_VAR} is set, so this check is not allowed to skip here, but {reason}.") + pytest.skip(f"{reason}. (Set {REQUIRE_GENERATOR_ENV_VAR}=1 to make this a failure instead of a skip.)") + + # Regenerate over a *copy* of the committed tree rather than into an empty directory: + # openapi-generator leaves already-existing files alone (the per-model stubs under + # generated/test/, for instance) and records only the files it wrote in + # .openapi-generator/FILES, so generating from empty yields a legitimately different + # manifest. Copying first reproduces what `make generate` actually does. + regenerated_dir = scratch_dir / "generated" + shutil.copytree(GENERATED_DIR, regenerated_dir, ignore=shutil.ignore_patterns("__pycache__")) + _run(_retarget(_recipe_command("openapi-generator-cli"), "./generated", str(regenerated_dir)), "openapi-generator") + _run_black(sorted(regenerated_dir.rglob("*.py"))) + + committed = _tree_contents(GENERATED_DIR) + regenerated = _tree_contents(regenerated_dir) + # model.py comes from datamodel-codegen, not from openapi-generator; the test above owns it. + committed.pop("model.py", None) + regenerated.pop("model.py", None) + + missing = sorted(set(regenerated) - set(committed)) + differing = sorted(path for path in regenerated if path in committed and committed[path] != regenerated[path]) + if not missing and not differing: + return + + report = ["generated/ is not what openapi-generator produces -- it looks hand-edited.", FIX_HINT, ""] + if missing: + report += ["Files the generator produces that are not committed:", *(f" generated/{p}" for p in missing), ""] + if differing: + report += ["Committed files that differ from a fresh run:", *(f" generated/{p}" for p in differing), ""] + diff_lines: List[str] = [] + for relative_path in differing: + diff_lines += _file_diff(relative_path, committed[relative_path], regenerated[relative_path]) + report.append(_truncated(diff_lines)) + pytest.fail("\n".join(report))