CORRECTNESS - lost-lease race: complete()/fail() did not check ownership, so a worker whose lease expired could mark a task done while another worker was running it, or requeue a task someone else owned. Reproduced, fixed with a CAS on (state, locked_by), pinned by two regression tests. - worker died on report failure: _run_one's docstring claimed no exception escapes the TaskGroup; fail()/complete() were outside the guarded block, so a DB blip cancelled every sibling provision on the pod. - claim query used an INNER join, which could strand a just-claimed task and report 'queue empty'. LEFT join. - InstanceRepo.set_error bypassed the state machine and had no callers. Deleted. - handle_deprovision ignored its CAS result, so a wrong-state instance kept a dangling endpoint and got re-provisioned by the drift check 60s later. - handle_verify re-notified on every retry: five pages for one halt. DEPLOY-BREAKING - the migration Job could never succeed: no Dockerfile copied migrations/, and migrate.py resolved the path relative to the source tree, which only works for an editable install. Added COPY + SVCFORGE_MIGRATIONS_DIR. - ServiceMonitor selector did not match the Service: API metrics never scraped. - SvcforgeReconcilerStale fired permanently from every pod, because the gauge is module-level and every service exports it as 0. Scoped to the reconciler job. - SvcforgeTaskFailed latched forever on a monotonic counter. Now increase()[15m]. - the digest guard accepted the all-zeros placeholder. - worker terminationGracePeriodSeconds was 60s against a 600s helm timeout. DEAD CODE THAT SHOULD NOT HAVE BEEN - adapters/k8s.py was never called, so tenant namespaces were never created and the first provision for a new team would fail. Wired into handle_provision. - adapters/redis.py was never imported by any service. Rate limiting is now wired into the API, failing open. - Settings.check_production() had no callers. Given an explicit environment and called from every entrypoint. OBSERVABILITY - the API never called obs.setup(): no JSON logs, no trace correlation, log_json silently inert. - LogNotifier's structured fields were discarded by the stdlib->structlog bridge. - bind_task_context cleared the 'service' binding for the life of every task. - split tasks_failed into task_attempts_failed and tasks_dead_lettered. SECURITY - trivy correctly blocked the worker/reconciler images: helm 3.16.2 and kubectl 1.31.2 carry CRITICAL Go stdlib CVEs. Bumped to helm 3.21.3 and kubectl 1.35.3, which also closes a four-minor skew against the v1.35.3 cluster. TESTS THAT COULD NOT FAIL - the concurrency cap test passed on a fully serial worker. - the alert/metric cross-check asserted a hardcoded list instead of reading the chart, so it could not catch a rename on the chart side. - fixed OTel tracer-provider pollution between test files. DOCS - ARCHITECTURE.md: mermaid diagrams, user stories, and the helm-vs-ArgoCD guarantee (verified with --dry-run=server). - AGENTS.md + CLAUDE.md. - prose sweep for back-and-forth phrasing across 19 files.
6.8 KiB
AGENTS.md — working in svcforge-reference
Rules for any AI agent (Claude Code, Cursor, Codex, Copilot, Gemini CLI) editing this repo. Read this before your first edit.
What this repo is, and the one rule that follows from it
This is a reference implementation: a complete, verified build of the system that
../learn-python/ teaches Phuc to build himself. It exists to be compared against, not
copied from.
THE RULE: never propose copying domain/, repo/, services/worker/, or the claim
query into learn-python/. Those four are the course. If asked to "help with Module 4",
help him find his own bug; do not paste repo/tasks.py. Scaffolding — pyproject.toml,
Dockerfiles, CI, the chart — is fair game to copy, because struggling with hatchling
teaches nothing.
learn-python/AGENTS.md has its own, stricter rules. They still apply over there.
Before you change anything
Read ARCHITECTURE.md. Every design choice here was paid for by a specific failure mode, and the comments say which. A change that removes a comment's subject usually removes a guard.
Four things that look like over-engineering and are not. Do not "simplify" them:
- The claim query is one statement.
repo/tasks.py::_CLAIM_SQL. Splitting theUPDATE ... WHERE id = (SELECT ... FOR UPDATE SKIP LOCKED)into select-then-update creates a double-provision race that testing will not catch. complete()andfail()checklocked_by. That check is a repro-confirmed bug fix, with regression tests intests/integration/test_tasks.py. Removing it lets a worker whose lease expired clobber a task another worker now owns.domain/has no I/O and noasync. CI greps for both.- Migrations never run at app startup. They are a Helm
pre-upgrade,pre-installhook.
Conventions
Layers. transport → domain ← repo/adapters. domain/ imports nothing from the other
three. repo/ knows SQL and not HTTP. adapters/ owns every subprocess and network call.
Python 3.12. Full annotations everywhere including tests. X | None, StrEnum,
Final, PEP 695 type aliases.
Errors. SvcforgeError in svcforge_core/errors.py is the base. New exception types
inherit from it, plus a stdlib base when the behaviour matters (HelmError(SvcforgeError, RuntimeError)).
Logging. obs.get_logger(__name__), structlog kwargs. Never stdlib logging directly,
and never extra={...} — the ProcessorFormatter bridge silently discards it. Never use
message or event as a field name; both are reserved and raise.
SQL. Raw psycopg3, no ORM. Parameters, never f-strings. Rows come back as dicts
(DictPool in repo/db.py); annotate with that alias rather than a bare
AsyncConnectionPool, which mypy resolves to tuple rows.
Subprocess. create_subprocess_exec with start_new_session=True. Never shell=True —
team is tenant input that reaches a helm release name. Timeouts kill the process group.
Protocols. A Protocol needs two implementations (a real one and a fake) or it should
be a concrete class.
Datetimes. Always aware, always UTC. Domain functions take now as a parameter;
only adapters hold a Clock.
Pinning. Everything by digest: base images, tool images, GitHub Actions by SHA.
A SHA-pinned third-party action still resolves its own dependencies by mutable tag —
that is how trivy-action broke. Prefer running tools directly from a digest-pinned image.
Writing style
Plain declaratives. No back-and-forth phrasing, which means none of:
- Rhetorical setup-knockdowns: "That is not a nicety. It is the whole design."
- not-X-but-Y pivots used for rhythm: "It is not a lock, but a lease."
- Throat-clearing: "It is worth noting that", "The key insight here", "Here's the thing".
- Rhetorical questions you then answer.
- Dramatic fragments for rhythm: "Not slow. Not fast. Meaningless."
Write the claim directly. Keep the why — it is the most valuable content in this repo — and drop the scaffolding around it. Factual contrasts that carry information stay ("LEFT join, not inner"; "202, never 201").
Testing
- Never mock the database. Integration tests run against real Postgres. Each pytest
process clones its own database (
tests/integration/conftest.py). - Fakes, not mocks, for helm/kubectl/Redis/clock.
tests/fakes.py. - A test that cannot fail is worse than no test. Before adding one, ask what change
would make it red. Bound assertions on both sides where it matters:
max_concurrent <= capalone passes on a fully serial worker. - Markers:
slow(>1s or hits real Upstash),e2e(needs a cluster).make testruns neither.
export SVCFORGE_TEST_DSN="postgresql://svcforge:svcforge@127.0.0.1:5432/svcforge_test"
make lint # ruff + ruff format + mypy --strict
make test # unit
make integration # needs the DSN above
make ci # every gate CI runs, same commands
Environment facts that will bite you
- This host has no Docker daemon, and must not get one. Its
containerdbelongs to a Kubernetes kubelet; installingdocker.iowould evict the runtime and take down every pod. You cannotdocker buildhere. Verify Dockerfiles by inspection andhadolint. - Postgres is local (apt, v16) for tests. testcontainers is the CI path.
- Redis is real Upstash on a free tier: 500K commands/month. Redis-hitting tests are
marked
slow. Do not write loops that spend thousands of commands; use the fakes. - Secrets live in
~/.config/svcforge/secrets.env, chmod 600, outside every repo. Never echo, log, commit, or paste a DSN or token..envis gitignored. - Infra changes go through Terraform/Ansible, never
kubectl edit. The Gitea runner's config lives inoci-k8s/k8s/roles/addons/tasks/main.yml.
CI/CD
.gitea/workflows/ci.yaml. Gates in order: ruff → mypy → unit+coverage → migrate+integration
→ bandit/gitleaks/pip-audit → hadolint → helm lint/template → build → trivy → push by digest
→ bump chart digests → ArgoCD syncs.
- CI holds no kubeconfig and must never hold one. Its last act is a git commit.
- Build once, promote by digest. The image is scanned before it is pushed.
- Every gate blocks. If a gate is failing, fix the cause. Do not add an ignore, lower a threshold, or mark it advisory. A CVE finding from trivy is real: bump the pinned version.
RUNBOOK.md has the setup steps and the traps that cost real time.
When you are unsure
Say so, and check. This repo's value is that its claims were executed, not asserted — the race conditions have reproductions, the timeout kill has a negative control, the coverage gate was caught measuring 0%. Guessing and sounding confident is the one failure mode that makes it worthless.