[cristim]

Closes #440

What

common.DropSummary.Add accepted a negative n and subtracted it from that reason's tally. A negative count from one filtering stage silently understated the run's printed total. Add now panics on a negative n, naming the reason and the value, because it is a programming error. The signature is unchanged, so no caller needs to change. A zero is still a no-op that creates no reason key.

Callers on origin/main: all non-negative, none changed

• This repo: pkg/recfilter/filters.go:151 (constant 1) and pkg/recfilter/sizing.go:33 (len), :84 and :192 (constant 1). providers has no callers.
• cloud-commitments-cli:
  • cmd/multi_service.go:473 and cmd/multi_service_helpers.go:697,709 pass len(...), and cmd/multi_service_filters.go:43 passes 1.
  • cmd/multi_service_helpers.go:619-621 pass FamilyDropCounts, which are built only from len or zero counters (providers/aws/recommendations/family_nu.go:279,290,295).
  • cmd/multi_service.go:513 passes dropped - belowMinCount. Each below-minimum entry is also counted as dropped, and pkg/scorer rejects Count < MinCount when MinCount > 0 (scorer.go:103), so the result is never negative. This is the latent -1 path from cli#1742, which is unreachable.

Commit (<= 400 additions+deletions)

1. fix(common): panic on a negative DropSummary count (44)

Local verification (macOS, pkg)

• go build, go vet and go test ./... pass with and without GOWORK=off, and golangci-lint reports 0 issues.
• The tests assert per-reason tallies, not only Total(). A +5/-5 pair would cancel in the total.
• Fail-before: with main's drop_summary.go, NegativePanics, NegativeCannotCancelInTotal and NegativePanicsOnNilReceiver fail. They pass with the guard, and removing it is the mutation probe.

Independent reviews follow as PR comments.

🤖 Generated with Claude Code

Summary by CodeRabbit

• Bug Fixes
  • Negative drop counts now trigger an error instead of reducing tallies. Zero counts leave reason tallies unchanged.
  • Drop counts continue to accumulate by reason and update the overall total.

[cristim]

Independent review B (claude-cc-libs-reviewB-518, Sonnet, cold brief, not the author)

Reviewed SHA: 842b0d3
Verdict: APPROVE. No surviving mutants.

Local verification (pkg, with and without GOWORK=off): go test ./... passes and golangci-lint reports 0 issues.

Tests:

• They read d.counts directly. No per-reason getter exists (only Total(), IsEmpty() and FormatOneLine()), and the existing tests in the file do the same, so this is the stricter check.
• The panic message is pinned with PanicsWithValue.
• The guard sits before the nil check, so the nil-receiver test is meaningful. Before this change, a nil receiver with n < 0 returned silently, so the new panic is a deliberate fail-loud change.

Mutation probes (all killed):

Mutation	Tests that fail
guard removed	3
panic replaced by a silent return	3
zero creates a key	Zero_NoOp, PerReasonTallies
guard moved after the nil check	NilReceiver

Downstream: cli#1742 keeps its optional half open. cli has to bump its pkg pin to a version that includes this change. Separately, the reportInstanceLimit kept == rec.Count case could produce -1 when Count == 0, but scorer.go:88 makes that unreachable today, so it is defence in depth only.

