feat(data-access): add scopeType and scopeId to Opportunity model

[HollywoodTonight]

Exposes the existing scope_type / scope_id DB columns via the JS model layer so offsite audits can tag opportunities with a brand scope.

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!

[github-actions]

This PR will trigger a minor release when merged.

[grigoreadobe]

Hey @HollywoodTonight,

Strengths

• Clean, appropriately scoped PR that surfaces existing DB columns through the model layer without inventing new persistence semantics (opportunity.schema.js:68-73).
• SCOPE_TYPES defined as a static on the model and consumed via Object.values(...) in the schema (opportunity.model.js:40-43, opportunity.schema.js:69) - new scope types update both the enum and the validator in one place.
• Input validation in allByScopeId uses hasText to fail fast before hitting the data layer (opportunity.collection.js:37-42).
• Tests cover input-validation branches and get/set accessors for both new attributes.
• JSDoc on allByScopeId explains the motivation ("without going through site association"), not just the signature (opportunity.collection.js:27-35).
• TypeScript declarations updated in sync with the JS additions (index.d.ts:38-41, 56).

Issues

Critical (Must Fix)

allByScopeId queries a composite index that is not declared in the schema
Files: opportunity.schema.js:71, opportunity.collection.js:43

The schema adds scopeType and scopeId via addAttribute only - no addIndex call covers the (scopeType, scopeId) pair. allByIndexKeys({ scopeType, scopeId }) is index-driven; without a registered composite GSI the call will either throw at query time or silently fall back to a full table scan. At meaningful Opportunity table sizes, a scan means multi-second p99 latency and throttled read capacity that bleeds across the whole table.

The unit tests cannot catch this because they stub allByIndexKeys (opportunity.collection.test.js:90). The only evidence the index might exist is the undocumented data-service version bump - if that version carries the GSI, it must be stated explicitly and verified.

Fix: add the composite index declaration to the schema (matching the addIndex pattern used by other allBy* methods in this collection), and add an integration test under test/it/postgrest/ that exercises allByScopeId against a real table without stubbing.

scopeId has no format or length constraint despite being authorization-bearing
File: opportunity.schema.js:72

addAttribute('scopeId', { type: 'string' }) accepts arbitrary values: CRLF characters (log injection), multi-KB payloads (index key bloat), and non-UUID strings. Compare to primary-key handling in this codebase, which uses validate: (value) => isValidUUID(value). scopeId controls tenant attribution on opportunities, and the companion audit-worker PR feeds brandId from an SQS response directly to setScopeId. The data-access layer is the last validation checkpoint before malformed values corrupt the GSI partition.

Fix:

import { isValidUUID } from '@adobe/spacecat-shared-utils';
.addAttribute('scopeId', {
  type: 'string',
  validate: (value) => !value || isValidUUID(value),
})

Also validate in allByScopeId before the index lookup, and add a test asserting invalid scopeId values are rejected.

Unexplained 4-major-version data-service image bump
File: test/it/postgrest/docker-compose.yml:20

Going from v1.67.8 to v5.1.1 in a PR titled "add scopeType and scopeId" is a significant undocumented change. Either:

• This is required because v5.x introduces the scope_type/scope_id columns and/or the backing GSI. State this explicitly, confirm test fixtures remain compatible across 4 major versions, and document the production deployment order (data-service must precede or accompany this code release).
• It is opportunistic cleanup. Move it to its own PR so its blast radius is independently reviewable.

A future bisect hitting this commit will need to unpack which of two simultaneous changes caused a regression. The image is also pinned by tag (mutable) rather than digest - pin by @sha256:... for reproducible builds.

Important (Should Fix)

No co-presence validation: scopeType and scopeId are independently optional
File: opportunity.schema.js:68-73

A record can be saved with scopeType='brand' and no scopeId, or vice versa. allByScopeId will silently miss half-scoped rows; downstream auth decisions using getScopeType() can attribute an opportunity to the wrong tenant. Add a cross-field validator enforcing both-or-neither, and add a unit test asserting a half-set scope is rejected.

TypeScript return types disagree with runtime behavior
File: index.d.ts:38-39

getScopeId(): string | null and getScopeType(): string | null declare null for absent values, but the unit tests assert to.be.undefined. Callers checking === null get a false negative on the unset case. Auth-path code relying on this falls through silently. Normalize the getters to return null, or change the TS declarations to string | undefined. Pick one and make code and types agree.

scopeType enum constraint may reject existing rows with NULL scope_type on hydration
File: opportunity.schema.js:69-71

