diff --git a/.claude/skills/lifecycle-test-triage/SKILL.md b/.claude/skills/lifecycle-test-triage/SKILL.md new file mode 100644 index 0000000..924fe1d --- /dev/null +++ b/.claude/skills/lifecycle-test-triage/SKILL.md @@ -0,0 +1,87 @@ +--- +name: lifecycle-test-triage +description: Triage a red or hanging runner-lifecycle test in service-python-fastapi (TestCreateToRunDocker, TestPythonFastAPILifecycle_Matrix, the uvicorn and SBOM tests) to tell a genuine regression from Docker-daemon contention, a missing tool that turned the test into a silent skip, or the default go test timeout. Use before concluding that a lifecycle failure came from your diff, and before raising a timeout or relaxing a wait. +--- + +# Triaging a lifecycle test + +These tests drive a real uvicorn through `uv` and real containers through the +Docker daemon. That makes them the ones worth trusting and the ones that fail +for reasons outside the diff. Work down this list before changing test code. + +## 1. Did it actually run? + +A missing tool skips, it does not fail, so a green run can mean almost nothing +was exercised: + +```bash +go test -v ./... 2>&1 | grep -E '^\s*--- SKIP' +``` + +| Missing | Skips | +| --- | --- | +| `uv` | the real uvicorn lifecycle | +| `syft`, `docker`, `uv` | the image SBOM inventory | +| `python3` | the active-environment test | +| `nix` | the lifecycle matrix's nix backend | +| a stopped daemon | the matrix's docker backend, and destroy-under-docker | + +CI installs `uv` and pins `syft` v1.48.0, but **has no `nix`** — so +`TestPythonFastAPILifecycle_Matrix/nix` is the one skip in a green CI run and +the nix path is covered nowhere. If a skip covers the surface you changed, +install the tool and re-run — do not report the green. + +A missing Docker daemon is the exception: `TestCreateToRunDocker` has no +tooling guard and fails rather than skips. That red is correct. Do not add a +skip guard to make it green — that converts the loudest signal in the suite +into the silent pass this whole section exists to prevent. + +## 2. `panic: test timed out after 10m0s` + +That is `go test`'s default, not an assertion. Read the `running tests:` block +the panic prints: when it names `TestPythonFastAPILifecycle_Matrix/docker` or +another container case, the suite was still making progress. CI finishes the +whole suite inside the default; a busy local daemon does not. + +```bash +go test -timeout 40m ./... +``` + +If it passes with the longer budget, the timeout was the environment. Say so +rather than raising the timeout in the repo. + +## 3. `main_test.go: too many tries` + +`TestCreateToRunDocker` polls `GET
/version` once a second and gives +up after ten tries. That budget is fixed and small — about eleven seconds — +while the surrounding work (image pull, container create, `uv sync`) is not +bounded by it at all. So this message means *the service did not answer within +eleven seconds of `Start` returning*, which a contended daemon produces on +completely healthy code. + +Check contention before suspecting the diff, then re-run the one test: + +```bash +docker ps --format '{{.Names}}\t{{.Status}}' +go test -run TestCreateToRunDocker -count=1 -timeout 20m -v ./... +``` + +Other lazybox sessions share this daemon. A pass in isolation is the verdict. + +Do **not** "fix" it by widening the retry budget. The poll loop is asserting +that a started service becomes reachable promptly; a longer wait hides a real +startup regression the next time there is one. + +## 4. When it is real + +Prove it the way the rule requires — put the suspected cause back and confirm +it breaks. Two failures are almost never environmental: + +- **Anything about locking.** `runtime.go` serializes Init/Start/Stop/Destroy + with a documented lock order (`initMu` → `runnerCreateMu` → `runnerMu`). A + lost lock is a data race, not a failed assertion, so plain `go test` stays + green. `go test -race ./...` is the verdict, and it is its own CI job. +- **A status carried in the response, not the error.** `Init` reports failure + through `InitStatus`, so a `require.NoError` alone passes over a failed Init + and surfaces later as an unrelated empty-network-mappings error. Read the + status message in the failure output before tracing the downstream symptom. diff --git a/AGENTS.md b/AGENTS.md new file mode 100644 index 0000000..6bc61b1 --- /dev/null +++ b/AGENTS.md @@ -0,0 +1,168 @@ +# Working in codefly-dev/service-python-fastapi + +This repo is one codefly agent binary: the **FastAPI specialization** of the +generic Python agent. It composes `github.com/codefly-dev/service-python`'s +`pkg/*` types and adds what FastAPI needs — uvicorn supervision and hot reload, +OpenAPI-derived REST endpoints, an opt-in `grpc.aio` listener, the Dockerfile +and kustomize recipes, and the scaffold a new FastAPI service is created from. +Dependency management is `uv`; poetry is not supported. + +It does **not** own: + +- **Generic Python behaviour** — `uv`, AST/ruff/pytest tooling, the Python + `Code` and `Tooling` surfaces. Those come from `service-python` and are + wired through unchanged in `main.go`. A defect there is fixed there. +- **The resource, network and configuration model** — ports, endpoints, + environment injection, readiness, the agent gRPC contracts. That is + `codefly-dev/core`. +- **The `codefly` CLI** (`codefly-dev/cli`, private — do not import it, do not + make CI depend on it) and user workspaces. + +When the change you need belongs to one of those, it gets made there. + +## How to behave + +Fleet standard — [handbook#68](https://github.com/obin-ai/handbook/issues/68). +These bite here because this agent sits between core's resolution machinery and +a user's Python process: nearly every defect is observed as "the service does +not come up", and the local substitute for whatever did not resolve is always +within reach. + +- **A gap in the tooling is a bug in the tooling — never a reason to reach + around it.** When something needs a step `codefly`, `uv` or a companion image + does not perform, the answer is a capability fixed in whichever tool owns it, + and named in the PR. Not a hand-assembled substitute — not as a "workaround", + not "just this once", not "until the capability lands". +- **Never hack. Provide the best fix, even when it spans repos.** The fix + living in `core` or `service-python` is not a reason to work around it here. + Open the PR there and consume the reviewed result. When it genuinely cannot + be fixed now, the deliverable is a precise issue against that owner plus an + explicitly labelled stopgap — never an unlabelled one. +- **Classify every change that makes something work**, in the PR body: a *fix* + at the place that owns the behaviour, or a *hack*. A hack does not become a + fix by working, by being small, by being local, or by the real fix belonging + elsewhere. +- **Never hardcode what the system resolves.** Ports, endpoint addresses, + injected environment and credentials are resolved *for* this agent and then + published by it: `Init` takes the port out of the network mappings, and the + environment the Python process receives is assembled by core's + `EnvironmentVariableManager` from endpoints and workspace configurations. + Typing one of those values in encodes something true only on one machine for + ten minutes — and it fails *quietly*: a runtime missing a credential can skip + registration silently, so the service boots, serves, and is simply absent. +- **Diagnose, do not pattern-match.** "It started working when I set X" is not + a diagnosis — set X back and confirm it breaks. Do not trust an error message + before checking its claim. `TestCreateToRunDocker` failing with `too many + tries` is the standing example: it reports a readiness timeout and usually + means the shared Docker daemon is contended, not that the diff broke startup. +- **Say what you did not verify.** Unverified is not the same as working. The + Docker-backed and image-scanning tests are the slow, real ones; if you did not + run them, the PR says so. + +## Build and test + +Derived from `.github/workflows/ci.yml`, which delegates to core's shared +`go-service-ci.yml` and adds a `race` job of its own. Go comes from `go.mod` +(1.27.0). All of it runs locally: + +```bash +go build ./... # CI: build job +go vet ./... # CI: build job +go mod tidy -diff # CI: build job +go test ./... # CI: test job (runs it with -v) +go test -race ./... # CI: race job +``` + +The `race` job exists because the runner lifecycle (Init / Start / Stop / +Destroy) is concurrent gRPC handlers over shared state: a lost lock is a data +race, not a failed assertion, so the plain `go test` above passes on code with +a mutex removed. Touch `runtime.go`'s locking and `-race` is the run that +matters. The lock order is documented on the struct fields — follow it. + +**Nothing is mocked.** The suite drives a real uvicorn through `uv`, real +containers through the Docker daemon, and a real `syft` scan of an image built +from the recipe this agent emits. Where a tool is absent most of these tests +skip rather than fake, so a green run proves less than it looks — `uv`, `syft`, +`python3` and `nix` all degrade silently. A missing Docker daemon does not: +`TestCreateToRunDocker` runs in a container runtime context with no tooling +guard, so it fails loudly. Never quiet that failure with a skip guard; a loud +red is the correct outcome and the skip would buy a false green. + +**CI has no `nix`, so the nix backend is covered nowhere.** +`testmatrix.ForEachEnvironment` skips any backend missing from the host, and +this repo does not pass `Only(...)` to make absence fatal — so +`TestPythonFastAPILifecycle_Matrix/nix` is the one skip in a green CI run. +Changing `nixflake.go` or `nix/flake.nix` is unverified by CI: exercise it +locally with `nix` installed, and say in the PR that you did. Closing that gap +is issue #49. + +CI installs `uv` and pins `syft` v1.48.0 in `setup-run` so the other halves do +run. The full skip matrix is in the `lifecycle-test-triage` skill. + +The suite is slow because it is real — CI finishes inside `go test`'s default +10-minute timeout, but a contended local Docker daemon pushes the container +cases past it. A local `panic: test timed out` in the lifecycle matrix is that, +not a finding; re-run the suite with `-timeout` raised before concluding +anything from it. + +## Where things live + +| Path | Owns | +| --- | --- | +| `main.go` | settings, agent advertisement, composition of the generic Python layer | +| `runtime.go` | Init/Start/Stop/Destroy, uvicorn supervision, hot reload, the lock order | +| `builder.go` | `Create` scaffolding, OpenAPI endpoint loading, Docker + kustomize recipes | +| `probes.go`, `probe.py` | endpoint health translated to Kubernetes probes / a local probe | +| `templates/` | **what ships** — embedded via `go:embed`, rendered on `Create` | +| `base/` | a checked-in sample service, embedded nowhere | + +`templates/` and `base/` look interchangeable and are not. Only `templates/` is +embedded in the binary, so **editing `base/` changes nothing a user receives**; +it has already drifted from `templates/factory/`. Dependabot watches +`/base/code` for pip updates and `/templates/builder` for the base image, so a +dependency bump landing in `base/code` is not a bump of the scaffold — carry it +into `templates/factory/code/pyproject.toml.tmpl` yourself. + +## Rules that bite + +- **Endpoint contracts are read from fixed locations in the *generated + service*** (not paths in this repo): `openapi/api.swagger.json` for REST and + `proto/api.proto` for gRPC, the latter being core's `standards.ProtoPath`. + Both Builder and Runtime re-derive endpoints from exactly there, and so does + any dependent service. The swagger file does not exist at `Create` time — the + service generates it at runtime from its own `src/openapi.py` — so absent is + a normal state there, not an error to code around. +- **Readiness is the endpoint's declared health check, never an open port.** + Probe intents (readiness / startup / liveness) stay separate through + `resources.PlanEndpointProbes`; collapsing them silently drops a predicate. +- **The runtime image is pinned.** `docker-image` rejects a bare name and + `:latest`. The default `codeflydev/python` companion is built by core and + repinned on release — prefer it. +- **A user's `flake.nix` is never overwritten.** The embedded flake is written + only when the source dir has none. +- **Release needs cgo.** The binary links core's tree-sitter grammars, so + releases go through the `goreleaser-cross` image; `conformance_test.go` is + what keeps `.goreleaser.yaml` and the release workflow honest about it. + +## Procedures + +Step-by-step procedures live in `.claude/skills/`, loaded on demand rather than +carried here: + +- `lifecycle-test-triage` — a lifecycle test is red or hanging, and you need to + tell a real regression from daemon contention, a silent skip, or the timeout. + +## Workflow + +- Branch and PR; never commit to `main`. Conventional Commits for the title. +- Bumping a dependency: `go get` does not tidy. Run `go mod tidy` afterwards, + or `ci / build` goes red on a change that passed every other local check. +- `agent.codefly.yaml` carries the agent version; releases are tag-driven + (`.github/workflows/releaser.yml`), never hand-built. +- Keep this file near ~150 lines, hard cap 200. Push depth into a nested + `AGENTS.md` beside what it describes, or into `.claude/skills/`. The cap and + each skill's frontmatter are asserted in `conformance_test.go`, so `go test + ./...` enforces them with no workflow change. +- There is no `CLAUDE.md`. If one is added, it is a one-line `@AGENTS.md` + pointer — one canonical source, never a second copy to drift. +- Treat this file as code: the PR that changes a process updates it. diff --git a/conformance_test.go b/conformance_test.go index 0735e23..0583adc 100644 --- a/conformance_test.go +++ b/conformance_test.go @@ -294,3 +294,98 @@ func TestReleaseCanCompileTheCgoTheBinaryNeeds(t *testing.T) { "so CGO_ENABLED=1 has no macOS or cross-linux compiler to use") } } + +// agentContextCap extracts the line budget AGENTS.md states for itself. The +// budget is parsed rather than hardcoded so the assertion is that the file +// obeys its own stated rule: a file that tells the next agent to stay under a +// number it has itself exceeded is an instruction that reads as false. +var agentContextCap = regexp.MustCompile(`hard cap (\d+)`) + +// skillFrontmatter is the Agent Skills contract: name and description are the +// only required fields, and the name is what the directory must be called. +type skillFrontmatter struct { + Name string `yaml:"name"` + Description string `yaml:"description"` +} + +func TestAgentContextFilesHonourTheirOwnContract(t *testing.T) { + raw, err := os.ReadFile("AGENTS.md") + if err != nil { + t.Fatalf("read AGENTS.md: %v", err) + } + lines := strings.Count(string(raw), "\n") + + match := agentContextCap.FindSubmatch(raw) + if match == nil { + t.Fatal("AGENTS.md does not state a hard cap; the length budget is unenforceable") + } + stated, err := strconv.Atoi(string(match[1])) + if err != nil { + t.Fatalf("parse hard cap %q: %v", match[1], err) + } + if lines > stated { + t.Errorf("AGENTS.md is %d lines but states a hard cap of %d; push depth into a skill", lines, stated) + } + + // A CLAUDE.md is optional, but when present it must stay a pointer rather + // than a second copy that drifts from this file. + if pointer, err := os.ReadFile("CLAUDE.md"); err == nil { + if strings.TrimSpace(string(pointer)) != "@AGENTS.md" { + t.Errorf("CLAUDE.md must be the single line @AGENTS.md, got %q", string(pointer)) + } + } +} + +func TestSkillsDeclareTheRequiredFrontmatter(t *testing.T) { + entries, err := os.ReadDir(filepath.Join(".claude", "skills")) + if err != nil { + if os.IsNotExist(err) { + t.Skip("no skills directory") + } + t.Fatalf("read skills dir: %v", err) + } + + valid := regexp.MustCompile(`^[a-z0-9-]{1,64}$`) + for _, entry := range entries { + if !entry.IsDir() { + continue + } + path := filepath.Join(".claude", "skills", entry.Name(), "SKILL.md") + raw, err := os.ReadFile(path) + if err != nil { + t.Errorf("%s: %v", path, err) + continue + } + // Frontmatter is the block between the leading --- fence and the next. + body := string(raw) + if !strings.HasPrefix(body, "---\n") { + t.Errorf("%s: no YAML frontmatter", path) + continue + } + end := strings.Index(body[4:], "\n---\n") + if end < 0 { + t.Errorf("%s: frontmatter is not terminated", path) + continue + } + var front skillFrontmatter + if err := yaml.Unmarshal([]byte(body[4:4+end]), &front); err != nil { + t.Errorf("%s: parse frontmatter: %v", path, err) + continue + } + if front.Name != entry.Name() { + t.Errorf("%s: frontmatter name %q does not match directory %q", path, front.Name, entry.Name()) + } + if !valid.MatchString(front.Name) { + t.Errorf("%s: name %q must be lowercase letters, digits and hyphens, at most 64 chars", path, front.Name) + } + // The description is all an agent sees before loading the skill, so an + // empty one makes the skill undiscoverable and an over-long one is + // rejected by the standard. + if front.Description == "" { + t.Errorf("%s: description is required", path) + } + if len(front.Description) > 1024 { + t.Errorf("%s: description is %d chars, over the 1024 limit", path, len(front.Description)) + } + } +}