docs: code standards distilled from PR reviews, linked from AGENTS.md (Junjie Zhou on Discord)

docs/code-standards.md gathers what reviews keep asking for: a change holds on its whole path and does only what its goal needs, a fix comes with a test that fails without it and uses real inputs, tests check behavior and stay out of the real machine, the checks run before a merge, Go's any, appdir.Getenv and agentenv.NotPaths, the GUI's t()/i18n, no native select, no click scroll and no left-border stripes, and the commit message style. Junjie Zhou's six review rules from Discord are its first two tables; the rest is from merged PR reviews (#901, #922, #924, #926, #930) and the GUI tests. AGENTS.md links to it.
This commit is contained in:
yetone committed 2026-10-06 01:37:48 +08:00
1 parent 29e6d14857
commit 926bcaa379
2 files changed
+55

No files matched your search

+4
View File
@@ -7,6 +7,10 @@ Before changing a documented subsystem, read its
When preparing or reviewing a PR, follow the same page's documentation update,
semantic change description, and review rules.
## Code standards
Write and review changes against the [code standards](docs/code-standards.md).
## Built-in subscriptions that a plugin serves
Some built-in subscriptions are deprecated. Reaching them can break their
+51
View File
@@ -0,0 +1,51 @@
# Code standards
These rules come from what PR reviews keep asking for. A reviewer checks
each one, and a PR that doesn't meet one gets a comment saying which. How to
describe and review a change to a subsystem is in the
[subsystem references](subsystems/README.md).
## What a change contains
| Rule | What a reviewer looks at |
| --- | --- |
| A change does only what its goal needs. | Every change in behavior has a reason. Unrelated refactors, extra features and policy changes go in a PR of their own. |
| A fix covers exactly the cases it means to. | Which cases change, and which neighboring ones stay the same. Different errors are not handled as one. |
| A change holds on the whole path. | Follow it from the user's input to the end result, through the callers, the state it changes and the side effects. One correct function doesn't make the whole operation correct. For example, an agent's model picks are saved under one id and read under another (#926). An update button doesn't update the binary that actually runs (#930). |
| Checks cover the edges the change touches. | For numbers, check the boundaries. For paths, check each platform (macOS, Linux, Windows). For concurrency, check races and stale state. Pick checks by risk. |
## Tests
| Rule | What a reviewer looks at |
| --- | --- |
| A fix comes with a test that fails without it. | The reviewer puts the old code back and runs the test, and the test must fail. A PR description says what it failed with. |
| Tests use real inputs. | Fixtures look like real requests, real serialized output and real files on disk. That includes missing fields, empty values and defaults. |
| Tests check behavior, not implementation. | Assert what the user or caller needs. Formatting changes and internal refactors shouldn't break a test, unless exact bytes are the requirement. |
| Tests stay out of the real machine. | Run them under a temp HOME with Go's caches pinned (see the snippet in [provider-plugins.md](subsystems/provider-plugins.md)). Never touch a real agent's config or `~/.config/magpie`. An agent's variable goes in `agentenv.Vars`, so the sandbox clears it. |
Before a merge, run `go vet`, `go build`, `GOOS=linux go build -tags nogui`
and `GOOS=windows go build`, plus `go test -tags nogui` for the packages the
change touches. For GUI changes, also run the Playwright tests in
`internal/gui/tests` in Chromium and WebKit. A review lists any check it
didn't run.
## Go
- Write `any`, not `interface{}`.
- Read environment variables that name a folder with `appdir.Getenv`, which ignores a relative path. A variable in `agentenv.Vars` that names no folder goes in `agentenv.NotPaths`.
- A comment says what the code is for, in a sentence or two. Name functions instead of giving line numbers, which go stale.
## GUI
- Every string the user sees goes through `t()` and gets its translations in `i18n.js`. Tests run in Chinese and English.
- Use no native `<select>`. Dropdowns use the app's own menu (`openProtoMenu`).
- A click never scrolls the page (`scrollOnPurpose`).
- Don't use colored left-border stripes. Mark state with a dot or a swatch.
## Commits
A commit message names the areas it changes, then says what the user now
sees, written as plain behavior: `gateway: a 429 that says the balance is
spent is out of credit, not a rate limit`. Give the issue number or the
reporter. The body says how it was verified, and which test fails without
the change.