fix(limits): correct dangling comments and redisStore edge cases

Independent review round approved the fusion itself (11 to 3 RTT/req,
precedence verified branch by branch, latch/fail-open checked under
Yaegi against a dead Redis). This commit addresses the follow-up items
raised against that same change.

Six doc comments in limits.go still named evaluateScope,
budgetViolation, or tokenBudgetViolation as if they were live
functions to consult. All three were removed by the fusion commit; the
comments now point at checkAndCount and buildBudgetProbes, where the
same logic lives today. One further comment claimed checkAndCount
still slices a "storeIncrMulti result" — it now slices
storeIncrAndGetMulti's own incrVals.

TestCheckAndCount_FusedViolationPrecedence's doc comment claimed every
subtest engineers two simultaneous violation conditions. The seventh
engineers only one (the group violates; the user carries no limits at
all) — it pins a different claim, that a later scope is still
evaluated when an earlier one has nothing to check. The comment now
says so.

redisStore.incrAndGetMulti gained the same len()==0 guard incrMulti
already has, and a table-driven test proves a RESP error or a
non-integer value on a read key fails the whole call, matching
respClient.getBatch's own equivalent table.

Re-ran the interpreted Yaegi harness after these changes: RTT/req
stays at 3.00, cmds/req stays at 74.00 — the fusion's throughput win is
unaffected by this round's fixes.

limits.go

@@ -32,7 +32,7 @@ const storeDownLatchFor = 5 * time.Second
 // Window identifiers used throughout windowKey, counter metrics, and
 // retry-after calculation. windowHour is stats-only (v0.2 data-layer
 // task, see hourWindowTTL): checkAndCount/account both write it, but no
