fix(billing): re-apply lost C.2 review nits (warn-on-unknown-priceId + 4 hardening)

[PierreBrisorgueil]

Context

PR #3743 (fix(billing): resolve plan via priceId map, not price.metadata.planId (#1250), merge commit 0693e497) was merged to devkit master with 5 unfixed Copilot review findings. A controller-side fix commit (11b00505) was prepared locally but never pushed to origin/feat/resolveplan-priceid-map — the push silently appeared to succeed (RTK wrapper reported "ok") while git ls-remote shows the SHA never reached origin. The 5 review threads were resolved on GitHub against the wrong commit, and the squash-merge captured only the agent's original content.

Net: 5 known bugs were live in devkit master. This PR re-applies all 5 fixes.

Closes #3757. Refs: #3743, plan 2026-05-30-trawl-devkit-perfect-alignment.md.

Findings re-applied

1. JSDoc accuracy (buildPriceIdToPlanMap): replaced stale comment with accurate description + @returns tag
2. Silent free-fallback warn log (resolvePlan): adds logger.warn when priceId is present but not in the map and metadata is empty — the operationally critical one, same silent-downgrade shape as the P0 from build(deps-dev): bump husky from 5.1.1 to 5.1.2 #1250
3. cancel_at typeof check: if (subscription.cancel_at) → if (typeof subscription.cancel_at === 'number') — prevents cancel_at = 0 (epoch) from being silently skipped
4. previousPlan validatePlan(): raw priceIdToPlan/metadata value validated before comparison — prevents stale/invalid metadata from triggering plan.changed + forceRotateForPlanChange with junk plan names
5. Test comment correction: corrects misleading "Suppress retryWithBackoff delays" claim — the mock makes the repo a no-op, it does NOT suppress setTimeout delays

Verification

• npm run lint → ESLint: No issues found
• npm run test:unit -- modules/billing/tests/billing.webhook → 1714/1714 pass
• npm audit --audit-level=high → pre-existing baseline (no new vulnerabilities)
• Push SHA verified via git ls-remote: 6ec833edb3dba2bb914f818f2db06041d502c055

Summary by CodeRabbit

• Bug Fixes

  • Improved subscription cancellation timing handling to properly validate timestamp values and support null values
  • Enhanced plan resolution logic with better fallback behavior and improved visibility when plans cannot be resolved
  • Strengthened plan-change detection accuracy through validation before comparison

• Tests

  • Updated test infrastructure documentation for clarity

[coderabbitai]

Warning

Review limit reached

@PierreBrisorgueil, we couldn't start this review because you've reached your PR review rate limit.

More reviews will be available in 56 minutes and 6 seconds. Learn how PR review limits work.

Your organization has run out of usage credits. Purchase more in the billing tab.

⌛ How to resolve this issue?

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.

We recommend that you space out your commits to avoid hitting the rate limit.

🚦 How do rate limits work?

CodeRabbit enforces hourly rate limits for each developer per organization.

Our paid plans include higher PR review limits than trial, open-source, and free plans. In all cases, reviews become available again over time. During sustained high-volume PR review activity, CodeRabbit may temporarily slow when the next review becomes available.

Please see our Fair Usage Limits Policy for further information.

ℹ️ Review info

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: ASSERTIVE

Plan: Pro

Run ID: f8da123a-c94c-4755-bc1d-b30e0172d2cd

📥 Commits

Reviewing files that changed from the base of the PR and between 6ec833e and b25da05.

📒 Files selected for processing (1)

• modules/billing/services/billing.webhook.service.js

Walkthrough

Updated billing webhook service to harden plan-resolution and subscription-update logic: plan fallback now validates legacy metadata and logs warnings on unknown priceIds, subscription updates enforce stricter cancel_at type checking and validated plan-change detection, and test setup comments clarify retry behavior.

Changes

Billing Webhook Plan Resolution and Subscription Update Tightening

