From 461708907adfebfbe40ecd4ce254a89a3cf2b434 Mon Sep 17 00:00:00 2001 From: yetone Date: Fri, 9 Oct 2026 03:48:15 +0800 Subject: [PATCH] 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. --- LESSONS.md | 129 +++++++++++++++++++++++++++-------------- docs/code-standards.md | 29 +++++++-- 2 files changed, 110 insertions(+), 48 deletions(-) diff --git a/LESSONS.md b/LESSONS.md index 05a8f22f..c0e58498 100644 --- a/LESSONS.md +++ b/LESSONS.md @@ -1,4 +1,4 @@ - + # 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. diff --git a/docs/code-standards.md b/docs/code-standards.md index 696c2a47..27333ac2 100644 --- a/docs/code-standards.md +++ b/docs/code-standards.md @@ -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 `