fix: change override condition of tier

[ravverma]

Summary

Update TierClient.createEntitlement so an existing entitlement's tier is only overridden when the customer is becoming PAID. Previously, any mismatch between the current and requested tier (as long as the current wasn't PAID) would trigger an override — which now doesn't fit with new additional tiers, we have four tiers now: PRE_ONBOARD, PLG, FREE_TRIAL, PAID.

Changes

• packages/spacecat-shared-tier-client/src/tier-client.js
  • createEntitlement: change the upgrade guard from
    currentTier !== tier && currentTier !== PAID to
    tier === PAID && currentTier !== PAID.
  • All other tier transitions are now no-ops (existing entitlement is returned as-is).
• packages/spacecat-shared-tier-client/test/tier-client.test.js
  • Updated PLG → FREE_TRIAL test to assert setTier/save are NOT called.
  • Updated PRE_ONBOARD → PLG test to assert setTier/save are NOT called.
  • Existing upgrade-to-PAID tests continue to pass unchanged.

Behavior Matrix

current \ target	PRE_ONBOARD	PLG	FREE_TRIAL	PAID
PRE_ONBOARD	keep	keep	keep	upgrade → PAID
PLG	keep	keep	keep	upgrade → PAID
FREE_TRIAL	keep	keep	keep	upgrade → PAID
PAID	keep	keep	keep	keep

Test Plan

• npm test -w packages/spacecat-shared-tier-client — 77 passing, 100% line/branch/function coverage on tier-client.js.
• Verified that only upgrades to PAID mutate the entitlement; all other transitions leave the existing entitlement untouched.
• Downstream consumers (spacecat-api-service, spacecat-audit-worker, spacecat-import-worker) to be re-validated after publish.

Notes

• One pre-existing test (should create entitlement with PRE_ONBOARD tier when nothing exists) fails both before and after this change because @mysticat/data-service-types does not yet expose PRE_ONBOARD in the ENTITLEMENT_TIER enum. Out of scope for this PR; tracked separately.

[solaris007]

Hey @ravverma,

Thanks for the fix. Four specialists reviewed this PR with a focus on backwards compatibility and cross-repo impact. The directional change is right, but there are several issues we want addressed before this ships to consumers.

Strengths

• Clean positive rewrite of the guard at packages/spacecat-shared-tier-client/src/tier-client.js:153. Much easier to read than the prior double negative.
• Intent-revealing comment at lines 151-152 naming the four tiers.
• Existing upgrade-to-PAID tests (PLG -> PAID, PRE_ONBOARD -> PAID, FREE_TRIAL -> PAID) still pass and correctly assert the new semantics.
• Symmetric test updates for PLG -> FREE_TRIAL and PRE_ONBOARD -> PLG no-ops; pre-existing enum gap flagged honestly in the PR body.
• Tier validation at tier-client.js:140 still throws on unknown strings, so the guard does not have to defend against garbage input.

Issues

Critical (Must Fix)

1. tier-client.test.js createEntitlement describe block: zero tests with currentTier = PAID. The entire invariant this PR enforces ("protect PAID from being overwritten") is unverified. Add PAID -> PAID, PAID -> PLG, PAID -> FREE_TRIAL, PAID -> PRE_ONBOARD; each asserts setTier/save are not called and getTier() still returns PAID.

2. tier-client.js:153-156 silent no-op with misleading return. A caller invoking createEntitlement('FREE_TRIAL') on a PLG org now receives existing.entitlement whose getTier() === 'PLG', with no log, no error, no flag. Callers in spacecat-api-service (plg-onboarding, llmo-onboarding) and spacecat-fulfillment-worker cannot detect their request was ignored. Fix: add this.log.warn({ orgId, currentTier, requestedTier }, 'Tier change skipped; createEntitlement only upgrades to PAID') in an else branch, or return { entitlement, siteEnrollment, tierChanged: boolean }.

3. tier-client.js:131-137 stale JSDoc. Still says "If entitlement exists with different tier, updates the tier." That is no longer true. Replace with the actual rule (upgrade-to-PAID only; all other targets are ignored when an entitlement exists) so consumers do not have to read source.

4. Semver: fix: commit on a behavior change to a published shared library. Conventional Commits + semantic-release will treat this as a patch. Consumers upgrading without noticing will silently lose the non-PAID-override behavior they may have been relying on. Promote to feat!: (or include a BREAKING CHANGE: footer) and bump major on publish.

5. Downgrade / subscription-end path unspecified. With this change, PAID -> FREE_TRIAL and PAID -> PRE_ONBOARD are silent no-ops here. If any fulfillment flow ever ends a paid subscription through createEntitlement, customers retain paid entitlement indefinitely (revenue-leakage and over-entitlement risk). Either document where subscription downgrade lives, or close the gap before merging.

Important (Should Fix)