type: Object.values(Opportunity.SCOPE_TYPES) defines a strict enum ['site', 'brand']. The PR description says the columns already exist, so historical Opportunity rows almost certainly have NULL for scope_type. Compare to lastAuditedAt (opportunity.schema.js:66) which uses validate: (value) => !value || isIsoDate(value) to explicitly tolerate absent values. The strict array-type declaration may cause hydration failures on legacy rows. Apply the same pattern or test explicitly against a DB snapshot containing NULL-scope rows.

SCOPE_TYPES.SITE creates an ambiguous dual-attribution model alongside the existing siteId field
Files: opportunity.model.js:41, opportunity.schema.js

The Opportunity already has siteId. Adding scopeType='site' with a separate scopeId creates two parallel site-association mechanisms that can disagree. Brand-scoped opportunities will also have a siteId of some primary site - queries via allBySiteId will return brand-scoped rows interleaved with site-scoped rows, silently changing the semantics of every existing allBySiteId call site.

If the current use case is brand-only, remove SITE from the enum until a concrete site-scoping use case exists. If SITE is needed now, document the invariant between siteId and scopeId for scopeType='site' and add a validator enforcing consistency.

Method name allByScopeId is misleading
Files: opportunity.collection.js:36, index.d.ts:56

The method requires both scopeType and scopeId - it is not a lookup by ID alone. allByScopeId('brand', uuid) reads as if 'brand' is a filter on an ID-based lookup, and the positional signature makes arg-flip easy. This is a public API that audit-worker services will bind against. Rename to allByScope(scopeType, scopeId) to match the existing allByAuditIdAndUpdatedAt naming convention. Update the TypeScript declaration at the same time.

setScopeType accepts any string; enum validation fires only at save time
Files: index.d.ts:41, opportunity.schema.js:69

The auto-generated setter writes to the record without validation; the schema's enum constraint fires only at save(). A bad scopeType from an SQS message fails at persist time under a different correlation ID, not at the call site. Narrow the TS type to 'site' | 'brand', or override the setter to validate against Object.values(SCOPE_TYPES) and throw immediately. Add a negative test asserting invalid values are rejected before save().

Minor (Nice to Have)

• allByScopeId JSDoc says "directly by brandId" (opportunity.collection.js:29) but the method is generic - will go stale when a second scope type is used.
• Collection error messages could include the method name for easier log triage: 'allByScopeId: scopeType is required' (opportunity.collection.js:38, 41).
• Setter types setScopeId(scopeId: string) accept no null (index.d.ts:40). When someone needs to clear scope on a re-run, there is no documented path. Decide the contract now.
• No test asserting that an invalid scopeType (e.g., 'page') is rejected by the schema enum constraint. Locks in regression coverage if the enum is accidentally widened.
• New accessors in index.d.ts:38-41 are inserted in the middle of an otherwise alphabetized accessor list.

Recommendations

1. Add an integration test under test/it/postgrest/ that round-trips an opportunity through setScopeType/setScopeId -> save -> allByScopeId -> assert result. This is the single most failure-prone part of this change and no current test covers it end-to-end.
2. Add a setScope(scopeType, scopeId) convenience setter that validates and sets both atomically, making it hard to produce a half-scoped record.
3. Consider extracting a Scopable schema mixin (addScopeAttributes(builder)) and a shared SCOPE_TYPES constant. Brand scoping is described as cross-cutting - one consumer is the right moment to establish the pattern; three consumers later is a coordination project.
4. Document the deployment order between this code, the data-service v5.1.1 bump, and the audit-worker companion PR. If the data-service must be at v5.x for the backing index to exist, the rollout sequence is a production-safety dependency.

Assessment

Ready to merge? No, with fixes.

Reasoning: The missing composite index declaration is a runtime failure on the headline feature that unit tests cannot catch (they stub allByIndexKeys), the unexplained 4-major-version data-service bump carries unreviewed operational risk, and the scopeId format gap on an authorization-bearing field should be closed before the companion audit-worker PR ships.

[grigoreadobe]

Critical - missing composite index AND no format constraint on scopeId.

Two issues at this attribute declaration:

Missing index for allByScopeId. allByIndexKeys({ scopeType, scopeId }) requires a registered composite GSI on (scopeType, scopeId). This diff adds addAttribute only - no addIndex call. Without the index, this call throws or full-scans at runtime. The unit test stubs allByIndexKeys (opportunity.collection.test.js:90) so CI does not catch this. Add the composite index declaration to the schema and add an integration test that exercises allByScopeId against a real table.

