fix(cli): apply --max-instances once run-wide, not per service and region

[cristim]

Closes #1608

The defect

--max-instances is documented in three places as a cap on the total number of instances purchased across all recommendations:

• cmd/main.go flag help: "Maximum total number of instances to purchase"
• docs/cli/README.md:39: "Hard cap on the total number of instances purchased across all recommendations"
• docs/cli/purchase-safety.md:129: recommends it as "a final safety cap for a first run"

On the main purchase path it was applied inside checkDuplicatesAndApplyLimit, which runs once per (service, region) pair via fetchAndFilterRegionRecs. Every pair independently kept up to MaxInstances, and nothing re-capped the merged slice before the confirmation prompt or the purchase loop.

cudly --all-services --purchase --max-instances 10 fans out to 6 RI services across every opted-in region, so a cap of 10 could authorise on the order of 1500 instances. The per-region log line ("limiting to 10 instances", printed once per region) read exactly like the cap was holding.

The asymmetry with the CSV path confirms this was a bug rather than a design choice: cmd/multi_service.go:481 applies the same helper globally to the whole loaded set.

A second, worse failure mode in the same code

The cap sat behind the if serviceClient != nil check in fetchAndFilterRegionRecs. For any region where createServiceClient returned nil, the cap was not applied at all — and those recommendations still merged into the set that reaches the purchase loop. So the same placement produced two distinct failures: the reported one multiplies the cap by the number of service/region pairs, and this one removes it entirely for the affected regions. Applying the cap post-merge closes both, because it no longer depends on anything that happens inside a region.

The fix

• Removed the cap from the per-region path, and renamed checkDuplicatesAndApplyLimit to checkDuplicates so the name matches what it does.
• Apply the cap once in scoreLimitAndDisplay, between scoring and rendering.

Why the cap runs after scoring, not on the merged set

This is a correctness property, not a stylistic preference, and it is the part of the fix that is easiest to get wrong.

ApplyInstanceLimit consumes its input in slice order and drops the tail, so whatever ordering it is handed decides which commitments get bought. Capping the merged allRecs slice directly would respect the cap while selecting in fetch order: the first service in the first region would consume the entire budget and every other service and region would get zero, regardless of how good their recommendations were. Running the cap after scorer.Score — which sorts Passed by savings percentage descending — means the surviving instances are the highest-savings ones run-wide.

Both placements satisfy "total ≤ cap", so a pre-scoring fix would pass a purely total-focused regression test while silently buying worse commitments. ApplyInstanceLimit and applyGlobalInstanceLimit both document the contract, and TestMaxInstancesKeepsHighestSavingsNotFirstFetched (below) enforces it, so a future refactor cannot violate it with a green suite.

Rendering happens after the cap so the scored table, the confirmation prompt and the purchase loop all describe the same post-cap set.

The rest

• No silent clamping. Every reduced and every dropped recommendation is named before the confirmation prompt, and the drops are counted into the end-of-run summary under a new --max-instances reason:

  🔒 --max-instances=7 caps the whole run: the 3 recommendations that passed scoring total 12 instances.
     Keeping the highest savings-percentage recommendations first. The following are reduced or dropped:
     • reduced: ec2 us-west-2 m5.large 4 → 2 instances
     • dropped: ec2 eu-west-1 m5.xlarge (3 instances)
     Proceeding with 7 instances across 2 recommendations (1 reduced, 1 dropped).
  

• A truncated recommendation never lands below --min-count. Moving the cap downstream of the scorer means truncation is no longer re-filtered by the scorer's --min-count gate, so a survivor trimmed to fit the budget could land under a floor the operator set (--min-count 5 --max-instances 10 purchased a rec at count=2). --min-count is a hard floor everywhere else — the scorer rejects recommendations under it outright, the scheduler's meetsMinCount drops them, and both docs describe it as dropping recommendations below the number, applied to "the adjusted instance count (after coverage scaling)". Truncation by the cap is another form of sizing, so the floor is re-applied after it: such a recommendation is dropped, not purchased short, and named with that reason under a --min-count-after-cap drop reason. The freed budget is deliberately not redistributed, since the next recommendation would face an even smaller remainder and the same floor.

  Blast radius is exactly this one flag. The scorer's other filters are scale-invariant: MinSavingsPct reads SavingsPercentage and MaxBreakEvenMonths reads BreakEvenMonths, both ratios unchanged by scaling Count, and the service filter never looks at Count.

• Fail closed on the path that cannot evaluate the cap. processRegionRecommendations (the legacy per-region entry point, reachable only from tests today) has no view of the rest of the run, so it cannot enforce a run-wide cap. It now refuses to purchase when --max-instances is set rather than purchasing uncapped. Dry runs continue so the recommendations are still reported. An uncapped over-purchase of reserved capacity is not reversible; a refused one is a support ticket.

• ApplyInstanceLimit no longer credits a non-positive Count back to the remaining budget (a negative count would otherwise raise remaining and let later recommendations exceed the cap).

Verification

TestMaxInstancesCapsWholeRunAcrossServicesAndRegions drives the real pipeline (fetchAllRecs → scoreLimitAndDisplay) over 2 services x 3 regions, each answering with 8 instances (48 natural total), with the cap set to 10. A single-service or single-region fixture stays green with the bug present, which is why it fans out on both dimensions.

Against the pre-fix behaviour (per-region cap restored, run-wide cap made a no-op):

  [FAIL] TestMaxInstancesCapsWholeRunAcrossServicesAndRegions
     Error:    "48" is not less than or equal to "10"
     Messages: --max-instances must cap the sum across every service and region,
               got 48 instances from 6 service/region pairs

After the fix:

$ go test ./cmd/ -run 'TestMaxInstances|TestDropTruncatedBelowMinCount|TestApplyGlobalInstanceLimit|TestApplyInstanceLimit|TestReportInstanceLimit'
Go test: 25 passed in 1 packages

The test also asserts the selection is value-ordered (the 50% rec keeps all 8 instances, the 45% rec is reduced to the remaining 2) and that the 4 fully-dropped recommendations appear in the drop summary.

The selection property gets its own test

The test above pins the total, and by construction it cannot pin the selection: both the correct post-scoring placement and an incorrect pre-scoring one satisfy total <= cap, so it passes under either. An ordering contract that lives only in a comment is one refactor away from being violated with a green suite.

TestMaxInstancesKeepsHighestSavingsNotFirstFetched closes that gap with a fixture where best-value selection and fetch-order selection disagree: the first-fetched service (RDS) carries 10-12% savings, the second-fetched (ElastiCache) carries 40-50%, and the cap admits 10 of 36 available instances. Every purchased instance must come from ElastiCache.

Verified by mutation — moving the cap in scoreLimitAndDisplay from result.Passed to the pre-scoring input:

TestMaxInstancesKeepsHighestSavingsNotFirstFetched     FAIL
    expected: "elasticache"
    actual  : "rds"
    Messages: the cap must consume the scorer's savings-sorted order, not fetch order;
              rds us-east-1 at 10% savings was bought while better recommendations were dropped

TestMaxInstancesCapsWholeRunAcrossServicesAndRegions   PASS

That contrast is the point: under a pre-scoring cap the run still respects the operator's ceiling, so the total-focused test stays green while the tool quietly buys the worst commitments available. Both tests pass once the cap is back after scoring.

