fix(gcp): take the compute CUD term from the recommendation plan (experimental, opt-in)

[cristim]

Supersedes #307 (same change, plus an experimental gate). Closes #291.

The GCP compute CUD term can be read from the recommendation's root Commitment plan. The payload shape is unverified against a live Recommender and no GCP project is available to check it, so the path is EXPERIMENTAL and off by default.

• New RecommendationParams.ExperimentalGCPPlanTerm (default false). Unset keeps the previous behaviour: params.Term, default "1yr", plan ignored, nothing skipped.
• Set: plan drives the term; conflicts, malformed or missing plans skip the recommendation (as in fix(gcp): take the compute CUD term from the recommendation plan #307). Each such recommendation logs an EXPERIMENTAL line.
• Doc comments on the flag, resolveRecTerm, rootCommitmentTerm and errBadPlan; known issue updated.
• Tests: fix(gcp): take the compute CUD term from the recommendation plan #307's plan tests run with the flag on; TestPlanTermOffByDefault covers the default.

Fixture-based evidence only; no live cloud call. Follow-ups: platform pin bump and a setting to enable the flag; platform UI label for experimental terms.

🤖 Generated with Claude Code

Summary by CodeRabbit

• New Features
  • An experimental option can use a GCP commitment’s plan to determine a recommendation’s purchase term. Recognized 12- and 36-month plans map to one- and three-year terms.
• Bug Fixes
  • With the experimental option enabled, recommendations with missing, malformed, or conflicting term information are skipped rather than assigned an incorrect term. Other valid recommendations can still be returned.
• Known Issues
  • When a recommendation has no root commitment plan, the experimental option may fall back to the supplied term; if none is supplied, it may use the one-year default.

[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: 29e292d8-9d9e-4725-ad73-c9da865cf7a0

📥 Commits

Reviewing files that changed from the base of the PR and between 77bc8a9 and 3228956.

⛔ Files ignored due to path filters: 1

• providers/gcp/go.sum is excluded by !**/*.sum

📒 Files selected for processing: 7

• known_issues/01_gcp_compute_nonroot_term.md
• providers/gcp/go.mod
• providers/gcp/recommendations_sdk_test.go
• providers/gcp/services/computeengine/client.go
• providers/gcp/services/computeengine/commitment_type.go
• providers/gcp/services/computeengine/dedupe_test.go
• providers/gcp/services/computeengine/plan_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

The GCP Compute Engine converter can derive a recommendation’s purchase term from its root Commitment plan when the experimental flag is enabled. It checks the plan against any explicit term and skips recommendations with invalid or conflicting plans.

Changes: GCP Compute recommendation term resolution

Layer / File(s)	Summary
Resolve terms from recommendation plans providers/gcp/services/computeengine/commitment_type.go, providers/gcp/services/computeengine/plan_test.go	The resolver maps supported root plans to canonical terms and checks them against explicit terms when the experimental flag is enabled. Tests cover plan mapping, term canonicalization, invalid or conflicting plans, and behavior with the flag disabled.
Apply resolved terms during conversion providers/gcp/services/computeengine/client.go, providers/gcp/services/computeengine/dedupe_test.go, providers/gcp/recommendations_sdk_test.go, providers/gcp/go.mod, known_issues/01_gcp_compute_nonroot_term.md	The converter uses the resolver and skips recommendations with invalid plans. Fixtures include root plan values. The GCP module dependency version changes. The known-issues entry documents the non-root recommendation fallback and its experimental status.

Priority: ➖ Normal

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

Change: Bug fix · Severity of issue fixed: High

Sequence Diagram(s)

sequenceDiagram
  participant GCPRecommender
  participant ComputeEngineClient
  participant resolveRecTerm
  GCPRecommender->>ComputeEngineClient: recommendation with root Commitment plan
  ComputeEngineClient->>resolveRecTerm: root plan and params.Term
  resolveRecTerm-->>ComputeEngineClient: canonical term or errBadPlan
  ComputeEngineClient->>ComputeEngineClient: skip recommendation when conversion returns errBadPlan

Loading

Merge Risk: ⚪ Minimal · up to 32289

No actionable merge-blocking issue is established. The plan-based behavior remains experimental and disabled by default because its live payload shape has not been verified.

Pre-merge checks: 3 · 2

Failed checks: 2 warnings

Check name	Status	Explanation	Resolution
Linked Issues check		Issue #291 requires the recommendation root Commitment plan to determine the term and requires conflicting explicit terms to be rejected. The opt-in path implements these rules and tests cover them. H…	Apply root-plan term resolution in the default path, or revise the linked issue requirements to explicitly accept opt-in behavior. Preserve rejection and skip handling for missing, malformed, unsupported, or conflicting plans, with automate…
Docstring Coverage		Docstring coverage is 75.00% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 16 functions across 5 files. (2 skipped: …	Write docstrings for the functions missing them to satisfy the coverage threshold.

Passed checks: 3 passed

Check name	Status	Explanation
Description Check		Check skipped - CodeRabbit’s high-level summary is enabled.
Title check		The title clearly and concisely describes the main change: using the GCP Compute CUD term from the recommendation plan behind an experimental opt-in.
Out of Scope Changes check		The changed implementation, tests, fixtures, documentation, and providers/gcp dependency pin support the plan-term behavior, its experimental gate, or its build validation. No unrelated change is de…

Full details: Linked Issues check

Explanation

Issue #291 requires the recommendation root Commitment plan to determine the term and requires conflicting explicit terms to be rejected. The opt-in path implements these rules and tests cover them. However, ExperimentalGCPPlanTerm defaults to false, so the normal path still uses params.Term or defaults to "1yr" and ignores the plan. The issue remains unfixed for the default behavior.

Resolution

Apply root-plan term resolution in the default path, or revise the linked issue requirements to explicitly accept opt-in behavior. Preserve rejection and skip handling for missing, malformed, unsupported, or conflicting plans, with automated tests for the default path.

Full details: Docstring Coverage

Explanation

Docstring coverage is 75.00% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 16 functions across 5 files. (2 skipped: 2 unsupported.)

• 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]

Gate review at head 01a465d00b0831170dcc19c2332c045f257a6562 (after gh pr update-branch from 422a545; git diff 422a545 01a465d -- <PR files> is empty, so the PR's own diff is unchanged).

Verdict: logic PASS, merge BLOCKED by CI lint.

Blocker (required check Lint Code (providers/gcp) fails, misspell):

• providers/gcp/services/computeengine/commitment_type.go:225: behaviour -> behavior
• providers/gcp/services/computeengine/plan_test.go:123: behaviour -> behavior

Also: main has moved again (a4fc226), so the PR will need another update from main after the fix.

What I checked (fixture/mock evidence only, no live cloud call):

1. Flag off is identical to base: resolveRecTerm (commitment_type.go:226-231) returns params.Term or "1yr" unchanged before touching the payload, matching base client.go:1100-1104 (term := params.Term; if term == "" { term = "1yr" }); termPlan validation follows as before. No plan read, no selectRootCommitment call, no errBadPlan reachable. The EXPERIMENTAL log line is flag-gated (client.go:1108).
2. Flag on: plan mapping uses computepb.Commitment_TWELVE_MONTH/THIRTY_SIX_MONTH string names (Commitment.plan, json plan, documented as TWELVE_MONTH (1 year) and THIRTY_SIX_MONTH (3 years) in computepb compute.pb.go). UNDEFINED_PLAN, unknown, empty, non-string and null plans, conflicts with an explicit term, and a root without plan or term all wrap errBadPlan and are skipped per-recommendation (client.go:485). The root plan key in the Recommender Commitment op is unverified live, which is why it is gated.
3. Probes (reverted after): removing the gate (if false && !params.ExperimentalGCPPlanTerm) fails TestPlanTermOffByDefault; swapping the 1yr mapping fails 4 plan tests; dropping the conflict check fails Skips/ConflictKeepsGood; dropping errBadPlan from the skip fails Skips/ConflictKeepsGood.
4. git grep ExperimentalGCPPlanTerm: set only in plan_test.go; no production caller flips it. Doc comments (types.go:532, commitment_type.go:162,167,223) and the PR body say experimental, off by default.

Local: GOTOOLCHAIN=go1.26.9, fresh clone at 01a465d, go test -race -short ./... in pkg, providers/aws, providers/azure, providers/gcp: all exit 0.

[cristim]

Gate re-run at head 4c0b5fd6e9cffc803e4e454fb75a1124b6ed0b4a.

Delta since 01a465d: 41750c1's tree equals git merge-tree --write-tree a4fc226 01a465d (a clean merge of main #386, no hand edits), and 4c0b5fd only changes comment spelling (commitment_type.go:225, plan_test.go:123). The PR's diff against main is unchanged (7 files, +304/-11). The misspell lint is now green.

Verdict: BLOCKED. Required check Unit Tests fails (and so does CI Success).

The "Verify standalone provider modules" step (.github/workflows/ci.yml:294, GOWORK: off) builds providers/gcp against its pinned pkg v0.0.0-20261012192523-3838efd56668 (providers/gcp/go.mod:11), and that version has no RecommendationParams.ExperimentalGCPPlanTerm:

• providers/gcp/services/computeengine/client.go:1108:12: params.ExperimentalGCPPlanTerm undefined
• providers/gcp/services/computeengine/commitment_type.go:227:13: same

Reproduced locally: GOWORK=off go build ./... in providers/gcp at this head fails the same way. The workspace build (go.work) passes, which is why go test per module exits 0.

Fix needed: make the gcp module build against a pkg version that contains the field. For example, land the pkg/common/types.go field first and bump providers/gcp/go.mod to that pkg pseudo-version here, or follow whatever cross-module convention the repo uses. The logic review from my previous comment still stands (flag-off identical to base; gate, mapping, conflict and errBadPlan probes all fail as expected).

[cristim]

Gate verdict at head 32289566fe6a3974b544ed0f73342f13381cf177: PASS, merging.

• Delta since 4c0b5fd: db5d3ae merges main 77bc8a9 (feat(common): add ExperimentalGCPPlanTerm to RecommendationParams #402). Its tree equals git merge-tree --write-tree of the two parents (clean auto-merge, no hand edits). 3228956 changes only providers/gcp/go.mod and go.sum, pinning pkg to v0.0.0-20261013221254-77bc8a993eb8. The pkg/common/types.go field now comes from main.
• Logic verdict from my earlier comments is unchanged: with the flag off, behavior matches base exactly; with it on, the gate, plan mapping, conflict check and errBadPlan skip all failed their probes as expected.
• Local (GOTOOLCHAIN=go1.26.9, git archive at head): in providers/gcp with GOWORK=off GOFLAGS=-mod=readonly, go mod verify, go build, go vet and go test -race -short ./... all exit 0. Workspace go test -race -short in pkg, providers/aws, providers/azure and providers/gcp also exit 0.
• CI at this SHA: all green, including Unit Tests with "standalone verification in providers/gcp". mergeStateStatus CLEAN, base main 77bc8a9.
• Labels mirror fix(gcp): compute cud purchase ignores the recommendation plan and uses params term #291. Body says Closes fix(gcp): compute cud purchase ignores the recommendation plan and uses params term #291, EXPERIMENTAL, off by default. Fixture evidence only; no live GCP call.