No format constraint on scopeId. type: 'string' accepts CRLF characters (log injection), oversized payloads, and non-UUID strings. scopeId is authorization-bearing: it controls tenant attribution and the companion audit-worker PR feeds brandId from an SQS payload directly to setScopeId. Apply UUID validation consistent with how primary keys are handled in this codebase:

import { isValidUUID } from '@adobe/spacecat-shared-utils';
.addAttribute('scopeId', {
  type: 'string',
  validate: (value) => !value || isValidUUID(value),
})

[grigoreadobe]

Important - no co-presence validation, and strict enum may reject existing NULL rows.

Two issues at scopeType and the scopeId pair:

No co-presence enforcement. Both attributes are independently optional. A record can be saved with scopeType='brand' and no scopeId, or vice versa. allByScopeId silently misses half-scoped rows; downstream auth using getScopeType() can attribute an opportunity to the wrong tenant. Add a cross-field validator enforcing both-or-neither.

Strict enum may break hydration of legacy rows. type: Object.values(Opportunity.SCOPE_TYPES) becomes ['site', 'brand']. The PR description says these columns already exist, so historical rows almost certainly have NULL for scope_type. Compare to lastAuditedAt which uses validate: (value) => !value || isIsoDate(value) to tolerate absent values. Apply the same pattern or test against a DB snapshot containing NULL-scope rows.

[grigoreadobe]

Critical - unexplained 4-major-version image bump.

v1.67.8 to v5.1.1 is a four-major-version jump with no explanation in the PR description. Either:

• This is required because v5.x introduces the scope_type/scope_id columns and/or the backing GSI. State this explicitly, confirm all test fixtures remain compatible, and document the production deployment order.
• It is opportunistic cleanup - move it to its own PR so its blast radius is independently reviewable.

A future bisect on this commit will need to unpack which of two simultaneous changes caused a regression. Also: the image is pinned by tag (mutable) - pin by @sha256:... digest for reproducible builds.

[grigoreadobe]

Important - TypeScript return types disagree with runtime behavior.

getScopeId(): string | null and getScopeType(): string | null declare null for the absent case, but the unit tests assert to.be.undefined. Callers checking === null get a false negative on the unset case. Auth-path code relying on this falls through silently.

Either normalize the getters to return null (preferred for JSON/DB API consistency), or change the TS declarations to string | undefined. Pick one and align code and types.

[grigoreadobe]

Important - method name is misleading, and two issues around validation.

Rename allByScopeId. The method requires both scopeType and scopeId - it is not a lookup by ID alone. allByScopeId('brand', uuid) reads as if 'brand' is a filter on an ID-based lookup, and the positional signature makes arg-flip easy. Rename to allByScope(scopeType, scopeId) to match the existing allByAuditIdAndUpdatedAt convention. Update index.d.ts:56 in the same change.

scopeType is not validated against the enum here. hasText only checks presence. A caller passing 'BRAND' or 'admin' passes through into allByIndexKeys. Add if (!Object.values(Opportunity.SCOPE_TYPES).includes(scopeType)) throw new Error(...) as a second line of defense, consistent with how the schema handles invalid values at write time.

setScopeType setter fires enum validation only at save() time. The auto-generated setter writes unchecked to the record. Narrow the TS type to 'site' | 'brand' or add an explicit setter override that validates immediately.

[grigoreadobe]

Hey @HollywoodTonight,

Strengths

Previously flagged issues now addressed:

• Schema index declaration added: .addIndex({ composite: ['scopeType'] }, { composite: ['scopeId'] }) matches the addIndex(partitionKey, sortKey) pattern used elsewhere in the codebase and follows the same structure as audit-url.schema.js. ElectroDB will route allByIndexKeys({ scopeType, scopeId }) to this GSI.
• scopeId UUID validation added: validate: (value) => !value || isValidUUID(value) brings scopeId to the same standard as primary key attributes in this codebase.
• scopeType attribute changed from strict array-type enum to a nullable-tolerant string with a validate guard, preventing hydration failures on historical rows with NULL scope_type.
• SCOPE_TYPES.SITE removed, eliminating the ambiguous dual-attribution model with siteId.
• Method renamed from allByScopeId to allByScope, resolving the misleading positional signature.
• TypeScript declarations corrected: getScopeId(): string | undefined, getScopeType(): string | undefined now match the runtime behavior asserted in unit tests.
• setScopeType(scopeType: 'brand') narrowed in index.d.ts, providing compile-time rejection of invalid scope types.
• Error messages now include the method name ('allByScope: scopeType is required'), improving log triage.
• JSDoc updated to be scope-generic, will not go stale on the second scope type.