This generalises past this flag. ApplyInstanceLimit drops the tail of whatever slice it is handed, so every caller picks a selection policy by picking an input ordering, and the test is what keeps that from silently regressing.

The --min-count floor gets its own test

TestMaxInstancesNeverPurchasesBelowMinCount runs 6 recommendations of 6 instances with --max-instances 10 and --min-count 5. The budget leaves room for 6 + 4, and that 4 is below the floor, so the second recommendation must be dropped rather than purchased short.

Without the floor re-applied after truncation:

[FAIL] TestMaxInstancesNeverPurchasesBelowMinCount
   Error:    "4" is not greater than or equal to "5"
   Messages: --min-count 5 was set, but rds us-west-2 would be purchased at count=4

TestMaxInstancesCapsWholeRunAcrossServicesAndRegions and TestMaxInstancesKeepsHighestSavingsNotFirstFetched both stay green under that same mutation, which is exactly why neither could catch it: one pins the total, one pins the ordering, and neither pins the floor. TestDropTruncatedBelowMinCount covers the helper's boundary directly, including a truncated count exactly equal to the floor (kept, not dropped).

The test also pins that each dropped recommendation is counted once: 5 of 6 recommendations are dropped, 1 to the floor and 4 to the budget, so reportInstanceLimit excludes the floor drops from its own tally rather than both reasons claiming the same recommendation.

A correction to an earlier claim in this PR

An earlier revision of this description said TestProcessService_WithInstanceLimit "pins the new contract". It did not. processRegionRecommendations assigns result.recommendations before the cap block, and assigned it in the same place pre-fix, so an assertion on the returned recommendations passed on pre-fix code and guarded nothing — a vacuous assertion, the same class this codebase is fixing elsewhere.

It now asserts on the dry-run results, which carry the recommendations actually handed to processPurchaseLoop — the slice the cap used to shrink. Restoring the per-region cap makes it fail:

[FAIL] TestProcessService_WithInstanceLimit
   Error:    Not equal:
             expected: 20
             actual  : 15

TestProcessService_InstanceLimitRefusesRealPurchase covers the fail-closed guard.

Note for reviewers on the test's runtime

TestMaxInstancesCapsWholeRunAcrossServicesAndRegions drives the real pipeline, so checkDuplicates reaches GetExistingCommitments and issues an unauthenticated AWS call per service/region pair. Those return 403 MissingAuthenticationToken, which the production code already handles as a warning ("Could not check for existing RIs"), so you will see six of those lines in the test output and the package takes about 7 seconds. This is the established pattern for the existing TestProcessService* tests, and the assertions hold whether or not the network is reachable, since either outcome lands on the same warning branch. Stubbing it would mean changing shared test infrastructure inside a money-path change, so it is deliberately left alone.

Gates, all green on 6daeddb:

Gate	Result
go build ./...	clean
go vet ./...	clean
go test ./...	6700 passed in 43 packages
gocyclo -over 10 -ignore "_test\.go" . (v0.6.0)	no findings, exit 0
golangci-lint run --timeout=10m (v2.10.1, the CI pin)	0 issues., exit 0

Scope notes

Three purchase dispatch sites exist in cmd/, and all three are accounted for:

1. cmd/multi_service.go:165 (main path) - capped by scoreLimitAndDisplay.
2. cmd/multi_service.go:466 (--input-csv path) - already capped globally in filterAndAdjustRecommendations; unchanged by this PR. The CSV path does not bypass the cap. It does select differently, though: runToolFromCSV never calls scorer.Score (which has exactly one caller, on the main path), so the CSV cap consumes the file in row order rather than by savings. The docs now say so explicitly instead of describing main-path behaviour as if it were universal.
3. cmd/multi_service_helpers.go:425 (legacy per-region path) - now fails closed.

--override-count does not share this defect. It is a per-recommendation count setter (rec.Count = overrideCount), not a total. Setting each recommendation's count to N is idempotent under repetition, so applying it once per region and once run-wide produce identical results. Its separate problem is #1611 (it mutates Count without scaling the cost fields), which is untouched here.

#1611 does still apply to ApplyInstanceLimit: truncating Count leaves CommitmentCost / EstimatedSavings describing the pre-truncation quantity, so whenever the cap reduces a recommendation, the money figures the operator approves on the confirmation screen are wrong. This PR reduces how often that fires (once per run instead of once per service/region) but does not fix it; that stays #1611's job, and the fix belongs at the point Count is mutated so it covers both helpers at once.

[coderabbitai]

Warning

Review limit reached

You’ve reached a temporary PR review limit under our Fair Usage Limits Policy.

Your recent review volume is higher than typical usage, so adaptive limits are currently applied.

Next review available in: 17 minutes

Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available.
You're only billed for reviews past your plan's rate limits ($0.25/file).

How can I continue?

After more reviews become available, a review can be triggered using the @coderabbitai review command as a PR comment. Alternatively, push new commits to this PR.

To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews.

How do review limits work?

CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability.

For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window.

Please refer docs for additional details.

Review details

⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: e4740995-a877-4e6e-8506-18b23da0d99c

📥 Commits

Reviewing files that changed from the base of the PR and between 3af7f4c and 93fe75e.

📒 Files selected for processing (7)

• cmd/multi_service.go
• cmd/multi_service_helpers.go
• cmd/multi_service_max_instances_test.go
• cmd/multi_service_test.go
• docs/cli/README.md
• docs/cli/filtering.md
• pkg/common/drop_summary.go

📝 Walkthrough

Walkthrough

The multi-service recommendation pipeline now fetches recommendations without per-region instance limits, scores them, applies one run-wide --max-instances cap, reports reductions and drops, and prevents unsupported legacy purchases.

Changes

Global instance limit

Layer / File(s)	Summary
Global scoring and limit application cmd/helpers.go, cmd/multi_service.go, pkg/common/drop_summary.go, docs/cli/filtering.md	The pipeline applies --max-instances after scoring across all services and regions. It keeps highest-savings recommendations, tracks dropped items, and reports the capped results. Non-positive counts do not restore budget.
Uncapped per-region processing cmd/multi_service_helpers.go, cmd/multi_service_test.go	Per-region processing now adjusts duplicates without applying the instance limit. The legacy entry point refuses non-dry-run purchases when it cannot enforce the run-wide cap.
Global limit regression coverage cmd/multi_service_max_instances_test.go, cmd/multi_service_test.go	Tests cover cross-service and cross-region limits, ordering, drop accounting, unset limits, non-positive counts, input immutability, reporting, and legacy purchase behavior.

Estimated code review effort: 3 (Moderate) | ~25 minutes

Sequence Diagram(s)

sequenceDiagram
  participant MultiServicePipeline
  participant ProcessService
  participant ScoreLimitAndDisplay
  participant ApplyGlobalInstanceLimit
  MultiServicePipeline->>ProcessService: Fetch uncapped recommendations
  MultiServicePipeline->>ScoreLimitAndDisplay: Score combined recommendations
  ScoreLimitAndDisplay->>ApplyGlobalInstanceLimit: Apply run-wide instance cap
  ApplyGlobalInstanceLimit-->>ScoreLimitAndDisplay: Return capped recommendations and drop data
  ScoreLimitAndDisplay-->>MultiServicePipeline: Render, confirm, and purchase capped set

Loading

Possibly related PRs

• LeanerCloud/CUDly#875: This change extends the existing drop-accounting pipeline with --max-instances tracking and reporting.