-// LimitsConfig field ever names it, so evaluateScope has nothing to
+// LimitsConfig field ever names it, so checkAndCount has nothing to
 // evaluate it against — GET /admin/api/usage/history (admin.go) is its
 // only reader.
 const (
@@ -48,7 +48,8 @@ const (
 // usage.prompt under metricTokIn and usage.completion under metricTokOut
 // separately, so the admin usage/history APIs can chart each direction on
 // its own. A TokensPerDay/TokensPerMonth limit still enforces a TOTAL
-// budget across both — see tokenBudgetViolation.
+// budget across both — see checkAndCount's own probe loop and
+// buildBudgetProbes' tokens field, below.
 const (
 	metricReq    = "req"
 	metricTokIn  = "tokin"
@@ -495,8 +496,8 @@ type limitScope struct {
 // totalScopeKind and totalScopeID name the synthetic "all LLM traffic"
 // scope withTotalScope (routes_unified.go) appends to every metered
 // route's scopes slice (v0.2 data-layer task): {kind: totalScopeKind, id:
-// totalScopeID}, limits always nil. evaluateScope's nil-limits check
-// (below) skips it during evaluation, so checkAndCount still counts it
+// totalScopeID}, limits always nil. checkAndCount's own nil-limits check
+// (below) skips it during evaluation, but still counts it
 // like any other scope — its req/tokin/tokout/cost counters accumulate
 // every LLM request across all users and groups combined, the admin
 // usage-history "total" series — but no configuration can ever throttle
@@ -885,8 +886,8 @@ func (l *limiter) failPolicyIncrAndGetMulti(entries []counterIncr, reads []strin
 	return incrVals, readVals, true
 }
 
-// storeDownViolation is the violation checkAndCount/budgetViolation return
-// when the configured store is unreachable and failOpen is false: the
+// storeDownViolation is the violation checkAndCount returns when the
+// configured store is unreachable and failOpen is false: the
 // request is refused instead of silently enforcing limits against a
 // non-shared fallback, so a backend outage cannot let every configured
 // limit go unenforced across a fleet of gateway instances.
@@ -974,8 +975,8 @@ func (l *limiter) getCounter(kind, id, metric, window string, t time.Time) (int6
 
 // checkAndCountKeysPerScope is the number of counterIncr entries
 // checkAndCount builds per scope (req:min, req:day, req:hour), and the
-// stride its flat storeIncrMulti result is sliced back into per-scope
-// counts by.
+// stride storeIncrAndGetMulti's flat incrVals result is sliced back into
+// per-scope counts by.
 const checkAndCountKeysPerScope = 3
 
 // budgetProbe names one token/cost budget checkAndCount's fused admission
@@ -1205,8 +1206,8 @@ func (l *limiter) account(scopes []limitScope, u usage, costMicros int64) {
 //
 // This is deliberately NOT folded into checkAndCount: a target scope
 // carries no limits by design (this round adds no config surface for
-// MCP/agent limits), so there is nothing for evaluateScope to check and no
-// violation this call could ever produce — it is pure accounting, exactly
+// MCP/agent limits), so there is nothing for checkAndCount to evaluate
+// and no violation this call could ever produce — it is pure accounting, exactly
 // like account() itself, hence the identical "no error return, ok
 // discarded" contract. Month exists here, uniquely, because the admin
 // dashboard's targets endpoint (admin.go's adminTargetCountersView)
@@ -1655,7 +1656,8 @@ const usageKeysPerScope = 8
 // tokensPerDay/tokensPerMonth pair (v0.2 data-layer task, tokens-in/
 // tokens-out split): a caller wanting the combined total a
 // LimitsConfig.TokensPerDay/TokensPerMonth limit is actually evaluated
-// against (tokenBudgetViolation) sums the two itself.
+// against (checkAndCount's own probe loop, via buildBudgetProbes) sums
+// the two itself.
 type scopeUsage struct {
 	kind               string // "user", "group", or "total", mirroring limitScope.kind
 	id                 string

limits_test.go

@@ -1320,9 +1320,15 @@ func TestTokenBudget_ExactBoundaryViolates(t *testing.T) {
 // within one scope, requests-per-minute, then requests-per-day, then
 // tokens-per-day, then tokens-per-month, then cost-per-day, then
 // cost-per-month; across scopes, the scope earlier in the given slice
-// (buildLimitScopes puts a user before their group) wins. Every subtest
-// engineers TWO simultaneously-true violation conditions and asserts only
-// the higher-precedence one is ever reported.
+// (buildLimitScopes puts a user before their group) wins. The first six
+// subtests each engineer TWO simultaneously-true violation conditions
+// (one per limit type in a precedence pair) and assert only the
+// higher-precedence one is ever reported; the seventh engineers only ONE
+// condition (the group violates, the user carries no limits at all) to
+// pin the separate claim that a later scope still gets evaluated at all
+// when an earlier one has nothing to check (review fix, 2026-08-22, round
+// 2: this doc comment previously claimed all seven subtests engineered
+// two conditions, which was not true of the seventh).
 func TestCheckAndCount_FusedViolationPrecedence(t *testing.T) {
 	t.Run("requests-per-minute beats requests-per-day in the same scope", func(t *testing.T) {
 		l := newLimiter(nil, true)

redis_store.go

@@ -134,6 +134,10 @@ func (s *redisStore) incrMulti(entries []counterIncr) ([]int64, error) {
 // (see its own doc comment) apply to the INCRBY half of this call; the GET
 // half is read-only and carries no such caveat.
 func (s *redisStore) incrAndGetMulti(entries []counterIncr, reads []string) ([]int64, []int64, error) {
+	if len(entries) == 0 && len(reads) == 0 {
+		return nil, nil, nil
+	}
+
 	cmds := make([][]string, 0, len(entries)*2+len(reads))
 	for _, e := range entries {
 		cmds = append(cmds,

redis_store_test.go

@@ -133,6 +133,73 @@ func TestRedisStore_IncrAndGetMulti_EmptyReads(t *testing.T) {
 	}
 }
 
+// TestRedisStore_IncrAndGetMulti_NoEntriesOrReads_SkipsThePipelineCall
+// asserts the empty-input guard (review fix, 2026-08-22, round 2 —
+// symmetry with incrMulti's own len(entries)==0 guard): no commands are
+// sent, and the call returns cleanly, when both entries and reads are
+// empty.
+func TestRedisStore_IncrAndGetMulti_NoEntriesOrReads_SkipsThePipelineCall(t *testing.T) {
+	ln := newFakeListener(t)
+	// No respStep script at all: any command reaching the fake server
+	// (including the connection-setup SELECT) would fail this test by
+	// timing out waiting for a script step that does not exist.
+	store := newRedisStore(newRESPClient(ln.Addr().String(), "", 0))
+	incrVals, readVals, err := store.incrAndGetMulti(nil, nil)
+	if err != nil {
+		t.Fatalf("incrAndGetMulti: %v", err)
+	}
+	if len(incrVals) != 0 || len(readVals) != 0 {
+		t.Errorf("incrVals=%v readVals=%v, want both empty", incrVals, readVals)
+	}
+}
+
+// TestRedisStore_IncrAndGetMulti_GetReplyErrors is the GET-reply-error
+// half of the perf-review round-3 pipelining test (review fix, 2026-08-22,
+// round 2 — parity with respClient.getBatch's own equivalent table,
+// TestRESPClient_GetBatch, resp_test.go): a RESP error reply or a
+// non-integer value on any read key fails the WHOLE call, matching
+// getBatch's own all-or-nothing contract, since incrAndGetMulti decodes
+// GET replies itself rather than delegating to getBatch.
+func TestRedisStore_IncrAndGetMulti_GetReplyErrors(t *testing.T) {
+	cases := []struct {
+		name          string
+		wantErrSubstr string
+		steps         []respStep
+	}{
+		{
+			name: "a RESP error reply on a read key fails the whole call",
+			steps: []respStep{
+				{wantArgs: []string{"INCRBY", "k", "1"}, reply: []byte(":1\r\n")},
+				{wantArgs: []string{"EXPIRE", "k", "60"}, reply: []byte(":1\r\n")},
+				{wantArgs: []string{"GET", "budget"}, reply: []byte("-ERR busy\r\n")},
+			},
+			wantErrSubstr: "ERR busy",
+		},
+		{
+			name: "a non-integer value on a read key fails the whole call",
+			steps: []respStep{
+				{wantArgs: []string{"INCRBY", "k", "1"}, reply: []byte(":1\r\n")},
+				{wantArgs: []string{"EXPIRE", "k", "60"}, reply: []byte(":1\r\n")},
+				{wantArgs: []string{"GET", "budget"}, reply: []byte("$3\r\nabc\r\n")},
+			},
+			wantErrSubstr: "non-integer value",
+		},
+	}
+	for _, c := range cases {
+		t.Run(c.name, func(t *testing.T) {
+			ln := newFakeListener(t)
+			steps := append([]respStep{{wantArgs: []string{"SELECT", "0"}, reply: []byte("+OK\r\n")}}, c.steps...)
+			runFakeRESPServer(t, ln, steps)
+
+			store := newRedisStore(newRESPClient(ln.Addr().String(), "", 0))
+			entries := []counterIncr{{key: "k", delta: 1, ttl: time.Minute}}
+			_, _, err := store.incrAndGetMulti(entries, []string{"budget"})
+			require.Error(t, err)
+			assert.Contains(t, err.Error(), c.wantErrSubstr)
+		})
+	}
+}
+
 // TestRedisStore_IncrBy_RoundsSubSecondTTLUp asserts a ttl under one
 // second is never sent to EXPIRE as 0 (which would delete the key
 // immediately) — it is rounded up to a 1s floor.