feat: SITES-40623 - token system in Spacecat

[sandsinh]

Please ensure your pull request adheres to the following guidelines:

• make sure to link the related issues in this description
• when merging / squashing, make sure the fixed issue references are visible in the commits, for easy compilation of release notes

Related Issues

Thanks for contributing!

[solaris007]

@sandsinh please have the courtesy of providing a minimum PR description / links to issues / specs, etc.

[solaris007]

Hey @sandsinh

Good foundation for the freemium token system. The data model aligns with the wiki spec, the schema composite key design is sound, and the hardcoded JSON config is a pragmatic starting choice. However, there are several issues that should be addressed before this is ready to merge - some are correctness bugs, others are architectural gaps.

Critical

1. useToken race condition - concurrent requests can over-consume tokens

token.collection.js:93-108 - The read-modify-write pattern is not atomic:

const token = await this.findBySiteIdAndTokenTypeAndCycle(siteId, tokenType, cycle);
const remaining = token.getRemaining?.() ?? 0;
if (remaining < 1) return null;
const used = (token.getUsed?.() ?? token.used ?? 0) + 1;
token.setUsed(used);
await token.save();

Two concurrent requests can both read used=9, total=10, both see remaining=1, and both succeed - granting 2 suggestions but consuming only 1 effective token. The spec explicitly requires atomic consumption.

Fix: Use a DynamoDB conditional update (ConditionExpression: "used < total") with an atomic increment. ElectroDB supports .where() conditions on patch operations. This pushes the atomicity guarantee to the database rather than the application layer.

2. Tests exercise methods that don't exist in the source

token.collection.test.js has describe('allBySiteIdAndTokenType') (lines ~118-142) and describe('create') (lines ~144-168) testing methods that are not implemented in TokenCollection. The allBySiteIdAndTokenType method doesn't exist anywhere. The create tests expect custom validation ('siteId, tokenType, and cycle are required') that is not in BaseCollection.create(). These tests will either fail or are testing phantom behavior.

Fix: Either implement the methods or remove/correct the tests.

3. findBySiteIdAndTokenTypeAndCycle manually constructs Token - bypasses the framework

token.collection.js:62-78 manually calls new Token(...) instead of using the base collection's #createInstance() pipeline. This skips attribute getters, read-time defaults, and record hydration. The eslint-disable-next-line new-cap comment is a code smell that this is working against the framework.

This method is also functionally redundant with findById above it, which correctly uses findByIndexKeys. All callers of findBySiteIdAndTokenTypeAndCycle (getRemainingToken, useToken) should use findByIndexKeys instead.

High

4. Grant immutability is not enforced

suggestion.schema.js - The grants attribute is a plain writable map. The spec says "Grants are immutable - once granted, cannot be revoked." But nothing prevents calling suggestion.setGrants(null) or overwriting an existing grant. The base model auto-generates setGrants() since the attribute is not readOnly.

Fix: Add write-once validation - either readOnly: true with a dedicated grant() method that bypasses it for the initial write, or a custom setter that throws if grants is already populated.

5. No enum validation on tokenType

token.schema.js - tokenType is type: 'string' with no constraint. Arbitrary values (including malformed strings) can be written. Token.TOKEN_TYPES defines valid values but the schema ignores them.

Fix: type: Object.values(Token.TOKEN_TYPES) - same pattern used for Suggestion.STATUSES in suggestion.schema.js.

6. Hardcoded freemium organization UUID

entitlement.collection.js:27 - FREEMIUM_ORGANIZATION_ID = 'ed79490b-...' is hardcoded in source. This creates several problems:

• Not environment-aware (dev/stage/prod may differ)
• Adding a second freemium org requires a code deploy
• isFreemium() lives on EntitlementCollection but never reads entitlement data - it's a misleading location for a pure string comparison

Consider making plan type a data-driven attribute on the Organization entity, or at minimum move the UUID to environment configuration.

