Files
svcforge/AGENTS.md
T
Nguyen Minh Phuc c76154aeaa
ci / lint (push) Successful in 34s
ci / unit (push) Successful in 1m41s
ci / types (push) Successful in 1m41s
ci / dockerfile (push) Successful in 18s
ci / security (push) Successful in 1m27s
ci / chart (push) Failing after 1m11s
ci / integration (push) Successful in 1m10s
ci / image (api) (push) Has been skipped
ci / image (reconciler) (push) Has been skipped
ci / image (worker) (push) Has been skipped
ci / bump (push) Has been skipped
review: fix 26 findings from a 4-agent audit
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.
2026-07-18 12:13:49 +00:00

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:

  1. The claim query is one statement. repo/tasks.py::_CLAIM_SQL. Splitting the UPDATE ... WHERE id = (SELECT ... FOR UPDATE SKIP LOCKED) into select-then-update creates a double-provision race that testing will not catch.
  2. complete() and fail() check locked_by. That check is a repro-confirmed bug fix, with regression tests in tests/integration/test_tasks.py. Removing it lets a worker whose lease expired clobber a task another worker now owns.
  3. domain/ has no I/O and no async. CI greps for both.
  4. Migrations never run at app startup. They are a Helm pre-upgrade,pre-install hook.

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=Trueteam 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 <= cap alone passes on a fully serial worker.
  • Markers: slow (>1s or hits real Upstash), e2e (needs a cluster). make test runs 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 containerd belongs to a Kubernetes kubelet; installing docker.io would evict the runtime and take down every pod. You cannot docker build here. Verify Dockerfiles by inspection and hadolint.
  • 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. .env is gitignored.
  • Infra changes go through Terraform/Ansible, never kubectl edit. The Gitea runner's config lives in oci-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.