Layer / File(s)	Summary
Plan Resolution & Fallback Hardening modules/billing/services/billing.webhook.service.js, modules/billing/tests/billing.webhook.priceid-map.unit.tests.js	JSDoc for priceId→plan reverse-map is expanded to describe module-load structure; resolvePlan validates legacy metadata via validatePlan and logs a warning with priceId before returning 'free' when resolution fails; test setup comment clarifies that retry recording is mocked to no-op but real delay timing is preserved.
Subscription Update Logic Tightening modules/billing/services/billing.webhook.service.js	cancel_at is set only when subscription.cancel_at is numeric (preserving null branch); plan-change detection computes previousRaw from price-map and metadata fallbacks, validates it via validatePlan to derive previousPlan, preventing invalid/stale values from triggering plan-changed events and meter-reset behavior.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20 minutes

Possibly related issues

• pierreb-devkit/Node#3757: Main PR reintroduces hardening for priceId→plan mapping, fallback warning logging, cancel_at type check, and plan validation—directly addresses the same findings.

Possibly related PRs

• pierreb-devkit/Node#3582: Both PRs modify plan-change detection and handling in handleSubscriptionUpdated within billing webhook service.
• pierreb-devkit/Node#3611: Both PRs refine subscription plan-resolution and plan-change behavior in billing webhook service; main PR tightens resolvePlan fallback and handleSubscriptionUpdated detection, while retrieved PR refactors effective-plan logic.
• pierreb-devkit/Node#3284: Both PRs enhance webhook plan resolution/validation in billing service; retrieved PR introduces validatePlan fallback to 'free', and main PR extends that validation to improve observability and cancel_at handling.

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 inconclusive)

Check name	Status	Explanation	Resolution
Description check	❓ Inconclusive	The PR description covers the critical context, root cause, the five fixes being re-applied, and verification results. However, it is missing structured sections from the template (Summary with What/Why, Scope with modules and risk level, and Validation checklist).	Restructure the description to match the template: add explicit Summary (What changed / Why) and Scope (Module impacted / Risk level) sections, include the verification checklist with check marks, and add a brief Guardrails section.

✅ Passed checks (4 passed)

Check name	Status	Explanation
Title check	✅ Passed	The title accurately describes the main change: re-applying five previously-missed fixes from a prior review, specifically mentioning the critical warn-on-unknown-priceId fix and hardening improvements.
Docstring Coverage	✅ Passed	No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
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.

_{✏️ Tip: You can configure your own custom pre-merge checks in the settings.}

✨ Finishing Touches

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Commit unit tests in branch fix/resolveplan-review-nits-followup

---

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

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

[Copilot]

Pull request overview

Re-applies five previously lost hardening/observability fixes in the billing Stripe webhook flow (priceId→plan resolution), to prevent silent plan downgrades and make plan-change detection more robust.

Changes:

• Updates buildPriceIdToPlanMap() JSDoc to accurately describe behavior and return type.
• Adds a logger.warn when falling back to free due to an unmapped priceId and missing metadata, and hardens cancel_at + previousPlan handling.
• Corrects a misleading unit-test comment about retry delay suppression.

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.

File	Description
modules/billing/services/billing.webhook.service.js	Adds warning on last-resort free fallback; hardens cancellation timestamp handling and previous-plan validation; updates JSDoc.
modules/billing/tests/billing.webhook.priceid-map.unit.tests.js	Corrects test comment to accurately describe what the mock does (no retry delay suppression).

[codecov]

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 90.14%. Comparing base (1230870) to head (b25da05).

Additional details and impacted files

@@           Coverage Diff           @@
##           master    #3758   +/-   ##
=======================================
  Coverage   90.13%   90.14%           
=======================================
  Files         150      150           
  Lines        4956     4961    +5     
  Branches     1573     1574    +1     
=======================================
+ Hits         4467     4472    +5     
  Misses        384      384           
  Partials      105      105           

Flag	Coverage Δ
integration	59.44% <87.50%> (+0.02%)	⬆️
unit	68.87% <100.00%> (+0.03%)	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

---

Continue to review full report in Codecov by Sentry.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 1230870...b25da05. Read the comment docs.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
• 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.