ci: add quality gate (lint, typecheck, test, docker build) - #27
Merged
Conversation
Introduces pyproject.toml with [tool.ruff]/[tool.pyright] — this repo had
zero lint/type-check configuration before this commit (only pytest.ini's
asyncio_mode). Rule set mirrors nullain-agent-sdk's own selection
(E,F,W,I,N,UP,B,S,A,C4,SIM,RUF) for ecosystem consistency, with two
measured adjustments:
- B008 dropped: measured 20 hits, 100% FastAPI's own Depends()/File()
idiom (function-call-in-default-argument IS how FastAPI's DI works) —
the rule structurally conflicts with the framework here, not a real bug.
- RUF001/RUF002 dropped: measured 4 hits, all legitimate pt-BR UI/docs
content ("3× superior", "Nullain × ZuckPay", an ℹ glyph), not typos.
- BLE001 added (flake8-blind-except, not in the SDK's set): this repo
already had 105 `# noqa: BLE001` annotations on broad `except Exception`
blocks before this rule was ever enabled — the intent was already
there; turning it on makes it real instead of a dead trail.
tests/* keeps the SDK's own S101/S105/S106 per-file-ignore (assert is
idiomatic in tests).
This commit applies only mechanical changes — `ruff check --fix` (74
auto-fixes: unused imports, import ordering, SIM/UP simplifications,
trailing whitespace, and removal of 31 now-orphaned `noqa` comments that
referenced rules never actually enabled — 9 stale BLE001/RUF013 plus 21
ARG001/ARG002 in tests/test_sandbox_aio.py, a rule never in scope here)
and `ruff format` (27 files reformatted, pure whitespace/wrapping). Zero
behavioral changes — full test suite green before and after (56 passed).
91 remaining findings (S110/E501/E402/B904/S608/S104/N818/etc.) require
human judgment and are fixed in a separate commit, on purpose — this one
stays pure mechanical noise so the behavioral diff is reviewable alone.
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
One commit (not split from the mechanical baseline — see note at bottom)
covering every ruff finding that needed a real decision rather than a
blind auto-fix, after the config/auto-fix baseline commit. Review by this
list, not the full diff — the behavior-changing points are itemized below;
everything else is mechanical residue from the same pass.
## Behavior changes (review these)
- agent/attachments_store.py:244 (fetch_attachments_by_ids) — a Neon
fetch failure was `except Exception: pass`, silently returning only
what's cached. Now logs at WARNING with (missing, user_id) before
falling through — same degrade-and-continue behavior, but the failure
is no longer invisible.
- agent/attachments_store.py:335 (delete_attachments_by_thread) — a Neon
delete failure was `except Exception: pass`, orphaning attachments with
no trace. Now logs at ERROR then re-raises. Observable behavior is
unchanged: the only caller (web/server.py's delete_thread route) already
wraps this in its own try/except and always returns {"ok": True}
regardless — re-raising here just makes the failure visible in logs
instead of doubly swallowed.
- B904 (raise-without-from), 2 sites — agent/attachments_store.py:83
(psycopg ImportError) and web/server.py:1032 (UnicodeEncodeError on
Host header) now chain `from exc`, preserving the original traceback
instead of discarding it.
- agent/attachments_store.py:159 (RUF006) — the fire-and-forget
`loop.create_task(_persist_async(...))` had no reference held, so the
event loop could garbage-collect it mid-execution. Now stored in a
module-level `_persist_tasks` set with a done-callback that discards it
on completion.
- agent/attachments_store.py:279 (S608) — reviewed, not a real SQL
injection: the f-string only interpolates `$1, $2, ...` positional
placeholders and a param count, never `file_ids`/`user_id` content
(which travel through `params`, properly parameterized). Documented
with a noqa + comment rather than restructured.
- web/server.py:1882 (S104, HOST=0.0.0.0) — reviewed, not a real
exposure: the app always runs in a container (Dockerfile CMD), where
0.0.0.0 is the correct bind address. Documented with noqa + comment.
- agent/sandbox_docker.py:91 (S108, hardcoded "/tmp") — reviewed, not
insecure temp-file usage: it's a Docker tmpfs mount spec
(noexec/nosuid/nodev, size-capped). Documented with noqa + comment.
- agent/zuckpay.py:71 (N818) — `ZuckPayNotConnected` renamed to
`ZuckPayNotConnectedError` (Error suffix on exception class); updated
at both call sites (agent/zuckpay.py, agent/zuckpay_tools.py).
- 17 S110 (try/except/pass) sites, resolved per the requested categories:
- attachments_store.py's two (above): real fix (log + propagate/re-raise).
- agent/redis_client.py:195 (distributed lock release): suppressed with
justification — the lock still expires via its own `ex=timeout` TTL,
so a failed delete self-heals.
- The remaining 14 (agent/external_mcp.py x2, agent/integrations.py x2,
agent/sandbox_docker.py x2, agent/wavespeed_tools.py x1, web/auth.py
x3, web/server.py x4): each is a real fallback path (falls through to
an in-memory/local alternative, or is genuinely best-effort cleanup)
— added a `logger.debug`/`.warning` call with rationale before
keeping the suppression, per "no noqa without a comment explaining
why." Three of these (web/auth.py's two Redis-lockout checks and its
CSRF-origin resolution) are security-relevant and log at WARNING
instead of DEBUG for that reason.
- Also newly found while fixing agent/sandbox_docker.py:180's S110: a
pre-existing bug where `ls_result` is computed but never parsed into
`files` — file-generation detection in the Docker sandbox driver is
dead. Left as-is (out of scope here, flagged in a comment) — filing
a follow-up issue separately.
## Residual mechanical fixes (same pass, no behavior change)
SIM102/103/108 (collapsed nested ifs, if/else→ternary, if/else→bool
expr), F841 (removed genuinely-unused local vars), B905 (explicit
`strict=False` on zip()), RUF005 (list unpacking instead of concat),
RUF012 (ClassVar annotations on test fixture classes), E402 (moved
misplaced mid-file imports to the top; web/auth.py also had a redundant
`import secrets as _secrets` shadowing the top-level `import secrets`,
removed), E501 (wrapped long lines; agent/wavespeed_tools.py's one
long line is a literal LLM-facing JSON template that can't be reflowed,
handled via a per-file-ignore in pyproject.toml with rationale), and
`contextlib.suppress(...)` swapped in for a few more try/except/pass
blocks whose exception is intentionally discarded (Windows chmod
incompatibility, idempotent WebSocket close).
Verified: full test suite green (56 passed) after every step, `ruff
check .` and `ruff format --check .` both clean.
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Third commit in the CI-quality-gate sequence (config+auto-fix, then
human-judgment lint fixes, now types) — see pyproject.toml's
[tool.pyright] added in the first commit (typeCheckingMode = "basic").
## Real bug found and fixed (not just a type annotation)
- requirements.txt / agent/attachments_store.py:82 — `psycopg_pool` is a
separate PyPI package (`psycopg-pool`), not bundled in `psycopg[binary]`
as the code's own ImportError message claimed. It was never actually
installed, so `_get_pool()`'s import always failed in any deploy —
meaning Neon attachment persistence has been silently falling back to
the in-memory-only cache (`_MEMORY`, no durability across restarts/
replicas) since this path was written, whenever NEON_DATABASE_URL was
configured. No data was lost mid-session (the memory cache always
populates first), but nothing survived a restart. Added
`psycopg-pool>=3.2.0,<4.0.0` to requirements.txt (same range as the
`psycopg[binary]` pin already there) and fixed the misleading error
message. Installed locally and confirmed pyright resolves the import.
- agent/sandbox.py:32 — `files: list[dict[str, Any]] = None` had a type
that didn't match its own default; declared `| None` explicitly (the
dataclass's `__post_init__` already normalizes None to `[]`, so this
is purely a declaration fix, no behavior change).
- agent/external_mcp.py:288 — `_load_store_decrypted()`'s ternary
(`store.get("mcps") if isinstance(...) else {}`) didn't narrow to
`dict` for pyright even though it's correct at runtime; rewrote with
an explicit intermediate variable + annotation instead of relying on
the ternary's inferred type.
- agent/models.py — `ModelOption.to_dict()` was typed
`dict[str, str | None]`, wider than reality (only `badge` is ever
`None`; `id`/`label`/`tagline`/`badge_tone` are always populated
strings). That imprecision was what made
`ModelOptionResponse(**o.to_dict())` in web/server.py look unsound to
pyright. Added a `ModelOptionDict` TypedDict mirroring the real
per-field nullability instead of suppressing the call site.
## Upstream/framework typing gaps (narrow `type: ignore`, each commented)
- agent/redis_client.py (x3), web/auth.py (x1) — redis-py's stubs type
hash/incr/hgetall calls as `ResponseT` (a union including the sync
pipeline-builder's own return type), even though these are plain
awaited/sync client calls that always return the concrete type at
runtime.
- agent/zuckpay.py:342 — `float(part.get("percentage"))` where the dict
value can genuinely be `None`; already caught by the surrounding
`except (TypeError, ValueError)`, so intentional, not a bug.
- agent/external_mcp.py (x2) — `MultiServerMCPClient`'s constructor wants
its own `Connection` TypedDict; the local `build_*_server_config()`
helpers return structurally-compatible plain dicts, not nominally
typed as one.
- web/server.py — slowapi's rate-limit exception handler doesn't match
FastAPI's general `ExceptionHandler` signature (known upstream
mismatch); `current_user(websocket)` passes a `WebSocket` where
`current_user` is typed for `Request` — both share `.session` via
`SessionMiddleware` at runtime, they just don't share a type hierarchy.
## Other real fix
- web/server.py's `_stream_with_keepalive` was declared `-> Any` despite
being an async generator (`yield` in its body) — that mismatch was
what confused pyright into treating its call site as a plain coroutine
("CoroutineType is not iterable"). Declared it accurately as
`AsyncGenerator[str, None]` instead of suppressing the call site.
Verified: full test suite green (56 passed), `ruff check .`/`ruff format
--check .` both clean, `pyright` 0 errors/0 warnings.
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
…ild) Fourth and final commit in the CI-quality-gate sequence — the workflow itself, now that the baseline is actually green (prior 3 commits: ruff/ pyright config + mechanical fixes, human-judgment lint fixes, type fixes). This repo had zero CI before this PR — nothing caught the ~5,000 lines of dead code (vision/ removal, earlier PR) or would have caught the psycopg-pool gap (this PR's third commit) before merge. Three jobs, all gating PRs into main and re-running on push to main: - static: ruff check + ruff format --check + pyright, ubuntu-only (lint/ type errors aren't OS-specific, no reason to triple the cost). - test: pytest, Python 3.12 only, ubuntu-only — this app only ever runs as a Linux container in production (Dockerfile: python:3.12-slim- bookworm), unlike a published library that has to support whatever its consumers run. A cross-platform/version matrix here would add CI time without covering any code path that actually ships. - docker-build: builds the actual Dockerfile with docker/build-push- action, push: false. This is the artifact that deploys (Coolify) and was, until now, the one thing no automation verified — confirmed locally with a real `docker build .` + a smoke-test import (psycopg_pool, web.server, agent.attachments_store all resolve inside the built image) before writing this workflow. Deliberately no deploy job — quality gate only, per scope. Deploy stays manual on Coolify. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
9 tasks
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
This repo had zero CI before this PR — no
.github/workflowsat all, no lint/type-check config, onlypytest.ini'sasyncio_mode = auto. That gap is what let ~5,000 lines of dead code accumulate unnoticed (thevision/removal, prior PR). This PR adds a quality gate: lint + typecheck + test + Docker build, all gating merges intomain.Design, approved before implementation:
nullain-agent-sdk's own ruff/pyright selection for ecosystem consistency, with two measured adjustments (B008dropped — 100% FastAPIDepends()/File()idiom, not a bug;RUF001/RUF002dropped — legitimate pt-BR typographic content) and one addition (BLE001— this repo already had 105# noqa: BLE001markers before the rule existed, formalizing existing intent).pyrightinbasicmode (notstrict— this codebase was never typed, strict would be a separate, larger effort).docker-build: builds the real Dockerfile,push: false— the one artifact nothing verified before.Commits (4, staged so the diff is reviewable in order)
chore(lint)—pyproject.tomlwith[tool.ruff]/[tool.pyright], thenruff check --fix(74 auto-fixes) +ruff format(27 files). Zero behavior change — full suite green before and after.fix(quality)— every ruff finding needing a real decision. Full itemized list in the commit body (two realexcept: passbugs fixed with logging + propagation inattachments_store.py, 2xB904raise...from,S608/S104/S108reviewed and documented as false-positives, anN818rename,RUF006task-reference fix, 17S110sites resolved per-category — real fix, justified suppression, or logged-then-suppressed).fix(types)— pyright findings inbasicmode. Includes a real production bug found via typecheck:psycopg_poolis a separate PyPI package frompsycopg[binary]and was never actually installed — meaning Neon attachment persistence has been silently falling back to in-memory-only (no durability across restarts) in any deploy withNEON_DATABASE_URLset. Fixed by addingpsycopg-pooltorequirements.txtand correcting the misleading error message. Verified inside a real built Docker image thatpsycopg_pool.AsyncConnectionPoolnow resolves.ci— the workflow itself, added last now that the tree is actually green.Test plan
ruff check ./ruff format --check .— cleanpyright— 0 errors, 0 warningspytest— 56 passed, throughout every commit in the sequencedocker build .(Docker Desktop) succeeded; smoke-tested inside the built image thatpsycopg_pool,web.server,agent.attachments_storeall import cleanly.github/workflows/ci.ymlYAML validated withyaml.safe_load🤖 Generated with Claude Code