🚥 Pre-merge checks | ✅ 5

✅ Passed checks (5 passed)

Check name	Status	Explanation
Docstring Coverage	✅ Passed	Docstring coverage is 81.82% which is sufficient. The required threshold is 80.00%.
Linked Issues check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title clearly and concisely describes the main change: applying --max-instances once across the entire run instead of per service and region.

✨ Finishing Touches

📝 Generate docstrings

• Create stacked PR
• Commit on current branch

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Commit unit tests in branch fix/1608-max-instances-global-cap

---

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

[coderabbitai]

Actionable comments posted: 2

🧹 Nitpick comments (2)

cmd/multi_service_max_instances_test.go (2)

86-99: 🩺 Stability & Availability | 🔵 Trivial | 🏗️ Heavy lift

Inject the service-client factory into the fetch path.

fetchAndFilterRegionRecs calls GetExistingCommitments for each of the six fixture pairs through real RDS and ElastiCache clients. The test mocks only GetRecommendations, so it depends on AWS credentials and network behavior.

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@cmd/multi_service_max_instances_test.go` around lines 86 - 99, The fetch path
in fetchAndFilterRegionRecs must use an injected service-client factory so tests
can provide mocked RDS and ElastiCache clients instead of calling real AWS
services. Thread the factory through fetchAllRecs and related callers, and
configure this test to use mocks for GetExistingCommitments while preserving the
existing GetRecommendations mock behavior.

Source: Coding guidelines

---

31-36: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value

Because the root module targets Go 1.26.5, remove the redundant svc, region := svc, region copies. The closures retain the correct per-iteration values without them.

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@cmd/multi_service_max_instances_test.go` around lines 31 - 36, Remove the
redundant `svc, region := svc, region` shadowing inside the nested loops that
build `maxInstancesFixtureServices` and `maxInstancesFixtureRegions`; rely on Go
1.26.5 per-iteration loop variable semantics while preserving the existing
savings calculation and closure behavior.

🤖 Prompt for all review comments with AI agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@cmd/multi_service_helpers.go`:
- Around line 410-422: Move the --max-instances refusal guard in the per-region
helper before createServiceClient and checkDuplicates, since it depends only on
cfg.MaxInstances and isDryRun; preserve dry-run reporting and the fail-closed
return for non-dry runs. In cmd/multi_service_helpers.go lines 410-422, apply
the guard before any AWS client or duplicate-check calls. In
cmd/multi_service_test.go lines 396-427, verify the test does not reach a real
AWS client and retains assert.Empty(t, results).

In `@cmd/multi_service.go`:
- Around line 219-223: Update applyGlobalInstanceLimit and its underlying
ApplyInstanceLimit flow so recommendations truncated from their original Count
also proportionally scale EstimatedSavings and CommitmentCost to the retained
count. Ensure the adjusted values are used by reporter.RenderSummary and
sumPassedRecs, while leaving recommendations unaffected when the cap does not
reduce Count.

---

Nitpick comments:
In `@cmd/multi_service_max_instances_test.go`:
- Around line 86-99: The fetch path in fetchAndFilterRegionRecs must use an
injected service-client factory so tests can provide mocked RDS and ElastiCache
clients instead of calling real AWS services. Thread the factory through
fetchAllRecs and related callers, and configure this test to use mocks for
GetExistingCommitments while preserving the existing GetRecommendations mock
behavior.
- Around line 31-36: Remove the redundant `svc, region := svc, region` shadowing
inside the nested loops that build `maxInstancesFixtureServices` and
`maxInstancesFixtureRegions`; rely on Go 1.26.5 per-iteration loop variable
semantics while preserving the existing savings calculation and closure
behavior.

🪄 Autofix

Fix all unresolved CodeRabbit comments on this PR:

• Push a commit to this branch (recommended)
• Create a new PR with the fixes

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: ff662051-92c3-44d6-bc4d-57d5d118297c

📥 Commits

Reviewing files that changed from the base of the PR and between ff808b2 and 3af7f4c.

📒 Files selected for processing (7)

• cmd/helpers.go
• cmd/multi_service.go
• cmd/multi_service_helpers.go
• cmd/multi_service_max_instances_test.go
• cmd/multi_service_test.go
• docs/cli/filtering.md
• pkg/common/drop_summary.go

[coderabbitai]

🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy lift

Scale cost and savings fields when the cap reduces Count.

ApplyInstanceLimit copies each recommendation and edits only Count. EstimatedSavings and CommitmentCost keep their pre-cap values. reporter.RenderSummary and sumPassedRecs both sum those fields.

A run capped from 8 instances to 2 therefore prints and confirms the savings and upfront commitment of 8 instances. The operator sees a number that does not describe the purchase.

Scale the per-instance economics with the reduced count, or state in the table and the prompt that the figures are pre-cap.

Sketch: scale the monetary fields inside the truncation step

 		adjusted := rec
 		if rec.Count > remaining {
+			ratio := float64(remaining) / float64(rec.Count)
 			adjusted.Count = remaining
+			adjusted.EstimatedSavings = rec.EstimatedSavings * ratio
+			adjusted.CommitmentCost = rec.CommitmentCost * ratio
+			adjusted.OnDemandCost = rec.OnDemandCost * ratio
 		}

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@cmd/multi_service.go` around lines 219 - 223, Update applyGlobalInstanceLimit
and its underlying ApplyInstanceLimit flow so recommendations truncated from
their original Count also proportionally scale EstimatedSavings and
CommitmentCost to the retained count. Ensure the adjusted values are used by
reporter.RenderSummary and sumPassedRecs, while leaving recommendations
unaffected when the cap does not reduce Count.

[cristim]

Correct, and deliberately out of scope here. Tracked as #1611.

Your reading of the consequence is exactly right: a run capped from 8 instances to 2 prints and confirms the savings and upfront commitment of 8, so the operator approves a number that does not describe the purchase. It is a money-display defect on the approval screen, which is the worst place for one.

Three reasons it is not being fixed in this PR.

It is not introduced here. ApplyInstanceLimit has always assigned .Count and left CommitmentCost, OnDemandCost, EstimatedSavings and RecurringMonthlyCost describing the pre-truncation quantity. Pre-fix, sumPassedRecs already summed a post-cap set carrying those same stale fields; the cap simply ran per (service, region) instead of once. This PR changes where the cap is applied, not what the helper does to the fields, and it reduces how often the desync fires (once per run rather than once per service/region) without making any instance of it worse.

The fix belongs one level down, at the point Count is mutated. ApplyCountOverride has the identical defect in the opposite direction: it raises Count and leaves the costs low, so --override-count 10 on a Count = 1 recommendation understates the upfront by 10x. Fixing only ApplyInstanceLimit here would leave the sibling broken and mean touching the same lines twice. Both should route through common.ScaleRecommendationCosts the way applyCoverage (cmd/helpers.go:173-178) and applyTargetCoverageRI already do, with a guard for oldCount == 0, and the regression test has to assert the cost fields rather than Count — a test that only checks Count passes today with the bug present.

Folding a cost-model change into a cap-placement change would make both unreviewable. This PR is already carrying the placement fix, a --min-count interaction, a fail-closed guard and four regression tests on a money path. Cost scaling touches every consumer of those fields — the confirmation prompt, the UpfrontPayment and EstimatedSavings CSV columns, the TOTAL row, and pkg/common/audit.go's EstimatedCost — and deserves its own diff and its own review.

