Repository navigation
fix(cli): DropSummary.Add silently accepts a negative count and understates other stages #1742
Description
DropSummary.Add accepts a negative n and silently subtracts it from the running total for that key. Since stages share the summary, a negative from one stage understates another, and the printed total no longer describes the run.
Found while reviewing PR #1725, where a latent path can produce exactly that.
The latent path
reportInstanceLimit's case kept == rec.Count: continue misfires when rec.Count == 0 and the rec is past len(after): the rec is skipped from dropped but still counted in belowMinCount, so drops.Add(DropMaxInstances, dropped-belowMinCount) receives -1.
Reproduced by the reviewer: the summary printed --max-instances=-1 with an understated total. The same zero-count rec also survives the tail-only floor loop in a non-tail position.
Why it is not urgent
Unreachable today, and the barrier was verified rather than assumed. pkg/scorer/scorer.go:88 filters Count <= 0 when MinCount > 0, and scoreLimitAndDisplay is the sole caller, feeding the same cfg.MinCount to both. So no zero-count recommendation reaches reportInstanceLimit on any live path.
Why it is still worth doing
The guard belongs on Add, not on the one caller that can currently reach it. A negative count is never meaningful for a drop tally, and Add is called from several stages — a guard there protects every one of them, including callers that do not exist yet. The reportInstanceLimit arithmetic is one way to produce a negative; it will not be the last.
Suggested: reject or clamp a negative n in Add, loudly enough that a caller producing one is discovered rather than silently absorbed. Given this codebase's recent history with silent-absorption defects, erroring is likely better than clamping.
Optionally also fix the kept == rec.Count condition so the zero-count case cannot reach Add negative in the first place. Defence in depth, but the Add guard is the load-bearing half.
Test note
A test asserting only the printed total will pass with the bug present whenever the negative happens to cancel out. Assert the per-key tallies, and include a case where a negative would be produced without the guard.
Activity
cristim commentedon Oct 16, 2026
Checked against origin/main and the pinned cloud-commitments-go:
DropSummary.Addlives in the shared module (pkg/common/drop_summary.go:41, still unguarded at the pinned version), so the load-bearing half is filed upstream as LeanerCloud/cloud-commitments-go#440. The CLI half (thekept == rec.Countcondition inreportInstanceLimit,cmd/multi_service.go:493) is deferred by [claude-cli-impl] while the open drafts #2203/#2204 edit that file; after #440 lands, this issue also needs a go pin bump and caller updates.cristim commentedon Oct 17, 2026
[claude-cli-impl] Marking status/blocked. The fix that matters is the guard on
DropSummary.Addin the shared module (LeanerCloud/cloud-commitments-go#440, still open). Once it ships, this issue needs a go pin bump and caller updates, plus the optionalreportInstanceLimitcondition fix in cmd/multi_service.go. External dependency: go#440.cristim commentedon Oct 20, 2026
Claiming. The go DropSummary.Add now panics on a negative count (go#518). Plan: fix cmd/multi_service.go:~519 (dropped can go negative) and the kept==rec.Count misfire on zero-count recs at :~508, with tests. [claude-cc-libs]
fix(cmd): never pass a negative count to DropSummary.Add in --max-ins…