refactor(arch): provider-module fragmentation forces copy-paste (fan-out, env reader, savings formula); + pkg/errors orphan, logging split

[cristim]

Problem (umbrella; spawn sub-issues as scoped). Report 12 cross-cutting items:

• A1/S2 Per-provider Go modules (providers/{aws,azure,gcp}/go.mod) share only pkg/, forcing copy-paste of provider-agnostic helpers. Decide: collapse provider modules into root, or add a shared module they all depend on. (Note: PR chore(dev): add go.work for gopls multi-module workspace (closes #516) #858 added go.work for gopls — does not resolve the import barrier.)
• D1/D2 Recommendations fan-out scaffold + serviceResult/mergeServiceResults triplicated → one generic CollectConcurrently[T].
• D3 shouldIncludeService duplicated — can move to pkg/common today (no barrier).
• D4 Positive-int env reader triplicated (concurrency.MaxParallelismFromEnv, execution.ConcurrencyFromEnv, gcpRegionConcurrency) → common.PositiveIntFromEnv.
• D5 Savings-% formula copy-pasted across ~10 clients with inconsistent divide-by-zero guards (NaN flows into scorer) → common.SavingsPct(onDemandTotal, committedTotal) returning 0 when onDemandTotal <= 0. (Pairs with I-09.)
• S1 pkg/errors has zero importers — adopt (recommended; helps 4xx/5xx consistency) or remove. No-delete: explicit decision.
• A2 Logging split stdlib log vs pkg/logging (~50/50, sometimes same file) — standardize library/provider on pkg/logging.
• A3 internal/api god files (handler_purchases 1787, router 825, handler_accounts 1490, handler_ri_exchange 844) + direct providers/* reach-through — route via a use-case layer, split by resource.
• A4 Frontend god files (recommendations.ts 4312, settings.ts 3222) — decompose into frontend/src/modules/.
• A5 FE/BE validation mirrored without cross-reference comments.

Evidence. Report 12 D1-D5, A1-A5, S1-S3, with exact file lists.

Suggested fix. Land the no-barrier ones first (D3, D4, D5, A2, S1 decision); make the A1 module decision and document the rationale; treat god-file splits (A3, A4) as incremental.

References. Source: report 12. The savings-formula guard (D5) is a prerequisite for I-09's clean fix. PR #858 (go.work).

---

Filed from automated adversarial code review (see docs/code-review/). Source finding(s): 12 A1-A5, D1-D5, S1.

---

Staged refactor plan (approved direction: full refactor incl. module collapse)

Full execution-ready plan: docs/code-review/14-refactor-plan.md (file:line call sites, tests, CI/Docker/Makefile implications, verification per stage).

HARD precondition — sequencing

Do not start until the in-flight PRs against feat/multicloud-web-frontend that touch the refactor's target files have merged. They rewrite the exact files this refactor rewrites (all provider clients, internal/api, internal/server/app.go, the frontend god files). Blocking set captured 2026-06-07: #1036 #1037 #1038 #1039 #1040 #1041 #1042 #1043 #1044 #1045 #1046 #1047 #1049 (+ PR #858 go.work, obsoleted by Stage 2). Land the refactor on its own branch off the shared branch and rebase before each stage (base keeps moving).

#1045/#1047 removed the fabricated-price fallbacks, not the savings-% formula — all 11 D5 sites still exist; SavingsPct DRYs what remains.

Stages (ordered by risk/value)

Stage	Scope	Effort	Risk
1 — no-module cleanups	D5 common.SavingsPct (fixes NaN-into-scorer), D4 common.PositiveIntFromEnv, D3 move shouldIncludeService, A2 logging standardize, S1 adopt pkg/errors	M (~1 wk, 5 small PRs)	Low-Med
2 — module collapse (A1/S2)	merge providers/{aws,azure,gcp} (+ optionally pkg/) into root module; drop 4 replace directives; no import-path rewrites (paths preserved); fix CI govulncheck loop, Dockerfile per-module COPYs, Makefile, go.work disposition	M-L (days)	Med-High (build blast radius)
3 — fan-out dedup (D1/D2/S3)	generic CollectConcurrently[T] in pkg/concurrency; rewrite the 3 provider fan-outs; ctx.Err-after-Wait + semaphore-at-call + deterministic-merge invariant tests	M (days)	Med (concurrency)
4 — god-file splits (A3/A4) + A5	A3 use-case layer + per-resource internal/api split (cut providers/* reach-through first); A4 decompose recommendations.ts/settings.ts into frontend/src/modules/; A5 FE/BE cross-ref comments	L (weeks, ~9 incremental PRs)	Med

Dependency graph: GATE → Stage 1 → Stage 2 → Stage 3 (3 hard-depends on the unified module). Stage 4 runs in parallel with 2/3 once Stage 1 lands (it touches internal/api/frontend, not the provider module graph). A4 also waits on #1046/#1049.

Proposed sub-issues (for owner approval — NOT yet created)

• refactor(common): centralise savings-% into common.SavingsPct, guard zero denominator (D5) — p1/severity:high/this-sprint/many/effort:m/type:bug
• refactor(common): common.PositiveIntFromEnv, delegate 3 env readers (D4) — p2/low/this-quarter/internal/effort:s/chore
• refactor(common): move shouldIncludeService to pkg/common (D3) — p2/low/this-quarter/internal/effort:s/chore
• refactor(logging): standardise provider/library layers on pkg/logging (A2) — p2/low/this-quarter/internal/effort:m/chore
• refactor(errors): adopt pkg/errors for api 4xx/5xx mapping (S1 decision: adopt) — p2/medium/this-quarter/many/effort:m/chore
• refactor(build): collapse provider Go modules into root module, drop replace directives (A1/S2) — p1/medium/this-quarter/internal/effort:l/chore
• refactor(concurrency): generic CollectConcurrently[T], dedup 3 provider fan-outs (D1/D2/S3) — p2/medium/this-quarter/internal/effort:m/chore
• refactor(api): route provider access via use-case layer + split god handlers (A3) — p2/medium/eventually/internal/effort:l/chore
• refactor(frontend): decompose recommendations.ts into modules/ (A4) — p2/low/eventually/internal/effort:l/chore
• refactor(frontend): decompose settings.ts into modules/ (A4) — p3/low/eventually/internal/effort:m/chore
• chore(validation): add FE/BE cross-reference comments for mirrored validation (A5) — p3/low/eventually/internal/effort:s/chore

[cristim]

Closing this as split across the monorepo split, since none of the umbrella's 12 items (A1-A5, D1-D5, S1) land in cloud-commitments-cli:

• A1/S2 (provider module collapse), D1/D2 (fan-out dedup), D3 (shouldIncludeService), D4 (positive-int env reader, this repo's share), D5 (savings-% formula), S1 (pkg/errors orphan), A2 (logging split, this repo's share) are all in providers/, pkg/ -- now cloud-commitments-go. Filed as refactor(arch): provider-module fragmentation, fan-out/env-reader/savings-formula dup, pkg/errors orphan (A1/D1-D5/S1/A2) [go part] cloud-commitments-go#114.
• D4 (this repo's other share), A2 (this repo's other share), A3 (internal/api god files), A4 (frontend god files), A5 (FE/BE validation drift) are all in internal/, frontend/ -- now cloud-commitments-platform. Filed as refactor(arch): internal/api god files, frontend god files, FE/BE validation drift, logging split (D4/A2-A5) [platform part] cloud-commitments-platform#341.

Verified via a fresh clone of all four split repos: cloud-commitments-cli (now cmd/*.go only) has no genuine instance of any of the 12 items -- the superficial serviceResults local variable in cmd/multi_service.go is an unrelated slice, not the serviceResult/mergeServiceResults duplication pattern the original names (that's specifically Azure's mergeServiceResults / GCP's mergeRegionResults, both in providers/). cloud-commitments-mcp is likewise unaffected and has no sibling filed.

Closing as not planned here since the real work all lands in the two sibling repos above.