#1611 has been updated with this analysis, including that the fix should land at the mutation point so it covers both helpers together.

Not resolving this thread; leaving it visible with the justification attached.

[coderabbitai]

@cristim, acknowledged. The defect predates this PR and the current change does not worsen it.

Deferring the cost-model fix to #1611 is appropriate. The fix should scale costs at every Count mutation point, including ApplyInstanceLimit and ApplyCountOverride, with regression tests for the monetary fields and zero-count handling.

I will leave this thread unresolved, as requested.

---

✏️ Learnings added

Learnt from: cristim
Repo: LeanerCloud/CUDly PR: 1725
File: cmd/multi_service.go:219-223
Timestamp: 2026-08-08T00:15:39.037Z
Learning: In the Go CLI codebase, `ApplyInstanceLimit` and `ApplyCountOverride` can change `common.Recommendation.Count` without scaling `CommitmentCost`, `OnDemandCost`, `EstimatedSavings`, and `RecurringMonthlyCost`. This pre-existing cost desynchronization is tracked in GitHub issue `#1611`. The eventual fix should occur at each `Count` mutation point through `common.ScaleRecommendationCosts`, guard against `oldCount == 0`, and test the scaled monetary fields rather than only the count.

Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.

_{You are interacting with an AI system.}

[cristim]

Adversarial review — PR #1725 @ 3af7f4cda

Independent review. I did not trust the implementer's report: every claim below was re-derived from the committed diff in a separate worktree (/private/tmp/claude/rev-1725, detached at 3af7f4cda), and the pre-fix comparisons were run in a second throwaway worktree where I reverted the source myself.

Verdict: the fix is correct and the core defect is closed. No blocking findings. Three non-blocking findings below (one behavioural, one test-quality, one docs), plus an assessment of the AWS-call caveat.

---

Gates I ran myself

Gate	Result
go build ./...	exit 0
go vet ./...	exit 0
go test ./...	6692 passed in 43 packages, exit 0
gocyclo -over 10 -ignore "_test\.go" . (v0.6.0, from repo root as CI does)	no output, exit 0
golangci-lint run --timeout=10m @ v2.10.1 (installed to a scratch GOBIN; my system binary is 2.11.4 and would have been the wrong ruleset)	0 issues., exit 0

