diff --git a/CLAUDE.md b/CLAUDE.md index e5f2eed3..7a15a561 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -173,6 +173,8 @@ All three inherit from `BaseHttpClient` (`http_base.py`) which provides shared r ## Coding Conventions +> **0. (BINDING) Follow [CONTRIBUTING.md](CONTRIBUTING.md) in full.** Every code change -- human or AI agent -- must satisfy the rules in `CONTRIBUTING.md`. Specifically, the "Code Quality Patterns" section is non-negotiable: dataclasses (not bare tuples) for multi-value returns; categorical arguments before variable ones; `ErrorCode` enum (never raw strings); file-size budgets; context managers over lambdas; named functions over assigned anonymous functions; `ty` clean for new code. The `.claude/settings.json` post-edit hooks run `ruff check --fix`, `ruff format`, and `ty check` after every edit -- when an AI agent edits a file in this repo, those checks fire automatically and any failure must be addressed before continuing. If a rule conflicts with an existing pattern in legacy code, **fix it in the PR you are touching** or open a follow-up issue; do not propagate the pattern. + 1. **Typer commands** are thin - they parse arguments, call a service, and format output. No business logic in commands. 2. **Services** receive `ConfigStore` and a `client_factory` callable via dependency injection. This enables easy testing with mocks. diff --git a/CONTRIBUTING.md b/CONTRIBUTING.md index 4c0ac4fc..ba2394a5 100644 --- a/CONTRIBUTING.md +++ b/CONTRIBUTING.md @@ -112,6 +112,146 @@ time.sleep(POLL_INTERVAL) if retries > MAX_RETRIES: ``` +## Code Quality Patterns + +These are the *signal* patterns that distinguish hand-written quality code +from LLM-generated boilerplate. Every PR -- human or AI -- must adhere. +The `/kbagent:review` agent checks for these; the post-edit hooks in +`.claude/settings.json` run `ruff` + `ty` after every file write so drift is +caught immediately. + +### Return values -- name them with dataclasses, not tuples + +**Single value**: name the function after what it returns (`get_user_id`, `count_active_jobs`). + +**Multiple values**: return a `@dataclass` (or `NamedTuple`/`BaseModel`) -- never a bare tuple beyond two values, and even two-element tuples should use a dataclass when the values are semantically distinct. Docstrings rot; dataclass field names do not. + +```python +# BAD -- caller has to remember positional meaning +def resolve_project(alias: str | None) -> tuple[str, ProjectConfig]: + ... + +resolved_alias, project = resolve_project(alias) # which is which? + +# GOOD -- self-documenting at every call site +@dataclass(frozen=True) +class ResolvedProject: + alias: str + config: ProjectConfig + +def resolve_project(alias: str | None) -> ResolvedProject: + ... + +resolved = resolve_project(alias) +resolved.alias, resolved.config # unambiguous +``` + +Migration note: existing `tuple[...]` returns in services are grandfathered, but **do not add new ones**. When you touch one for an unrelated reason and the surface is small, convert it. + +### Argument order -- stable first, variable last + +Put **categorical / constant** arguments (error code, type, mode flag) BEFORE **dynamic / contextual** arguments (message text, payload). This matches the convention LLMs are trained on (most Python stdlib follows it: `logging.log(level, msg)`, `raise SomeError(code, message)`), so models will get the call sites right by default. + +```python +# BAD -- LLMs will guess the order wrong +formatter.error(message="Bucket not found", error_code=ErrorCode.NOT_FOUND) + +# GOOD -- category first, then the variable part +formatter.error(error_code=ErrorCode.NOT_FOUND, message="Bucket not found") + +def log_failure(error_code: ErrorCode, message: str) -> None: ... +def raise_api_error(error_code: ErrorCode, *, message: str, status: int) -> None: ... +``` + +Required positional ordering ONLY when callers will pass positionally; otherwise keyword-only via `*,` and the order is moot at call sites but still matters in the signature for readability. + +### Error codes -- enum only, never raw strings + +All error codes go through `ErrorCode` (`src/keboola_agent_cli/errors.py`). Raw string literals like `"bucket_not_found"` or `"invalid_token"` are forbidden in `raise`, `formatter.error(error_code=...)`, and anywhere they cross a layer boundary. `make check-error-codes` rejects raw `error_code="..."` literals at CI time. + +```python +# BAD +raise KeboolaApiError(message="...", error_code="not_found") + +# GOOD +from .errors import ErrorCode +raise KeboolaApiError(error_code=ErrorCode.NOT_FOUND, message="...") +``` + +If a new category appears, **add it to `ErrorCode`** and `_ERROR_CODE_TO_TYPE` in the same PR. Do not introduce ad-hoc strings. + +### File-size budgets -- split when concerns drift + +Hard ceiling per file: + +| Layer | Soft ceiling | Hard ceiling | +|-------|--------------|--------------| +| `commands/*.py` | 800 LOC | 1200 LOC | +| `services/*.py` | 1000 LOC | 1500 LOC | +| `client.py` / `manage_client.py` | 1500 LOC | 2000 LOC | + +When a file crosses the **soft** ceiling, the next PR that adds material to it should split first. When a file crosses the **hard** ceiling, splitting is required before merging more functionality into it. + +How to split: +- `client.py` mixing multiple Keboola subsystems (Storage, Queue, Sandboxes, Manage proxy, AI, encryption, ...) → split by **endpoint family**, e.g. `client/storage.py`, `client/queue.py`, `client/sandboxes.py`. Keep `BaseHttpClient` shared. +- A service crossing the ceiling almost always mixes orchestration with parsing/transformation → extract pure helpers into a sibling `_helpers.py` or `_transformers.py`. + +This is a guideline driven by review feedback (kbagent 0.31.0: `client.py` ≈3000 LOC, `storage_service.py` ≈2180 LOC, `sync_service.py` ≈2765 LOC); the soft ceilings exist so the situation does not get worse before it gets better. + +### Resource management -- `with` over lambdas + +LLM-generated code routinely wraps `open()`/`httpx.Client()`/temp-file/lock creation in a lambda or a "create-and-forget" call, leaking file descriptors or connections. **Use a context manager every time the resource has `__enter__`/`__exit__`.** + +```python +# BAD -- descriptor leaks if anything raises +opener = lambda: open(path, "r") # noqa: avoid-lambda-as-resource +content = opener().read() + +# BAD -- httpx client not closed on exception +client = httpx.Client() +response = client.get(url) + +# GOOD +with open(path) as f: + content = f.read() + +with httpx.Client() as client: + response = client.get(url) +``` + +### Named functions over throwaway lambdas + +Single-expression `sort` keys and `filter` predicates are fine as lambdas. Anything else -- assigned to a variable, used multiple times, doing branching, or carrying domain meaning -- gets a named `def`. Names are the cheapest documentation in the codebase. + +```python +# BAD +parse_row = lambda r: {"id": r[0], "name": r[1], "active": r[2] == "Y"} +rows = [parse_row(r) for r in raw] + +# GOOD +def _parse_storage_row(raw: tuple[str, str, str]) -> dict[str, Any]: + return {"id": raw[0], "name": raw[1], "active": raw[2] == "Y"} + +rows = [_parse_storage_row(r) for r in raw] + +# FINE -- single expression, throwaway, no domain meaning +items.sort(key=lambda x: x.priority) +``` + +### Type checking -- `ty` is mandatory (warnings) for new code + +We use Astral's [`ty`](https://github.com/astral-sh/ty) (same vendor as `uv` and `ruff`). It is fast (Rust), installs in <1s, and runs on every edit via the post-edit hook in `.claude/settings.json`. It also runs in the pre-commit hook in warning mode (does not block commits) and is exposed via `make typecheck`. + +Rules: +- **New code** -- must pass `make typecheck` clean. Adding any `# type: ignore` requires a one-line comment explaining why. +- **Existing code** -- grandfathered; do not regress existing warnings, but cleanups outside the PR's scope are not required. +- Type-hint every function signature (already a rule in "Python conventions" above); `ty` enforces that the hints are *correct*, not just present. + +```bash +make typecheck # full check, exit code reflects pass/fail +make typecheck-warn # same, but always exit 0 (used by hooks) +``` + ## Keboola API Best Practices ### Reference implementation @@ -239,6 +379,10 @@ before the PR is mergeable. - [ ] **CLI-layer tests** -- use `CliRunner`, test JSON output, error exit codes - [ ] **E2E tests** -- add a test in `tests/test_e2e.py` that exercises the command against a real Keboola project (requires `E2E_API_TOKEN` + `E2E_URL`). Run `make test-e2e` to verify. Every CLI command must have E2E coverage - [ ] **Run `make check`** before committing (lint + format + full test suite) +- [ ] **Run `make typecheck`** -- `ty` must pass for any new code (existing warnings grandfathered) +- [ ] **No new `tuple[...]` returns** -- multi-value returns use a `@dataclass` ([Code Quality Patterns](#code-quality-patterns)) +- [ ] **No raw error-code strings** -- `make check-error-codes` enforces `ErrorCode` enum usage +- [ ] **File-size budgets respected** -- see the table in [Code Quality Patterns](#code-quality-patterns); split before crossing the hard ceiling ### UX considerations diff --git a/Makefile b/Makefile index f50f42ff..9666e5eb 100644 --- a/Makefile +++ b/Makefile @@ -1,6 +1,6 @@ .DEFAULT_GOAL := help -.PHONY: help install install-mcp sync test test-unit test-integration test-e2e test-e2e-invite test-file lint lint-fix format format-check skill-check skill-gen version-sync version-check changelog changelog-check check-error-codes check clean hooks +.PHONY: help install install-mcp sync test test-unit test-integration test-e2e test-e2e-invite test-file lint lint-fix format format-check typecheck typecheck-warn skill-check skill-gen version-sync version-check changelog changelog-check check-error-codes check clean hooks help: ## Show this help message @grep -E '^[a-zA-Z0-9_-]+:.*?## .*$$' $(MAKEFILE_LIST) | awk 'BEGIN {FS = ":.*?## "}; {printf " \033[36m%-18s\033[0m %s\n", $$1, $$2}' @@ -44,6 +44,12 @@ format: ## Format code with ruff format-check: ## Check code formatting (no changes) uv run ruff format . --check +typecheck: ## Run ty type-checker (Astral). Fails on any error. + uv run ty check + +typecheck-warn: ## Run ty in warning-only mode (always exits 0; used by hooks) + @uv run ty check || true + skill-gen: ## Regenerate SKILL.md from CLI command tree uv run python scripts/generate_skill.py diff --git a/pyproject.toml b/pyproject.toml index 7d1d1a05..bd218ce0 100644 --- a/pyproject.toml +++ b/pyproject.toml @@ -59,6 +59,13 @@ dev = [ "pytest-httpx>=0.30", "pytest-asyncio>=0.23", "ruff>=0.8", + "ty>=0.0.33", "bandit>=1.9.4", "pip-audit>=2.10.0", ] + +[tool.ty.src] +include = ["src", "tests", "scripts"] + +[tool.ty.rules] +unresolved-import = "warn" diff --git a/scripts/post-edit-quality.sh b/scripts/post-edit-quality.sh new file mode 100755 index 00000000..0215a2d9 --- /dev/null +++ b/scripts/post-edit-quality.sh @@ -0,0 +1,63 @@ +#!/usr/bin/env bash +# Post-edit quality gate: ruff fix/format + ty type-check on a single file. +# Invoked by Claude Code's PostToolUse hook (.claude/settings.json) after +# Edit/Write/MultiEdit on Python files. +# +# Reads tool input JSON from stdin (Claude Code hook contract). Extracts +# the edited file path; if it is a .py file under our project, runs: +# 1. ruff check --fix --quiet (auto-fix safe lint issues) +# 2. ruff format --quiet (canonical formatting) +# 3. ty check (type errors -- reported as warnings) +# +# Exit code 0 = continue. Non-zero = Claude sees the failure as a tool result +# and must address it before next action (per CONTRIBUTING.md "Code Quality +# Patterns"). +# +# Manual invocation for debugging: +# echo '{"tool_input":{"file_path":"src/keboola_agent_cli/cli.py"}}' \ +# | scripts/post-edit-quality.sh + +set -euo pipefail + +PAYLOAD="$(cat)" + +FILE_PATH="$(printf '%s' "$PAYLOAD" \ + | python3 -c 'import json,sys; d=json.loads(sys.stdin.read()); print(d.get("tool_input",{}).get("file_path",""))' \ + 2>/dev/null || true)" + +if [ -z "$FILE_PATH" ]; then + exit 0 +fi + +case "$FILE_PATH" in + *.py) ;; + *) exit 0 ;; +esac + +if [ ! -f "$FILE_PATH" ]; then + exit 0 +fi + +# Run from repo root so ruff/ty pick up pyproject.toml config. +REPO_ROOT="$(git -C "$(dirname "$FILE_PATH")" rev-parse --show-toplevel 2>/dev/null || true)" +if [ -z "$REPO_ROOT" ]; then + exit 0 +fi +cd "$REPO_ROOT" + +FAILED=0 + +if ! uv run --quiet ruff check --fix --quiet "$FILE_PATH"; then + echo "post-edit: ruff check found unresolved issues in $FILE_PATH" >&2 + FAILED=1 +fi + +uv run --quiet ruff format --quiet "$FILE_PATH" || true + +# ty: report but do NOT fail the hook -- type checker is in warning-only mode +# while we migrate. Switch this to FAILED=1 when the codebase is clean. +if ! uv run --quiet ty check "$FILE_PATH" 2>&1 | tail -5; then + : # warnings only +fi + +exit $FAILED diff --git a/scripts/pre-commit b/scripts/pre-commit index 6aecd44d..3019ecc3 100755 --- a/scripts/pre-commit +++ b/scripts/pre-commit @@ -23,6 +23,18 @@ if [ -n "$STAGED" ]; then echo "Format check failed. Run 'make format' to fix, then re-stage." exit 1 fi + + # Type check (ty -- warnings only, does NOT block commits while migrating) + # Switch this to a hard `exit 1` once the codebase is clean. + if command -v uv >/dev/null 2>&1; then + TY_OUTPUT="$(uv run --quiet ty check $STAGED 2>&1 || true)" + if echo "$TY_OUTPUT" | grep -qiE "^(error|warning)"; then + echo "" + echo "pre-commit: ty type-check reported issues (warnings, not blocking):" + echo "$TY_OUTPUT" | tail -20 + echo "Run 'make typecheck' for full output." + fi + fi fi # Auto-regenerate SKILL.md if CLI commands changed diff --git a/uv.lock b/uv.lock index 521fd517..d0fd8aa0 100644 --- a/uv.lock +++ b/uv.lock @@ -463,6 +463,7 @@ dev = [ { name = "pytest-asyncio" }, { name = "pytest-httpx" }, { name = "ruff" }, + { name = "ty" }, ] [package.metadata] @@ -488,6 +489,7 @@ dev = [ { name = "pytest-asyncio", specifier = ">=0.23" }, { name = "pytest-httpx", specifier = ">=0.30" }, { name = "ruff", specifier = ">=0.8" }, + { name = "ty", specifier = ">=0.0.33" }, ] [[package]] @@ -1229,6 +1231,31 @@ wheels = [ { url = "https://files.pythonhosted.org/packages/c7/18/c86eb8e0202e32dd3df50d43d7ff9854f8e0603945ff398974c1d91ac1ef/tomli_w-1.2.0-py3-none-any.whl", hash = "sha256:188306098d013b691fcadc011abd66727d3c414c571bb01b1a174ba8c983cf90", size = 6675, upload-time = "2025-01-15T12:07:22.074Z" }, ] +[[package]] +name = "ty" +version = "0.0.35" +source = { registry = "https://pypi.org/simple" } +sdist = { url = "https://files.pythonhosted.org/packages/4e/53/440e7b1212c4b0abbd4adb7aed93f4971aa1f8dca386ac5515930afa9172/ty-0.0.35.tar.gz", hash = "sha256:8375c240ab38138a19db07996c9808fb7a92047c1492e1ce587c2ef5112ad3a9", size = 5629237, upload-time = "2026-05-10T18:25:17.105Z" } +wheels = [ + { url = "https://files.pythonhosted.org/packages/d4/84/19662ee881675815b7fafff940a365be1985730465afd9b75cb2edd5f8b3/ty-0.0.35-py3-none-linux_armv6l.whl", hash = "sha256:85ae1e59b9fb0b40e9d84fe61b29653c5f2f5e78b487ece371a7a38c20c781cf", size = 11198741, upload-time = "2026-05-10T18:24:49.378Z" }, + { url = "https://files.pythonhosted.org/packages/62/df/7e5b6f83d85b4d2e5b72b5dceb388f440acc10679417bd46f829b9200fab/ty-0.0.35-py3-none-macosx_10_12_x86_64.whl", hash = "sha256:709dbb7af4fcadb1196863c00b8791bbbbcc9dacbe15a0ff17f0af82b35d415b", size = 10948304, upload-time = "2026-05-10T18:24:58.246Z" }, + { url = "https://files.pythonhosted.org/packages/59/94/72d7263aca055cde427f0ebcf08d6a74e5a5fee1d1e7fdd553696089cecb/ty-0.0.35-py3-none-macosx_11_0_arm64.whl", hash = "sha256:2cb0877419ab0c8708b6925cb0c2800b263842bd3c425113f200538772f3a0cc", size = 10407413, upload-time = "2026-05-10T18:24:37.422Z" }, + { url = "https://files.pythonhosted.org/packages/b6/23/fda6fae8a81ce0cb5f24cdfe63260e110c7af8844e31fa07d1e6e8ef0232/ty-0.0.35-py3-none-manylinux_2_17_aarch64.manylinux2014_aarch64.whl", hash = "sha256:e7afbcfc61904b7e82e7fe1a1db832a40d8f01e69dee1775f6594e552980536c", size = 10932614, upload-time = "2026-05-10T18:24:47.401Z" }, + { url = "https://files.pythonhosted.org/packages/72/3d/b98d8d4aa1a5ed6daaf15864e838f605ca7b1e8b93b7e17b96ed4bc4dfed/ty-0.0.35-py3-none-manylinux_2_17_armv7l.manylinux2014_armv7l.whl", hash = "sha256:b61498cc3e4178031c079951257fbdb209a891b4feb10ad6c40f615a51846f41", size = 10962982, upload-time = "2026-05-10T18:24:44.88Z" }, + { url = "https://files.pythonhosted.org/packages/18/c4/2881aad71bf6fb2f8df17fc8e4bc89e904e54490a3ee747b5ef73f98ac85/ty-0.0.35-py3-none-manylinux_2_17_i686.manylinux2014_i686.whl", hash = "sha256:573b1eacda349fc8dba0d767b41631c3a6f66412363127c5bf2b1b40a1d898d2", size = 11476274, upload-time = "2026-05-10T18:24:42.4Z" }, + { url = "https://files.pythonhosted.org/packages/34/0f/7717650adaeaddd23eea70470e2c26d3f0b9b18fdc7f26ec9552d6001f17/ty-0.0.35-py3-none-manylinux_2_17_ppc64le.manylinux2014_ppc64le.whl", hash = "sha256:a7209746158d6393c1040aa64b3ca29622e212ea7d8bae22ba50dbcbb4f96f0a", size = 12012027, upload-time = "2026-05-10T18:25:00.752Z" }, + { url = "https://files.pythonhosted.org/packages/22/c9/1a16cb4aab6f4707d8f550772e91abc26d1c8870f19b5e2453ad10bb8209/ty-0.0.35-py3-none-manylinux_2_17_s390x.manylinux2014_s390x.whl", hash = "sha256:4466a1470aa4418d49a9aa45d9da7de42033addd0a2837c5b2b0eb71d3c2bcd3", size = 11648894, upload-time = "2026-05-10T18:25:12.44Z" }, + { url = "https://files.pythonhosted.org/packages/18/a1/a977c0e07e9f88db9c67f90c6342a4dc4422c8091fa07bf26521870687c5/ty-0.0.35-py3-none-manylinux_2_17_x86_64.manylinux2014_x86_64.whl", hash = "sha256:eb44bb742d52c309dcaa6598bcf4d82eb4bf1241b9e4940461e522e30093fe8b", size = 11560482, upload-time = "2026-05-10T18:25:05.172Z" }, + { url = "https://files.pythonhosted.org/packages/d6/c1/a5fb11227d5cc4ac3f29a115d8c8bc817578e8ef6907d1e4c914ddbf45ee/ty-0.0.35-py3-none-manylinux_2_31_riscv64.whl", hash = "sha256:34b219250736c989b2670a03782c61315f523f3a2be37f1f90b1207e2212c188", size = 11718495, upload-time = "2026-05-10T18:24:54.12Z" }, + { url = "https://files.pythonhosted.org/packages/3c/cb/e92e4317388b6d1fd821a46941b448a8a1ff0bf13e22147c5167d8fa1b00/ty-0.0.35-py3-none-musllinux_1_2_aarch64.whl", hash = "sha256:88e2ac497decc0940ef1a07571dee8a746112a93a09cdc7f8bca0099752e2e05", size = 10900815, upload-time = "2026-05-10T18:25:02.941Z" }, + { url = "https://files.pythonhosted.org/packages/e9/4f/03bd87388a92567f262f35ac64e10d2be047d258f2dfcf1405f500fa2b90/ty-0.0.35-py3-none-musllinux_1_2_armv7l.whl", hash = "sha256:02cae51b53e6ec17d5d827ff1a3a76fd119705b56a92156e04399eda6e911596", size = 10998051, upload-time = "2026-05-10T18:25:14.68Z" }, + { url = "https://files.pythonhosted.org/packages/b4/60/6edbc375ee6073973200096168f644e1081e5e55a7d42596826465b275de/ty-0.0.35-py3-none-musllinux_1_2_i686.whl", hash = "sha256:11871d730c9400d899ac0b9f3d660ed2e7e433377c8725549f8250a36a7f2620", size = 11148910, upload-time = "2026-05-10T18:24:51.842Z" }, + { url = "https://files.pythonhosted.org/packages/4d/b1/a845d2066ed521c477450f436d4bd353d107e7c02dd6536a485944aaf892/ty-0.0.35-py3-none-musllinux_1_2_x86_64.whl", hash = "sha256:1ad0a2f0530d0933dcc99ad36ac556c63e384ea72ab9a18d23ad2e2c9fd61c73", size = 11671005, upload-time = "2026-05-10T18:24:56.223Z" }, + { url = "https://files.pythonhosted.org/packages/73/81/1d5912a54fb66b2f95ac828ae61d422ef5afeae1263e4d231e40796c229f/ty-0.0.35-py3-none-win32.whl", hash = "sha256:0e25d63ec4ab116e7f6757e44d16ca9216bca679d19ecc36d119cf80faada61a", size = 10481096, upload-time = "2026-05-10T18:24:39.976Z" }, + { url = "https://files.pythonhosted.org/packages/3b/36/1c7f8632bfec1c321f01581d4c940a3617b24bd3e8b37c8a7363d33fbfc4/ty-0.0.35-py3-none-win_amd64.whl", hash = "sha256:6a0a6d259f6f2f8f2f954c6f013d4e0b5eba68af6b353bf19a47d59ec254a3d5", size = 11555691, upload-time = "2026-05-10T18:25:07.792Z" }, + { url = "https://files.pythonhosted.org/packages/7a/fb/59325221bce52f6e833d6865ce8360ef7d5e1e21151b38df6dc77c4327a7/ty-0.0.35-py3-none-win_arm64.whl", hash = "sha256:619c52c0fb2aa21961a848a1995135ad3b6d0a9aa54da0194e60f679cc200e13", size = 10925457, upload-time = "2026-05-10T18:25:10.352Z" }, +] + [[package]] name = "typer" version = "0.24.1"