Skip to content

feat(common): preserve proven commitment currency metadata - #410

Merged
cristim merged 1 commit into
mainfrom
feat/commitment-currency-contract
Oct 15, 2026
Merged

cristim merged 1 commit into
mainfrom
feat/commitment-currency-contract

Conversation

@cristim

@cristim cristim commented Oct 15, 2026 •

Copy link
Copy Markdown
Member

Recommendation currently drops a proven commitment currency during JSON round trips. Add optional CommitmentCurrency metadata for CommitmentCost and RecurringMonthlyCost, with empty meaning unknown and presence requiring source-proven amounts and currency for every required payment leg.

The field deliberately makes no claim about savings or on-demand baseline currency. Existing value-copy scaling preserves it without new helper logic. No provider population, platform transport, default currency, FX, historical backfill or dependency movement is included. Publish the standalone shared module before provider modules reference the new API.

Validation: known EUR, explicit unknown and legacy absence through JSON decode/encode and actual ScaleRecommendationCosts at ratios 1 and 2, with money scaling and typed round trips. The test failed by runtime assertion before the field existed. Separate scratch mutations removing omitempty and clearing metadata during scaling also fail by the intended assertions and were restored.

Two retained independent source passes and the same-byte staged review approved tree 43719e7adf4dc3d567433fd7333dd0e46f7128b2. Fresh committed cold review completed two passes with no findings at 9eea7a039874d3168f8b36ea4790e86ccdb7e8a7. Its independent public consumer additionally covers ratios 0 / 0.5 / 2, nil and known-zero recurring cost, and pointer isolation; parent plus new regression and separate semantic mutations fail as expected.

Standalone pkg full short race suite, build, official golangci-lint 2.10.1 and normal local commit hooks passed with Go 1.26.9, GOWORK=off and ambient cloud credentials disabled. The cold reviewer independently repeated the full suite and build from an owned archive. Added comments: 2 / 38 nonblank lines (5.3%).

Closes #409
Refs #124
Prerequisite #408 is merged and published. Provider population, platform transport and currency-safe enforcement remain subsequent work tracked by the broader issue; this contract does not make existing currency-blind guardrails safe.

Summary by CodeRabbit

  • New Features

    • Recommendations can now show the currency associated with commitment and recurring monthly costs. The currency is left unspecified when it is unknown or when amounts and currency are not confirmed for every required payment.
  • Bug Fixes

    • Known commitment currencies are preserved when recommendation data is scaled or converted between formats.

Keep commitment currency separate from savings and baseline amounts.
Preserve unknown legacy values and currency metadata through scaling.
@cristim cristim added priority/p2 Backlog-worthy severity/medium Moderate harm urgency/this-sprint Within the current sprint impact/many Affects most users effort/s Hours type/feat New capability triaged Item has been triaged labels Oct 15, 2026
@coderabbitai

coderabbitai Bot commented Oct 15, 2026 •

Copy link
Copy Markdown
Contributor

Review in Change Stack →

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: 28f59103-4189-4778-8ea0-73dffb18fd97
📥 Commits

Reviewing files that changed from the base of the PR and between a3fb251 and 9eea7a0.

📒 Files selected for processing: 2
  • pkg/common/types.go
  • pkg/common/types_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

Recommendation now has optional commitment currency metadata. Tests check its JSON handling and verify that commitment costs scale while the currency is retained.

Changes: Recommendation commitment currency

Layer / File(s) Summary
Commitment currency contract and tests
pkg/common/types.go, pkg/common/types_test.go
Recommendation adds CommitmentCurrency for commitment and recurring monthly costs. Tests cover known, empty, and absent currency values, JSON round-tripping, and cost scaling.

Priority: ➖ Normal

Estimated code review effort: 2 (Simple) | ~10 minutes

Change: Feature · Severity of issue fixed: Medium

Merge Risk: ⚪ Minimal · up to 9eea7

No merge-blocking issue is identified in this shared-contract change; it is ready for normal merge checks.

Pre-merge checks: Passed 4 · Failed 1

Failed Failed checks: 1 warning

Check name Status Explanation Resolution
Docstring Coverage Warning Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 3 functions across 2 files. Write docstrings for the functions missing them to satisfy the coverage threshold.
Passed Passed checks: 4 passed
Check name Status Explanation
Description Check Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check Passed The title clearly and concisely describes the main change: preserving proven commitment currency metadata in the shared common types.
Linked Issues check Passed The active direct issue is #409. pkg/common/types.go adds Recommendation.CommitmentCurrency with json:"commitment_currency,omitempty" and documents that empty means unknown and that populated me…
Out of Scope Changes check Passed The reviewed change is limited to the shared Recommendation type and its tests. The new field, semantic comment, and regression test directly implement #409. No unrelated provider population, platfo…
  • 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

cristim commented Oct 15, 2026

Copy link
Copy Markdown
Member Author

Independent committed review covers exact head 9eea7a039874d3168f8b36ea4790e86ccdb7e8a7, tree 43719e7adf4dc3d567433fd7333dd0e46f7128b2, base a3fb25103204341399c624a0ce7b242fc89d3b15.

Two retained implementation/staged review passes and two fresh committed cold passes found no actionable findings. The fresh reviewer inspected the exact archived source and ran an independent native Go consumer importing the actual public common package. It checked legacy absent and explicit unknown JSON omission, known EUR, scaling ratios 0 / 0.5 / 2, nil versus known-zero recurring cost, and pointer isolation. The original parent combined with the new regression compiled and failed on the intended known-EUR runtime assertion. Separate scratch mutations clearing currency in the actual scaler and removing JSON omitempty also failed the intended assertions and were restored.

