From 7026548cef74ecee14278e18d3480dc4a852ffbf Mon Sep 17 00:00:00 2001 From: Antoine Toussaint <312536060+antoinetoussaint-byte@users.noreply.github.com> Date: Mon, 21 Sep 2026 12:54:01 -0400 Subject: [PATCH 1/3] docs: adopt the fleet agent-context standard (#42) Add a root AGENTS.md stating what this repo owns and does not own, the build and test commands derived from .github/workflows/ci.yml, and the behavioural rules from obin-ai/handbook#68 anchored to code that already behaves that way. Add one skill, lifecycle-test-triage, for the procedure that is repeated and easy to get wrong: telling a real runner-lifecycle regression from a contended Docker daemon, a tool-missing skip, or the default go test timeout. Co-Authored-By: Claude Opus 5 (1M context) --- .claude/skills/lifecycle-test-triage/SKILL.md | 79 +++++++++ AGENTS.md | 160 ++++++++++++++++++ 2 files changed, 239 insertions(+) create mode 100644 .claude/skills/lifecycle-test-triage/SKILL.md create mode 100644 AGENTS.md diff --git a/.claude/skills/lifecycle-test-triage/SKILL.md b/.claude/skills/lifecycle-test-triage/SKILL.md new file mode 100644 index 0000000..6d1db71 --- /dev/null +++ b/.claude/skills/lifecycle-test-triage/SKILL.md @@ -0,0 +1,79 @@ +--- +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 | +| `docker` | destroy-under-docker | +| `syft`, `docker`, `uv` | the image SBOM inventory | +| `python3` | the active-environment test | + +CI installs `uv` and pins `syft` v1.48.0. If a skip covers the surface you +changed, install the tool and re-run — do not report the green. + +## 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..108f991 --- /dev/null +++ b/AGENTS.md @@ -0,0 +1,160 @@ +# 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. Tests skip rather than fake when a tool is +absent, so a green run on a bare machine has proved much less than it looks: + +| Missing | Silently skips | +| --- | --- | +| `uv` | the real uvicorn lifecycle | +| `docker` | destroy-under-docker | +| `syft` (+ `docker`, `uv`) | the image SBOM inventory | + +CI installs `uv` and pins `syft` v1.48.0 in `setup-run` for exactly that +reason. Get them locally too, or say which halves you did not exercise. + +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 under ~150 lines (hard cap 200). Push depth into a nested + `AGENTS.md` beside what it describes, or into `.claude/skills/`. +- 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. From e2f1387bec34e12132b8e6303b2c14fbf567bc00 Mon Sep 17 00:00:00 2001 From: Antoine Toussaint <312536060+antoinetoussaint-byte@users.noreply.github.com> Date: Mon, 21 Sep 2026 17:50:03 -0400 Subject: [PATCH 2/3] docs: correct the skip matrix in the agent-context files (#42) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The table claimed a missing Docker daemon degrades to a silent skip. TestCreateToRunDocker runs in a container runtime context with no tooling guard, so it fails loudly instead, and the files now say so along with a warning not to quiet it with a skip guard. It also omitted nix. CI has no nix and the matrix skips any absent backend, so TestPythonFastAPILifecycle_Matrix/nix is the one skip in a green CI run and the nix path is covered nowhere — which the files have to state, since they direct attention at nix elsewhere. Co-Authored-By: Claude Opus 5 (1M context) --- .claude/skills/lifecycle-test-triage/SKILL.md | 16 ++++++++--- AGENTS.md | 27 +++++++++++-------- 2 files changed, 28 insertions(+), 15 deletions(-) diff --git a/.claude/skills/lifecycle-test-triage/SKILL.md b/.claude/skills/lifecycle-test-triage/SKILL.md index 6d1db71..924fe1d 100644 --- a/.claude/skills/lifecycle-test-triage/SKILL.md +++ b/.claude/skills/lifecycle-test-triage/SKILL.md @@ -21,12 +21,20 @@ go test -v ./... 2>&1 | grep -E '^\s*--- SKIP' | Missing | Skips | | --- | --- | | `uv` | the real uvicorn lifecycle | -| `docker` | destroy-under-docker | | `syft`, `docker`, `uv` | the image SBOM inventory | | `python3` | the active-environment test | - -CI installs `uv` and pins `syft` v1.48.0. If a skip covers the surface you -changed, install the tool and re-run — do not report the green. +| `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` diff --git a/AGENTS.md b/AGENTS.md index 108f991..71e707e 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -81,17 +81,22 @@ 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. Tests skip rather than fake when a tool is -absent, so a green run on a bare machine has proved much less than it looks: - -| Missing | Silently skips | -| --- | --- | -| `uv` | the real uvicorn lifecycle | -| `docker` | destroy-under-docker | -| `syft` (+ `docker`, `uv`) | the image SBOM inventory | - -CI installs `uv` and pins `syft` v1.48.0 in `setup-run` for exactly that -reason. Get them locally too, or say which halves you did not exercise. +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. + +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 From 98c03ffcc52e199fd8cab199cfaded6ec8accf00 Mon Sep 17 00:00:00 2001 From: Antoine Toussaint <312536060+antoinetoussaint-byte@users.noreply.github.com> Date: Mon, 21 Sep 2026 19:41:43 -0400 Subject: [PATCH 3/3] test: enforce the agent-context contract that review caught it violating (#42) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit AGENTS.md told the next agent to keep the file under ~150 lines while being 165 itself. The stated budget was stricter than the fleet standard it cites — handbook#68 says "~150 lines, hard cap ~200" — so the file was the thing that was wrong, not its length. It now states the fleet's budget. Nothing could have caught that, which is the real cause: an agent-context file is followed literally and no test read it. The conformance test now parses the hard cap out of AGENTS.md and asserts the file obeys the number it publishes, so the budget can never again be advice the artifact itself contradicts, and it holds each skill to the Agent Skills frontmatter contract. Each assertion was proved red by reintroducing the defect it guards. Co-Authored-By: Claude Opus 5 (1M context) --- AGENTS.md | 9 +++-- conformance_test.go | 95 +++++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 101 insertions(+), 3 deletions(-) diff --git a/AGENTS.md b/AGENTS.md index 71e707e..6bc61b1 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -93,7 +93,8 @@ red is the correct outcome and the skip would buy a false green. 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. +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. @@ -158,8 +159,10 @@ carried here: 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 under ~150 lines (hard cap 200). Push depth into a nested - `AGENTS.md` beside what it describes, or into `.claude/skills/`. +- 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)) + } + } +}