Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions CLAUDE.md
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Expand Down
144 changes: 144 additions & 0 deletions CONTRIBUTING.md
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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

Expand Down
8 changes: 7 additions & 1 deletion Makefile
Original file line number Diff line number Diff line change
@@ -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}'
Expand Down Expand Up @@ -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

Expand Down
7 changes: 7 additions & 0 deletions pyproject.toml
Original file line number Diff line number Diff line change
Expand Up @@ -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"
63 changes: 63 additions & 0 deletions scripts/post-edit-quality.sh
Original file line number Diff line number Diff line change
@@ -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
12 changes: 12 additions & 0 deletions scripts/pre-commit
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
27 changes: 27 additions & 0 deletions uv.lock

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

Loading