The fresh archive passed standalone GOWORK=off GOTOOLCHAIN=go1.26.9 go test -count=1 -race -short ./... and go build ./...; the author also passed official golangci-lint 2.10.1 and all normal local commit hooks at this exact tree. Cloud credentials and metadata access are disabled for these fixture/library checks.

This verifies the shared public contract only. Provider source-leg completeness, provider population, platform persistence, displays, currency-safe guardrails and live economics are subsequent work. No default currency, FX, historical backfill, provider or platform code changes are included. The CSV tag follows existing shared metadata conventions; no CSV serialization framework or CLI writer was added.

Follow-up issues filed: none for this 39-line contract leaf, confirmed against the exit checklist. Remaining currency-loss class work is explicitly tracked by #124; provider and platform transport follow after the merged standalone shared module is published. CI must pass on this exact SHA before a normal merge.

@cristim

cristim commented Oct 15, 2026

Copy link
Copy Markdown
Member Author

CI diagnosis: the GCP lint job failed before source analysis during golangci-lint 2.10.1 config verify. Fetching https://golangci-lint.run/jsonschema/golangci.v2.10.jsonschema.json timed out while awaiting response headers. The shared pkg and other provider lint jobs passed. Evidence: https://github.com/LeanerCloud/cloud-commitments-go/actions/runs/38707989074/job/116332062840 . No code, linter configuration or bypass change is indicated. After this workflow reaches a terminal state, one targeted retry of the failed job and its normal dependents will be attempted; CI remains required.

@cristim

cristim commented Oct 15, 2026

Copy link
Copy Markdown
Member Author

Workflow attempt 1 is terminal: Unit Tests, Integration Tests, security, shared pkg lint and other provider lint all passed. Only GCP schema-fetch lint and its dependent CI Success aggregate failed. Verified the actual job database ID and unchanged head 9eea7a0, then requested one targeted rerun of job 116332062840 and its normal dependencies in run 38707989074. A separate 30-second background watcher tracks the retry. No source, linter configuration, rebase or protection-bypass changes were made.

@cristim

cristim commented Oct 15, 2026

Copy link
Copy Markdown
Member Author

Final gate at exact9eea7a039874d3168f8b36ea4790e86ccdb7e8a7/basea3fb25103204341399c624a0ce7b242fc89d3b15: all CI is SUCCESS after the single targeted schema-timeout retry, pre-commit SUCCESS, live CLEAN/MERGEABLE, zero review threads. Automatic CodeRabbit completed on this exact head and explicitly reported no actionable comments. Its generic docstring-coverage warning is dismissed: this change adds no production function, the new field has its semantic comment, and the test name and assertions explain the test; adding restatement comments conflicts with the rationale-only comment convention. No source change or review waiver is needed. Independent two-pass committed review and actual public-consumer evidence are recorded above. Proceeding with normal protected matched-head squash merge, without bypasses.

@cristim
cristim merged commit 498b74f into main Oct 15, 2026
24 of 26 checks passed
@cristim

cristim commented Oct 15, 2026

Copy link
Copy Markdown
Member Author

PR #410 merged normally at main 498b74f31f01aca9b2c795d1dce9d85c790eabea, after all required CI and the automatic CodeRabbit review completed green on reviewed head 9eea7a039874d3168f8b36ea4790e86ccdb7e8a7. The single targeted CI retry resolved the external schema-fetch timeout without a source/config change. The merged tree exactly matches reviewed tree 43719e7adf4dc3d567433fd7333dd0e46f7128b2.

Published module: github.com/LeanerCloud/cloud-commitments-go/pkg@v0.0.0-20261015085730-498b74f31f01, whose download origin matches the merge SHA. The actual downloaded shared module builds standalone. A separate native consumer module requires this exact published version, with no workspace or replace directive, and passes legacy/unknown JSON omission, known EUR, ratios 0 / 0.5 / 2, nil and known-zero recurring charges, and pointer isolation through the public scaler.

This completes the shared commitment-currency contract for #409. Provider modules can now pin the published API before populating it. No provider source completeness, platform transport, currency-safe guardrail, live cloud economics, purchase or deployment result is claimed. Broader work remains in #124. Follow-up issues filed: none for this contract leaf, confirmed against the exit checklist; remaining scope is explicitly recorded on #124.

Post-merge main build/test run 38709401903 and pre-commit run 38709401920 have independent background watchers. Owner-safe merged branch/worktree cleanup follows the verified clean state.

@cristim
cristim deleted the feat/commitment-currency-contract branch October 15, 2026 09:00
@cristim

cristim commented Oct 15, 2026

Copy link
Copy Markdown
Member Author

Post-merge main CI is terminal green at 498b74f: build/test run 38709401903 and pre-commit run 38709401920 completed SUCCESS. All PR, retry, CR, merge and main-run watchers are reaped. The exact published standalone module and separate pinned public-consumer verification are recorded above. Merged worktree and local/remote feature branch cleanup is complete.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

effort/s Hours impact/many Affects most users priority/p2 Backlog-worthy severity/medium Moderate harm triaged Item has been triaged type/feat New capability urgency/this-sprint Within the current sprint

Projects

None yet

Development

Successfully merging this pull request may close these issues.

feat(common): preserve proven commitment currency on Recommendation

1 participant