Issues

Critical (Must Fix)

Integration test still absent (packages/spacecat-shared-data-access/test/unit/models/opportunity/opportunity.collection.test.js)

The unit test for allByScope still stubs allByIndexKeys directly. This means the GSI declaration in opportunity.schema.js is never exercised - the test passes even if the index name, composite keys, or ElectroDB routing are misconfigured. The data-service image bump from v1.67.8 to v5.1.1 is presumably what adds the backing DynamoDB GSI, but there is currently zero test coverage confirming the index resolves correctly at query time.

Add an integration test under test/it/postgrest/ that:

• Creates an Opportunity with scopeType='brand' and a valid scopeId UUID
• Saves it
• Calls allByScope('brand', scopeId) and asserts the opportunity is returned
• Calls allByScope('brand', differentUUID) and asserts an empty result

This is the only way to prove the schema declaration, data-service migration, and ElectroDB routing work together end-to-end.

Important (Should Fix)

GSI partition key creates a single-valued hot partition (packages/spacecat-shared-data-access/src/models/opportunity/opportunity.schema.js:80)

With scopeType as the GSI partition key, every scope-indexed opportunity in the table lands in the same DynamoDB partition ('brand'). DynamoDB cannot split a partition across nodes - this is a single write/read hotspot for the entire scoped-opportunity workload. The audit-url schema avoids this by using siteId (a UUID, high cardinality) as its GSI partition key.

The access pattern here is always "give me all opportunities for a specific brand UUID" - scopeId is the high-cardinality discriminator, not scopeType. Invert the index:

.addIndex({ composite: ['scopeId'] }, { composite: ['scopeType'] })

Each brand UUID gets its own DynamoDB partition. Fix while the index is still new and no data is in production.

Co-presence validation still missing (packages/spacecat-shared-data-access/src/models/opportunity/opportunity.schema.js:74-79)

A record can still be saved with scopeType='brand' and scopeId=undefined, or vice versa. allByScope guards at query time with hasText, but the persisted data itself is not constrained. A half-scoped row will silently disappear from allByScope results and surface as unexplained attribution gaps in the brand UI.

Add a cross-field validator enforcing both-or-neither, and add unit tests asserting a half-set scope is rejected on save.

Data-service image bump unexplained; tag not pinned by digest (packages/spacecat-shared-data-access/test/it/postgrest/docker-compose.yml:20)

The bump from v1.67.8 to v5.1.1 has no explanation in the PR description or commit message of why v5.x is required - specifically whether it carries the DynamoDB migration that creates the scope_type/scope_id columns and the backing GSI. If v5.1.1 is the production deployment prerequisite for this code change, that deployment dependency must be documented so the release team knows the data-service must be at v5.x before this code is deployed.

The tag v5.1.1 is mutable - the same tag can be updated to a different image without notice, breaking reproducible CI. Pin to a digest: image: 682033462621.dkr.ecr.us-east-1.amazonaws.com/mysticat-data-service@sha256:<digest>

Minor (Nice to Have)

• opportunity.collection.test.js - Both allByScope positive tests still use 'brand-uuid-123' and 'brand-uuid-no-results' as scopeId values. These are not valid UUIDs and are inconsistent with the UUID validation now on the schema attribute. Replace with valid UUID strings (e.g., 'a1b2c3d4-e5f6-7890-abcd-ef1234567890'). When someone reads these tests alongside the UUID validator, the mismatch is confusing.
• index.d.ts:37 - setScopeId(scopeId: string) has no null/undefined overload. There is currently no documented path to un-scope an opportunity after it has been scoped. Decide the contract: if clearing scope is valid, add setScopeId(scopeId: string | null): Opportunity; if not, add a comment.
• No unit test asserts that an invalid scopeType string (e.g., 'page') is rejected by the schema validate guard. Add one to lock in regression coverage.

Recommendations

1. The integration test is the highest-leverage missing piece. Before it exists, neither the index declaration, the data-service version, nor the allByScope routing can be considered verified. Ship the IT test alongside the schema change, not as a follow-up.
2. Document the deployment sequence in the PR description: data-service image must be at v5.x in production before this code is deployed, since the GSI and columns do not exist in earlier versions.
3. Consider a setScope(scopeType, scopeId) convenience setter that validates and writes both atomically. With co-presence enforced only at save time, there is a window between setScopeType and setScopeId where the in-memory object is in a half-scoped state. An atomic setter eliminates that window.

