|
|
|
|
@@ -0,0 +1,514 @@
|
|
|
|
|
# NetBird Agent Guidelines
|
|
|
|
|
|
|
|
|
|
**NetBird** is an open-source connectivity platform: a WireGuard®-based overlay
|
|
|
|
|
network with a control plane. The **agent** (`client/`) runs on user machines as
|
|
|
|
|
a privileged daemon and manages the WireGuard interface, routing, firewall, and
|
|
|
|
|
DNS. **Management** (`management/`) is the control plane and REST/gRPC API,
|
|
|
|
|
**Signal** (`signal/`) brokers peer handshakes, **Relay** (`relay/`) carries
|
|
|
|
|
traffic when a direct tunnel is impossible, and **Proxy** (`proxy/`) is the
|
|
|
|
|
identity-aware proxy behind Agent Network.
|
|
|
|
|
|
|
|
|
|
This file applies to the whole repository, and is the single source of truth for
|
|
|
|
|
agent guidance here. `CLAUDE.md` is a one-line pointer to it — keep the guidance
|
|
|
|
|
in this file, not duplicated there.
|
|
|
|
|
|
|
|
|
|
## Contents
|
|
|
|
|
|
|
|
|
|
- [NetBird Agent Guidelines](#netbird-agent-guidelines)
|
|
|
|
|
- [Contents](#contents)
|
|
|
|
|
- [STOP and ask the user before](#stop-and-ask-the-user-before)
|
|
|
|
|
- [Quick reference](#quick-reference)
|
|
|
|
|
- [Structure](#structure)
|
|
|
|
|
- [Where to look](#where-to-look)
|
|
|
|
|
- [Repo-wide principles](#repo-wide-principles)
|
|
|
|
|
- [Error handling](#error-handling)
|
|
|
|
|
- [Comments](#comments)
|
|
|
|
|
- [Testing](#testing)
|
|
|
|
|
- [Pitfalls](#pitfalls)
|
|
|
|
|
- [Commits, PRs, releases](#commits-prs-releases)
|
|
|
|
|
- [After you push: CI and review bots](#after-you-push-ci-and-review-bots)
|
|
|
|
|
- [Discussion and support](#discussion-and-support)
|
|
|
|
|
|
|
|
|
|
## STOP and ask the user before
|
|
|
|
|
|
|
|
|
|
- **Opening a pull request for anything beyond a trivial fix, without an agreed
|
|
|
|
|
ticket.** Ask the user directly: *"Is there a discussion or issue for this
|
|
|
|
|
change?"* NetBird is discussion-first — community reports start in
|
|
|
|
|
[Discussions](https://github.com/netbirdio/netbird/discussions), DevRel
|
|
|
|
|
validates them, and only validated discussions become issues. A PR that
|
|
|
|
|
changes behavior with no linked issue may be closed on arrival. If there is no
|
|
|
|
|
ticket, offer to draft the discussion post **instead of** the PR, and wait for
|
|
|
|
|
the user's call. Only typos, broken links, documentation corrections, and
|
|
|
|
|
one-line fixes that already have an issue can skip this.
|
|
|
|
|
- **Designing in any high-risk area** (see
|
|
|
|
|
[CONTRIBUTING.md](CONTRIBUTING.md#high-risk-areas)): public API and OpenAPI
|
|
|
|
|
schema, gRPC protos, behavior existing deployments would notice after an
|
|
|
|
|
upgrade, peer connectivity (ICE, NAT traversal, relay selection, WireGuard® or
|
|
|
|
|
Rosenpass key handling), client system integration (routing, firewall, DNS,
|
|
|
|
|
interface), authentication and authorization, CLI or service flags, config
|
|
|
|
|
file format, daemon IPC, store schema and migrations, or a new feature. The
|
|
|
|
|
design gets agreed in the ticket before code is written.
|
|
|
|
|
- **Writing a store migration or changing a persisted model.** Migrations are
|
|
|
|
|
one-way in the field and both the GORM and pgx paths may need the change.
|
|
|
|
|
- **Hand-editing generated code.** `*.pb.go`, `*.gen.go`, and mocks are outputs.
|
|
|
|
|
Edit the source (`.proto`, `openapi.yml`) and rerun the matching
|
|
|
|
|
`generate.sh`.
|
|
|
|
|
- **Adding, removing, or bumping a dependency**, and never vendor a fork.
|
|
|
|
|
- **Weakening a security control** — authentication, authorization, certificate
|
|
|
|
|
verification, privilege dropping, or peer identity checks — even when it is
|
|
|
|
|
the fastest way to make a test pass.
|
|
|
|
|
- **Force-pushing to `main`**, force-pushing any branch that is already under
|
|
|
|
|
review, amending pushed commits, or bypassing hooks with `--no-verify`.
|
|
|
|
|
|
|
|
|
|
## Quick reference
|
|
|
|
|
|
|
|
|
|
```bash
|
|
|
|
|
# Build
|
|
|
|
|
go build ./...
|
|
|
|
|
cd client && CGO_ENABLED=0 go build . # agent
|
|
|
|
|
cd management && go build . # management service
|
|
|
|
|
cd signal && go build . # signal service
|
|
|
|
|
|
|
|
|
|
# Verify (run before every push)
|
|
|
|
|
go fmt ./...
|
|
|
|
|
make lint # golangci-lint on files changed vs origin/main (also the pre-push hook)
|
|
|
|
|
make lint-all # full-repository lint, matches CI
|
|
|
|
|
make test-unit # host-safe unit tests, -tags devcert, no sudo
|
|
|
|
|
make test-privileged # privileged-tagged suite in a Docker container with NET_ADMIN
|
|
|
|
|
make setup-hooks # wire make lint into .githooks/pre-push
|
|
|
|
|
|
|
|
|
|
# Narrow runs
|
|
|
|
|
go test ./client/internal/dns/...
|
|
|
|
|
go test -race -run TestPeerConn ./client/internal/peer/...
|
|
|
|
|
PRIV_RUN=TestNftablesManager PRIV_PKGS=./client/firewall/nftables/... make test-privileged
|
|
|
|
|
|
|
|
|
|
# Code generation (never hand-edit the output)
|
|
|
|
|
./shared/management/http/api/generate.sh # REST types from openapi.yml
|
|
|
|
|
./shared/management/proto/generate.sh
|
|
|
|
|
./shared/signal/proto/generate.sh
|
|
|
|
|
./client/proto/generate.sh
|
|
|
|
|
./flow/proto/generate.sh
|
|
|
|
|
|
|
|
|
|
# Run locally (lab only, never on a machine you rely on)
|
|
|
|
|
sudo ./client/netbird up --log-level debug --log-file console
|
|
|
|
|
sudo ./client/netbird down # teardown: restores routing, firewall, DNS
|
|
|
|
|
./signal/signal run --log-level debug --log-file console
|
|
|
|
|
./management/management management --log-level debug --log-file console --config ./management.json
|
|
|
|
|
```
|
|
|
|
|
|
|
|
|
|
`netbird up` needs root and rewrites the host's routing table, firewall rules,
|
|
|
|
|
DNS configuration, and WireGuard® interface. Run it only in a disposable test
|
|
|
|
|
environment (a VM, container, or throwaway host) that you can rebuild, never on
|
|
|
|
|
a workstation or server whose connectivity matters. Run `sudo netbird down`
|
|
|
|
|
before you stop working, before rebuilding the binary, and on every failure
|
|
|
|
|
path, so the host's networking state is restored instead of left half-applied.
|
|
|
|
|
See [Pitfalls](#pitfalls) for why cleanup on every exit path matters.
|
|
|
|
|
|
|
|
|
|
## Structure
|
|
|
|
|
|
|
|
|
|
```text
|
|
|
|
|
netbird/
|
|
|
|
|
├── client/ NetBird agent
|
|
|
|
|
│ ├── cmd/ agent CLI
|
|
|
|
|
│ ├── internal/ agent business logic (engine, peer, dns, routemanager, ...)
|
|
|
|
|
│ ├── server/ daemon for background execution
|
|
|
|
|
│ ├── proto/ daemon gRPC protos
|
|
|
|
|
│ ├── iface/ WireGuard® interface management
|
|
|
|
|
│ ├── firewall/ nftables, iptables, pf, WFP, userspace backends
|
|
|
|
|
│ ├── ssh/ built-in SSH server and client
|
|
|
|
|
│ ├── ui/ desktop UI (Wails v3 + React)
|
|
|
|
|
│ ├── android/, ios/ mobile bindings
|
|
|
|
|
│ ├── wasm/ WebAssembly build
|
|
|
|
|
│ └── mdm/, system/ MDM policy, host information
|
|
|
|
|
├── management/ control plane
|
|
|
|
|
│ └── server/ account, peer, groups, networks, posture, permissions,
|
|
|
|
|
│ settings, store, http (REST), idp, integrations, migration
|
|
|
|
|
├── signal/ handshake broker (peer/, server/)
|
|
|
|
|
├── relay/ relay service (protocol/, server/, healthcheck/)
|
|
|
|
|
├── proxy/ identity-aware proxy (llm/, acme/, accesslog/, middleware/, tcp/, udp/)
|
|
|
|
|
├── agent-network/ Agent Network overview
|
|
|
|
|
├── shared/ imported by both agent and services
|
|
|
|
|
│ ├── management/ proto/, client/, http/api (OpenAPI + generated types)
|
|
|
|
|
│ ├── signal/ proto/, client/
|
|
|
|
|
│ └── relay/, auth/, sshauth/, metrics/
|
|
|
|
|
├── e2e/ end-to-end suites and harness
|
|
|
|
|
├── encryption/, dns/, route/, stun/, sharedsock/, util/, flow/
|
|
|
|
|
├── infrastructure_files/ docker compose and getting-started templates
|
|
|
|
|
└── release_files/ files packaged into releases
|
|
|
|
|
```
|
|
|
|
|
|
|
|
|
|
## Where to look
|
|
|
|
|
|
|
|
|
|
| Task | Location |
|
|
|
|
|
| --------------------------- | ------------------------------------------------------------ |
|
|
|
|
|
| REST API / OpenAPI | `shared/management/http/api/` + `management/server/http/` |
|
|
|
|
|
| Management gRPC protocol | `shared/management/proto/` |
|
|
|
|
|
| Signal protocol | `shared/signal/proto/` |
|
|
|
|
|
| Daemon IPC protocol | `client/proto/` |
|
|
|
|
|
| Peer connection and NAT | `client/internal/peer/` |
|
|
|
|
|
| Network map handling | `client/internal/engine.go`, `shared/management/networkmap/` |
|
|
|
|
|
| Routing | `client/internal/routemanager/`, `route/` |
|
|
|
|
|
| Firewall backends | `client/firewall/` |
|
|
|
|
|
| DNS | `client/internal/dns/`, `dns/` |
|
|
|
|
|
| WireGuard® interface | `client/iface/` |
|
|
|
|
|
| Persistence and migrations | `management/server/store/`, `management/server/migration/` |
|
|
|
|
|
| IdP integrations | `management/server/idp/` |
|
|
|
|
|
| Permissions model | `management/server/permissions/` |
|
|
|
|
|
| LLM routing / Agent Network | `proxy/internal/llm/`, `agent-network/` |
|
|
|
|
|
| End-to-end tests | `e2e/` |
|
|
|
|
|
|
|
|
|
|
## Repo-wide principles
|
|
|
|
|
|
|
|
|
|
1. **Run `go fmt` on every modified Go file.** Formatting is not optional.
|
|
|
|
|
2. **Zero unaddressed diagnostics.** Fix IDE and linter warnings on code you
|
|
|
|
|
touch, and delete imports, helpers, and parameters your refactor orphaned.
|
|
|
|
|
Exception: unused parameters in shared code may be consumed by builds outside
|
|
|
|
|
this repository — do not remove them, ask instead.
|
|
|
|
|
3. **Function comments are mandatory for exported functions**, written as full
|
|
|
|
|
sentences with a period, starting with the identifier name.
|
|
|
|
|
4. **Prefer private functions and constants.** Export only what a caller outside
|
|
|
|
|
the package genuinely needs.
|
|
|
|
|
5. **Early returns and guard clauses.** Handle errors and edge cases first
|
|
|
|
|
instead of nesting `if`/`else` chains.
|
|
|
|
|
6. **Split complex functions.** If a function trips a complexity warning, break
|
|
|
|
|
it into named helpers rather than silencing the warning.
|
|
|
|
|
7. **Avoid LLM-slop tells:** em dashes, hedging narration, restating the diff in
|
|
|
|
|
prose, trailing summaries. Defaults, not absolute bans. Applies to code,
|
|
|
|
|
comments, commit messages, and PR descriptions alike.
|
|
|
|
|
8. **Concurrency: do a two-pass race analysis after every change** that adds
|
|
|
|
|
shared state. Guard maps and slices with a mutex, keep critical sections
|
|
|
|
|
short, and run `go test -race` on the touched packages.
|
|
|
|
|
9. **Cross-platform builds must keep working.** The agent targets Linux, macOS,
|
|
|
|
|
Windows, FreeBSD, Android, and iOS. When you add a platform-specific file,
|
|
|
|
|
add the counterpart or a build-tagged fallback for the others.
|
|
|
|
|
10. **Never hand-edit generated files.** Change the source and regenerate.
|
|
|
|
|
11. **Never log secrets** — private keys, setup keys, tokens, PAT values — and
|
|
|
|
|
keep peer IPs and hostnames out of logs above debug level.
|
|
|
|
|
|
|
|
|
|
## Error handling
|
|
|
|
|
|
|
|
|
|
Use single-assignment form when the error is only needed inside the `if`:
|
|
|
|
|
|
|
|
|
|
```go
|
|
|
|
|
// Good
|
|
|
|
|
if err := someCall(); err != nil {
|
|
|
|
|
return fmt.Errorf("context: %w", err)
|
|
|
|
|
}
|
|
|
|
|
|
|
|
|
|
// Bad - unnecessary split
|
|
|
|
|
err := someCall()
|
|
|
|
|
if err != nil {
|
|
|
|
|
return fmt.Errorf("context: %w", err)
|
|
|
|
|
}
|
|
|
|
|
```
|
|
|
|
|
|
|
|
|
|
Use multiple assignment when the value is needed after the block:
|
|
|
|
|
|
|
|
|
|
```go
|
|
|
|
|
result, err := someCall()
|
|
|
|
|
if err != nil {
|
|
|
|
|
return fmt.Errorf("context: %w", err)
|
|
|
|
|
}
|
|
|
|
|
```
|
|
|
|
|
|
|
|
|
|
Add short, meaningful context, and **do not** start `fmt.Errorf` messages with
|
|
|
|
|
obvious words like "failed to" or "error":
|
|
|
|
|
|
|
|
|
|
```go
|
|
|
|
|
// Good
|
|
|
|
|
return fmt.Errorf("parse remote address: %w", err)
|
|
|
|
|
return fmt.Errorf("listen on %s: %w", addr, err)
|
|
|
|
|
|
|
|
|
|
// Bad
|
|
|
|
|
return fmt.Errorf("failed to parse remote address: %w", err)
|
|
|
|
|
return fmt.Errorf("error listening on %s: %w", addr, err)
|
|
|
|
|
|
|
|
|
|
// "failed" is fine in log messages
|
|
|
|
|
log.Debugf("failed to parse remote address: %v", err)
|
|
|
|
|
```
|
|
|
|
|
|
|
|
|
|
Skip the wrapping when a function only extracts or delegates and the wrap would
|
|
|
|
|
add nothing:
|
|
|
|
|
|
|
|
|
|
```go
|
|
|
|
|
func parseAddr(addr string) (string, int, error) {
|
|
|
|
|
host, portStr, err := net.SplitHostPort(addr)
|
|
|
|
|
if err != nil {
|
|
|
|
|
return "", 0, err
|
|
|
|
|
}
|
|
|
|
|
// ...
|
|
|
|
|
}
|
|
|
|
|
```
|
|
|
|
|
|
|
|
|
|
Log the errors you choose not to act on:
|
|
|
|
|
|
|
|
|
|
- `log.Debugf()` for errors that do not affect program flow but help debugging.
|
|
|
|
|
- `log.Tracef()` for very verbose errors that would otherwise spam logs.
|
|
|
|
|
- **Never ignore** errors from writes, network sends, or critical cleanup.
|
|
|
|
|
- Close errors may be ignored for read-only operations; log them at debug for
|
|
|
|
|
writes.
|
|
|
|
|
|
|
|
|
|
## Comments
|
|
|
|
|
|
|
|
|
|
Comment the **why**, never the **what**. Default to no comment, and add one only
|
|
|
|
|
when a hidden constraint or workaround would surprise a future reader. Never
|
|
|
|
|
reference the current task, PR, or your own changes in a comment.
|
|
|
|
|
|
|
|
|
|
```go
|
|
|
|
|
// Bad - trailing comments explaining the obvious
|
|
|
|
|
defer localConn.Close() // Close the connection
|
|
|
|
|
if err != nil { // Check if error occurred
|
|
|
|
|
|
|
|
|
|
// Good
|
|
|
|
|
defer localConn.Close()
|
|
|
|
|
|
|
|
|
|
// Good - explains a non-obvious constraint
|
|
|
|
|
// Use incremental checksum update per RFC 1624 for performance.
|
|
|
|
|
checksum = updateChecksum(checksum, oldPort, newPort)
|
|
|
|
|
```
|
|
|
|
|
|
|
|
|
|
### Length budget
|
|
|
|
|
|
|
|
|
|
- **90 characters per line.** Wrap the comment, do not run past it.
|
|
|
|
|
- **250 characters per comment**, roughly three wrapped lines. Doc comments on
|
|
|
|
|
exported identifiers may exceed it when the API genuinely needs the
|
|
|
|
|
explanation; inline comments inside a function body may not.
|
|
|
|
|
|
|
|
|
|
The budget is a smell detector, not a rule to game. Do not compress a needed
|
|
|
|
|
explanation into cryptic shorthand to fit — if a block of code needs more than
|
|
|
|
|
250 characters of prose, the code is doing too much. Fix the code:
|
|
|
|
|
|
|
|
|
|
- **Extract a named function.** A well-named function replaces the comment: the
|
|
|
|
|
name says *what*, the body shows *how*, and the comment you no longer write
|
|
|
|
|
was the *what* anyway. Clean Code calls this "explain yourself in code".
|
|
|
|
|
- **Extract a named constant or predicate.** `if isExpiredSetupKey(key)` needs
|
|
|
|
|
no comment; `if key.ExpiresAt.Before(now) && !key.Revoked && key.UsageLimit > 0`
|
|
|
|
|
does.
|
|
|
|
|
- **Keep the surviving comment for the why** — the RFC, the kernel quirk, the
|
|
|
|
|
ordering constraint. That part is usually one or two lines.
|
|
|
|
|
|
|
|
|
|
### Long switch and if/else chains
|
|
|
|
|
|
|
|
|
|
A `switch` whose cases carry multi-line explanations is the usual place this
|
|
|
|
|
budget is breached, and the comment is a symptom. In order of preference:
|
|
|
|
|
|
|
|
|
|
1. **Extract each case body into a named function.** The case becomes one line,
|
|
|
|
|
the name carries the meaning, and the switch reads as a table of contents.
|
|
|
|
|
2. **Replace the switch with a lookup table** — `map[Kind]handlerFunc` — when the
|
|
|
|
|
branches are uniform. Adding a case stops meaning editing a growing function.
|
|
|
|
|
3. **Replace conditional with polymorphism** when branches vary by type and the
|
|
|
|
|
same switch shape starts appearing in more than one place. Clean Code's rule
|
|
|
|
|
of thumb: tolerate a switch statement if it appears **once**, is buried in a
|
|
|
|
|
factory that returns an interface, and no other switch dispatches on the same
|
|
|
|
|
type. A second switch over the same enum is the signal to introduce the
|
|
|
|
|
interface.
|
|
|
|
|
|
|
|
|
|
Do not restructure a switch purely to satisfy the budget when the cases are one
|
|
|
|
|
line each and self-evident — a flat, boring `switch` over an enum is fine and
|
|
|
|
|
needs no comments at all.
|
|
|
|
|
|
|
|
|
|
Explanatory comments in tests are welcome — they document the scenario being set
|
|
|
|
|
up, and the 250-character budget does not apply to them.
|
|
|
|
|
|
|
|
|
|
## Testing
|
|
|
|
|
|
|
|
|
|
- **Unit tests** live beside the code as `_test.go`. `make test-unit` runs the
|
|
|
|
|
host-safe set with `-tags devcert` and no sudo.
|
|
|
|
|
- **Privileged tests** carry the `privileged` build tag and mutate host
|
|
|
|
|
networking. They run through `make test-privileged`, inside a Docker container
|
|
|
|
|
with `NET_ADMIN`. Never bypass that harness by running them directly on the
|
|
|
|
|
host.
|
|
|
|
|
- **End-to-end suites** live in `e2e/` with a shared harness.
|
|
|
|
|
- **Test real behavior, not API existence.** Assert on the observable end state
|
|
|
|
|
a consumer would see — bytes that arrived, the packet after translation, the
|
|
|
|
|
row after the write — not merely that a method exists or returns an error.
|
|
|
|
|
- **Avoid mocks for code we own.** Exercise the real store, manager, or
|
|
|
|
|
controller and assert what the caller actually receives.
|
|
|
|
|
- **`require` for setup and preconditions, `assert` for the conditions under
|
|
|
|
|
test.** Use `require` whenever a later line would panic or be meaningless
|
|
|
|
|
otherwise.
|
|
|
|
|
- **Message guidance:** optional for `NoError`/`Error`; always give context for
|
|
|
|
|
comparison, boolean, and collection assertions.
|
|
|
|
|
|
|
|
|
|
```go
|
|
|
|
|
server, err := StartTestServer()
|
|
|
|
|
require.NoError(t, err, "Test server setup must succeed")
|
|
|
|
|
defer server.Close()
|
|
|
|
|
|
|
|
|
|
result, err := client.DoOperation()
|
|
|
|
|
assert.NoError(t, err)
|
|
|
|
|
assert.Equal(t, expectedResult, result, "Result should match expected")
|
|
|
|
|
```
|
|
|
|
|
|
|
|
|
|
## Pitfalls
|
|
|
|
|
|
|
|
|
|
- **The agent runs as root.** Anything touching routing, firewall, DNS, or the
|
|
|
|
|
interface can take a user's machine off the network. Prefer a reversible
|
|
|
|
|
change and make sure cleanup runs on every exit path.
|
|
|
|
|
- **Management has two account loaders** (GORM and pgx). Adding a relation to an
|
|
|
|
|
account often means updating both, or it silently comes back empty in
|
|
|
|
|
production.
|
|
|
|
|
- **`go test ./...` without `-tags devcert` skips tests** that need the
|
|
|
|
|
development certificate. Use `make test-unit`.
|
|
|
|
|
- **`make lint` only checks the diff against `origin/main`.** CI runs
|
|
|
|
|
`make lint-all`; run it too before pushing a large change.
|
|
|
|
|
- **Protos are consumed by released clients.** An old agent must keep working
|
|
|
|
|
against a new Management, so fields are added, never renumbered or removed.
|
|
|
|
|
- **Windows requires the wintun driver**, and the daemon serves a named pipe
|
|
|
|
|
(`npipe://netbird`) rather than loopback TCP. Loopback TCP carries no caller
|
|
|
|
|
identity, so privileged operations are refused over it.
|
|
|
|
|
|
|
|
|
|
## Commits, PRs, releases
|
|
|
|
|
|
|
|
|
|
- **PR titles must start with a bracketed tag.** Before you propose a title,
|
|
|
|
|
**read [`.github/workflows/pr-title-check.yml`](.github/workflows/pr-title-check.yml)
|
|
|
|
|
and take the allowed tags from the `allowedTags` array in that file.** It is
|
|
|
|
|
the only source of truth, it changes as components are added, and the check
|
|
|
|
|
runs on every title edit — a tag that is not in that array is a red build. Do
|
|
|
|
|
not rely on a list memorized from anywhere else, including this file.
|
|
|
|
|
|
|
|
|
|
```text
|
|
|
|
|
[client] Authorize daemon IPC callers by their local identity
|
|
|
|
|
[management,client] Add MDM policy support
|
|
|
|
|
```
|
|
|
|
|
|
|
|
|
|
Multiple tags are comma-separated inside one pair of brackets. Match the tag
|
|
|
|
|
to the component you actually changed, not to the one you read the most.
|
|
|
|
|
|
|
|
|
|
- **Use the repository's PR template.** Fill in
|
|
|
|
|
[`.github/pull_request_template.md`](.github/pull_request_template.md) rather
|
|
|
|
|
than replacing it with your own summary: describe the change, link the issue,
|
|
|
|
|
tick the checklist honestly (including "ran locally" and "single purpose"),
|
|
|
|
|
and complete the documentation section. Do not tick a box you have not
|
|
|
|
|
verified, and do not delete rows that do not apply.
|
|
|
|
|
|
|
|
|
|
- **Keep the PR description short.** Under 1000 words on top of the template's
|
|
|
|
|
own text, and usually far less — a few paragraphs. Reviewers read the diff;
|
|
|
|
|
the description exists to explain what the diff cannot say for itself. This is
|
|
|
|
|
well below what an agent will produce by default, so cut before you post.
|
|
|
|
|
|
|
|
|
|
- **Body: why before what.** Lead with the problem and the reason for this
|
|
|
|
|
approach, then the shape of the change. No bullet list of files changed, no
|
|
|
|
|
per-function walkthrough, no restating the diff in prose, no trailing summary
|
|
|
|
|
section, no self-congratulatory closing line.
|
|
|
|
|
|
|
|
|
|
- **No `Co-Authored-By` or tool-attribution trailers in the PR description**,
|
|
|
|
|
and none in commits either. Contributors own their contributions. Whatever
|
|
|
|
|
tooling produced the diff, the person opening the PR is its author: they have
|
|
|
|
|
read every line, they can explain why it works, they can answer review
|
|
|
|
|
questions without going back to a model, and they are accountable for the
|
|
|
|
|
consequences of merging it. Do not add a trailer, footer, or description line
|
|
|
|
|
that spreads that ownership onto a tool.
|
|
|
|
|
|
|
|
|
|
- **Commit subjects follow the same `[scope] Subject` convention.** Keep the
|
|
|
|
|
subject short, and use the body for why before what. No bullet lists of files
|
|
|
|
|
changed.
|
|
|
|
|
|
|
|
|
|
- **Push review fixes as separate commits.** The PR is squashed on merge, so
|
|
|
|
|
there is no reason to rewrite history mid-review; many small commits make the
|
|
|
|
|
re-review readable.
|
|
|
|
|
|
|
|
|
|
- **Do not force-push a branch that is under review.** A force-push detaches
|
|
|
|
|
existing review comments from the lines they were written against, destroys
|
|
|
|
|
the "changes since your last review" diff a reviewer relies on, and discards
|
|
|
|
|
the CI history that showed which commit broke what. Add commits instead —
|
|
|
|
|
including for fixups and reverts. Force-push only when there is no
|
|
|
|
|
alternative: a rebase to clear a genuine conflict, or removing a secret or a
|
|
|
|
|
large binary that was committed by mistake. When you must, ask the user first,
|
|
|
|
|
then say so in a PR comment so reviewers know their anchors moved. Never
|
|
|
|
|
force-push `main`, and never force-push a branch you do not own.
|
|
|
|
|
|
|
|
|
|
- **One PR, one purpose.** Split refactors out of fixes and fixes out of
|
|
|
|
|
features.
|
|
|
|
|
|
|
|
|
|
- **Keep the PR small.** Size is the single strongest predictor of how long a PR
|
|
|
|
|
waits. Aim for **under ~400 changed lines across under ~20 files**; past
|
|
|
|
|
roughly **1000 lines or 50 files** a community PR is likely to be sent back to
|
|
|
|
|
be split, or left unreviewed until it is. Large PRs from outside the core team
|
|
|
|
|
may be blocked outright when the size was never agreed in the ticket —
|
|
|
|
|
reviewing a sprawling change against a privileged networking daemon is a
|
|
|
|
|
security risk in itself, not just a time cost.
|
|
|
|
|
|
|
|
|
|
Judge the size by hand-written code: exclude generated output, `go.sum`,
|
|
|
|
|
vendored files, and test fixtures from the estimate, but do not use their
|
|
|
|
|
presence to argue a 3000-line PR is small.
|
|
|
|
|
|
|
|
|
|
When a change genuinely cannot be small — a protocol migration, a
|
|
|
|
|
cross-component rename — agree the split in the ticket **before** writing
|
|
|
|
|
code, and land it as a sequence of PRs that each build, test, and make sense
|
|
|
|
|
on their own. Propose that split to the user rather than opening one large PR
|
|
|
|
|
and hoping.
|
|
|
|
|
|
|
|
|
|
- **User-facing changes need a docs PR** in
|
|
|
|
|
[netbirdio/docs](https://github.com/netbirdio/docs), linked from the PR
|
|
|
|
|
description.
|
|
|
|
|
|
|
|
|
|
## After you push: CI and review bots
|
|
|
|
|
|
|
|
|
|
Opening the PR is not the end of the task. Watch the run, read what the bots
|
|
|
|
|
say, and drive the PR to green before you report the work as done.
|
|
|
|
|
|
|
|
|
|
```bash
|
|
|
|
|
gh pr checks <pr> --watch # all checks, live
|
|
|
|
|
gh run view <run-id> --log-failed # only the failing steps
|
|
|
|
|
gh pr view <pr> --comments # bot and human review comments
|
|
|
|
|
```
|
|
|
|
|
|
|
|
|
|
**Never report a change as finished while checks are pending or red**, and never
|
|
|
|
|
describe a red PR as passing. If you ran out of turn before CI finished, say
|
|
|
|
|
which checks were still running.
|
|
|
|
|
|
|
|
|
|
### The checks
|
|
|
|
|
|
|
|
|
|
- **Go tests** — `golang-test-{linux,darwin,windows,freebsd}.yml`, sharded per
|
|
|
|
|
component. A failure in a component you did not touch is usually a real
|
|
|
|
|
interaction, not noise; read the log before assuming flake.
|
|
|
|
|
- **golangci-lint** — `golangci-lint.yml` runs the full repository, while
|
|
|
|
|
`make lint` only checks your diff. A clean local lint does not guarantee green
|
|
|
|
|
CI on a large change.
|
|
|
|
|
- **PR Title Check** — `pr-title-check.yml`, see above.
|
|
|
|
|
- **Codecov** — uploaded from the Linux test workflow with per-component flags
|
|
|
|
|
(`unit,client`, `unit,management`, `unit,relay`, `unit,proxy`, `unit,signal`,
|
|
|
|
|
`integration,management`). Coverage on new code should not go backwards. Add
|
|
|
|
|
tests for the paths you introduced; do not adjust thresholds or exclude files
|
|
|
|
|
to clear the report.
|
|
|
|
|
- **CodeRabbit** — configured in [`.coderabbit.yaml`](.coderabbit.yaml): `chill`
|
|
|
|
|
profile, auto-review on every non-draft PR, TypeScript/JavaScript/SVG paths
|
|
|
|
|
filtered out. Chat auto-reply is on, so `@coderabbitai` in a comment reaches
|
|
|
|
|
it.
|
|
|
|
|
- **SonarCloud** — project `netbirdio_netbird`, quality gate on new code (bugs,
|
|
|
|
|
vulnerabilities, code smells, duplication, coverage).
|
|
|
|
|
- **Snyk** — dependency and code scanning.
|
|
|
|
|
|
|
|
|
|
Sonar and Snyk report as GitHub App checks rather than workflows in this
|
|
|
|
|
repository, so their detail lives on the PR check, not in the Actions logs.
|
|
|
|
|
|
|
|
|
|
### Handling bot findings
|
|
|
|
|
|
|
|
|
|
- **Read every comment and act on it.** Either fix it, or reply with the reason
|
|
|
|
|
it does not apply. Do not bulk-resolve threads to clear the count, and do not
|
|
|
|
|
silently ignore a finding because the check is advisory.
|
|
|
|
|
- **Bots are frequently wrong here.** NetBird has privileged, platform-specific,
|
|
|
|
|
and concurrency-heavy code that static analysis reads poorly. A confident
|
|
|
|
|
CodeRabbit or Sonar comment can still be nonsense. Verify the claim against
|
|
|
|
|
the code before you change anything — never edit correct code just to silence
|
|
|
|
|
a bot.
|
|
|
|
|
- **Security findings get the opposite default.** For a Snyk or Sonar
|
|
|
|
|
vulnerability, or a CodeRabbit comment about authentication, authorization,
|
|
|
|
|
certificate verification, or key handling, assume it is real until you have
|
|
|
|
|
disproved it. Surface it to the user rather than dismissing it yourself.
|
|
|
|
|
- **A new vulnerable dependency is a stop.** Bumping or replacing dependencies
|
|
|
|
|
needs the user's decision, as above.
|
|
|
|
|
- **Never change a workflow, threshold, lint exclusion, or bot config to make a
|
|
|
|
|
check pass.** If a check is genuinely wrong, say so and let the user decide.
|
|
|
|
|
- **Do not paper over flakes with blind re-runs.** Identify the failure first. If
|
|
|
|
|
it is a known flake, name it; if you cannot tell, report it as unresolved
|
|
|
|
|
rather than re-running until it goes green.
|
|
|
|
|
|
|
|
|
|
## Discussion and support
|
|
|
|
|
|
|
|
|
|
- Discussions: <https://github.com/netbirdio/netbird/discussions>
|
|
|
|
|
- Slack: <https://docs.netbird.io/slack-url>
|
|
|
|
|
- Docs: <https://docs.netbird.io>
|
|
|
|
|
- Security: <https://github.com/netbirdio/netbird/security/policy> — never in public
|
|
|
|
|
- Contribution process: [CONTRIBUTING.md](CONTRIBUTING.md)
|