7. findById override breaks the base class contract

token.collection.js:43-50 changes findById(id) to findById(siteId, tokenType, cycle). This is a Liskov Substitution Principle violation - any code generically calling collection.findById(someId) will break silently. Either keep the base signature (accept a composite key object) or name it differently.

Medium

8. Token type and opportunity type namespaces are disjoint

Token.TOKEN_TYPES uses BROKEN_BACKLINK (singular) while OPPORTUNITY_TYPES uses BROKEN_BACKLINKS (plural, value 'broken-backlinks'). The COMPLETE_HANDLERS map in suggestion-complete.js uses opportunity type values. token-grant-config.json uses token type values. There is no mapping between them anywhere in this PR. Callers must know which namespace to use in which context - this is error-prone and will lead to bugs.

9. No test coverage for isGranted

suggestion-complete.test.js tests all the completeness functions but has zero tests for isGranted(). This function is exported as public API with TypeScript declarations - it needs tests.

10. No orchestration layer for the grant workflow

The full "grant a token to a suggestion" flow requires: check isFreemium -> check isSuggestionComplete -> call useToken -> set grants on suggestion -> save. Nothing in this PR encapsulates that sequence. Each consumer (api-service, audit-worker) must implement it independently, risking inconsistency. Consider a grantTokenToSuggestion() method.

11. Unnecessary defensive getter pattern

token.model.js:43-46 - this.getTotal?.() ?? this.total ?? 0 - the BaseModel constructor always generates getTotal() and getUsed(). The optional chaining and property fallbacks are unnecessary. Same pattern in useToken. Simplify to this.getTotal() / this.getUsed().

12. belongs_to Site reference creates a potentially redundant GSI

token.schema.js:28 - addReference('belongs_to', 'Site', ['tokenType', 'cycle']) auto-creates a siteId FK attribute and a GSI on { composite: ['siteId'] }. Since siteId is already part of the primary key via withPrimaryPartitionKeys(['siteId', 'tokenType']), the GSI partially overlaps. Verify this is intentional and not just wasting DynamoDB capacity.

Low

13. No cycle format validation - cycle accepts any string. The config says YYYY-MM but nothing enforces it. Consider a regex validator.

14. No TTL on token records - Old cycle tokens persist forever. Consider withRecordExpiry() to auto-clean tokens older than ~90 days.

15. STATUS_NEW = 'NEW' is a local constant in suggestion-complete.js instead of referencing Suggestion.STATUSES.NEW. If the status value changes, this silently breaks.

16. import ... with { type: 'json' } syntax in token-grant-config.js requires Node.js 21+. Verify this is compatible with the project's Node.js target.

17. Missing "all" index on Token schema - No .addAllIndex(...) means no way to query all tokens across sites for admin/reporting.

[sandsinh]

@sandsinh please have the courtesy of providing a minimum PR description / links to issues / specs, etc.

Sorry @solaris007. I do not how reviews were requested on the PR. It is too early. We are still in design and try out phase. Please ignore the PR for now.

[solaris007]

@sandsinh i think the reviews were added because you're changing database models - and we're migrating to Postgres soon...

[solaris007]

Note on the useToken race condition (item #1 in review) and the PostgreSQL migration:

This code will eventually move to mysticat-data-service (PostgREST). The race condition concern still applies if the same read-modify-write pattern is carried over, but PostgreSQL makes the atomic fix trivial - a single conditional UPDATE handles it:

UPDATE tokens
SET used = used + 1
WHERE site_id = $1 AND token_type = $2 AND cycle = $3
  AND used < total
RETURNING *;

PostgreSQL's row-level locking serializes concurrent updates on the same row, so the second concurrent request blocks until the first commits, then re-evaluates used < total against the updated value.

For the current DynamoDB implementation, this still needs a conditional expression or transaction. But when porting to PostgreSQL, make sure to use a single conditional UPDATE rather than porting the JS-level read-check-write pattern.