Assessment

Ready to merge? No, with fixes.

Reasoning: Three prior Critical items were addressed (index declaration, UUID validation, hydration enum fix) and five Important items closed out - solid progress. The integration test absence is the remaining blocker: the index routing cannot be verified without it, and the unexplained data-service major-version bump is exactly what the IT test is supposed to validate. The GSI partition key design should also be corrected while no production data exists yet.

Next Steps

1. Add the integration test for allByScope (verifies index routing end-to-end).
2. Invert the GSI composite order: scopeId as partition key, scopeType as sort key.
3. Add co-presence validation to the scopeId attribute.
4. Document the data-service deployment prerequisite and pin the image by digest.
5. Minor items are optional.

[grigoreadobe]

Hot partition: scopeType has only one enum value ('brand'), so this GSI partition key concentrates all scoped opportunities in a single DynamoDB partition. DynamoDB cannot split that partition as the table grows. Invert: scopeId (UUID, high cardinality) should be the partition key and scopeType the sort key:

.addIndex({ composite: ['scopeId'] }, { composite: ['scopeType'] })

Fix while no production data exists yet.

[grigoreadobe]

Co-presence gap: there is no cross-field validator enforcing that scopeType and scopeId are both set or both absent. A record saved with only one of the two will silently disappear from allByScope results. Add a validate function on scopeId that checks item.scopeType is consistent, and add unit tests for each half-set combination.

[grigoreadobe]

Unexplained bump + mutable tag: the jump from v1.67.8 to v5.1.1 needs a comment or PR description entry explaining that v5.x carries the DynamoDB migration for scope_type/scope_id columns and the backing GSI. Without this, nobody on the release team knows the data-service must be at v5.x before this code ships.

Also, tag-pinned images are mutable - the same tag can change without notice. Pin to a digest instead:

image: 682033462621.dkr.ecr.us-east-1.amazonaws.com/mysticat-data-service@sha256:<digest>

[grigoreadobe]

Hey @HollywoodTonight,

Strengths

Previously flagged issues now addressed:

• Integration test gap closed: test/it/opportunity/opportunity.test.js:385-444 creates real records and round-trips through the GSI end-to-end - not a stub. The two-brand isolation assertion confirms the index discriminates correctly on scopeId.
• GSI partition key corrected: .addIndex({ composite: ['scopeId'] }, { composite: ['scopeType'] }) puts the high-cardinality UUID as the partition key and eliminates the prior hot-partition risk.
• Co-presence validation added at create time: opportunity.collection.js:36-42 correctly rejects half-scoped records using Boolean(scopeType) !== Boolean(scopeId), handling undefined, null, and empty string consistently.
• Test UUIDs replaced with valid v4 UUIDs throughout, aligning unit tests with the schema's isValidUUID guard.
• All four co-presence quadrants covered in unit tests (scopeType-only, scopeId-only, both-set, neither-set) plus the null-item edge case.

Issues

Critical (Must Fix)

Co-presence invariant is not enforced on update, save, or bulk-write paths
Files: opportunity.collection.js:36-42, opportunity.schema.js:70-78

The create() override guards a single entry point. Two write paths bypass it entirely:

• setScopeType('brand'); await instance.save() - auto-generated setters and BaseModel.save() do not route through the collection override. A caller can call setScopeId(undefined) on a fully-scoped record and save it; both attribute validators accept absent values (!value || ...), so the half-scoped state persists silently.
• updateByKeys({ opportunityId: X }, { scopeId: '<new-uuid>' }) - patches directly via Electro without calling the override. This silently re-attributes the record to a different tenant.

Concrete impact: allByScope('brand', brandA) returns an opportunity now carrying scopeId=brandB, breaking tenant isolation. Authorization decisions made on allByScope reads become wrong without any validation error or log signal.

Fix (pick one - option a is simpler and stronger):

• a) Mark scopeType and scopeId as readOnly: true in opportunity.schema.js. Scope is an authorization boundary; treating it as immutable after creation closes all three write paths at once.
• b) If scope must be mutable, add the same XOR guard to createMany, updateByKeys, and a model-level save pre-flight so all paths enforce the invariant.

A unit test calling setScopeId(undefined); await save() and asserting a ValidationError would have caught this gap.

Important (Should Fix)