Nit: CHANGELOG.md has entries for recent fix PRs (#493, #472, #449). An optional "Changed" line for the new panic would match that convention.

[cristim]

Independent review A (claude-cc-libs-reviewA-518, Sonnet, cold brief, not the author): caller proofs

Reviewed SHA: 842b0d3
Verdict: APPROVE. No actionable findings.

Callers on origin/main:

• platform: no Go reference to DropSummary or recfilter (docs/audits only).
• go: pkg/recfilter/filters.go:151 and sizing.go:84,192 pass the constant 1, and sizing.go:33 passes len(recs).
• cli:
  • cmd/multi_service.go:473 and cmd/multi_service_helpers.go:697,709 pass len(...), and multi_service_filters.go:43 passes 1.
  • multi_service_helpers.go:619-621 pass FamilyDropCounts, which are set only from len(indices) (family_nu.go:279,290) or zeroDrops++ (:295,:310) and summed with += (:207-209).
  • multi_service.go:513, dropped - belowMinCount:
    • With MinCount <= 0, dropTruncatedBelowMinCount returns nil (:452), so the subtraction is dropped - 0.
    • With MinCount > 0, every input has Count >= MinCount >= 1. scorer.go:103 rejects lower counts on the scorer path, and applyMinCountFloor (multi_service_csv_cap.go:58-68) does the same on the CSV path.
    • ApplyInstanceLimit (helpers.go:205-238) returns a prefix of its input, so each removed tail entry has kept = 0 and Count >= 1 and is counted in default: dropped++. That gives dropped >= len(removed).
    • An empty input never reaches this code, because capBinds (:390) requires total > MaxInstances > 0.
  • No reachable negative exists anywhere.

Design:

• The guard runs before the nil check, so a negative on a nil receiver panics, and that case is tested.
• The type is documented as not concurrency-safe and has no lock, so the panic path holds nothing.
• Panic beats returning an error: an error would break the signature for a case unreachable today, and callers would likely ignore it with _ =, bringing back the silent absorption.

Local verification (pkg, with and without GOWORK=off): go build, go vet and go test ./... pass, and golangci-lint reports 0 issues.

Mutation probes (all killed):

Mutation	Tests that fail
n < 0 changed to n <= 0	Zero_NoOp
guard removed	3
n < -1	2

[coderabbitai]

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info

⚙️ Run configuration

• Configuration used: Repository: LeanerCloud/cloud-commitments-go/.coderabbit.yaml
• Review profile: CHILL
• Plan: Essentials
• Run ID: 175fc8f1-46ee-470b-83f0-884f05dc2f2f

📥 Commits

Reviewing files that changed from the base of the PR and between a276ec0 and a903b5f.

📒 Files selected for processing: 2

• pkg/common/drop_summary.go
• pkg/common/drop_summary_test.go

Included review availability: This review used your included allowance. 0 included reviews remain after this review. Your included PR review attempts over the past 7 days set your current allowance at 1 review per hour.

📝 Walkthrough

DropSummary.Add now panics when n is negative. Its documentation clarifies that zero does not add a reason key. Tests cover accepted counts, negative counts, and nil receivers.

Changes: Drop summary count validation

Layer / File(s)	Summary
Validate additions and verify tallies pkg/common/drop_summary.go, pkg/common/drop_summary_test.go	DropSummary.Add panics on negative counts before its nil-receiver or zero-count checks. Tests verify per-reason accumulation, zero-count handling, unchanged tallies after a panic, and panic behavior on a nil receiver.

Priority: ⬇️ Low

Estimated code review effort: 2 (Simple) | ~8 minutes

Change: Bug fix · Severity of issue fixed: Low

Merge Risk: ⚪ Minimal · up to a903b

The change is mergeable with no identified current-head risk.

Pre-merge checks: 4 · 1

Failed checks: 1 warning

Check name	Status	Explanation	Resolution
Docstring Coverage		Docstring coverage is 20.00% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 5 functions across 2 files.	Write docstrings for the functions missing them to satisfy the coverage threshold.

Passed checks: 4 passed

Check name	Status	Explanation
Description Check		Check skipped - CodeRabbit’s high-level summary is enabled.
Title check		The title clearly and concisely identifies the main change: negative DropSummary counts now cause a panic.
Linked Issues check		Issue #440 requires DropSummary.Add to reject negative counts and tests that protect per-reason tallies. pkg/common/drop_summary.go now panics before tally mutation for n < 0, with the reason an…
Out of Scope Changes check		The changes are limited to DropSummary.Add documentation and validation, plus focused tests for the behavior required by issue #440. No unrelated production code or caller changes are present.

Full details: Linked Issues check

Explanation

Issue #440 requires DropSummary.Add to reject negative counts and tests that protect per-reason tallies. pkg/common/drop_summary.go now panics before tally mutation for n < 0, with the reason and value in the message. pkg/common/drop_summary_test.go checks per-reason accumulation, zero-count behavior, negative panics, cancellation prevention, and a nil receiver. The unchanged signature is compatible with the reported non-negative callers.

• Fix all pre-merge checks with AI

✨ Finishing Touches: 💡 1

📝 Generate docstrings: 💡

• Commit to this branch
• Create a new PR

🧪 Generate unit tests (beta)

• Commit to this branch
• Create a new PR

• Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

_{Comment @coderabbitai help to get the list of available commands.}

[cristim]

Merge gate verdict (claude-cc-libs)

Every gate holds at head a903b5f:

• Two independent reviews approve the reviewed head 842b0d3. The branch was then updated from main, and the PR diff is byte-identical to the reviewed diff (compared mechanically).
• CI is green on a903b5f, which is the reviewed change on top of current main.
• 0 unresolved threads; MERGEABLE/CLEAN.

CodeRabbit is waived as a merge gate under the 2026-10-19 decision. Squash-merging.