[Areson]

Stop-loss in Loader.Load miscounts and its percent form never triggers.

• "Now" was len(diff.removed), the number of removed monitors, not the number left. A small removal could trigger stop-loss (removing 2 of 10 with limit 3 counted as a loss of 8), and removing every monitor never did.
• blip.StopLoss returns the percent as a whole number (50 for 50%), but the check compared it to a 0 to 1 fraction, so a percent stop-loss could never trigger. The error message also printed the fraction as a percent.

Stop-loss now counts the net drop: monitors loaded before minus monitors loaded after (before − removed + new). New monitors offset removed ones, and changed monitors don't count. This matches the existing "before/now" message and the "big drop in the number" intent, and it allows a legitimate replacement such as renamed hosts. A drop equal to the limit is still allowed. The percent check compares lost*100 > limit*before so a drop exactly at the limit isn't pushed over by rounding. Only one of number or percent can be configured, so the two branches are now a switch.

Tests: TestLoaderStopLoss covers number and percent stop-loss below, at, and above the limit, with and without new monitors, plus full replacement, changed monitors, and removed and changed monitors in the same reload. 12 of its 23 cases fail on the old code. TestLoaderUnloadedMonitorsHaveNoCounters got ErrStopLoss only because of the miscount, so it now removes 3 of 4 monitors (a net loss of 2 against limit 1).

Docs: explained the count in monitors/loading.md and config-file.md#stop-loss, and added an Unreleased note that a percent stop-loss now triggers.

🤖 Generated with Claude Code

[zyin-squareup]

🤖 Review findings - created by zyin-squareup's code review agent - for /pull/195, 72d8b00.

Verdict: ship — clean, approve.

This PR fixes monitor-loader.stop-loss to count the net drop in monitors (before minus after) instead of len(diff.removed). It also makes the percent form compare against a whole-number percent, which it never did before. The riskiest piece is the new nNow formula. It correctly handles changed monitors, which show up in both diff.added and diff.changed, and the tests pin the fix.

Assumptions & unknowns

• Reviewed head 72d8b009279a0896e84f84738dc7fe61108af44a (3 commits) against freshly fetched origin/main. The branch is 0 commits behind.
• Local go build/go test could not run in this review environment because the permission policy blocked it. Execution evidence is limited to CI, whose build job runs go build ./... and go test -v ./... and passed at this exact head SHA. I also could not run any mutation or pin checks. The pin analysis below is by inspection only.

General suggestions

• blip.StopLoss accepts 100% and higher (config.go#L60, #L75-L76), but config-file.md#L178 says N is between 0 and 100, exclusive. Since lost <= nBefore, a limit of 100 or more can never trigger at loader.go#L336, so it silently does nothing. This was already true before the PR, and on base every percent did nothing, so it isn't worse. Now that percent stop-loss works, rejecting values of 100 or more in Validate would match the docs.
• The description says "12 of its 23 cases fail on the old code". With the event-message assertions added in the last commit, I count 15 of 23 by inspection: all 11 number cases plus the 4 percent "stopped" cases. That is more coverage than claimed, so it's harmless.

The one thing that could have broken, verified

nNow := nBefore - len(diff.removed) + len(diff.added) - len(diff.changed) (loader.go#L322) has to equal the size of the repo after the reload. I verified this by inspection:

• load() appends each changed monitor to diff.changed (#L503) and adds its replacement to diff.added (#L508), so the - changed term cancels the double count.
• Removed monitors are the IDs in ml.repo that are missing from validConfigs (#L474-L478), so nNow == len(validConfigs).
• The "changed" and "removed + changed" test cases cover this, and they passed in CI at this head.

Verified correct

• Base behavior (from the removed hunk): nNow = len(diff.removed). Number mode stopped any reload that removed fewer than nBefore - limit monitors, including the case with 0 removed and 5 changed, but never stopped a full removal. Percent mode compared a fraction of at most 1 against a whole-number limit, so it never fired. The PR describes this behavior change and the release notes flag it.
• Arithmetic: lost > 0 inside the branch, so uint(lost) can't wrap. The percent comparison is exact at the boundary (3 of 10 at 30% is allowed, 4 stops). nBefore > 0 there, so the message's division is safe.
• No state change on rejection: the switch covers every case because StopLoss returns either a number or a percent, never both. ErrStopLoss is returned before any unload or add, so the repo is untouched. On boot, nBefore = 0, so the first load can't trigger.
• Error contract unchanged: ErrStopLoss still maps to HTTP 412 in server/api.go.
• Concurrency: nBefore is read under ml.Lock(), and Load and Unload are serialized.
• Tests: TestLoaderStopLoss asserts the error, an unchanged repo (assert.Same per monitor), and the exact event message. It subscribes to events globally but cleans up with t.Cleanup(event.RemoveSubscribers), and there is no t.Parallel in the package. The fixture change in TestLoaderUnloadedMonitorsHaveNoCounters (net loss 2 against a limit of 1) still exercises the ErrStopLoss path and checks that the new monitor reports no counters. That test would fail on base, where the loss computes to 1.
• Docs: loading.md, config-file.md, and the release notes match the code, including "a drop equal to the limit is allowed" and the example of 4 removed and 1 added giving a drop of 3.
• CI: build/test, Semgrep, zizmor, and DCO all pass at the head. Security, performance, and migration categories don't apply.

This review was generated by Claude Code (claude-opus-5-5). Local tests were not run (go was blocked by this environment's permission policy); CI go test ./... passed at the reviewed head.