Data-service image bump still unexplained and unpinned by digest
File: test/it/postgrest/docker-compose.yml:20

Confirmed via file read at HEAD: the default is still ${MYSTICAT_DATA_SERVICE_TAG:-v5.1.1}, no digest pin. This is the third review round this remains open:

1. The 4-major-version jump from v1.67.8 to v5.1.1 has no explanation in the PR description or any commit message. If v5.x carries the migration that creates the scope_type/scope_id columns and the backing GSI, this code cannot be deployed before that data-service version is live in each target environment. That deployment-order dependency must be stated explicitly.
2. A mutable tag means two CI runs at the same commit can resolve to different image content. For a 4-major-version bump, a tag repoint or registry compromise is invisible to PR review.

Fix: Pin to digest (...@sha256:<digest>) and add one sentence to the PR description explaining what v5.x provides and that it must be deployed before this code.

IT test records are not cleaned up after the allByScope suite
File: test/it/opportunity/opportunity.test.js:401-411

oppA and oppB are created with deterministic brand UUIDs and never removed. The outer afterEach at line 59 does not cover these new records. If the IT suite runs against a persistent database, repeated runs accumulate brand-scoped opportunities on siteId. The current include/not.include assertions survive accumulation, but any future test asserting a count on siteId will silently pick up these leaked records.

Fix: Add an after() hook inside the allByScope describe block calling oppA.remove() and oppB.remove(), or generate random UUIDs per run using crypto.randomUUID().

No test asserting an invalid scopeType value is rejected by the schema validate guard
File: opportunity.schema.js:72

Carried over from round 2. The schema defines validate: (value) => !value || Object.values(Opportunity.SCOPE_TYPES).includes(value) for scopeType and validate: (value) => !value || isValidUUID(value) for scopeId. Neither has a test exercising the rejection path. Without coverage, a future enum widening or accidental removal of the validate guard goes undetected until runtime.

Fix: Add two assertions - one for scopeType: 'page' (invalid enum value) and one for scopeId: 'not-a-uuid' (invalid UUID format) - each confirming a ValidationError is thrown.

Minor (Nice to Have)

• opportunity.collection.js:39 - ValidationError constructed without entity context; convention elsewhere in the collection layer passes this as the second argument for consistent error logging: new ValidationError('scopeType and scopeId must both be set or both be absent', this).
• opportunity.collection.js:37 - item || {} is defensive code for a case super.create already handles. The associated "handles null item gracefully" unit test validates internal delegation behavior rather than the co-presence rule itself.
• test/it/opportunity/opportunity.test.js:399 - Test description says "returns only opportunities matching the given brandId" but the method is generic. Will go stale when a second scope type is added. Update to "returns only opportunities matching the given scope".
• opportunity.collection.js:38 - Using hasText() (already imported) instead of Boolean() would match the allByScope validation style: if (hasText(scopeType) !== hasText(scopeId)).
• No documentation that scopeType and scopeId must be cleared together when removing scope from a record. Once the update path is addressed, the un-scoping contract should be noted in the JSDoc.
• No backfill plan noted for historical rows with absent scopeType/scopeId. These are correct as sparse-GSI non-entries, but any analytics query assuming "all opportunities for a brand are reachable via allByScope" will miss legacy rows. A one-line note in the PR description or a tracked follow-up is sufficient.

Recommendations

1. Mark scopeType and scopeId as readOnly: true - simplest path to a fully enforced tenant boundary without requiring overrides on every mutation path.
2. File a follow-up to introduce a validateRecord hook on SchemaBuilder so cross-field invariants live in the schema rather than per-collection method overrides. Reference it in a code comment so the next contributor sees the trajectory.
3. Document the GSI partition design rationale (high-cardinality PK = scopeId, low-cardinality SK = scopeType) in the schema JSDoc so future index additions don't repeat the hot-partition lesson.
4. If scope becomes a cross-collection concept, extract a Scopeable mixin at that point - one consumer is too early, three is too late.

Assessment

Ready to merge? No, with fixes.

Reasoning: Strong progress this round - the integration test, GSI inversion, and create-path co-presence check are all correct and close the prior blockers. The remaining issue is that co-presence enforcement is incomplete: update and save paths can silently produce the exact half-scoped records that create() refuses to write, on a field that controls tenant attribution. Either mark scope as read-only or extend the guard to all write paths before landing.

Next Steps

1. Address the Critical: add readOnly: true to scopeType/scopeId in the schema (preferred), or extend the co-presence check to cover update, save, and createMany paths.
2. Address the 3 Important items: document and pin the data-service image, add IT cleanup, and add the missing validate-guard tests.
3. Minor items are optional.

