You signed in with another tab or window. Reload to refresh your session.You signed out in another tab or window. Reload to refresh your session.You switched accounts on another tab or window. Reload to refresh your session.Dismiss alert
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.)
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.)
S1pkg/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.
A3internal/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.
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
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.
Activity
fix(gcp): FOLD-1047 correctness findings (10-M2,M3,M7,L1,L2,L3,L4,N3)
fix(gcp): wire Count/pricing into GCP converters; refuse fabricated p…
fix(recs): stop double-amortizing upfront in effectiveSavingsPct (AWS…
cristim commentedon Sep 27, 2026
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:shouldIncludeService), D4 (positive-int env reader, this repo's share), D5 (savings-% formula), S1 (pkg/errorsorphan), A2 (logging split, this repo's share) are all inproviders/,pkg/-- nowcloud-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.internal/apigod files), A4 (frontend god files), A5 (FE/BE validation drift) are all ininternal/,frontend/-- nowcloud-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(nowcmd/*.goonly) has no genuine instance of any of the 12 items -- the superficialserviceResultslocal variable incmd/multi_service.gois an unrelated slice, not theserviceResult/mergeServiceResultsduplication pattern the original names (that's specifically Azure'smergeServiceResults/ GCP'smergeRegionResults, both inproviders/).cloud-commitments-mcpis likewise unaffected and has no sibling filed.Closing as not planned here since the real work all lands in the two sibling repos above.