Skip to content

contributing.md

Latest commit

 

History

History
195 lines (165 loc) · 13.7 KB

contributing.md

File metadata and controls

195 lines (165 loc) · 13.7 KB
Raw

Contributing

Thanks for working on plumb. AGENTS.md is the compact, always-loaded contract; this page owns the detailed build, code-style, testing, and delivery guidance. Read the brief first, then use this page while changing the public source.

Set up

git clone https://github.com/plumbkit/plumb
cd plumb
make install-hooks    # REQUIRED after every fresh clone
make build

make install-hooks installs a pre-commit hook that runs golangci-lint run --allow-parallel-runners ./..., so formatting and lint issues are caught before they reach the tree. The hook only CHECKS: an unformatted file fails the commit, the hook lists it, and you run golangci-lint run --fix ./..., review, re-stage and commit again. It never rewrites files itself, because a rewrite after the commit's content was chosen left a dirty tree behind a passing commit. --allow-parallel-runners keeps a peer agent's lint from failing your commit on the shared lock. make check-pre-commit (part of make verify) pins both behaviours against a stub linter.

Keep your golangci-lint on the pinned version. GOLANGCI_LINT_VERSION in .github/workflows/ci.yml is the single source of truth, and the pre-commit hook reads it to warn when your binary differs. The warning matters because a newer binary brings newer gofumpt rules, so the hook can fail on files your commit never touched. Do not reformat those as part of your commit. To match the pin: go install github.com/golangci/golangci-lint/v2/cmd/golangci-lint@v<pinned> — note that a Homebrew-installed golangci-lint drifts on its own schedule and will re-break the match on the next brew upgrade. When the newer version is the one we want instead, bump the pin and commit any reformatting as its own commit.

Build & verify

Command Does
make build Compile to ./plumb, version stamped from git/VERSION.
make test go test ./...
make test-pure Build the whole tree and run the account tests with CGO_ENABLED=0 — the mode .goreleaser.yml releases in, and the mode no other command here uses. A C compiler turns cgo on by default, so every other gate compiles a linux && !cgo file out of its build entirely: internal/account/home_linux_pure.go is in no make test, make verify or make integration-test binary at all. Linux only, and the target refuses elsewhere rather than passing quietly — darwin has no cgo/pure split. Runs in CI on ubuntu-latest and inside make verify-full-container.
make test-race go test -race ./...
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. 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. It runs the CGO_ENABLED=0 leg (make test-pure) too, which the image's own gcc would otherwise hide. 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. 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 different versions and disagree.

Why the CHANGELOG placement guard is not in verify. A rebase replays a CHANGELOG.md addition under whatever heading happens to sit at that offset, and it never conflicts, so the entry lands silently in an already-released section — this has needed a by-hand fix four times. scripts/check-changelog-placement.sh catches it by diffing against the merge-base, which means it needs a base ref, and a working tree has not got one. make verify and the pre-commit hook have to keep working offline and in a shallow clone, so the guard runs instead as a step in CI's verify job, with the base SHA taken from the pull-request event. Run it by hand with make check-changelog-placement (defaults to origin/main); its regression suite is make check-changelog-placement-test. The whole-file half of the pair — no version number in two headings — is make check-changelog, and that one is in verify and the hook.

Why make lint-cross exists. golangci-lint only analyses files matching the current GOOS, so a Linux make lint never sees sandbox_darwin*.go / process_darwin*.go and a macOS run never sees the _linux files. This is not hypothetical: the widened linter set was trialled on Linux only and passed clean, then failed CI's macOS leg on the one usetesting hit hiding behind a build tag. CI's two-OS matrix is the backstop, not the first line.

Test temporary directories are inside the checkout. make test sets GOTMPDIR=$(CURDIR)/.testcache, so t.TempDir() is repository-descended. Tests must not assume their temporary path lives outside the repository. Reproduce the CI shape with GOTMPDIR=$PWD/.testcache go test ./....

Tests never touch the live session registry. In a test binary, session.Dir prints the reason and exits the process if it resolves to the registry that the binary's start-up environment points at, which is the developer's own daemon's. It exits rather than panics because the daemon's recover() sites would swallow a panic and let the test pass. Every package whose tests reach the registry, directly or through daemon, connection, hook, TUI or web code, has a TestMain (session_isolation_main_test.go) that points XDG_DATA_HOME at a temporary directory for the whole binary. A test that needs a registry of its own sets t.Setenv("XDG_DATA_HOME", t.TempDir()) as usual, which wins over TestMain's. Do not add an override that outranks it: one directory shared by every test lets a session another test left live hold a name this one asks for.

Coverage and vulnerability checks are deliberately separate from verify. Coverage re-runs the whole suite instrumented and vulnerability scanning needs the network. CI runs both, plus test-race and integration jobs. Treat the coverage floor as a ratchet: raise it when sustainable; never lower it to make a red build green.

Code style (essentials)