[rarescheseli]

Hey @HollywoodTonight,

Strengths

The save() override at opportunity.model.js:51-58 is well-implemented: it uses the same hasText() XOR check and ValidationError(message, this) pattern as collection.create(), delegates cleanly to super.save(), and all four co-presence quadrants are unit-tested at opportunity.model.test.js:413-453. The pattern also aligns with how consumer.model.js handles similar invariants elsewhere in the codebase - not a new design, just a consistent extension.

Previously flagged issues now addressed:

• Round 3 Critical (save-path co-presence): the setter+save path (setScopeType(...); save()) is now guarded.
• Round 3 Important (IT cleanup): after() hook at opportunity.test.js:401-403 removes oppA/oppB via ?.remove() with .filter(Boolean) - correctly handles partial create failures.
• Round 3 Important (validate-guard tests): opportunity.collection.test.js:108-122 adds direct assertions on both schema validators. Valid values, invalid enum, non-UUID, and null are all exercised.
• Round 3 minors: ValidationError now passes this as entity context in opportunity.collection.js:39; hasText() replaces Boolean() at line 36; IT test description corrected from "brandId" to "scope" at opportunity.test.js:407.
• The setScopeId(scopeId: string | null | undefined) and setScopeType(scopeType: 'brand' | null | undefined) declarations in index.d.ts:35-36 are backward-compatible relaxations that match runtime behavior and let applyScopeToOpportunity type-check without casts.

Issues

Important (Should Fix)

Co-presence invariant is still bypassable via bulk write paths

• Also posted as inline comment on opportunity.collection.js

The round-3 fix established that co-presence must hold across all write paths. The new save() override covers create(item) and setScopeX(); save(). Three paths inherited from BaseCollection still reach the database without going through either guard:

• updateByKeys(keys, { scopeId: '<other-brand-uuid>' }) patches via entity.patch(keys).set(...).go(), which never instantiates the model or calls create(). A caller can also use this to re-attribute an existing opportunity to a different brand's scopeId without touching scopeType.
• createMany([...]) iterates raw items through entity.put(item).params() and does not call this.create(). Per-attribute validators run; co-presence does not.
• saveMany(items) / _saveMany(items) calls entity.put(updates).go() directly, bypassing model instantiation entirely.

How to fix - two options, both in-scope for this repo:

Option a (preferred): mark scopeType and scopeId as readOnly: true in opportunity.schema.js. The base model's auto-generated setters are skipped for readOnly attributes, so post-create mutation is impossible by construction. Co-presence enforcement then lives only in create(), closing all three bulk paths at once and removing the need for the save() override. This was round-3's option (a); the author chose option (b), but option (b) demonstrably requires three more overrides to be complete.

Option b: override createMany and updateByKeys on OpportunityCollection with the same hasText(scopeType) !== hasText(scopeId) guard. Note that updateByKeys must account for the post-update merged state, which requires reading the existing record - meaningfully more complex than option (a).

---

Data-service image still tag-pinned; PR description missing deployment prerequisite

• Also posted as inline comment on docker-compose.yml

What the new commit fixed: the comment at lines 18-19 explains the bump rationale and the override mechanism. Useful for developers running tests locally.

What remains open: (a) the image is still pinned by mutable tag (:v5.1.1). ECR tags can be re-pushed - if v5.1.1 is ever updated upstream, CI will silently test against a different binary and git log will not identify the change. (b) The PR description still does not mention that mysticat-data-service v5.x is a deployment prerequisite. A docker-compose comment is read by engineers running tests; it is not read by engineers sequencing a deploy. If this code lands in an environment still on a pre-v5 data-service, the failure at allByScope or scope writes will not point back to this PR.

How to fix: (a) pin by digest: image: ...:v5.1.1@sha256:<digest> (Compose supports tag-plus-digest; the tag stays readable). Keep MYSTICAT_DATA_SERVICE_TAG override for local dev. (b) Add one sentence to the PR description: "Requires mysticat-data-service v5.x (adds scope_type/scope_id columns and the scopeId/scopeType GSI). Confirm target environment is on v5.x before deployment."

---

Missing regression test for the clear-then-save transition

• Also posted as inline comment on opportunity.model.test.js