1. Caller audit across dependent repos before release. Search createEntitlement( usage in spacecat-api-service (PLG/LLMO onboarding controllers, Slack onboarding actions), spacecat-fulfillment-worker (aem-sites + llmo handlers), spacecat-audit-worker, spacecat-import-worker. Any caller that was relying on non-PAID upgrades (e.g. PRE_ONBOARD -> PLG on onboarding completion) is now a silent prod regression. Post the list in this PR before merge.

2. tier-client.js:153 hard-coded === PAID couples the library to today's tier ladder. When ENTERPRISE or any other tier arrives, the predicate is wrong. Consider tier ordinal rank (targetRank > currentRank) or an explicit transition-policy map. Four tiers make this look cheap; at six it costs every call site.

3. Transition policy lives in the wrong layer. Encoding tier-transition business rules in a shared client library forces every consumer to bump and redeploy when policy changes. Long-term, this belongs in the entitlement/data service with the client being a thin wrapper. This PR widens existing debt rather than creating it, but worth tracking.

4. packages/spacecat-shared-tier-client/README.md (around lines 119 and 178) needs updating. The canonical example still shows createEntitlement('FREE_TRIAL') with no mention of the upgrade-only rule. Add a transition-rules table so consumers do not have to read source or tests.

5. Test matrix beyond the PAID row is incomplete. 4 tiers = 16 transition cells; the suite covers about 6. Suggest replacing the existing two renamed tests with a parameterised forEach over the full 16-cell transition table so future tier additions (e.g. ENTERPRISE) are a one-row change.

6. PRE_ONBOARD enum gap promised but not delivered. The PR body documents PRE_ONBOARD as a supported tier in the behavior matrix, but @mysticat/data-service-types does not yet expose it, so createEntitlement('PRE_ONBOARD') throws Invalid tier at tier-client.js:140-142. Documentation and runtime behavior diverge until the enum lands.

Minor (Nice to Have)

1. tier-client.js:153 - extract isUpgradeToPaid(currentTier, tier) so the branch reads self-documenting and is testable in isolation.
2. tier-client.js:151-152 - drop the inline "Tiers: PRE_ONBOARD, PLG, FREE_TRIAL, PAID" comment. The enum is the source of truth and this list will rot.
3. Consider splitting createEntitlement into ensureEntitlement(tier) (for new) and upgradeToPaid() (for transitions). The method name currently does triple duty (create / idempotent get / PAID-only upgrade).
4. Add a null/undefined currentTier test case for defensive coverage.

Recommendations

Documentation gap in mysticat-architecture. We checked. The only tier doc is products/aso/tier.md, which is about operational tier config (scan_frequency, max_pages, goal_facts, priority). The entitlement tier enum (PRE_ONBOARD / PLG / FREE_TRIAL / PAID) and transition rules are not documented anywhere. This PR encodes an architectural decision ("only upgrades to PAID are material; other transitions are no-ops in the shared client") as a one-line inline comment. Concrete proposal:

• Create mysticat-architecture/platform/concepts/entitlement-tier.md documenting the enum, the transition state machine, the authoritative owner (TierClient today, data service future), and the consumer list. Frontmatter scope: platform, status: decided.
• Create mysticat-architecture/platform/decisions/NNN-entitlement-tier-transitions.md as the ADR recording this decision.
• Rename products/aso/tier.md to operational-tier.md (or move to platform/concepts/operational-tier.md) to end the name collision with the entitlement tier concept.
• Cross-link from the tier-client README to the architecture doc.

Per DOCUMENTATION-GUIDE.md criterion "contract shared across repos", platform/ is the correct tier for this.

Beyond documentation: consider an integration test in at least one consumer repo once the version bumps, to confirm the new no-op does not break onboarding or fulfillment flows.

Assessment

Ready to merge? No, with fixes.

Reasoning: The logic is right, but the PR ships a silent behavior change on a shared library under a patch-level commit, with stale JSDoc, zero tests verifying the PAID invariant, no observability on the skipped-tier path, and an unspecified downgrade story. Address the five Critical items (tests with currentTier = PAID, log or signal the no-op, fix the JSDoc, promote to feat!:, resolve the downgrade path), audit callers, and file the architecture docs. Tier-ladder extensibility can be a tracked follow-up.

Next Steps

1. Land the five Critical items in this PR (PAID-as-current tests, log/return-signal on skip, JSDoc, commit type, downgrade-path decision).
2. Run the caller audit and document results in the PR body.
3. Update the README transition-rules example.
4. File the mysticat-architecture docs (concept + ADR + rename) as a separate PR.
5. Minor items are optional.

[solaris007]

Critical: silent no-op with misleading return.

When this guard is false, the method still returns existing.entitlement from below, whose getTier() does NOT match the tier argument the caller passed. Callers in spacecat-api-service (PLG/LLMO onboarding) and spacecat-fulfillment-worker have no way to detect their requested tier was ignored: no log, no error, no flag on the return object.

Fix (pick one):

• Add this.log.warn({ orgId, currentTier, requestedTier: tier }, 'Tier change skipped; createEntitlement only upgrades to PAID') in an else branch, OR
• Return { entitlement, siteEnrollment, tierChanged: boolean } so callers can branch on it.

Important (separate finding, same line): the hard-coded === ENTITLEMENT_TIERS.PAID couples the library to today's 4-tier ladder. When ENTERPRISE or any variant is added, this predicate is wrong at every call site. Prefer a tier-ordinal comparison or an explicit transition-policy map.

[solaris007]

Critical: the createEntitlement describe block has zero tests with currentTier = PAID.

The entire invariant this PR enforces ("protect PAID from being overwritten") is unverified. Please add, each asserting setTier/save are NOT called and the returned entitlement's getTier() still returns PAID:

• PAID -> PAID
• PAID -> PLG
• PAID -> FREE_TRIAL
• PAID -> PRE_ONBOARD

Also Important: consider replacing these two renamed tests with a parameterised forEach over the full 16-cell transition table (4 tiers x 4 targets). Future tier additions (e.g. ENTERPRISE) become a one-row change instead of N new it blocks.

Minor: the stale JSDoc at tier-client.js:131-137 still says "If entitlement exists with different tier, updates the tier" and needs updating to reflect the PAID-only rule.