These are the detailed rules behind the compact contract in AGENTS.md:

  • Australian English in all prose, comments, logs, and error strings (initialise, behaviour, colour…). Exception: identifiers from external specs (LSP method names, etc.) keep their canonical spelling.
  • log/slog only — never log or fmt.Println for logging.
  • Wrap errors with context: fmt.Errorf("loading config: %w", err).
  • context.Context first parameter on every blocking/I/O function.
  • State the concurrency contract in every type's doc comment.
  • Do no real work in init(); wire dependencies through constructors.
  • Add no globals except the documented TUI style variables and the intentional pathLocks / mutationRunLock process-wide tool locks.
  • Comments explain why, not what. No what-comments.
  • Max ~600 lines per production file and ~900 per _test.go; additions to scripts/.filesize-baseline need a justification. Cyclomatic complexity is capped at 15 for first-party non-test functions.
  • Every //nolint directive carries an inline justification. .golangci.yml's rejected-linter list is deliberate; re-review and document the hits before enabling one.
  • Every Tool.Execute() is a thin orchestrator over parse → run → format steps.

Commit conventions

<type>(<scope>): <short summary>

[optional body: why, not what]

Types: feat, fix, refactor, test, docs, ci, chore. Prefer one commit per discrete change, each with a CHANGELOG.md entry — bisectable history over squashed PRs.

Adding things

  • A new MCP tool — follow the add-mcp-tool project skill in .claude/skills/ (the checklist, including the thin-orchestrator Execute() pattern and the lean-profile decision). Implement the Tool interface, take WriteDeps for write tools, register it in registerAllTools (internal/cli/conn_register.go), add tests, and document it in docs/tools.md.
  • A new LSP adapter — see docs/adding-an-lsp.md.
  • A new config field — add it to config.Config, update defaults, validate in validate(), and document it in docs/configuration.md.

Testing

  • Tests live next to the code (_test.go, same package); table-driven where it fits.
  • internal/lsp, internal/cache, and internal/tools need meaningful coverage. Use WriteDeps{} as the zero-value setup for write tools, and keep per-session isolation tests in the package whose state they protect.
  • Internal tool-to-tool Execute calls use the target's canonical parameter names. Aliases resolve only at the MCP dispatch boundary; the in-process call guard test enforces this.
  • Integration tests requiring external binaries (gopls, pyright) are gated with //go:build integration.
  • Don't chase TUI coverage.
  • make cover instruments every package with -coverpkg=./...; an untested package therefore counts against the whole-tree floor. The canonical floor is in scripts/check-coverage.sh; use make cover-report to find gaps.

Version, daemon, and risk

The Makefile resolves the build version from an exact Git tag, then VERSION, then the short commit hash, and stamps the source revision alongside it. Change VERSION during development; do not create a tag for each iteration. Full output contracts live in docs/cli-reference.md.

After rebuilding or reinstalling, run plumb restart: a source change never activates inside the old daemon process. The resilient proxy reconnects clients.

Concurrency, the rate limiter, read tracking, and the stats schema carry subtle cross-session invariants. Inspect the architecture and existing tests before changing them, keep each change discrete, and add its CHANGELOG.md entry.

TUI conventions (Bubble Tea v2)

  • Import paths are v2 only: charm.land/bubbletea/v2, charm.land/lipgloss/v2, charm.land/bubbles/v2. Never mix in the v1 packages — type/API incompatibilities.
  • Model is exported; NewModel(logPath) constructs, Run(logPath) is the entry point. View() returns tea.View (v.AltScreen = true). Keys: tea.KeyPressMsg, match via msg.String().
  • Sections (opened with /): Dashboard, Sessions, Memory, Logs, Settings (0–4). Settings (internal/tui/model_settings.go) is a two-pane editor: a left Scope column (Global + each workspace) and a right rows pane with General/LSP tabs; tab/shift+tab cycle focus, [/] resize.
  • Settings persistence is scope-aware: Global rows write global config (config.Save), workspace rows write a sparse override to .plumb/config.toml. Each row carries a reload-tier numeral (¹ live / ² next session / ³ restart) and a ⁴ override / ⁵ inherited mark; only Theme and Log level apply live. ctrl+t opens the theme picker.
  • List and [lsp.<lang>] rows open shared pop-up editors (model_settings_listeditor.go, model_settings_texteditor.go), auto-saving on close; overlays dim via dimLines() + spliceOverlay().
  • Rebindable keys: the twelve navigation/action keys are configurable via the global-only [ui.keys] table (unknown actions and key conflicts are warned at startup with deterministic resolution; overlay/popup keys and the vim aliases stay fixed). The help overlay and Sessions footer render the live bindings. See the [ui.keys] config section.
  • Theme system: ActiveTheme/ActiveThemeName are globals in internal/tui/theme.go; lipgloss style vars are rebuilt by RebuildStyles() after any mutation. Adding a Theme field means updating every theme literal — TestTheme_AllFieldsSet catches omissions.