The four existing save() tests cover static quadrants - all starting from an unconfigured instance. The most realistic misuse pattern (a caller loads a fully-scoped record, clears one field with setScopeId(null), then saves) is not covered. The override handles this correctly - hasText(null) === false fires the XOR - but a future refactor that short-circuits on "unchanged fields" could silently break the guard with the current test set.

How to fix: add a fifth test case - pre-set instance.record.scopeType = 'brand' and instance.record.scopeId = '11111111-1111-1111-1111-111111111111', call instance.setScopeId(null), assert instance.save() rejects with the same ValidationError message.

Minor (Nice to Have)

save() ValidationError message carries no record identity

File: opportunity.model.js:52-55

The message is 'scopeType and scopeId must both be set or both be absent'. The entity instance is passed as the second arg so a handler that unpacks ValidationError.entity gets full context - but anything that logs only err.message produces an undiagnosable line in production. Including this.getId() plus the current field values in the message string is a one-line improvement.

---

Defensive item || {} in OpportunityCollection.create() obscures intent

File: opportunity.collection.js:34

super.create(null) would fail at the base collection level anyway. The || {} means a null item passes the co-presence check (both fields absent, hasText returns false for both) then fails one frame deeper with a generic base-class error rather than the co-presence message. Either remove || {} and rely on the base class, or add an explicit early null check with a clear ValidationError.

Recommendations

• If readOnly: true is adopted (option a above), the save() override and its four unit tests can be removed, simplifying the surface. The schema validators and create() guard remain and are sufficient.
• A one-line JSDoc note on setScopeType and setScopeId that both must be set or cleared together would surface the contract to consumers before they hit the runtime error. The current null | undefined TS overloads imply "you can clear one."

Assessment

Ready to merge? No, with fixes.

Reasoning: The save-path co-presence fix is correct and the addressed round-3 items are solid. The co-presence invariant is now documented in three places and enforced on two of an estimated five write paths - a state that is harder to reason about than either "always enforced" or "explicitly deferred." Completing option (a) or option (b) before the companion audit-worker PR lands removes a real batch-ingestion risk without requiring significant rework.

Next Steps

1. Address the bulk write gap - readOnly: true on both schema attributes (preferred) or override createMany and updateByKeys on OpportunityCollection.
2. Pin the data-service image to a digest and add a one-sentence deployment prerequisite to the PR description.
3. Add the partial-clear regression test.
4. Minor items are optional.

[rarescheseli]

This create() override guards a single write entry point. Three paths inherited from BaseCollection still bypass both this guard and the new Opportunity.save() override:

• updateByKeys(keys, { scopeId: '<other-brand-uuid>' }) - patches via entity.patch(keys).set(...).go() without model instantiation. Can also re-attribute an existing opportunity to a different brand's scopeId without touching scopeType.
• createMany([...]) - calls entity.put(item).params() per item, does not delegate to this.create().
• saveMany(items) / _saveMany(items) - calls entity.put(updates).go() directly.

Preferred fix: mark scopeType and scopeId as readOnly: true in opportunity.schema.js. Post-create mutation becomes impossible by construction, co-presence only needs to live in create(), and all three bulk paths are closed at once - without adding more overrides.

[rarescheseli]

The comment added in this commit explains the bump policy for developers running tests locally - that is an improvement. Two gaps remain:

1. The tag :v5.1.1 is mutable. If it is ever re-pushed in ECR, CI will silently test against a different binary. Pin by digest: image: ...:v5.1.1@sha256:<digest> (Compose supports tag-plus-digest; the tag stays human-readable).
2. The PR description still does not note that mysticat-data-service v5.x is a deployment prerequisite for this code change. A docker-compose comment is read by engineers running tests; it is not read by engineers sequencing a deploy. Please add one sentence to the PR body: "Requires mysticat-data-service v5.x (adds scope_type/scope_id columns and the scopeId/scopeType GSI). Confirm target environment is on v5.x before deployment."

[rarescheseli]

The four tests here cover static quadrants (neither-set, scopeType-only, scopeId-only, both-set) all starting from an unconfigured instance. The most realistic misuse pattern is missing: loading a fully-scoped record and clearing one field before saving.

Please add a fifth case:

it('throws ValidationError when a fully-scoped record has scopeId cleared', async () => {
  instance.record.scopeType = 'brand';
  instance.record.scopeId = '11111111-1111-1111-1111-111111111111';
  instance.setScopeId(null);
  await expect(instance.save()).to.be.rejectedWith(
    'scopeType and scopeId must both be set or both be absent',
  );
});

The override handles this correctly today (hasText(null) === false), but this test locks in the behavior against future refactors.