docs: the 10-08 review moves real use and GUI-in-both-engines into the code standards, adds gofmt and a Linux/Windows go vet to the checks, and adds lessons on Windows CI, telemetry disclosure, timing tests, guarantee tests, real vendor pages, CI's Go version and large commits without a body
192 commits and 18 releases on 10-08 (through 3c629133). The two lessons
seen a third day keep their concrete steps in acceptance criteria 4 and 5
(git grep a reworded string, a gateway guard through the real server
stack). bbf99143's syscall.Getsid shows a cross-OS build doesn't compile
_test.go, so vet joins the checks. #1318's test that passes with the waiter
held is filed as #1358.
Ran: read both files after the edit; docs only, no code changed.
This commit is contained in:
1 parent
3c629133c2
commit
461708907a
2 files changed
+110
-48
No files matched your search
+85
-44
@@ -1,4 +1,4 @@
|
||||
<!-- reviewed-through: 8b14afea (2026-10-07 22:09 +0800) -->
|
||||
<!-- reviewed-through: 3c629133 (2026-10-09 02:48 +0800) -->
|
||||
# Lessons from merged work
|
||||
|
||||
magpie's code is written, reviewed, merged and released by agents. Each night
|
||||
@@ -18,6 +18,12 @@ it, a red test on main is a bug now, and a merge records what was run at
|
||||
the reviewed head. They are listed under
|
||||
[Moved to the code standards](#moved-to-the-code-standards).
|
||||
|
||||
2026-10-08 had 192 commits and 18 releases (v0.1.1105 to v0.1.1122), each
|
||||
tagged after a green run. Two more lessons were seen a third day and moved:
|
||||
real use before building on it, and GUI tests in both engines and every
|
||||
language. The acceptance checks now include `gofmt -l` and a Linux and
|
||||
Windows `go vet`, since a cross-OS build skips `_test.go`.
|
||||
|
||||
## Reading the report
|
||||
|
||||
**Reproduce the reporter's exact case, on the surface they use, before you
|
||||
@@ -148,47 +154,36 @@ could match the same key.**
|
||||
|
||||
## Verification
|
||||
|
||||
**"Not tried with the real thing" means don't build more on it, and don't
|
||||
tell users it works.**
|
||||
- Cursor Private Inference shipped four times without the real app. The
|
||||
real client's key and User-Agent never reached the new path (387afd07).
|
||||
- Add to PATH was "not tried on Windows" and failed for the site's
|
||||
magpie-windows-amd64.exe for 8.5h (90b1ac79). The Windows box was there.
|
||||
- The Trae CN check-in shipped on three guesses (#808, #821). Design the
|
||||
experiment that tells the guesses apart, or ask the reporter to run it.
|
||||
- A platform report (WSL, Windows) is checked on that platform: ssh to the
|
||||
box. A GOOS build is not a test.
|
||||
- 10-07: b2345078 (#1051) let listed web pages call the gateway with a key.
|
||||
Its tests called Handler without lanGuard, never the running server.
|
||||
There every such call got 401 until 7837f90d, 15 minutes later. A test of
|
||||
a gateway guard goes through the real server stack, or the built binary.
|
||||
- 10-07: #1050's WSL path (a running and a stopped distro) and cda11d84's
|
||||
Windows data folder (tangle778 on X) were checked by unit tests only. The
|
||||
Windows box and its WSL2 distro were there. cda11d84 also misses a Desktop
|
||||
or Documents that OneDrive moved (the Windows 11 default). It switches an
|
||||
existing Downloads-portable user who also has an installed copy to the
|
||||
installed data, and says so only in the log.
|
||||
- Done right on 10-07: 6f65ffb5 checked the WSL CLIs on the Windows box's
|
||||
WSL2 distro. a24458ca (#1063) said plainly that no real Copilot Business
|
||||
seat was tried, built its fixtures from VS Code's tests and live Pro+
|
||||
bytes, and kept the issue open.
|
||||
- Seen 2× (2026-10-05, 2026-10-07).
|
||||
**Windows CI only compiles the tests, so a Windows-only red shows
|
||||
nowhere.** Run the package on the Windows box before calling a Windows
|
||||
failure "the same on main".
|
||||
- TestCursor was red on Windows for 3 days, from 585f7f7d until #1248.
|
||||
- 5d0c1dca's 16 and 76 Windows failures were called the same as main.
|
||||
Neither was filed. #923 is the open work to run more of the suite there.
|
||||
- Seen 1× (2026-10-08).
|
||||
|
||||
**A GUI change runs its tests in both Chromium and WebKit, at narrow widths,
|
||||
in every language.**
|
||||
- ~40 GUI commits ran WebKit only, without saying Chromium wasn't run.
|
||||
- 3ec6b4da squeezed key names to "…" (#841). 653cb8ee shrank the ZCode
|
||||
question to 0px at 440px.
|
||||
- 43a65950 broke gui-ja/gui-de placeholders for 3.5h. Run gui-ja and gui-de
|
||||
for every new `t()` string.
|
||||
- When you reword a string, `git grep` the old text under internal/gui/tests.
|
||||
4a87b306 left routing-served red for 8 releases.
|
||||
- Test the empty case: 6fc0afe8's price editor couldn't price a model with
|
||||
no list price (4c001cef).
|
||||
- 10-06: 4c170cba's Japanese string had a third `{agent}`, and gui-ja was red
|
||||
until 34bfb9ca. 29e6d148 (#929) kept a clicked chip in view in Chromium
|
||||
only; c6e318e1 did WebKit 40 minutes later.
|
||||
- Seen 2× (2026-10-05, 2026-10-06).
|
||||
**What telemetry sends is an allow-list, and a change to what is sent
|
||||
re-derives every place that discloses it.**
|
||||
- 169c23c1 sent `omp#` profile ids and the user's own model ids, which
|
||||
aren't in the public catalog (fixed in 2cdae80a: only known ids are sent).
|
||||
- 7d663a27 sent each partner's daily counts, while the READMEs' Privacy and
|
||||
the site's "What does magpie send about me?" still listed what was sent
|
||||
before (b4aa62af).
|
||||
- Grep the READMEs (en, zh) and site/public/docs (en, zh, ja) for the
|
||||
privacy text in the same commit.
|
||||
- Seen 1× (2026-10-08).
|
||||
|
||||
**A test never loads a real third-party page.**
|
||||
- 9755201b's test opened ChatGPT, whose Cloudflare challenge hung it.
|
||||
9f0831b2 and 88f2eb8a did the same with other vendor pages.
|
||||
- Serve the page from httptest, built from bytes saved from the real one.
|
||||
- Seen 1× (2026-10-08).
|
||||
|
||||
**Reproduce on CI's Go version.**
|
||||
- #1184 couldn't be reproduced on go1.27.1 locally. CI ran 1.26.x, and the
|
||||
cause was a Go 1.26.8 segfault. Docker `golang:1.26.8` with
|
||||
`GOTOOLCHAIN=local` matches CI.
|
||||
- Seen 1× (2026-10-08).
|
||||
|
||||
## Concurrency and tests
|
||||
|
||||
@@ -202,7 +197,10 @@ handoff that lets another goroutine look at it.**
|
||||
- The Claude subscription run recorded run.tools after continueWith had
|
||||
already answered the agent, so a quick turn read the old tools
|
||||
(TestToolSearchLoadKeepsTheRun, c8690ca0).
|
||||
- Seen 1× (2026-10-06).
|
||||
- 10-08: 01e66e0f's claudeCredentials.marshal wrote into the raw map that
|
||||
every copy of the struct shares. A struct copied by value that holds a
|
||||
map is not a copy (15afd988).
|
||||
- Seen 2× (2026-10-06, 2026-10-08).
|
||||
|
||||
**A fake or child process a test starts ends when the test does.**
|
||||
- TestClaudeSignInByPaste's fake `claude auth login` polled every 50ms for 2
|
||||
@@ -213,7 +211,25 @@ handoff that lets another goroutine look at it.**
|
||||
spun at 96% CPU for 30 minutes.
|
||||
- Size -count to -timeout. A timeout panic is not a hang until its stacks
|
||||
show one.
|
||||
- Seen 1× (2026-10-06).
|
||||
- 10-08: #1318's detached goroutine outlived its test; its review caught it.
|
||||
- Seen 2× (2026-10-06, 2026-10-08).
|
||||
|
||||
**A timing test leaves room on both sides.**
|
||||
- Leave slack against a fake's sleep (8d6e9bad). Keep a threshold
|
||||
classifier's fixture far from every threshold it has (5e17a183). A
|
||||
wall-clock bound fails under CI load (bc7c62dc).
|
||||
- When fixing one, list every timing assumption in the test, not only the
|
||||
one that failed.
|
||||
- Reproduce a timing flake by injecting the delay, not by rerunning until
|
||||
it fails (0f8e4ebc).
|
||||
- Seen 1× (2026-10-08).
|
||||
|
||||
**A test named for a guarantee fails when the guarantee breaks.**
|
||||
- #1318's TestHistoryWriteHoldsNoWaiter still passes with the waiter held:
|
||||
nothing in it waits while the history write is held. Filed as #1358 with
|
||||
a design that fails on the old code.
|
||||
- Before merging, break the guarantee by hand and watch the test go red.
|
||||
- Seen 1× (2026-10-08).
|
||||
|
||||
## Red tests and releases
|
||||
|
||||
@@ -236,6 +252,10 @@ cause:
|
||||
and CI never runs the GUI suite, so nothing shows it. e95852e3 (#948) named
|
||||
TestCursor (Windows) and TestOTelRetryAfterBound the same way. Both pass on
|
||||
macOS main and on CI. Neither is filed.
|
||||
- 10-08: 6ab4c16f fixed the run 0f8e4ebc had already fixed. bc7c62dc, #1216,
|
||||
a2a7fb11, 5d0c1dca and #950 each shipped past "fails the same on main".
|
||||
- Done right on 10-08: #1184 found the Go 1.26.8 segfault behind its flake,
|
||||
and 30a8ce29 fixed its flake at the cause.
|
||||
- Done right: 2bd49cc5's race was fixed at its cause in 25 minutes
|
||||
(28049bd2, -race -count=40 -cpu 1,2). 60f87223, b1cba654, 443f1cbc and
|
||||
395c4bc5 fixed flakes at their cause instead of retrying. 06420a71 fixed
|
||||
@@ -255,7 +275,9 @@ finished green. A cancelled run is not a pass.**
|
||||
`npm view`. 2f135ed4's middleware packages reached npm 5h after the
|
||||
release. Done right on 10-07: 35485f2d, 01aaa8e9 and 92e4fc32 each checked
|
||||
the package on npm first.
|
||||
- Seen 1× (2026-10-05).
|
||||
- 10-08: 01e66e0f's own run was cancelled, so its red showed up on the next
|
||||
commit's run. Every tag that day waited for a green run.
|
||||
- Seen 2× (2026-10-05, 2026-10-08).
|
||||
|
||||
## Merging and closing
|
||||
|
||||
@@ -284,6 +306,12 @@ it doesn't.**
|
||||
GUI tests up to date.
|
||||
- Seen 2× (2026-10-05, 2026-10-06).
|
||||
|
||||
**A large commit has a body: what changed, why, and what was run.**
|
||||
- 586f2bce changed 5133 lines with a subject line only. bb8a3e30, ed4476b9,
|
||||
e22d5eef and d2241a57 had no body either, so a later review can't tell
|
||||
what was verified.
|
||||
- Seen 1× (2026-10-08).
|
||||
|
||||
## Moved to the code standards
|
||||
|
||||
These were seen on three or more days. The rules and their evidence are in
|
||||
@@ -301,3 +329,16 @@ each one.
|
||||
past "fails the same on origin/main".
|
||||
- **Merge only the reviewed head, and write what was run before the merge**
|
||||
(10-05, 10-06, 10-07). On 10-07 #1037 merged with no record.
|
||||
- **Real use before building on it, or telling users it works** (10-05,
|
||||
10-07, 10-08). Acceptance criterion 5. On 10-08 bbf99143 "not tried on
|
||||
Linux" broke Linux vet with the WSL2 distro there, and #1185 was reviewed
|
||||
on macOS only.
|
||||
- **GUI tests in both engines, at narrow widths, in every language**
|
||||
(10-05, 10-06, 10-08). Acceptance criterion 4. On 10-08 aff4f9f2's zh-TW
|
||||
missed later strings, and usage-ledger was red ~7h after 586f2bce.
|
||||
- **A fix reaches every sibling** was missed again on 10-08: #1239 fixed
|
||||
Air 9h after a8290d27, and 173ab71e fixed Gemini bodies after #950.
|
||||
Done right on 10-08: a8290d27 itself, 9fcd332b (cmd.exe quoting at one
|
||||
choke point, tried on the Windows box), 74a809dc with 84f37ac9
|
||||
(re-derived account keys), #1271/#1275 (built-in and plugin together),
|
||||
3aeb7d1b (a refusal scoped to the model) and #1288.
|
||||
+25
-4
@@ -45,9 +45,14 @@ listed as not run, never left out.
|
||||
HOME. A test never reads or writes a live agent config (`~/.codex`,
|
||||
`~/.claude`, `~/.claude.json`, `~/.gemini` and the rest) or
|
||||
`~/.config/magpie`. Packages set this up in their `TestMain`, most with `testenv`.
|
||||
3. **The build and the suite.** Run `go vet ./...`, `go build`,
|
||||
3. **The build and the suite.** Run `gofmt -l` on the changed files (it
|
||||
prints nothing), `go vet ./...`, `GOOS=linux go vet -tags nogui ./...`,
|
||||
`GOOS=windows go vet ./...`, `go build`,
|
||||
`GOOS=linux go build -tags nogui`, `GOOS=windows go build` and
|
||||
`go test -tags nogui ./...`. In a chain of commands, set
|
||||
`go test -tags nogui ./...`. A cross-OS build doesn't compile
|
||||
`_test.go` files; vet does. On 10-08 bbf99143's test called
|
||||
`syscall.Getsid`, which Linux's syscall package doesn't have, and
|
||||
bb8a3e30 and e7de897f shipped unformatted. In a chain of commands, set
|
||||
`set -o pipefail`, so that `go test | tail && git push` stops on a
|
||||
failure.
|
||||
4. **GUI tests in both engines.** A change to `internal/gui/assets` runs the
|
||||
@@ -59,12 +64,28 @@ listed as not run, never left out.
|
||||
change adds an API the GUI calls, a selector or class other pages share,
|
||||
a CSS feature older WebKit lacks (`:has()`), or a layout breakpoint. On
|
||||
10-07, 479865bc, 134d388b, 61838f7c and 51626258 cleared four tests left
|
||||
red by commits that had run only their own page's tests.
|
||||
red by commits that had run only their own page's tests. On 10-08
|
||||
aff4f9f2's zh-TW was built before 685fa5dc's and 9dc0121f's strings
|
||||
(b1e795e5 added four 10 minutes later), and usage-ledger was red for
|
||||
about 7 hours after 586f2bce. When you reword a string, `git grep` the
|
||||
old text under `internal/gui/tests`, and test the empty case (a model
|
||||
with no list price, an account with no usage).
|
||||
5. **Real use where possible.** A change to a provider, subscription or
|
||||
agent is also checked against the real thing: a real account or key, the
|
||||
agent's real config format, the vendor's real reply. Do this in a sandbox
|
||||
HOME with copies of the credentials. Never touch the live gateway or a
|
||||
running magpie. If a real check isn't possible, say so.
|
||||
running magpie. If a real check isn't possible, say so. What wasn't
|
||||
tried with the real thing isn't built on and isn't told to users as
|
||||
working. A platform report (Windows, WSL, Linux) is checked on that
|
||||
platform; the Windows box and its WSL2 distro are there, and a GOOS
|
||||
build is not a test. A test of a gateway guard goes through the real
|
||||
server stack or the built binary (b2345078 called Handler without
|
||||
lanGuard, and every such call got 401). When the guesses about a vendor
|
||||
differ, design the check that tells them apart, or ask the reporter to
|
||||
run it. On 10-08 bbf99143 said "not tried on Linux" and
|
||||
broke Linux vet; #1185's Linux-only reap failure was reviewed on macOS
|
||||
only. a24458ca (#1063), 5d0c1dca (MSIX not tried, said plainly) and
|
||||
#1328 (on the Windows box, six break checks) are the model.
|
||||
6. **The UI rules** under [GUI](#gui) hold: `t()` strings, the app's own
|
||||
menu instead of `<select>`, no scroll on click, no colored left-border
|
||||
stripes.
|
||||
|
||||
Reference in new issue
Block a user