CI on this head is red — but not because of this diff. CI - Build & Test run 31223209301: 12/14 jobs green; the only real failure is Security Scanning → npm audit --audit-level=high (frontend) on js-yaml GHSA-5p4m-2wfm-xmqj and nanoid GHSA-2v37-7h3g-55p8. This PR touches no JS or lockfile (cmd/*.go, pkg/common/drop_summary.go, docs/cli/filtering.md only). Pre-existing repo-wide red, tracked separately. Lint Code, Unit Tests, Integration Tests, E2E Tests all green.

---

Claims re-derived

1. ApplyInstanceLimit had exactly two non-test call sites — CONFIRMED (execution).
git grep -n "ApplyInstanceLimit(" HEAD~1 → cmd/multi_service.go:481 and cmd/multi_service_helpers.go:580, exactly the stated lines. The containing helper checkDuplicatesAndApplyLimit had two callers (multi_service_helpers.go:411 legacy, :647 main fetch), so the per-region cap reached both per-region entry points.

2. Completeness of the cap — CONFIRMED (reading + grep), independently enumerated.
I worked backwards from the API call rather than forwards from the flag. Within cmd/, serviceClient.PurchaseCommitment is invoked at exactly one site: cmd/multi_service_helpers.go:247 inside executePurchase. executePurchase has exactly two callers — purchaseSingleRec (cmd/multi_service.go:336) and processPurchaseLoop (cmd/multi_service.go:615). That closes the enumeration at three dispatch paths, matching the PR body:

• cmd/multi_service.go:165 → executePurchasePipeline → purchaseSingleRec. Capped by scoreLimitAndDisplay.
• cmd/multi_service.go:466 (--input-csv) → processPurchaseLoop. Capped globally at cmd/multi_service.go:547-551 in filterAndAdjustRecommendations, on the whole loaded set before the group-by-service/region loop. Not a bypass — confirmed by reading, and the only thing that runs afterwards is adjustRecsForDuplicates, which can only reduce counts.
• cmd/multi_service_helpers.go:425 (legacy per-region) → processPurchaseLoop. Fail-closed.

--max-instances exists only in cmd/ and pkg/common (repo-wide grep). internal/purchase/execution.go:1086 and mcp/tools/purchase.go:638 are separate purchase surfaces with their own controls, not driven by this flag — out of scope for this cap, but worth stating explicitly so "all paths" isn't read wider than it is.

The legacy refusal genuinely prevents a purchase, not just a display step — CONFIRMED (reading + execution). cmd/multi_service_helpers.go:418-422 returns result before the sole processPurchaseLoop call at :425, and result.results is still the empty slice. processService/processRegionRecommendations have no non-test callers (git grep, production files only), so this path is test-reachable today; the guard is future-proofing, correctly placed.

3. Placement — CONFIRMED (reading).
scorer.Score (pkg/scorer/scorer.go:56-67) sorts Passed by SavingsPercentage desc, then EstimatedSavings desc, then Service|Region|ResourceType asc. Deterministic, best-first, as the doc comment on applyGlobalInstanceLimit claims.

Nothing between the cap and the purchase re-expands or re-fetches. scoreLimitAndDisplay assigns the capped slice to result.Passed before RenderTable/RenderExcluded/RenderSummary, and runPurchaseAndReport then feeds the same scoredResult.Passed to both sumPassedRecs (the confirmation prompt) and executePurchasePipeline (the purchase loop). executePurchasePipeline reads rec.Count only. Table, prompt and purchase loop observe the identical post-cap set — no approve-one-thing-buy-another gap.

4. Off-by-one and budget accounting — CONFIRMED (reading + execution).
The sum of positive adjusted.Count can never exceed maxInstances: remaining starts at the cap, the loop breaks at remaining <= 0, each rec is clamped to remaining before append, and remaining decreases by exactly the appended positive count. Cannot overshoot by one. The cap below the first rec case keeps a single reduced rec at exactly the cap (covered by the table test). A zero Count neither consumes nor releases budget (behaviour unchanged from pre-fix, where remaining -= 0 was already a no-op). A negative Count no longer credits budget back — that is the real delta, and TestApplyInstanceLimitNonPositiveCountDoesNotCreditBudget pins it.

5. The fail-open — CONFIRMED (reading HEAD~1). Real. Pre-fix cmd/multi_service_helpers.go:645-648 called checkDuplicatesAndApplyLimit — which contained the cap — only under if serviceClient != nil, while fetchAllRecs appended the returned recs to all unconditionally. A region whose createServiceClient returned nil contributed fully uncapped recommendations to the merged set. Moving the cap to scoreLimitAndDisplay, which runs on the merged set with no client dependency, closes it.

6. --override-count does not share the defect — CONFIRMED (reading). ApplyCountOverride (cmd/helpers.go:491) is a per-recommendation setter (result[i].Count = int(overrideCount)), so per-region and run-wide application are identical. It is applied at cmd/multi_service_helpers.go:556 during fetch, i.e. before the run-wide cap, so --override-count 100 --max-instances 10 correctly yields 10.

7. Test quality — CONFIRMED by reverting the source myself, not by reading pasted output.
In a second worktree at the same commit I made applyGlobalInstanceLimit a no-op and restored the per-region cap inside fetchAndFilterRegionRecs:

[FAIL] TestMaxInstancesCapsWholeRunAcrossServicesAndRegions
   Error Trace: cmd/multi_service_max_instances_test.go:102
   Error:       "48" is not less than or equal to "10"

Exactly the claimed failure. The assertion is on CalculateTotalInstances(scored.Passed) over the merged output of fetchAllRecs across both services and all three regions, so it is a genuine sum-across-the-whole-run assertion, not a per-service total. The 2x3 fan-out is necessary: each region's single 8-instance rec sits under the cap of 10, so the defect only shows in the sum.

---

Findings

F1 — Low/Medium · behavioural · not mentioned in the PR body

Moving the cap after scoring lets a survivor land below --min-count.

scorer.Score applies rec.Count < cfg.MinCount (pkg/scorer/scorer.go:87) and then applyGlobalInstanceLimit truncates the tail rec. Pre-fix the cap ran during fetch, upstream of scoring, so a truncated rec was re-checked against MinCount; post-fix it is not. Demonstrated by execution with a throwaway probe against the committed code (--min-count 5 --max-instances 10, two 8-instance recs):

survivor 0: ec2 m5.large  count=8 (min-count=5)
survivor 1: ec2 m5.xlarge count=2 (min-count=5)   <-- below the user's stated minimum

The user asked for no commitment smaller than 5 instances and gets a 2-instance purchase. Direction of the error is under-purchase, not over-spend, so it does not undermine the fix, and it only fires when both flags are set and the cap bites mid-recommendation. --min-savings-pct and --max-break-even-months are scale-invariant and unaffected; --min-count is the only count-dependent scorer filter, so this is the whole blast radius.

Cheapest proportionate fix: after truncation, drop the tail rec if cfg.MinCount > 0 && kept < cfg.MinCount and report it as a --max-instances drop. Alternatively, just document the interaction. I'd accept either, or a follow-up issue — this should not block the merge.

F2 — Low · test quality

TestProcessService_WithInstanceLimit's new assertion is vacuous; the PR body overstates it.

The body says it "now pins the new contract (the per-region path returns the full 20 instances, uncapped)". It does not: result.recommendations = filteredRecs is assigned at cmd/multi_service_helpers.go:398, before the dedup/cap block, and it was assigned in the same place pre-fix (HEAD~1:cmd/multi_service_helpers.go:397). So assert.Equal(t, 20, CalculateTotalInstances(recs)) was already true before this PR.

Confirmed by execution — with the legacy path reverted to pre-fix in my scratch worktree:

[FAIL] TestProcessService_InstanceLimitRefusesRealPurchase
[PASS] TestProcessService_WithInstanceLimit

TestProcessService_InstanceLimitRefusesRealPurchase is a real guard (pre-fix it gets 2 cancelled results from the non-TTY confirmation path instead of an empty slice, so it correctly distinguishes "refused before the loop" from "cancelled inside it"). TestProcessService_WithInstanceLimit is a correct assertion but not a regression guard. Suggest softening its comment, or asserting on the value the change actually moved. Not worth a re-push on its own.

F3 — Low · docs

The new docs/cli/filtering.md paragraph is true for the default pipeline but false for --input-csv.

"It is applied after scoring, so the instances that survive are the highest-savings ones run-wide" — runToolFromCSV never calls scorer.Score. On the CSV path the cap is applied in filterAndAdjustRecommendations in CSV file order, so which recommendations survive is whatever order the file happens to be in. The "reduced or dropped are listed by name" sentence is also main-path only; the CSV path prints the old one-line 🔒 Applied instance limit: … at cmd/multi_service.go:551. Worth one clause scoping the paragraph. Separately, docs/cli/README.md:39 still reads only "Applied after coverage scaling" and doesn't say the cap is run-wide — that's the line most likely to be read first.

F4 — Nit

ApplyInstanceLimit keeps a Count <= 0 rec in the returned slice (it only skips the budget decrement). Not reachable on the main path today — adjustRecommendationsAgainstExisting (cmd/helpers.go:670) drops Count <= 0, and sizing has explicit *-sized-to-zero drop reasons — so this is hardening only. Consequence if it ever were: in reportInstanceLimit, a zero-count rec past the truncation point hits kept == rec.Count (0 == 0) and is silently skipped (harmless), while a negative one prints dropped: … (-5 instances) (cosmetic).

F5 — Nit

"Keeping the highest-savings recommendations first" (cmd/multi_service.go:263) is really highest savings percentage. A 1-instance 60% rec outranks a 50-instance 55% rec and consumes budget first. This is consistent with the scorer's canonical ranking and with the order the table displays, so it's defensible and I'm not asking for a change — but the phrase reads as absolute dollars.

---

The AWS-call caveat — assessed

Verified by running it. The two new tests make 12 real outbound AWS calls (2 services x 3 regions x 2 tests) to rds.* / elasticache.*, each returning 403 MissingAuthenticationToken, handled as a warning in checkDuplicates (cmd/multi_service_helpers.go:581-583). TestMaxInstancesCapsWholeRunAcrossServicesAndRegions takes 7.37s.

The "established pattern" claim is accurate — I ran the pre-existing, untouched TestProcessService_WithOverrideCount and it produces the identical 403 warning. This PR amplifies the pattern 6x, it doesn't introduce it.

Is it flaky in CI? No. .github/workflows/ci.yml has no configure-aws-credentials step and no AWS secrets in any job, so the 403 branch is deterministic there. The residual risks are:

1. Network-isolated runner — the SDK would retry with backoff before erroring. Still green, just slower. Degrades to slow, not red.
2. A developer running go test ./... with live credentials — GetExistingCommitments would succeed, and NewDuplicateChecker(0) resolves to DefaultDuplicateCheckLookbackHours = 24 (cmd/helpers.go:24), so any RI purchased in that account in the last 24h keyed db.t3.small|<region>|<engine> would be deducted and break require.Equal(t, naturalTotal, CalculateTotalInstances(allRecs)). Narrow, and note the fixture uses db.t3.small for both RDS and ElastiCache (ElastiCache nodes are cache.*), which is what would collide.

Neither is introduced by this PR and neither justifies blocking it. Injecting a fake ServiceClient so checkDuplicates never touches the network would fix the whole family of TestProcessService* tests at once — a good standalone cleanup issue, out of scope here.

---

Proportionality

Clean. No new mode, flag, or config knob; no second source of truth for the cap. ApplyInstanceLimit stays the single truncation primitive and gains a doc comment stating the caller's obligation. checkDuplicatesAndApplyLimit → checkDuplicates is a rename that makes the name match the behaviour, and it sheds the now-unused cfg parameter. Net +75 production lines, most of it the no-silent-clamping reporting, which is the right thing to spend lines on for a money path.

Residual risk

• F1 is the only behavioural surprise, and it errs toward buying less than asked.
• The #1611 money-field staleness on a reduced recommendation (CommitmentCost / EstimatedSavings still describe the pre-truncation quantity) now feeds the run-wide confirmation prompt via sumPassedRecs. I checked whether this PR makes it worse: it does not — pre-fix runPurchaseAndReport already summed a post-cap set with the same stale fields. The PR discloses it and correctly leaves it to fix(cli): --override-count and --max-instances mutate Count without scaling costs, misstating the spend #1611. Worth keeping visible: the number on the confirmation screen can overstate savings whenever the cap reduces a recommendation.
• Ordering by savings percentage rather than absolute dollars means a run capped low can spend its budget on many small high-percentage commitments. Intentional and consistent with the displayed ranking; flagging only so it's a known property rather than a surprise.

[cristim]

Adversarial review — delta at 735d58be (test(cli): pin the --max-instances selection order, not just the total)

Follow-up to my review at 3af7f4cda. I re-checked the head before reporting and reviewed the current SHA, in a fresh worktree at 735d58be.

Verdict on the delta: the new test does exactly what it claims. Verified by mutation, not by reading. My previous verdict is unchanged — the fix is correct, no blocking findings.

---

Production code is byte-identical to 3af7f4cda

Verified, not assumed:

$ git diff --stat 3af7f4cda 735d58be -- . ':(exclude)*_test.go'
(empty)

$ git diff --name-only 3af7f4cda 735d58be
cmd/multi_service_max_instances_test.go

So every code conclusion from my previous comment carries over unchanged: cap completeness, the budget arithmetic in ApplyInstanceLimit, the placement/ordering argument, the closed fail-open, and findings F1/F3/F4/F5. I re-ran the completeness enumeration on this head anyway rather than lean on the diff: PurchaseCommitment is still called at exactly one site in cmd/ (multi_service_helpers.go:247), executePurchase still has exactly two callers (multi_service.go:336, :615), and ApplyInstanceLimit still has exactly two call sites (multi_service.go:250 run-wide, :549 CSV). No uncapped path.

Gates, re-run by me on 735d58be

Gate	Result
go build ./...	exit 0
go vet ./...	exit 0
go test ./...	6693 passed / 43 packages, exit 0 (exactly +1 vs 3af7f4cda, the new test)
gocyclo -over 10 -ignore "_test\.go" .	no output, exit 0
golangci-lint run --timeout=10m @ v2.10.1	0 issues., exit 0

CI on this head: same picture as before — only Security Scanning → npm audit (frontend) fails, same js-yaml / nanoid advisories, unrelated to a Go-test-only commit.

---

The premise, confirmed independently

Before the new test landed I checked the claim myself on 3af7f4cda by moving applyGlobalInstanceLimit to before scorer.Score in scoreLimitAndDisplay (call it M1 — a run-wide cap applied in fetch order). TestMaxInstancesCapsWholeRunAcrossServicesAndRegions passed under M1. The diagnosis was right, and the reason is worth recording: the old fixture set savings := 50.0 - 5*float64(i*len(regions)+j), i.e. 50/45/40/35/30/25 in exactly the fan-out order, so fetch order and savings order coincided. Even the test's explicit selection assertions (50% keeps 8, 45% reduced to 2) survived M1, because the first-fetched recs were the best ones. The blind spot was in the fixture, not in the assertions.

newMaxInstancesMockClientWith(count, savingsFor) is the right fix for that: it makes the fan-out-position → savings mapping a parameter, so the degenerate case stays available for the total test while the new test can invert it.

Mutation matrix — all four runs executed here, none taken on trust

Mutation	…CapsWholeRun…	…KeepsHighestSavingsNotFirstFetched	…NotAppliedWhenUnset
none (735d58be as committed)	PASS	PASS	PASS
M1 — run-wide cap moved before scoring	PASS	FAIL	PASS
M2 — per-region cap, i.e. the original #1608 defect	FAIL	FAIL	PASS

M1 failure:

[FAIL] TestMaxInstancesKeepsHighestSavingsNotFirstFetched
   multi_service_max_instances_test.go:189
   Not equal:  expected: "elasticache"   actual: "rds"

M2 failure:

[FAIL] TestMaxInstancesCapsWholeRunAcrossServicesAndRegions
   Error: "48" is not less than or equal to "10"
[FAIL] TestMaxInstancesKeepsHighestSavingsNotFirstFetched
   Not equal:  expected: 10   actual: 36

This is exactly the contrast that was asked for, and the two tests are genuinely complementary rather than redundant: M1 is caught only by the new test, M2 is caught by both. A test that only caught the mutation it was written against would be weak evidence; this one also catches the bug that actually shipped.

The fixture earns that. RDS is maxInstancesFixtureServices[0] and gets 10/11/12%; ElastiCache is fetched second and gets 40/45/50%, with the best rec (50%) in eu-west-1, the last region of the last service — the worst possible position for a fetch-order cap. 6 recs x 6 instances = 36 available against a cap of 10, so the cap has to discard 26 instances and every surviving one must be ElastiCache. toolCfg.MinSavingsPct = 0 is set deliberately so the 10-12% recs reach the cap rather than being scored out first — without that the test would pass for the wrong reason, and the inline comment says so.

require.Equal(t, maxInstances, CalculateTotalInstances(scored.Passed)) being labelled "proves nothing on its own here … asserted only to keep the fixture honest" is accurate and I'd rather it stay than be removed.

Effect on my earlier F2

F2 flagged TestProcessService_WithInstanceLimit as vacuous. That is unchanged — it still passes on pre-fix code, so it is still a correct assertion rather than a regression guard. But F2's weight drops: the primary regression coverage is now a complementary pair that pins both total and selection, which is materially better than what I reviewed at 3af7f4cda. I would not hold the PR for F2.

One new observation on the delta

The cmd package now makes 18 real outbound AWS calls across these three tests (6 per test, all 403 MissingAuthenticationToken, handled as warnings), up from 12. Measured wall-clock for the three: 3.86s + 1.45s + 0.95s ≈ 6.3s, package total 10.06s. My earlier assessment stands unchanged — CI has no AWS credentials configured in ci.yml, so the 403 branch is deterministic there and this is not flaky; it is slow and network-dependent. The new test adds a sixth of that cost for a genuinely valuable property, which I think is a good trade. Injecting a fake ServiceClient remains the right standalone cleanup for the whole TestProcessService* / TestMaxInstances* family.

Unchanged findings from 3af7f4cda

• F1 (Low/Med) — the post-scoring cap can leave a survivor below --min-count (demonstrated: --min-count 5 --max-instances 10 → a 2-instance purchase). Production code is byte-identical, so this still reproduces. Still not blocking; errs toward under-purchase.
• F3 (Low, docs) — the new filtering.md paragraph is main-path only; runToolFromCSV never calls scorer.Score and caps in CSV file order. docs/cli/README.md:39 still omits that the cap is run-wide.
• F4 / F5 — nits, unchanged.

Recommendation unchanged: merge once #1716 unblocks Security Scanning. The test work on this commit strengthened the PR; nothing in it changed my read of the fix.

[cristim]

Adversarial delta review, 3af7f4cda to 93fe75e5f

Independent reviewer. Re-derived from the committed code in a clean worktree at the current head; nothing taken from the PR description on trust. Findings split into execution-verified and reading-derived.

Delta scope: three commits, not two, and the branch was rebased

3af7f4cda is not an ancestor of the current head, so a plain git diff between them mixes the real delta with rebase noise (a frontend/package-lock.json change from 9102e1c2a, which is the new base). Reconstructed:

commit
0e01fc799	rebased equivalent of the reviewed 3af7f4cda. Verified content-identical over cmd/ docs/ pkg/ (empty diff), so the rebase preserved exactly what was reviewed
f6c0821ec	test-only, adds TestMaxInstancesKeepsHighestSavingsNotFirstFetched
326112eca	--min-count floor after truncation
93fe75e5f	hoists the --max-instances refusal guard

So three commits landed since the review, one of them test-only. Everything below covers all three.

Gates, run locally at the head

gate	result
go build ./...	clean
go vet ./...	clean
go test -race -count=1 ./...	exit 0, 31 packages ok, 0 FAIL
gocyclo -over 10 -ignore "_test\.go" .	clean
golangci-lint run at CI-pinned v2.10.1	0 issues.

CI status correction: Security Scanning has now completed a run on this head, with conclusion success. Checked the raw check-runs API rather than the summary: 19 named checks plus CI Success, all completed, all success, no null or queued conclusions. The earlier caveat about unconfirmed npm clearance no longer applies to 93fe75e5f.

---

Verdict: no live defect. Both new behaviours hold up under mutation.

1. The --min-count floor drop (execution-verified)

The prefix invariant holds. A 200,000-case fuzz over ApplyInstanceLimit (slice lengths 0-5, counts in -4..9 including zero and negatives, caps in -2..17) confirms all three properties the design leans on: the result is always a positional prefix of the input, at most one element's Count differs, and that element is always the last kept. The mechanism is tight: truncation only fires when rec.Count > remaining with remaining > 0, so adjusted.Count = remaining, remaining hits exactly 0, and the next iteration breaks.

One correction to the reasoning in the doc comment, not the code: the invariant reportInstanceLimit actually depends on is guaranteed by ApplyInstanceLimit's append-then-break shape plus kept = kept[:len(kept)-1], not by the "reduces at most one recommendation" argument the comment gives. The conclusion is right; the stated justification is over-specified.

To show the stake, with a deliberately non-prefix after the reporter misattributes completely: before = [A:5, B:4, C:3], after = [A:5, C:3] prints reduced: B 4 → 3 (B was removed) and dropped: C (3) (C was kept whole). The shipped code cannot construct that after.

Freed budget is not redistributed. before = [A:9, B:100, C:1], cap 10, floor 2. B truncates to 1 and is floor-dropped, freeing exactly the 1 instance C needs. Result is [A:9]; C is not admitted.

Exact boundary is correct and agrees with the scorer. cmd/multi_service.go:290 uses strict <, so a truncation landing exactly on the floor is kept (verified: truncated to 3 with --min-count 3 is kept and reported as reduced; with --min-count 4 it is dropped). pkg/scorer/scorer.go:88 uses strict < too. No off-by-one between the two gates.

2. Drop counting is exactly once (execution-verified)

The important case, a cap-drop and a floor-drop in the same run: before = [A:9, B:100, C:5, D:6], cap 10, floor 2 gives Dropped 3 recs: --max-instances=2, --min-count-after-cap=1, total 3 against 3 recommendations actually removed. B, which was reduced 100 → 1 and then floor-dropped, is reported as dropped and not also as reduced (0 reduced in the trailer), because a floor-dropped rec lands at i >= len(after) and takes the default branch. The double-count the implementer caught itself on is genuinely fixed.

DropSummary.Add early-returns on n == 0, so the unconditional drops.Add(DropMinCountAfterCap, len(removed)) does not emit spurious zero entries.

3. The guard hoist (reading-derived, with the test verified by mutation)

The hoisted guard is genuinely unconditional: it sits immediately after result.recommendations = filteredRecs with no intervening branch, and the only earlier returns are the two "no recommendations" paths that could not purchase anyway.

Nothing that should run before a refusal was skipped. result.recommendations is assigned from filteredRecs, not adjustedRecs, both before and after the hoist, so checkDuplicates never influenced the returned recommendations on this path; adjustedRecs fed only processPurchaseLoop, which is unreachable on a refusal. The dry-run path is untouched (&& !isDryRun still falls through to the client, the duplicate check and the loop). The only behavioural loss is the "Service client not yet implemented" warning no longer printing on a refusal, which is immaterial.

Worth stating plainly, in the change's favour: under the old placement this unit test issued a real outbound AWS call (DescribeReservedDBInstances, 403 MissingAuthenticationToken). The hoist removes a live network dependency from the test suite, on top of the control-flow simplification.

4. The NotContains assertion bites (execution-verified)

Moving the guard back below createServiceClient / the nil-client return / checkDuplicates makes the test fail, at exactly that assertion:

multi_service_test.go:449:
    Error: "... ⚠️  Warning: Could not check for existing RIs: ... StatusCode: 403 ..."
           should not contain "Could not check for existing RIs"
    Messages: the refusal path must not reach a cloud API call
--- FAIL: TestProcessService_InstanceLimitRefusesRealPurchase

The other two assertions (NotEmpty(recs), Empty(results)) stay green under that mutation, so NotContains is the only thing pinning the hoist. It is load-bearing.

Stream check (the way this class of assertion usually turns out vacuous): the warning is emitted by AppLogger.Printf at cmd/multi_service_helpers.go:590, and captureAppOutput rebinds both os.Stdout and AppLogger to the same pipe. Streams match; no vacuity from a capture mismatch.

Two caveats worth recording:

• It is environment-conditional, not hermetic. It bites because the describe call fails for lack of credentials. In an environment with working RDS credentials the describe would succeed, emit no warning, and the assertion would pass even pre-hoist. Valid in credential-less CI; not a guarantee.
• The refusal message itself uses stdlib log.Printf (multi_service_helpers.go:413, same at :696) and goes to stderr, which captureAppOutput does not rebind. Nothing asserts on it today, but a future assert.Contains(out, "Refusing to purchase") written through captureAppOutput would be silently vacuous. These are the only two stdlib-log calls in a file that otherwise uses AppLogger throughout. Routing them through AppLogger is a one-line fix that makes the refusal text assertable.

5. The earlier findings (execution-verified)

Prior F1 (min-count) is addressed above. Prior F2 (TestProcessService_WithInstanceLimit vacuous) is genuinely repaired, not repaired in name. Under a restored pre-fix per-region cap:

• the repaired assertion fails, expected: 20 / actual: 15, with 💳 Purchasing 10 instances and 💳 Purchasing 5 instances visible in the same output, matching the claimed 10 + 5 truncation exactly;
• the old assertion form (assert.Equal(t, 20, CalculateTotalInstances(recs))) passes under that same mutation, green while the run is demonstrably over-capped.

That pair is the proof: the original was vacuous, and switching to the results total is a real repair.

---

Non-blocking findings

N1 (latent) reportInstanceLimit misclassifies a zero-count recommendation, and the tally can go negative

Two latent issues share one root cause: reportInstanceLimit's case kept == rec.Count: continue shortcut is wrong when rec.Count == 0 and the rec is past len(after). Zero equals zero, so the rec is skipped and never tallied in dropped, while it is counted in belowMinCount. Reproduced:

before = [A:4, Z:0, C:100], --max-instances=5 --min-count=3
result = [A:4]
drops  = "Dropped 1 recs: --max-instances=-1, --min-count-after-cap=2"

drops.Add(DropMaxInstances, dropped-belowMinCount) is called with -1. A negative count is printed for the reason and the run-wide total is understated. Negative-count recs do not trigger this (they take default and are counted); only exact zero does.

The same zero-count rec also survives dropTruncatedBelowMinCount's tail-only loop in a non-tail position, leaving a below-floor survivor:

before = [A:3, Z:0, C:100], cap 10  ->  ApplyInstanceLimit = [3, 0, 7]
dropTruncatedBelowMinCount(minCount=2) -> kept = [3, 0, 7]   <- Z:0 is below the floor and kept

Both are unreachable end-to-end today, and I verified the barrier rather than assuming it: pkg/scorer/scorer.go:88 filters every Count <= 0 whenever MinCount > 0, and scoreLimitAndDisplay is the sole caller of applyGlobalInstanceLimit, feeding the same cfg.MinCount to the scorer (:217) and to the floor drop (:255) with no mutation in between. When MinCount == 0 non-positive counts do reach passed, but then dropTruncatedBelowMinCount early-returns. So the guard is real but lives in a different package from the code that depends on it.

Related robustness gap, worth fixing on its own merits: DropSummary.Add has no n < 0 guard (pkg/common/drop_summary.go:41 checks only d == nil || n == 0). Verified that a negative silently subtracts from other stages' tallies under the same key, and on a fresh summary yields Total() == -1 with IsEmpty() == false. Cheapest proportionate fix, in preference order: guard n < 0 inside Add (protects every caller, not just this one), or clamp at the call site, or key the dropped classification off i >= len(after) instead of kept == rec.Count. All one-liners.

N2 (pre-existing, out of scope) The CSV path is the reference the PR cites, and it has the defect the PR just fixed

The PR body cites the CSV path as evidence that global semantics were always intended. That is fair for global versus per-region, but the CSV path does not meet the standard this PR sets:

• runToolFromCSV (cmd/multi_service.go:440) calls filterAndAdjustRecommendations, which applies ApplyInstanceLimit at :610 before any scoring. By the PR's own correctness argument, capping in load order rather than savings order spends the budget on the first-loaded recommendations regardless of quality. The selection property the PR is careful to get right on the main path is wrong here.
• More than that: there is no scorer and no --min-count gate anywhere on the CSV path. scoreLimitAndDisplay is called only at :140 (the main pipeline), and cfg.MinCount is referenced only by that path's scorer config at :217 and by the new drop code. So --min-count is not enforced at all in CSV mode, and a cap-truncated recommendation below the floor is purchased short there — exactly the defect 326112eca fixes on the main path.

Pre-existing and correctly left alone in this PR, but it deserves its own issue, and the "asymmetry with the CSV path" argument in the body should be narrowed to the global-versus-per-region point it actually supports.

N3 (pre-existing) Three vacuous money-path assertions in cmd/

Applying the assertion-arity lesson from #1735: this PR introduces no MockConfigStore assertions, and its own refusal test correctly uses assert.Empty(t, results, ...) rather than a mock assertion. But cmd/multi_service_test.go:1349, :1411 and :1486 all use the name-only form

mockClient.AssertNotCalled(t, "PurchaseCommitment")

on MockServiceClient, whose PurchaseCommitment(ctx, rec, opts) takes three arguments. Under stock testify an empty expectation is diffed against the real arguments and every one counts as a difference, so these cannot fail. They guard "a dry run must not purchase", which is a money-path guard adjacent to this PR's && !isDryRun condition. MockServiceClient is not MockConfigStore, so PR #1735's shadowing does not cover it; these belong to the follow-up in #1740.

---

Summary

The delta is sound. The --min-count floor is correct at the boundary, agrees with the scorer's gate, does not redistribute freed budget, and preserves the prefix invariant the reporter depends on. Drop counting is exactly once, including the combined case. The hoist is unconditional, skips nothing that mattered, and removes a live AWS call from the test suite. Both assertions the change rests on were verified to fail when the behaviour is reverted, and the previously-vacuous test is genuinely repaired rather than reworded. All gates pass at CI-pinned versions and CI is fully green on this head including Security Scanning.

Nothing blocking. I would take the DropSummary.Add negative guard (N1) since it is one line and protects every caller, and file N2 as an issue. N3 is already covered by #1740.

Reviewer notes: the fuzz over ApplyInstanceLimit, the boundary and redistribution cases, the drop-count cases including the negative-tally reproduction, both mutation checks (guard hoist and prior-F2 repair), and all five gates are execution-verified. The hoist's control-flow analysis, the N2 call-graph tracing and the N3 arity reasoning are reading-derived, though N2's central claim (no scorer and no min-count gate on the CSV path) was confirmed by exhaustive grep of every MinCount and scorer.Score reference in cmd/, not inferred.

[cristim]

Merging with two gate deviations, both recorded rather than waived.

1. The one open thread is answered, not outstanding. cmd/multi_service.go:223 (cost/savings not scaled when the cap reduces Count) is correct and was declined here with a written justification, tracked as #1611. It will never auto-resolve, and hand-resolving a thread to clear a gate is prohibited in this repo — so it stays open and the PR merges with the reasoning visible.

That finding is real and is now corroborated three ways: the implementer flagged it, CodeRabbit found it independently, and #1611 has since been widened with the sibling defect in the opposite direction (ApplyCountOverride raises Count and leaves costs low, so --override-count 10 on a Count = 1 rec understates the upfront by 10x). One defect at one place — Count mutated without rescaling derived fields — reachable from two helpers. Fixing it here would have meant a cost-model change inside a cap-placement PR, and touching the same lines twice.

2. CodeRabbit's verdict is against 3af7f4cda, three commits back. Its quota is shared and throttled. What stands in its place is a full independent adversarial review plus a delta review of every subsequent commit, both posted above.

The delta review verified, by execution rather than reading:

• Security Scanning has completed on 93fe75e5f with success — checked against the raw check-runs API: 19 named checks plus CI Success, all completed, no null or queued conclusions. The npm advisories are genuinely clear on this head.
• The tail-only floor drop preserves the prefix invariant reportInstanceLimit depends on, confirmed by a 200k-case fuzz over ApplyInstanceLimit. Freed budget is not redistributed. The boundary is strict < in both gates, so there is no off-by-one between the floor and the scorer.
• Drop counting is exactly-once on a combined cap-drop plus floor-drop run.
• The guard hoist is genuinely unconditional, and nothing required was skipped by moving it earlier.
• The new NotContains assertion bites — moving the guard back fails it, and it is the only assertion pinning the hoist.
• The previously vacuous TestProcessService_WithInstanceLimit is genuinely repaired: under a restored pre-fix cap the new form fails expected 20 / actual 15 while the old form passes green under the identical mutation.

Follow-ups filed from the review, none blocking:

• fix(cli): --input-csv enforces neither --min-count nor savings-ordered capping, silently #1741 (p1) — the --input-csv path enforces neither --min-count nor savings-ordered capping, silently. This PR's body cited the CSV path as the reference for correct global semantics; that was wrong, and the correction is a live defect rather than a documentation nit.
• fix(cli): DropSummary.Add silently accepts a negative count and understates other stages #1742 (p3) — DropSummary.Add accepts a negative count and silently subtracts from other stages. Unreachable today; the barrier was verified, not assumed.
• fix(test): name-only AssertNotCalled is vacuous on every mock type, not just MockConfigStore #1740 — three AssertNotCalled(t, "PurchaseCommitment") sites guarding "a dry run must not purchase" are vacuous, on a mock outside fix(test): make 78 unfailable MockConfigStore assertions able to fail #1735's fix.