diff --git a/REPO_POLICIES.md b/REPO_POLICIES.md index bc2f161..2256291 100644 --- a/REPO_POLICIES.md +++ b/REPO_POLICIES.md @@ -1,6 +1,6 @@ --- title: Repository Policies -last_modified: 2026-07-06 +last_modified: 2026-09-08 --- This document covers repository structure, tooling, and workflow standards. Code @@ -60,17 +60,28 @@ style conventions are in separate documents: prerequisite since nvm requires bash. yarn is then pinned via `corepack prepare yarn@ --activate`. Never install "latest" or "lts"; always exact versions. `script/cibuild` runs the CI build: it changes to the - repo root and runs `docker build .`; the Gitea workflow calls it. Four further - scripts are our own extensions to the standard: `script/check` runs - `script/test`, `script/lint`, and `script/fmt-check`; `script/precommit` is - what the git pre-commit hook runs, and it calls `script/check`; - `script/install-precommit` installs the git pre-commit hook (the `make hooks` - target shims to it); and `script/projectname` (literally that filename) simply - outputs the project's name. Scripts that need the name call - `script/projectname` — e.g. `script/docker` assembles its image tag from it — - so those scripts stay byte-identical across all repos. Repo-type-specific - pre-commit extras (e.g. `go mod tidy` verification in Go repos) belong in - `script/precommit`, not in the hook itself. Model scripts are at + repo root, runs `script/bootstrap`, runs `script/check`, and builds the image + with the version; the Gitea workflow calls it. **`script/cibuild` runs + `script/bootstrap` first**, because the workflow checks out the repo and runs + nothing else, while `script/fmt-check` runs the formatter on the host: on a + pristine checkout with nothing installed the run dies there, after the + containerised gates have passed. **The bootstrap alone is not enough**: + `script/bootstrap` installs node and yarn under nvm and leaves neither on the + `PATH` of the shell that called it, so a bare `yarn` still exits 127. The host + entrypoints that need yarn — `script/fmt` and `script/fmt-check` — therefore + source nvm for the pinned node version before invoking it, exactly as + `script/bootstrap`'s own install step does. A runner carrying nothing but + docker and git then gets through `script/check`. Four further scripts are our + own extensions to the standard: `script/check` runs `script/test`, + `script/lint` and `script/fmt-check`; `script/precommit` is what the git + pre-commit hook runs, and it calls `script/check`; `script/install-precommit` + installs the git pre-commit hook (the `make hooks` target shims to it); and + `script/projectname` (literally that filename) simply outputs the project's + name. Scripts that need the name call `script/projectname` — e.g. + `script/docker` assembles its image tag from it — so those scripts stay + byte-identical across all repos. Repo-type-specific pre-commit extras (e.g. + `go mod tidy` verification in Go repos) belong in `script/precommit`, not in + the hook itself. Model scripts are at `https://git.eeqj.de/sneak/prompts/raw/branch/main/script/`. The README must document the provided scripts in an **Entrypoints** section (see the README requirements below). @@ -89,87 +100,140 @@ style conventions are in separate documents: contributor should be able to understand the entire development workflow by reading the Makefile. -- Every repo should have a `Dockerfile`. All Dockerfiles must run `make check` - as a build step so the build fails if the branch is not green. For non-server - repos, the Dockerfile should bring up a development environment and run - `make check`. For server repos, `make check` should run as an early build - stage before the final image is assembled. Dockerfiles install development - prerequisites by running `script/bootstrap` rather than duplicating installs - inline; COPY `script/` and the dependency manifests (`package.json` + - `yarn.lock`, `go.mod` + `go.sum`, etc.) before running it so the bootstrap - layer stays cached until dependencies change. +- Every repo should have a `Dockerfile`, and it carries the repo's gates: a + `lint` phase and a `test` phase, with the final stage depending on both so the + image cannot be built unless they pass. For non-server repos the final stage + brings up a development environment; for server repos it is the runtime image. + Dockerfiles install development prerequisites by running `script/bootstrap` + rather than duplicating installs inline; COPY `script/` and the dependency + manifests (`package.json` + `yarn.lock`, `go.mod` + `go.sum`, etc.) before + running it. -- **Dockerfiles must use a separate lint stage for fail-fast feedback.** Go - repos use a multistage build where linting runs in an independent stage based - on the `golangci/golangci-lint` image (pinned by hash). This stage runs - `make fmt-check` and `make lint` before the full build begins. The build stage - then declares an explicit dependency on the lint stage via - `COPY --from=lint /src/go.sum /dev/null`, which forces BuildKit to complete - linting before proceeding to compilation and tests. This ensures lint failures - surface in seconds rather than minutes, without blocking on dependency - download or compilation in the build stage. +- **Linting and testing run in Docker, as phases of the `Dockerfile`.** There is + no separate lint file. `script/lint` and `script/test` each build one phase + and nothing else: - The standard pattern for a Go repo Dockerfile is: + ```sh + docker build --no-cache --target lint -t "$(script/projectname)-lint" . + docker build --no-cache --target test -t "$(script/projectname)-test" . + ``` + + **A stage that is not the last one in the file is built only when the final + stage's chain depends on it, or when `--target` names it.** That is why the + two gates are always invoked by name here, and why the final stage carries a + `COPY --from=` of a harmless file from each of them: without that edge a + plain `docker build .` builds the last stage alone and exits 0 having linted + and tested nothing. + + **Every `docker build` in `script/` is tagged**, here and in + `script/cibuild` and `script/docker`. An untagged build leaves a dangling + image behind on every invocation, on every developer host and every CI + runner; a tagged one replaces the previous image. + + Inside a phase the tool is invoked directly — `golangci-lint`, `go test`, + `eslint`, `prettier` — never through `make lint` or `script/test`, which are + themselves a `docker build` and would recurse into a daemon that does not + exist in a build step. Formatting is the exception and stays on the host: + `script/fmt` writes the working tree, and `script/fmt-check` is its + read-only twin. + + **No lint verdict may come from a host invocation of the linter.** On a + shared host golangci-lint reads a result cache keyed on file content rather + than location, so a second checkout of the same content is served the first + one's findings, and a host-global lock in `$TMPDIR` makes concurrent runs + exit non-zero with `parallel golangci-lint is running` — a status a caller + cannot tell from real findings. Both have produced wrong verdicts in this + org, in both directions. A container has its own cache, its own `TMPDIR` and + a digest-pinned binary, so neither is reachable. + +- **Any build that runs checks is built with `--no-cache`.** Docker invalidates + a `COPY` layer only when the copied content changes, so on an unchanged tree + the check `RUN` is served from cache, nothing executes, and the build still + exits 0. Every `docker build` in `script/` therefore passes `--no-cache`: + `script/lint`, `script/test`, `script/cibuild` and `script/docker` are the + four, and there is no fifth — `script/check` runs the two gate phases and + `script/fmt-check`, and builds no image of its own. A bare `docker build .` is + not evidence that anything ran: a sub-second build reporting success is a + cache hit, not a result. Never invalidate by pruning — `docker builder prune` + and friends destroy a build cache shared with every other build on the host. + +- **The gate phases are separate stages, and the build stage depends on both.** + The lint phase is based on the `golangci/golangci-lint` image (pinned by + hash), so lint failures surface in seconds rather than after a full compile, + and the test phase is based on the Go image. The canonical Go repo + `Dockerfile`: ```dockerfile - # Lint stage — fast feedback on formatting and lint issues + # Lint phase # golangci/golangci-lint:v2.x.x, YYYY-MM-DD FROM golangci/golangci-lint@sha256:... AS lint WORKDIR /src COPY go.mod go.sum ./ RUN go mod download COPY . . - RUN make fmt-check - RUN make lint + RUN golangci-lint run --config .golangci.yml ./... - # Build stage + # Test phase # golang:1.x-alpine, YYYY-MM-DD + FROM golang@sha256:... AS test + WORKDIR /src + COPY go.mod go.sum ./ + RUN go mod download + COPY . . + RUN go test -timeout 90s -race -cover ./... || \ + { echo "--- Rerunning with -v for details ---"; \ + go test -timeout 90s -race -v ./...; exit 1; } + + # Build stage. Nothing is wanted from either phase above; the copies + # are what make BuildKit build them first, so this stage cannot run + # unless lint and test passed. + # golang:1.x-alpine, YYYY-MM-DD FROM golang@sha256:... AS builder + COPY --from=lint /src/go.sum /dev/null + COPY --from=test /src/go.sum /dev/null WORKDIR /src - - # Force BuildKit to run the lint stage before proceeding - COPY --from=lint /src/go.sum /dev/null - COPY go.mod go.sum ./ RUN go mod download COPY . . - RUN make test ARG VERSION=dev RUN CGO_ENABLED=0 go build -trimpath \ -ldflags="-s -w -X main.Version=${VERSION}" \ -o /app ./cmd/app/ - # Runtime stage + # Runtime stage, and the last one FROM alpine@sha256:... COPY --from=builder /app /usr/local/bin/app ENTRYPOINT ["app"] ``` Key points: - - The lint stage uses the `golangci/golangci-lint` image directly (it - includes both Go and the linter), so there is no need to install the - linter separately. - - `COPY --from=lint /src/go.sum /dev/null` is a no-op file copy that creates - a stage dependency. BuildKit runs stages in parallel by default; without - this line, the build stage would not wait for lint to finish and a lint - failure might not fail the overall build. + - The lint phase uses the `golangci/golangci-lint` image directly (it has + both Go and the linter), so nothing needs installing. + - `COPY --from= /src/go.sum /dev/null` is a no-op copy whose only + purpose is the ordering edge. BuildKit runs stages in parallel by default, + and a stage nothing depends on is not built at all, so without these two + lines a red gate would not fail the build. + - Keep the runtime stage last, and if you add a stage after it, give it the + same two copies. A plain `docker build .` builds the last stage's chain + and nothing else. - If the project uses `//go:embed` directives that reference build artifacts - (e.g. a web frontend compiled in a separate stage), the lint stage must + (e.g. a web frontend compiled in a separate stage), the lint phase must create placeholder files so the embed directives resolve. Example: `RUN mkdir -p web/dist && touch web/dist/index.html web/dist/style.css`. - The lint stage should not depend on the actual build output — it exists to - fail fast. - If the project requires CGO or system libraries for linting (e.g. - `vips-dev`), install them in the lint stage with `apk add`. - - The build stage runs `make test` after compilation setup. Tests run in the - build stage, not the lint stage, because they may require compiled - artifacts or heavier dependencies. + `vips-dev`), install them in the lint phase with `apk add`. + - `ARG VERSION=dev` is declared in the stage that compiles and supplied by + `script/docker` and `script/cibuild`; no stage may call `git describe`. - Every repo should have a Gitea Actions workflow (`.gitea/workflows/`) that - runs `script/cibuild` (which runs `docker build .`) on push. Since the - Dockerfile already runs `make check`, a successful build implies all checks - pass. + runs `script/cibuild` on push, and checks out the repo as its only other step. + That script bootstraps, runs the gate phases, and then builds the image, so a + successful run means every check passed; a bare `docker build .` does not + carry the same guarantee, because its gate phases may come from the cache. The + image build is uncached and so runs the gate phases a second time. That is the + price of the rule above, and it is worth paying: the image that ships is built + from a run of its own gates rather than from a cache entry. - Use platform-standard formatters: `black` for Python, `prettier` for JS/CSS/Markdown/HTML, `go fmt` for Go. Always use default configuration with @@ -189,14 +253,21 @@ style conventions are in separate documents: module under test to verify it compiles/parses. There is no excuse for `make test` to be a no-op. -- `make test` must complete in under 20 seconds. Add a 30-second timeout in the - Makefile. +- `make test` must complete in under 60 seconds. That is the hard cap, and a + suite that exceeds it fails. Under 20 seconds is the target. A suite between + 20 and 60 seconds is still green, but the overage must be filed as an + improvement bug against that repo. Add a 90-second timeout to the test + invocation (`go test -timeout 90s`). The backstop deliberately sits above the + hard cap so that it catches a genuinely hung test rather than a merely slow + one. -- **`make test` should use the conditional verbose rerun pattern.** Run tests - without `-v` (verbose) first. If tests fail, automatically rerun with `-v` to - show full output. This keeps CI logs and `docker build` output clean on - success (just package/suite summaries) while providing full diagnostic detail - on failure (every test case, every assertion). The general shell pattern: +- **The test command should use the conditional verbose rerun pattern.** Run + tests without `-v` (verbose) first. If tests fail, automatically rerun with + `-v` to show full output. This keeps CI logs and `docker build` output clean + on success (just package/suite summaries) while providing full diagnostic + detail on failure (every test case, every assertion). The command lives in the + `test` phase of the `Dockerfile`, since `script/test` builds that phase; the + Makefile form below is the same pattern for any repo-local invocation: ```makefile test: @@ -209,11 +280,24 @@ style conventions are in separate documents: ```makefile test: - @go test -timeout 30s -race -cover ./... || \ + @go test -count=1 -timeout 90s -race -cover ./... || \ { echo "--- Rerunning with -v for details ---"; \ - go test -timeout 30s -race -v ./...; exit 1; } + go test -count=1 -timeout 90s -race -v ./...; exit 1; } ``` + `-count=1` is required on both invocations: it defeats Go's test _result_ + cache, so the target cannot report a pass it did not earn, and the rerun + reproduces a failure instead of replaying it. It leaves the build cache + alone, so it costs the runtime of the suite and no recompilation. + + Note that this is a second, independent cache, stacked below the Docker + layer cache that [issue #26](https://git.eeqj.de/sneak/prompts/issues/26) + addresses. `CHECK_EPOCH` guarantees the `RUN make test` _step_ re-executes; + it does not guarantee `go test` inside that step does any work, because the + `GOCACHE` baked into earlier image layers survives into the re-executed + step. They are two separate defects requiring two separate fixes, and a fix + for one must not be recorded as covering the other. + Python example: ```makefile @@ -239,10 +323,83 @@ style conventions are in separate documents: must be in `.gitignore`. No exceptions. - `.gitignore` should be comprehensive from the start: OS files (`.DS_Store`), - editor files (`.swp`, `*~`), language build artifacts, and `node_modules/`. - Fetch the standard `.gitignore` from - `https://git.eeqj.de/sneak/prompts/raw/branch/main/.gitignore` when setting up - a new repo. + editor files (`.swp`, `*~`), in-repo agent scratch directories (`.claude/`), + language build artifacts, and `node_modules/`. Fetch the standard `.gitignore` + from `https://git.eeqj.de/sneak/prompts/raw/branch/main/.gitignore` when + setting up a new repo. These patterns are written to `.gitignore`'s own + semantics, in which an unanchored pattern already matches at every depth; they + are not a `.dockerignore` and must not be transplanted into one unmodified. + +- **`.dockerignore` does not use `.gitignore` semantics, and copying patterns + across unmodified leaves secrets in the build context.** Docker matches with + `moby/patternmatcher`: `filepath.Match` semantics plus a `**` extension, so + `*` does not cross `/` and a pattern without a leading `**/` is anchored at + the build-context root. A `.dockerignore` listing `.env`, `*.pem` and `*.key` + therefore excludes only the copies at the repository root, while `config/.env` + and `certs/server.key` still reach the context and can land in an image layer + — which is more dangerous than a short file with no secret patterns at all, + because it reads as solved and stops anyone looking. Give every + depth-independent pattern the `**/` prefix and leave only genuinely + root-anchored entries unprefixed: `.git`, and the repo's own host-built + binary, written `/myapp` and never `**/myapp`, which would also match + `cmd/myapp/` and delete the package directory from the context. Matching is + case-sensitive, and an ALL-CAPS twin per pattern still misses `Server.Key`, so + secret names use character ranges — `**/*.[kK][eE][yY]`, `**/*.[pP][eE][mM]`, + and likewise for `.envrc` and the extensionless SSH keys. Where such a pattern + also catches something the build needs, re-include it with a negation + (`!docs/example.env`); deleting the pattern reopens the exposure for every + other file it covers. Fetch the standard `.dockerignore` from + `https://git.eeqj.de/sneak/prompts/raw/branch/main/.dockerignore` and extend + it with the repo's own artifacts. + +- **In-repo agent scratch belongs in both files, written to each file's own + semantics.** `.claude/` holds one worktree per in-flight agent — an entire + additional checkout of the repo — so under `COPY . .` the build context + inflates by a multiple of the repo and another session's unreviewed work can + be copied into an image layer. In `.gitignore` the entry is `.claude/`, + unanchored. In `.dockerignore` it is `.claude`, anchored and with **no** `**/` + prefix, because the prefixed form would also delete any nested directory of + that name from the build. Anchoring carries a known gap that the canonical + `.dockerignore` states in its own comment, since consuming repos receive the + file and not the tracker: the directory is created in the agent's working + directory, so a repo running agents in subdirectories still ships + `services/api/.claude/` and must add its own anchored entry there. + +- **Excluding `.git` means `git describe` cannot run inside any build stage, and + it fails quietly there.** In a build stage there is no repository, so + `git describe` writes nothing to stdout, `-X main.Version=` comes out empty, + the binary reports no version at all, and the build still exits 0. Compute the + version on the host and thread it in as a build arg. `script/docker` and + `script/cibuild` do this, byte-identically across repos: + + ```sh + # Own line: a failing command substitution inside an argument does not + # trip `set -e`, so the inline form degrades to an empty constant. + version="$(git describe --tags --always --dirty 2>/dev/null || true)" + [ -n "$version" ] || version="unknown" + docker build --no-cache \ + --build-arg VERSION="$version" \ + -t "$(script/projectname)" . + ``` + + `--always` makes an untagged repo yield an abbreviated commit hash rather + than failing, and the `[ -n "$version" ]` line is the single place the + fallback is applied — a live check that fires on a build from an export with + no `.git` and on a repository with no commits yet. Do not fold it into the + substitution as `|| echo unknown`, which makes the guard unreachable. The + Dockerfile's side is `ARG VERSION=dev` in the stage that compiles, declared + there because `ARG` is stage-scoped; passing `VERSION` to a repo whose + Dockerfile declares no such `ARG` is ignored and costs nothing, which is why + the scripts stay byte-identical. One consequence for CI: the standard + checkout action clones shallow and fetches no tags, so a repo that embeds a + tag-derived version must set `fetch-depth: 0` on its checkout step. + +- **Verify `.dockerignore` by enumerating the image, not by reading the + patterns.** Plant files at the root _and_ at least two directories deep, build + a probe image that does `COPY . .`, and list what actually landed + (`docker run --rm --entrypoint find IMAGE /app`). The `transferring context` + size is not a substitute: a nested secret is a few bytes, and BuildKit + transfers only the delta from the previous build. - **No build artifacts in version control.** Code-derived data (compiled bundles, minified output, generated assets) must never be committed to the @@ -258,9 +415,45 @@ style conventions are in separate documents: - Make all changes on a feature branch. You can do whatever you want on a feature branch. -- `.golangci.yml` is standardized and must _NEVER_ be modified by an agent, only - manually by the user. Fetch from - `https://git.eeqj.de/sneak/prompts/raw/branch/main/.golangci.yml`. +- `.golangci.yml` is standardized. The vendored copy in a consuming repo must + _NEVER_ be modified by an agent: fetch it from + `https://git.eeqj.de/sneak/prompts/raw/branch/main/.golangci.yml` and keep it + byte-identical, so that no repo can quietly loosen its own linting. Linter + configuration changes are made to the canonical copy in the `prompts` repo and + reach consuming repos by re-vendoring; an agent may open a PR against + canonical, which only the user merges. One list is exempt from byte-identity, + because it cannot be written once for every repo: the `deny` list of the + `test-support` depguard rule, where a repo names its own test-support packages + by full import path. A repo adds entries there and changes nothing else, and a + re-vendor carries its entries forward. The canonical golangci-lint version is + v2.12.2 (released 2026-05-06), pinned as the digest of the lint phase's base + image + (`golangci/golangci-lint@sha256:5cceeef04e53efe1470638d4b4b4f5ceefd574955ab3941b2d9a68a8c9ad5240`, + which reports `2.12.2 built with go1.26.2 from c0d3ddc9`). That digest is the + only pin, since no repo installs golangci-lint on the host: bumping the + version means changing it and nothing else. + +- **`script/bootstrap` installs a pinned tool by comparing versions, never by + testing presence.** An `if ! command -v ; then install; fi` guard tests + `PATH` only, so on an already-provisioned machine the pin is inert and a + version bump is a silent no-op — while the Dockerfile, installing into a clean + image, gets the pinned version, so a local `make check` and `make docker` can + disagree about what the tool even is. The canonical form: + - compares the installed version against the pin over the **whole** version + token; a parser that stops at the first `-` reports `2.12.2` for a host + running `2.12.2-rc1` and skips the install; + - treats absent, non-zero, empty or unrecognised `--version` output as a + mismatch, so the failure direction is a redundant install and never a + skipped one; + - after installing, re-resolves the binary the way callers do — `hash -r`, + then through `PATH`, not through the directory the installer wrote to — + and fails naming the resolved path, since an install that a shadowing + binary hides succeeds while changing nothing any caller sees; + - is actually called, and prints the version on both success paths: a + function defined and never invoked has the same exit status and the same + empty output as one that worked. + + Keep it POSIX sh: no arrays, no `[[`, no `grep -P`. - When pinning images or packages by hash, add a comment above the reference with the version and date (YYYY-MM-DD). @@ -379,7 +572,9 @@ style conventions are in separate documents: language-specific config). Everything else goes in a subdirectory. Canonical subdirectory names: - `bin/` — executable scripts and tools - - `cmd/` — Go command entrypoints + - `cmd/` — Go command entrypoints; thin only: one `main.go` per binary whose + body is a single call into `internal/` or `pkg/`, no project logic in + `cmd/` - `configs/` — configuration templates and examples - `deploy/` — deployment manifests (k8s, compose, terraform) - `docs/` — documentation and markdown (README.md stays in root) diff --git a/TODO.md b/TODO.md index da0ced6..50a6837 100644 --- a/TODO.md +++ b/TODO.md @@ -31,6 +31,21 @@ P2: security: per-IP rate limiting on the image routes # Completed Steps +- 2026-10-04 `REPO_POLICIES.md` matches the canonical copy again (closes #196): + it is replaced, unchanged, by `prompts/REPO_POLICIES.md` from `sneak/prompts` + `main`. The rules it adds that pixa's tree breaks are filed: + https://git.eeqj.de/sneak/pixa/issues/202 (lint and tests as `Dockerfile` + phases built with `--no-cache`), https://git.eeqj.de/sneak/pixa/issues/203 + (the workflow's `script/docker-smoke` step), + https://git.eeqj.de/sneak/pixa/issues/204 (`.claude/` in `.gitignore`), + https://git.eeqj.de/sneak/pixa/issues/205 (`.dockerignore` patterns at every + depth), https://git.eeqj.de/sneak/pixa/issues/206 (a thin + `cmd/pixad/main.go`) and https://git.eeqj.de/sneak/pixa/issues/208 + (`fetch-depth: 0` on the CI checkout, so the build sees the tags). Its rule + that no build stage runs `git describe` is not followed: pixa takes the + version from the `.git` in the build context, per + https://git.eeqj.de/sneak/pixa/issues/166, as the copy on `sneak/prompts` + `next` already says. - 2026-10-04 a URL made on the generator page with a `ttl` is tested to expire (closes #199): a new test in `internal/handlers` makes a URL on the generator page with a `ttl` of one second, checks that `/v1/e/` serves it at