diff --git a/.github/PULL_REQUEST_TEMPLATE.md b/.github/PULL_REQUEST_TEMPLATE.md index 4fad67df..e2a54dbf 100644 --- a/.github/PULL_REQUEST_TEMPLATE.md +++ b/.github/PULL_REQUEST_TEMPLATE.md @@ -13,7 +13,7 @@ See CONTRIBUTING.md for the full guide. ## Checklist -- [ ] `make verify` is green; `make verify-full` if the change touches `//go:build integration` code +- [ ] `make verify` is green; `make verify-full-container` if the change touches `//go:build integration` code - [ ] `CHANGELOG.md` entry added - [ ] Prose is in Australian English (-ise/-isation, behaviour, …); spec identifiers left canonical - [ ] New/changed behaviour is covered by tests (integration tests gated with `//go:build integration`) diff --git a/AGENTS.md b/AGENTS.md index fd9c0fbb..8b30d6b1 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -76,12 +76,12 @@ Detailed rules and exceptions live in [`docs/contributing.md`](docs/contributing - Keep tests beside code and table-driven where appropriate. `internal/lsp`, `internal/cache`, and `internal/tools` require meaningful coverage; write-tool tests use `WriteDeps{}` and session-isolation tests stay with the package they protect. - Internal tool-to-tool `Execute` calls use canonical parameter names because aliases resolve only at the MCP dispatch boundary. - Gate tests needing external binaries with `//go:build integration`; do not chase TUI coverage. -- Use `topology_affected` during the edit loop. Before delivery run `make verify-full`; run `make lint-cross` after platform-constrained or linter-config changes. `make cover` and `make vuln` remain separate CI/on-demand gates. +- Use `topology_affected` during the edit loop. Before delivery run `make verify-full-container` (the full gate in Docker: its daemon-starting suites refuse on a developer machine); run `make lint-cross` after platform-constrained or linter-config changes. `make cover` and `make vuln` remain separate CI/on-demand gates. - `make test` puts `t.TempDir()` under repository `.testcache`; tests must not assume a system-temp ancestry. ## Delivery and risk -- `make install-hooks` is required after cloning; `make verify-full` (verify + the integration suite) defines ready to commit. `make help` and [`docs/contributing.md`](docs/contributing.md) own the full command matrix. +- `make install-hooks` is required after cloning; `make verify-full` (verify + the integration suite), run via `make verify-full-container` or CI, defines ready to commit. `make help` and [`docs/contributing.md`](docs/contributing.md) own the full command matrix. - Version resolution is exact tag → `VERSION` → short commit. After rebuilding, run `plumb restart`; source changes do not activate in an old daemon. - Use conventional commit types (`feat`, `fix`, `refactor`, `test`, `docs`, `ci`, `chore`). Keep changes bisectable and add a `CHANGELOG.md` entry for each discrete change. - Treat concurrency, the rate limiter, read tracking, and the stats schema as high-risk invariants; inspect their tests and architecture before editing. diff --git a/CHANGELOG.md b/CHANGELOG.md index 21df68d8..812a0c84 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -232,6 +232,22 @@ `workspace_sessions`, and `plumb mail`. A live session in another checkout of the project also counts as a peer for the wake watcher's longer window. No read path runs git. +- **The daemon-starting test suites no longer run on a developer machine.** + `cmd/smoke` (the `integration` tag) and `cmd/clientsmoke` (the `clients`, + `clients_e2e` and `clients_conformance` tags) start plumb daemons of their + own, and a developer machine should run exactly one: the daemon its sessions + use. Their `TestMain` now refuses, exit status 2, before building or starting + anything, unless it finds `GITHUB_ACTIONS=true`, a Linux container marker + (`/.dockerenv`, `/run/.containerenv`), or `PLUMB_SEPARATE_ENV=vm` on Linux + (`internal/separateenv`). This guards against running them by accident; it is + not a security boundary. CI is unaffected. The new + `make verify-full-container` runs `make verify-full` in a Linux container + with pinned Go, golangci-lint and language servers (and Debian trixie's git), + as a non-root user, + on the repository's history with the working tree laid over it. It fails if + two plumb daemons run at once during the suite or one is still running after + it. It replaces a host `make verify-full` as the pre-submit gate. + `make docker-integration` now runs with `--init`. ### Fixed diff --git a/CONTRIBUTING.md b/CONTRIBUTING.md index 48cbf3e4..f4a0ba65 100644 --- a/CONTRIBUTING.md +++ b/CONTRIBUTING.md @@ -43,18 +43,24 @@ would have fixed. ## The definition of "ready to commit" ```sh -make verify-full # build + test + lint + the integration suite — run before every push +make verify-full-container # the full gate, in Docker — run before every push ``` `make verify-full` is the full gate: the same checks as `make verify` plus -`make integration-test`, with the unit tests run once rather than twice. -`make verify` alone is the fast gate CI runs on every pull request; it COMPILES -the `//go:build integration` suite without running it, printing that on success, -and a failing integration test does not turn it red. `make integration-test` -SKIPS tests whose language server is absent, so install gopls and pyright if you -want the whole suite to execute. Run `make verify-full` before submitting, and -`make lint-cross` after platform-constrained or linter-config changes. Other -useful targets: `make test`, `make test-race`, `make lint`, `make tidy`. +`make integration-test`, with the unit tests run once rather than twice. Its +`cmd/smoke` suite starts plumb daemons of its own, and a developer machine must +run exactly one plumb daemon (the one its sessions use), so that suite refuses +to run outside CI, a container or a declared VM. `make verify-full-container` +runs the whole gate in a Linux container with pinned tools, streams in your +working tree, and fails if a plumb daemon outlives it. +`make verify` alone is the fast gate CI runs on every pull request, and the one +to run on your machine; it COMPILES the `//go:build integration` suite without +running it, printing that on success, and a failing integration test does not +turn it red. `make integration-test` SKIPS tests whose language server is +absent; the container has gopls, pyright and the npm language servers. Run +`make verify-full-container` before submitting, and `make lint-cross` after +platform-constrained or linter-config changes. Other useful targets: `make test`, +`make test-race`, `make lint`, `make tidy`. **Formatting note:** apply formatting via `golangci-lint run --fix ./...`, never the standalone `gofumpt -w` binary — the two can pin different versions and produce phantom @@ -101,6 +107,11 @@ real-client tier also gates releases and runs nightly. make install-clients # or: ./scripts/install-clients.sh ``` +These tiers start plumb daemons of their own, so like `cmd/smoke` they refuse on a +developer machine: run them in CI (the client-conformance workflow), in a container you +have installed the client CLIs into (the `make verify-full-container` image has none), or +in a disposable VM with `PLUMB_SEPARATE_ENV=vm`. + Prerequisites: Node 20+ (npm clients), Python 3.8+ (hermes), `curl` (script-installed clients). Some installers drop binaries in `~/.local/bin` or `~/.npm-global/bin` — add those to `PATH` if a binary isn't found. @@ -182,7 +193,7 @@ From a fork, the flow is: ```sh git checkout -b my-change # branch off an up-to-date main # … make your change, then: -make verify-full # must be green +make verify-full-container # must be green git push origin my-change # push to YOUR fork ``` @@ -190,7 +201,7 @@ Then open a pull request from your fork's branch against `plumbkit/plumb`'s `mai refresh a long-running branch, `git fetch upstream && git rebase upstream/main`. - Keep PRs focused and reviewable. -- Ensure `make verify-full` is green and a `CHANGELOG.md` entry is included. +- Ensure `make verify-full-container` is green and a `CHANGELOG.md` entry is included. - Fill out the PR template — it asks the questions that speed up review. **What to expect on `main`.** `main` is a protected branch: every PR must pass the full diff --git a/Makefile b/Makefile index c38158dd..aceaef7a 100644 --- a/Makefile +++ b/Makefile @@ -56,7 +56,7 @@ UNAME_S := $(shell uname -s) CODESIGN_ID := $(if $(CODESIGN_IDENTITY),$(CODESIGN_IDENTITY),-) CODESIGN_BUNDLE := com.plumbkit.plumb -.PHONY: build tool-sizes web-ui web-ui-audit test test-race integration-test fuzz build-integration lint lint-tags lint-cross check-size check-brief check-changelog check-site-claims check-verify-disclosure check-changelog-placement check-changelog-placement-test check-pre-commit cover cover-report vuln tidy-check verify verify-full run clean tidy install install-hooks hooks codesign ts-wasm swift-wasm install-clients clients-test clients-test-auth clients-test-conformance build-clients docker-integration docker-cleanroom site blog demo-gif +.PHONY: build tool-sizes web-ui web-ui-audit test test-race integration-test fuzz build-integration lint lint-tags lint-cross check-size check-brief check-changelog check-site-claims check-verify-disclosure check-changelog-placement check-changelog-placement-test check-pre-commit cover cover-report vuln tidy-check verify verify-full run clean tidy install install-hooks hooks codesign ts-wasm swift-wasm install-clients clients-test clients-test-auth clients-test-conformance build-clients docker-integration docker-cleanroom verify-full-container site blog demo-gif $(TESTCACHE): mkdir -p $(TESTCACHE) @@ -190,12 +190,21 @@ DOCKER_PLATFORM_FLAG := $(if $(DOCKER_PLATFORM),--platform $(DOCKER_PLATFORM),) # working tree; named volumes cache the Go build + module caches across reruns. docker-integration: docker build $(DOCKER_PLATFORM_FLAG) -f build/docker/integration.Dockerfile -t plumb-integration build/docker - docker run --rm $(DOCKER_PLATFORM_FLAG) \ + docker run --rm --init $(DOCKER_PLATFORM_FLAG) \ -v "$(CURDIR)":/src \ -v plumb-gocache:/root/.cache/go-build \ -v plumb-gomod:/go/pkg/mod \ plumb-integration +# verify-full-container runs `make verify-full` in a Linux container, the local +# way to run the whole gate. The gate's daemon-starting suites (cmd/smoke, +# cmd/clientsmoke) refuse on a developer machine, which must run exactly one +# plumb daemon; they run only in CI, a container or a declared VM +# (internal/separateenv). The working tree streams in as git sees it; no plumb +# daemon may outlive the suite. See scripts/verify-full-container.sh. +verify-full-container: + DOCKER_PLATFORM=$(DOCKER_PLATFORM) ./scripts/verify-full-container.sh + # docker-cleanroom proves a fresh Debian with NO toolchain can install and run # plumb end-to-end: a multi-stage build compiles the binary, then a slim runtime # (bash + python3 only) drives the two-agents MCP demo. The demo's exit code is @@ -434,9 +443,9 @@ verify: build test $(VERIFY_CHECKS) @printf '\n%s\n%s\n%s\n%s\n%s\n\n' \ 'verify: PASSED — but the //go:build integration suite was COMPILED, not RUN.' \ ' A failing integration test does not fail this target.' \ - ' Run it before submitting: make integration-test' \ - ' (skips tests whose language server is absent; install gopls and pyright)' \ - ' Whole gate in one command: make verify-full' + ' Its cmd/smoke suite (make integration-test) starts plumb daemons, so it runs' \ + ' in CI or a container, never beside the plumb daemon you are using.' \ + ' Whole gate, in Docker: make verify-full-container' # verify-full is verify's gates plus the integration suite that verify only # compiles. It runs the suite once, without verify's separate `test` step, because @@ -454,4 +463,4 @@ install-hooks: mkdir -p "$$hooks"; \ cp scripts/pre-commit "$$hooks/pre-commit"; \ chmod +x "$$hooks/pre-commit"; \ - echo "Pre-commit hook installed at $$hooks/pre-commit. Run 'make verify' before every push — and 'make verify-full' when the change touches integration-tagged code." + echo "Pre-commit hook installed at $$hooks/pre-commit. Run 'make verify' before every push — and 'make verify-full-container' when the change touches integration-tagged code." diff --git a/build/docker/verify.Dockerfile b/build/docker/verify.Dockerfile new file mode 100644 index 00000000..b90f3a2d --- /dev/null +++ b/build/docker/verify.Dockerfile @@ -0,0 +1,48 @@ +# verify.Dockerfile — the toolchain for `make verify-full-container`. +# +# `make verify-full` runs suites that start plumb daemons of their own +# (cmd/smoke, cmd/clientsmoke), and a developer machine must run exactly one +# plumb daemon, so the full gate runs here instead (or in CI). The image holds +# only tools, never the source: scripts/verify-full-container.sh streams the +# working tree in at run time. +# +# Every tool is pinned, so a green run means the same thing tomorrow. Keep Go in +# step with go.mod's toolchain line, golangci-lint with GOLANGCI_LINT_VERSION in +# .github/workflows/ci.yml, and the language servers with the integration job. +# trixie, not bookworm: its git (2.47) has `merge-tree -X`, which the git tool's +# tests exercise; bookworm's 2.39 does not. +FROM golang:1.27.2-trixie + +# Node only to install the npm-published language servers. +COPY --from=node:22.23.3-bookworm /usr/local/bin/node /usr/local/bin/node +COPY --from=node:22.23.3-bookworm /usr/local/lib/node_modules /usr/local/lib/node_modules +RUN ln -s ../lib/node_modules/npm/bin/npm-cli.js /usr/local/bin/npm \ + && ln -s ../lib/node_modules/npm/bin/npx-cli.js /usr/local/bin/npx + +ARG GOPLS_VERSION=v0.23.0 +ARG GOLANGCI_LINT_VERSION=v2.14.0 +RUN go install "golang.org/x/tools/gopls@${GOPLS_VERSION}" \ + && go install "github.com/golangci/golangci-lint/v2/cmd/golangci-lint@${GOLANGCI_LINT_VERSION}" \ + && go clean -cache -modcache + +# typescript@5: plain `typescript` resolves to 7, which ships no tsserver (see +# the integration job in ci.yml). +RUN npm install -g --no-audit --no-fund \ + pyright@1.1.414 typescript-language-server@6.0.2 typescript@5.9.3 vscode-langservers-extracted@4.10.0 + +ENV PATH="/go/bin:${PATH}" + +# The suite runs as an ordinary user: tests that make a directory unwritable +# with chmod prove nothing as root, who can write it anyway. Its caches live in +# its home, created here so the named cache volumes mounted over them start out +# owned by it. +RUN useradd --create-home --uid 1000 verify \ + && mkdir -p /work /home/verify/.cache/go-build /home/verify/.cache/golangci-lint /home/verify/go/pkg/mod \ + && chown -R verify:verify /work /home/verify +USER verify +ENV HOME=/home/verify GOPATH=/home/verify/go GOCACHE=/home/verify/.cache/go-build \ + GOMODCACHE=/home/verify/go/pkg/mod GOLANGCI_LINT_CACHE=/home/verify/.cache/golangci-lint + +# Fail the image, not the run, when a tool is broken. +RUN gopls version && golangci-lint --version && pyright --version \ + && typescript-language-server --version && command -v vscode-html-language-server diff --git a/cmd/clientsmoke/harness_test.go b/cmd/clientsmoke/harness_test.go index 5e7260d8..dd65df86 100644 --- a/cmd/clientsmoke/harness_test.go +++ b/cmd/clientsmoke/harness_test.go @@ -51,6 +51,7 @@ import ( "testing" "time" + "github.com/plumbkit/plumb/internal/separateenv" "github.com/plumbkit/plumb/internal/sqlitex" ) @@ -65,6 +66,9 @@ var plumbBin string var realHome string func TestMain(m *testing.M) { + // The clients start plumb daemons of their own: refuse on a developer + // machine before anything is built or created (internal/separateenv). + separateenv.RequireForTestMain("cmd/clientsmoke") realHome = os.Getenv("HOME") root, err := findRepoRoot() if err != nil { diff --git a/cmd/smoke/main_test.go b/cmd/smoke/main_test.go new file mode 100644 index 00000000..7effa46b --- /dev/null +++ b/cmd/smoke/main_test.go @@ -0,0 +1,18 @@ +//go:build integration + +package smoke_test + +import ( + "os" + "testing" + + "github.com/plumbkit/plumb/internal/separateenv" +) + +// TestMain refuses on a developer machine before any test builds plumb or +// starts a `plumb serve`, whose daemon would sit beside the developer's own: +// the suite runs in CI or `make verify-full-container` (internal/separateenv). +func TestMain(m *testing.M) { + separateenv.RequireForTestMain("cmd/smoke") + os.Exit(m.Run()) +} diff --git a/docs/contributing.md b/docs/contributing.md index 1650ea30..868e70db 100644 --- a/docs/contributing.md +++ b/docs/contributing.md @@ -46,14 +46,15 @@ both behaviours against a stub linter. | `make lint` | `golangci-lint run` via `scripts/lint-with-retry.sh` — retries with bounded backoff on the shared-cache lock ("parallel golangci-lint is running"), so a peer agent's lint does not read as a failure of this one. | | `make lint-tags` | Lint and format-check the files behind custom build tags (`integration`, `clients*`, `parity`, `race`), which `make lint` never analyses. `scripts/build-tags.sh` reads the tags from the `//go:build` lines of every Go file that is not ignored, so a new tag is covered without a config change. Part of `make verify`. It lints this OS's files only; `make lint-cross` covers the other's. | | `make verify` | Build, test, lint (including `make lint-tags`), compile integration/client binaries, check file/brief/changelog limits, and verify `go.mod` tidiness — the fast gate CI runs on every pull request. It does not RUN the integration suite, and prints that on success. | -| `make verify-full` | The same checks as `make verify` plus `make integration-test` — the full **"ready to commit"** gate, with the unit tests run once. `integration-test` SKIPS tests whose language server is absent, so install gopls and pyright to run all of it. | +| `make verify-full` | The same checks as `make verify` plus `make integration-test` — the full **"ready to commit"** gate, with the unit tests run once. Its `cmd/smoke` suite starts plumb daemons of its own and refuses on a developer machine (`internal/separateenv`), so run it in CI or through `make verify-full-container`. `integration-test` SKIPS tests whose language server is absent. | +| `make verify-full-container` | `make verify-full` in a Linux container with pinned Go, golangci-lint and language servers: streams in the working tree as git sees it, runs with `--init` and `--rm`, and fails if a plumb daemon is left after the suite. The local way to run the full gate. | | `make lint-cross` | Statically lint and vet the other supported OS, its tagged files included; required after platform-constrained or linter-config changes. | | `make cover` | Enforce the whole-tree statement floor from `scripts/check-coverage.sh`. | | `make vuln` | Run `govulncheck`; requires network access. | | `make tidy` | `go mod tidy` | | `make clean` | Remove build artefacts. | | `make install-clients` | Install every supported client CLI, for local client-integration testing. | -| `make clients-test` / `make clients-test-auth` | On-demand connection/auth tiers that drive each installed client CLI headless (own build tags, never part of `make verify`) — see the comments above these targets in the `Makefile`. | +| `make clients-test` / `make clients-test-auth` | On-demand connection/auth tiers that drive each installed client CLI headless (own build tags, never part of `make verify`) — see the comments above these targets in the `Makefile`. Like `cmd/smoke` they start plumb daemons, so they refuse outside CI, a container or a declared VM. | > **Formatting:** always format via `golangci-lint run --fix ./...` (the fix the > hook names when it fails), not a standalone `gofumpt` binary — the two can pin diff --git a/internal/arch/layers.go b/internal/arch/layers.go index 2aa14c16..3f48c396 100644 --- a/internal/arch/layers.go +++ b/internal/arch/layers.go @@ -94,6 +94,7 @@ var Layers = map[string]Layer{ "internal/textfmt": LayerFoundation, // stdlib-only text primitives, no lipgloss "internal/toolerror": LayerFoundation, // stdlib-only structured tool-error contract "internal/redact": LayerFoundation, + "internal/separateenv": LayerFoundation, // where a test suite may start its own plumb daemon "internal/theme": LayerFoundation, // UI-agnostic hex palettes, no lipgloss "internal/render": LayerFoundation, // pure presentation helpers "internal/quality": LayerFoundation, // post-write analyser interface diff --git a/internal/separateenv/separateenv.go b/internal/separateenv/separateenv.go new file mode 100644 index 00000000..7d8e714e --- /dev/null +++ b/internal/separateenv/separateenv.go @@ -0,0 +1,111 @@ +// Package separateenv tells a test harness whether it runs somewhere a plumb +// daemon of its own may be started. +// +// A developer machine runs exactly one plumb daemon, the one its sessions use. +// Test suites that start, kill or restart daemons (cmd/smoke, cmd/clientsmoke) +// therefore run only in a separate environment: a CI runner, a container or a +// disposable VM. Their TestMain calls RequireForTestMain, which refuses on +// anything else before the suite creates a directory, builds a binary or +// starts a process. +// +// This guards against running such a suite on a developer machine by accident +// (an agent, a habit, a stale recipe); it is not a security boundary, since +// anyone can fake the evidence. It accepts exactly three kinds: +// - GITHUB_ACTIONS=true, on any OS: a hosted CI runner; +// - Linux with a readable regular-file container marker, /.dockerenv +// (Docker) or /run/.containerenv (Podman); +// - Linux with PLUMB_SEPARATE_ENV=vm: an explicit declaration that this is a +// disposable VM. +// +// Nothing else counts: no CI=true, no override, no case folding, and a marker +// that cannot be read counts as absent. testdata/vectors.json is byte-identical +// to the vectors of the same rule in the dsh-plumb-identity e2e. +package separateenv + +import ( + "fmt" + "os" + "runtime" + "strings" +) + +// The kinds of separate environment Detect reports. +const ( + GitHubActions = "github-actions" + Container = "container" + VM = "vm" +) + +// Probe is what Detect reads, injectable for tests. +type Probe struct { + Getenv func(string) string + GOOS string + Readable func(path string) bool +} + +// ContainerMarkers are the files a container runtime creates at the root. +func ContainerMarkers() []string { + return []string{"/.dockerenv", "/run/.containerenv"} +} + +// Detect returns the kind of separate environment p describes, or "" when it +// may be a developer machine. +func Detect(p Probe) string { + if p.Getenv("GITHUB_ACTIONS") == "true" { + return GitHubActions + } + if p.GOOS != "linux" { + return "" + } + for _, marker := range ContainerMarkers() { + if p.Readable(marker) { + return Container + } + } + if p.Getenv("PLUMB_SEPARATE_ENV") == "vm" { + return VM + } + return "" +} + +// Current is Detect for this process. +func Current() string { + return Detect(Probe{Getenv: os.Getenv, GOOS: runtime.GOOS, Readable: ReadableFile}) +} + +// ReadableFile reports whether path is a regular file this process can open +// for reading. Anything else, including an error, is false. +func ReadableFile(path string) bool { + info, err := os.Stat(path) + if err != nil || !info.Mode().IsRegular() { + return false + } + f, err := os.Open(path) //nolint:gosec // a fixed marker path, opened only to prove it is readable + if err != nil { + return false + } + _ = f.Close() + return true +} + +// Refusal is the message a suite named what prints when Current is "". +func Refusal(what string) string { + return strings.Join([]string{ + what + ": refusing to run here: it starts plumb daemons of its own, and a developer", + "machine must run exactly one plumb daemon (the one its sessions use).", + "Run it in a separate environment instead: `make verify-full-container`", + "(Docker), or CI. Accepted evidence: GITHUB_ACTIONS=true; Linux with a readable", + "/.dockerenv or /run/.containerenv; Linux with PLUMB_SEPARATE_ENV=vm (a disposable VM).", + }, "\n") +} + +// RequireForTestMain exits the test binary with status 2 and Refusal(what) +// unless this process runs in a separate environment. Call it first in +// TestMain, before anything is created or started. +func RequireForTestMain(what string) { + if Current() != "" { + return + } + fmt.Fprintln(os.Stderr, Refusal(what)) + os.Exit(2) +} diff --git a/internal/separateenv/separateenv_test.go b/internal/separateenv/separateenv_test.go new file mode 100644 index 00000000..20dfa78a --- /dev/null +++ b/internal/separateenv/separateenv_test.go @@ -0,0 +1,131 @@ +package separateenv + +import ( + "encoding/json" + "errors" + "os" + "os/exec" + "path/filepath" + "slices" + "strings" + "testing" +) + +type vector struct { + Name string `json:"name"` + Platform string `json:"platform"` + Env map[string]string `json:"env"` + Readable []string `json:"readable"` + Expect *string `json:"expect"` +} + +func TestDetect_SharedVectors(t *testing.T) { + raw, err := os.ReadFile(filepath.Join("testdata", "vectors.json")) + if err != nil { + t.Fatal(err) + } + var doc struct { + Cases []vector `json:"cases"` + } + if err := json.Unmarshal(raw, &doc); err != nil { + t.Fatal(err) + } + if len(doc.Cases) == 0 { + t.Fatal("no vectors") + } + // The vectors are shared with the Node copy of this rule, which names + // platforms as process.platform does. + nodePlatformToGOOS := map[string]string{"win32": "windows"} + for _, c := range doc.Cases { + t.Run(c.Name, func(t *testing.T) { + goos := c.Platform + if mapped, ok := nodePlatformToGOOS[goos]; ok { + goos = mapped + } + got := Detect(Probe{ + Getenv: func(k string) string { return c.Env[k] }, + GOOS: goos, + Readable: func(p string) bool { return slices.Contains(c.Readable, p) }, + }) + want := "" + if c.Expect != nil { + want = *c.Expect + } + if got != want { + t.Fatalf("Detect = %q, want %q", got, want) + } + }) + } +} + +func TestContainerMarkers_MatchTheVectors(t *testing.T) { + if got, want := ContainerMarkers(), []string{"/.dockerenv", "/run/.containerenv"}; !slices.Equal(got, want) { + t.Fatalf("ContainerMarkers = %v, want %v", got, want) + } +} + +func TestReadableFile(t *testing.T) { + dir := t.TempDir() + file := filepath.Join(dir, "marker") + if err := os.WriteFile(file, nil, 0o600); err != nil { + t.Fatal(err) + } + if !ReadableFile(file) { + t.Error("an empty regular file (Docker writes /.dockerenv empty) must be readable") + } + if ReadableFile(dir) { + t.Error("a directory is not a marker") + } + if ReadableFile(filepath.Join(dir, "missing")) { + t.Error("a missing path is not a marker") + } + dangling := filepath.Join(dir, "dangling") + if err := os.Symlink(filepath.Join(dir, "missing"), dangling); err != nil { + t.Fatal(err) + } + if ReadableFile(dangling) { + t.Error("a dangling symlink is not a marker") + } + locked := filepath.Join(dir, "locked") + if err := os.WriteFile(locked, nil, 0o000); err != nil { + t.Fatal(err) + } + if os.Geteuid() != 0 && ReadableFile(locked) { + t.Error("a file this process cannot read is not a marker") + } +} + +// TestRequireForTestMain_RefusesOnAHost runs this test binary again as a child +// that calls RequireForTestMain with the CI and VM evidence removed, so only a +// container marker could let it through. +func TestRequireForTestMain_RefusesOnAHost(t *testing.T) { + if os.Getenv("SEPARATEENV_CHILD") == "1" { + RequireForTestMain("separateenv-child") + os.Exit(0) + } + child := Probe{Getenv: func(string) string { return "" }, GOOS: "linux", Readable: ReadableFile} + if Detect(child) != "" { + t.Skip("this process runs in a container, so the child would be let through") + } + cmd := exec.Command(os.Args[0], "-test.run=^TestRequireForTestMain_RefusesOnAHost$") //nolint:gosec // re-runs this test binary + cmd.Env = append(withoutKeys(os.Environ(), "GITHUB_ACTIONS", "PLUMB_SEPARATE_ENV"), "SEPARATEENV_CHILD=1") + out, err := cmd.CombinedOutput() + var exit *exec.ExitError + if !errors.As(err, &exit) || exit.ExitCode() != 2 { + t.Fatalf("child: err=%v, want exit status 2\n%s", err, out) + } + if !strings.Contains(string(out), "separateenv-child: refusing to run here") { + t.Fatalf("child output lacks the refusal:\n%s", out) + } +} + +func withoutKeys(env []string, keys ...string) []string { + out := env[:0:0] + for _, kv := range env { + key, _, _ := strings.Cut(kv, "=") + if !slices.Contains(keys, key) { + out = append(out, kv) + } + } + return out +} diff --git a/internal/separateenv/testdata/vectors.json b/internal/separateenv/testdata/vectors.json new file mode 100644 index 00000000..ffdcc7cf --- /dev/null +++ b/internal/separateenv/testdata/vectors.json @@ -0,0 +1,23 @@ +{ + "comment": "Shared vectors for the separate-environment rule (PLAN-526). Byte-identical copies live in dsh-plumb-identity test/fixtures/separate-env-vectors.json and plumb internal/separateenv/testdata/vectors.json; every implementation must give `expect` for each case. `readable` lists the container markers that are readable regular files; whether a path qualifies is tested separately against the real filesystem.", + "cases": [ + { "name": "darwin, no evidence", "platform": "darwin", "env": {}, "readable": [], "expect": null }, + { "name": "darwin, GitHub Actions", "platform": "darwin", "env": { "GITHUB_ACTIONS": "true" }, "readable": [], "expect": "github-actions" }, + { "name": "linux, GitHub Actions", "platform": "linux", "env": { "GITHUB_ACTIONS": "true" }, "readable": [], "expect": "github-actions" }, + { "name": "GITHUB_ACTIONS=1 is not exact", "platform": "linux", "env": { "GITHUB_ACTIONS": "1" }, "readable": [], "expect": null }, + { "name": "GITHUB_ACTIONS=TRUE is not exact", "platform": "darwin", "env": { "GITHUB_ACTIONS": "TRUE" }, "readable": [], "expect": null }, + { "name": "CI=true alone is not evidence", "platform": "linux", "env": { "CI": "true" }, "readable": [], "expect": null }, + { "name": "linux, no evidence", "platform": "linux", "env": {}, "readable": [], "expect": null }, + { "name": "linux, /.dockerenv", "platform": "linux", "env": {}, "readable": ["/.dockerenv"], "expect": "container" }, + { "name": "linux, /run/.containerenv", "platform": "linux", "env": {}, "readable": ["/run/.containerenv"], "expect": "container" }, + { "name": "darwin ignores container markers", "platform": "darwin", "env": {}, "readable": ["/.dockerenv"], "expect": null }, + { "name": "linux, declared VM", "platform": "linux", "env": { "PLUMB_SEPARATE_ENV": "vm" }, "readable": [], "expect": "vm" }, + { "name": "VM declaration is case-sensitive", "platform": "linux", "env": { "PLUMB_SEPARATE_ENV": "VM" }, "readable": [], "expect": null }, + { "name": "VM declaration is not trimmed", "platform": "linux", "env": { "PLUMB_SEPARATE_ENV": "vm " }, "readable": [], "expect": null }, + { "name": "PLUMB_SEPARATE_ENV=1 is not a declaration", "platform": "linux", "env": { "PLUMB_SEPARATE_ENV": "1" }, "readable": [], "expect": null }, + { "name": "darwin cannot declare a VM", "platform": "darwin", "env": { "PLUMB_SEPARATE_ENV": "vm" }, "readable": [], "expect": null }, + { "name": "windows, no evidence", "platform": "win32", "env": {}, "readable": [], "expect": null }, + { "name": "GitHub Actions wins over a VM declaration", "platform": "linux", "env": { "GITHUB_ACTIONS": "true", "PLUMB_SEPARATE_ENV": "vm" }, "readable": [], "expect": "github-actions" }, + { "name": "container marker wins over a VM declaration", "platform": "linux", "env": { "PLUMB_SEPARATE_ENV": "vm" }, "readable": ["/.dockerenv"], "expect": "container" } + ] +} diff --git a/internal/separateenv/wiring_test.go b/internal/separateenv/wiring_test.go new file mode 100644 index 00000000..a61506ca --- /dev/null +++ b/internal/separateenv/wiring_test.go @@ -0,0 +1,56 @@ +package separateenv + +import ( + "go/ast" + "go/parser" + "go/token" + "path/filepath" + "testing" +) + +// TestGuardIsFirstInEachDaemonStartingTestMain pins the wiring the library +// tests cannot: CI and the verify container both count as separate +// environments, so deleting a guard call would leave every run green. Each +// suite that starts plumb daemons must call RequireForTestMain as the first +// statement of its TestMain, before anything is built or created. +func TestGuardIsFirstInEachDaemonStartingTestMain(t *testing.T) { + for _, file := range []string{ + filepath.Join("..", "..", "cmd", "smoke", "main_test.go"), + filepath.Join("..", "..", "cmd", "clientsmoke", "harness_test.go"), + } { + t.Run(file, func(t *testing.T) { + f, err := parser.ParseFile(token.NewFileSet(), file, nil, parser.SkipObjectResolution) + if err != nil { + t.Fatal(err) + } + var main *ast.FuncDecl + for _, decl := range f.Decls { + if fn, ok := decl.(*ast.FuncDecl); ok && fn.Recv == nil && fn.Name.Name == "TestMain" { + main = fn + } + } + if main == nil || main.Body == nil || len(main.Body.List) == 0 { + t.Fatal("no TestMain with a body") + } + stmt, ok := main.Body.List[0].(*ast.ExprStmt) + if !ok { + t.Fatal("the first statement of TestMain is not a call") + } + call, ok := stmt.X.(*ast.CallExpr) + if !ok { + t.Fatal("the first statement of TestMain is not a call") + } + sel, ok := call.Fun.(*ast.SelectorExpr) + pkg, pkgOK := func() (*ast.Ident, bool) { + if !ok { + return nil, false + } + id, ok := sel.X.(*ast.Ident) + return id, ok + }() + if !ok || !pkgOK || pkg.Name != "separateenv" || sel.Sel.Name != "RequireForTestMain" { + t.Fatalf("the first statement of TestMain must be separateenv.RequireForTestMain(...)") + } + }) + } +} diff --git a/scripts/pre-commit b/scripts/pre-commit index cd6fc6e3..a2f1992e 100644 --- a/scripts/pre-commit +++ b/scripts/pre-commit @@ -1,6 +1,6 @@ #!/bin/sh # Pre-commit: build + format + lint. -# For the full sweep (including tests) run: make verify-full +# For the full sweep (including tests) run: make verify-full-container set -e go build ./... @@ -26,7 +26,7 @@ fi if [ -z "$GOLANGCI_LINT" ]; then echo "pre-commit: golangci-lint not found on PATH, in \$GOBIN, or in \$GOPATH/bin." >&2 echo " install it (https://golangci-lint.run/welcome/install/), or set GOBIN," >&2 - echo " or commit with --no-verify if you must and run 'make verify-full' before pushing." >&2 + echo " or commit with --no-verify if you must and run 'make verify-full-container' before pushing." >&2 exit 1 fi diff --git a/scripts/verify-full-container.sh b/scripts/verify-full-container.sh new file mode 100755 index 00000000..4b252a77 --- /dev/null +++ b/scripts/verify-full-container.sh @@ -0,0 +1,193 @@ +#!/usr/bin/env bash +# verify-full-container.sh — run `make verify-full` in a Linux container. +# +# The full gate includes suites that start plumb daemons of their own +# (cmd/smoke, cmd/clientsmoke), and a developer machine must run exactly one +# plumb daemon: those suites refuse to run outside CI, a container or a declared +# VM (internal/separateenv). This is the local way to run the whole gate. +# +# The container gets the repository's history (a git bundle of every ref, tags +# included, which the version and history-reading tests need) checked out at +# HEAD, and over it the working tree as git sees it (tracked and untracked +# files, not ignored ones, minus deleted ones), so uncommitted changes are +# verified and host build output is not. It runs as a non-root user. The container runs with --init (exited processes are +# reaped, not counted) and --rm: its exit ends every process it started. +# While the suite runs, the plumb daemons are counted about every 200 ms (plus +# the time a scan takes): at most one may run at a time, and at least one must +# be seen, since cmd/smoke always starts one. Afterwards none may be left: they +# are looked for for 30 s or more. +# The run fails if either rule breaks, or if the process table cannot be read. +# +# make verify-full-container (DOCKER_PLATFORM=linux/amd64 for amd64) + +set -uo pipefail + +root=$(git rev-parse --show-toplevel) || exit 1 +platform=() +[ -n "${DOCKER_PLATFORM:-}" ] && platform=(--platform "$DOCKER_PLATFORM") +image=plumb-verify + +docker build ${platform[@]+"${platform[@]}"} -q -f "$root/build/docker/verify.Dockerfile" -t "$image" "$root/build/docker" > /dev/null || exit 1 + +name=plumb-verify-$(date +%s)-$$ + +inside=' +set -uo pipefail +cd /work || exit 70 +git -c init.defaultBranch=main clone -q --no-checkout /in/repo.bundle . && git checkout -q --detach "$(cat /in/head)" || exit 70 +tar -xf /in/tree.tar || exit 70 +while IFS= read -r -d "" f; do rm -f -- "$f"; done < /in/deleted || exit 70 + +# The plumb daemons alive now, by their own argv; "?" when the process table +# cannot be read. A process that exited mid-scan is skipped, but an empty +# listing is not "none": this shell is a process, so /proc must list one. +# Until exec, a forked child still shows its parent argv, so a daemon starting +# gopls or git briefly looks like a second daemon. The kernel marks that state +# with PF_FORKNOEXEC (0x40) in the stat flags (field 9), so stat is read FIRST +# and a flagged process is skipped; only then is cmdline read, which after exec +# is the process own. A real plumb daemon counts, whatever its parent. +daemons() { + local n=0 dir stat argv listed=0 rest matched="" + for dir in /proc/[0-9]*; do + [ -d "$dir" ] || continue + listed=1 + if ! stat=$({ cat "$dir/stat"; } 2> /dev/null); then + [ -d "$dir" ] && { echo "?"; return; } + continue + fi + # "pid (comm) state ppid pgrp session tty tpgid flags ...": comm may hold + # spaces or ")", so the greedy match ends at the last ") ". A line without + # that header, or naming another pid, is not trusted. + [[ "$stat" =~ ^([0-9]+)\ [\(](.*)[\)]\ (.*)$ ]] && [ "${BASH_REMATCH[1]}" = "${dir#/proc/}" ] || { echo "?"; return; } + rest=${BASH_REMATCH[3]} + flags="" + read -r _ _ _ _ _ _ flags _ <<< "$rest" + [[ "$flags" =~ ^[0-9]+$ ]] || { echo "?"; return; } + (( flags & 0x40 )) && continue # forked, not yet exec-ed + if ! argv=$({ tr "\0" " " < "$dir/cmdline"; } 2> /dev/null); then + [ -d "$dir" ] && { echo "?"; return; } + continue + fi + [[ "$argv" =~ ^([^\ ]*/)?plumb\ daemon(\ |$) ]] && { n=$((n + 1)); matched="$matched ${dir#/proc/}"; } + done + [ "$listed" -eq 1 ] || { echo "?"; return; } + echo "$matched" > /tmp/daemons.now 2> /dev/null # diagnostics only + echo "$n" +} + +# The peak during the suite and the number of samples, rewritten atomically to +# /tmp/peak after every sample ("?" sticks once the table could not be read). +supervise() { + local peak=0 samples=0 n + while :; do + n=$(daemons) + [[ "$n" =~ ^[0-9]+$ ]] || n="?" # anything but a count is an unread table + samples=$((samples + 1)) + if [ "$n" = "?" ]; then peak="?"; elif [ "$peak" != "?" ] && [ "$n" -gt "$peak" ]; then peak=$n; fi + # Diagnostics only (never the verdict): who the daemons were when more than + # one ran, so an overlap can be traced to the tests that caused it. + if [ "$n" != "?" ] && [ "$n" -gt 1 ] && [ "$({ wc -l < /tmp/overlaps; } 2> /dev/null || echo 0)" -lt 40 ]; then + for pid in $(cat /tmp/daemons.now 2> /dev/null); do + echo "$(date +%T.%N) pid=$pid ppid=$(awk "{print \$4}" /proc/$pid/stat 2> /dev/null) home=$(tr "\0" "\n" < /proc/$pid/environ 2> /dev/null | sed -n "s/^HOME=//p") started=$(ps -o lstart= -p $pid 2> /dev/null)" >> /tmp/overlaps + done + fi + # A checkpoint that cannot be written ends the watcher (status 3), which the + # checks after the suite reject, rather than leave an older record trusted. + { echo "$peak $samples" > /tmp/peak.new && mv /tmp/peak.new /tmp/peak; } || exit 3 + sleep 0.2 + done +} +rm -f /tmp/peak /tmp/peak.new /tmp/overlaps +supervise & +supervisor=$! + +# Every test runs: the build cache volume is reused between runs, and a test +# result replayed from it would prove nothing, least of all about daemons. +GOFLAGS=-count=1 make verify-full +rc=$? +echo "verify-full-container: make verify-full exited $rc" + +# Trust the peak only from a watcher that ran for the whole suite: alive now, +# stopped by us (TERM, so it exits 143), with a readable, numeric record. +watched=yes +if kill -0 "$supervisor" 2> /dev/null; then + kill -TERM "$supervisor" + wait "$supervisor" + status=$? + [ "$status" -eq 143 ] || { watched=no; echo "verify-full-container: the daemon watcher exited $status, not 143 on our TERM"; } +else + watched=no + echo "verify-full-container: the daemon watcher died during the suite" +fi +peak="?" samples=0 +read -r peak samples < /tmp/peak 2> /dev/null +[[ "$samples" =~ ^[0-9]+$ ]] && [ "$samples" -gt 0 ] || { watched=no; echo "verify-full-container: the daemon watcher left no samples"; } +if [ "$watched" = no ]; then + [ "$rc" -eq 0 ] && rc=1 +else + case "$peak" in + 0) echo "verify-full-container: no plumb daemon was seen during the suite ($samples samples), though cmd/smoke starts one: the watcher saw nothing"; [ "$rc" -eq 0 ] && rc=1 ;; + 1) echo "verify-full-container: at most 1 plumb daemon ran at a time during the suite ($samples samples)" ;; + "?") echo "verify-full-container: the process table could not be read during the suite"; [ "$rc" -eq 0 ] && rc=1 ;; + *) echo "verify-full-container: $peak plumb daemons ran at once during the suite (at most 1)"; [ "$rc" -eq 0 ] && rc=1 + sed "s/^/verify-full-container: overlap: /" /tmp/overlaps 2> /dev/null ;; + esac +fi + +seen=0 +for _ in $(seq 1 60); do + n=$(daemons) + [[ "$n" =~ ^[0-9]+$ ]] || { echo "verify-full-container: cannot read the process table"; exit 1; } + [ "$n" -gt "$seen" ] && seen=$n + sleep 0.5 +done +if [ "$seen" -gt 0 ]; then + echo "verify-full-container: $seen plumb daemon(s) still running after the suite (watched 30 s)" + [ "$rc" -eq 0 ] && rc=1 +else + echo "verify-full-container: no plumb daemon left after the suite (watched 30 s)" +fi +exit "$rc" +' + +in=$(mktemp -d "${TMPDIR:-/tmp}/plumb-verify-in.XXXXXX") || exit 1 +trap 'rm -rf "$in"' EXIT +# A signal kills the container but does not exit: the run still ends with the +# engine's answer on whether the container is gone, and then fails. +interrupted=0 +trap 'interrupted=1; docker kill "$name" > /dev/null 2>&1' INT TERM HUP +( + cd "$root" || exit 1 + git bundle create "$in/repo.bundle" --all 2> /dev/null && + git rev-parse HEAD > "$in/head" && + git ls-files -z --deleted > "$in/deleted" && + git ls-files -z --cached --others --exclude-standard | + while IFS= read -r -d '' f; do { [ -e "$f" ] || [ -L "$f" ]; } && printf '%s\0' "$f"; :; done | + COPYFILE_DISABLE=1 tar --no-xattrs --null -T - -cf "$in/tree.tar" # no macOS ._* AppleDouble entries or xattr headers +) || { echo "verify-full-container: could not package the repository (git bundle, ls-files or tar failed; see above)" >&2; exit 1; } +chmod -R a+rX "$in" + +docker run --rm --init --name "$name" ${platform[@]+"${platform[@]}"} \ + -v "$in:/in:ro" \ + -v plumb-verify-v2-gocache:/home/verify/.cache/go-build \ + -v plumb-verify-v2-gomod:/home/verify/go/pkg/mod \ + -v plumb-verify-v2-lintcache:/home/verify/.cache/golangci-lint \ + "$image" bash -c "$inside" & +child=$! # in the background and waited for, so the trap can run meanwhile +wait "$child" +rc=$? +kill -0 "$child" 2> /dev/null && { wait "$child"; rc=$?; } +[ "$interrupted" -eq 1 ] && { echo "verify-full-container: interrupted" >&2; rc=130; } + +# Container exit is what ends a SIGKILL'd run's processes, so "gone" must be +# an answer from the engine, never the failure to get one. +if ! listed=$(docker ps -a --filter "name=^/$name\$" --format '{{.Names}}'); then + echo "verify-full-container: could not ask docker whether container $name is gone" >&2 + rc=1 +elif [ -n "$listed" ]; then + echo "verify-full-container: container $name still exists" >&2 + rc=1 +else + echo "verify-full-container: container $name is gone, so every process it started has ended" +fi +exit "$rc"