fix(brands): reject anchoring a brand to another org's site

[aliciadriani]

Summary

• serenity-docs#346 traced a recurring defect where a brand's organization_id diverges from its own site's organization_id (Tata Capital, BMW, Toyota, KDDI, ...), introduced by hand re-parents/backfills with no guardrail.
• upsertBrand and updateBrand had no check that a new baseSiteId actually belongs to the brand's org before persisting site_id. Adds assertSiteBelongsToOrg(), called from both functions on every path that assigns a new anchor (fresh create / first anchor, and the update-side first-set / pending re-point) — the immutable-once-set branches are unaffected since they already refuse to change an existing site_id.
• Rejects with a typed 409 (brand_site_org_mismatch) instead of silently persisting the mismatch.

Test plan

• npx mocha test/support/brands-storage.test.js — 220 passing, including 2 new tests covering the guard directly (fresh create + update paths) and updated mocks for existing baseSiteId tests
• npm test (full suite) — 16989 passing
• npm run lint — clean

[github-actions]

This PR will trigger a patch release when merged.

[codecov]

Codecov Report

❌ Patch coverage is 93.22034% with 4 lines in your changes missing coverage. Please review.

Files with missing lines	Patch %	Lines
src/support/brands-storage.js	93.22%	4 Missing ⚠️

📢 Thoughts on this report? Let us know!

[MysticatBot]

Hey @aliciadriani,

⚠ Degraded review - no spec document was found for this change (searched the PR links, the touched repos' docs, the architecture/guidelines docs, and linked Jira). This review covers code-level quality but could not validate the change against an agreed design, so confidence is reduced. Add a spec link (PR template section 4) and re-request review for a full-confidence pass.

Verdict: Request changes - two small issues to address before merge; the guard logic itself is correct.
Complexity: MEDIUM - medium diff, single-service validation addition.
Changes: Adds an org-membership guard (assertSiteBelongsToOrg) to prevent anchoring a brand to a site owned by a different organization, rejecting with a typed 409 (2 files).
Note: CI checks are still pending - confirm they pass before merge.

Must fix before merge

1. [Important] updateBrand passes UUID as brandName, producing opaque error messages - src/support/brands-storage.js:1439 (details inline)
2. [Important] Missing rejection test for the pending-brand re-point path (isPending && existing.site_id) - test/support/brands-storage.test.js (details inline)

Non-blocking (2): minor issues and suggestions

• nit: No test for the anchorSiteError DB-error branch (the 4 lines Codecov flagged). A single test with sites: { data: null, error: { message: 'connection reset' } } would close the gap. - src/support/brands-storage.js:362
• suggestion: Consider a DB-level constraint (brands.organization_id must match sites.organization_id for the referenced site_id) as follow-up defense-in-depth to eliminate the theoretical TOCTOU window between the SELECT and the write.

---

_{Skill: pr-review | Model: us.anthropic.claude-opus-4-6-v1[1m] | Duration: 3m 29s | Cost: $7.12 | Commit: 4538d50f9821ae8c21ebf6af73b87543f2cfcf47
If this code review was useful, please react with 👍. Otherwise, react with 👎.}

[MysticatBot]

issue (blocking): updateBrand passes brandId (a UUID) as the fourth argument to assertSiteBelongsToOrg, but the parameter is named brandName and is interpolated into the user-facing error message: Cannot anchor brand "${brandName}" to site....

This produces messages like Cannot anchor brand "22222222-2222-4222-..." to site abc which are unhelpful for operators triaging 409s in logs.

Fix: Add name to the existing fetch in updateBrand (change .select('site_id, status, updated_at') to .select('site_id, status, updated_at, name')) and pass existing?.name || brandId as the fourth argument. This aligns both call sites.

[MysticatBot]

issue (blocking): The updateBrand guard fires on two sub-conditions: !existing?.site_id (first set) OR isPending (re-point of a pending brand with an existing site). The new rejection test only covers the first sub-condition (site_id: null).

A pending brand that already has a site_id attempting to re-point to a cross-org site is a distinct code path that lacks a rejection test.

Suggested test:

it('rejects re-pointing a pending brand to a cross-org site', async () => {
  const client = createCapturingClient({
    brands: [
      { data: { site_id: 'old-site', status: 'pending' }, error: null },
    ],
    sites: { data: null, error: null },
  });
  const err = await updateBrand({
    organizationId: ORG_ID,
    brandId: BRAND_ID,
    updates: { baseSiteId: 'other-orgs-site' },
    postgrestClient: client,
  }).catch((e) => e);
  expect(err.status).to.equal(409);
  expect(err.code).to.equal('brand_site_org_mismatch');
});

[aliciadriani]

Ran an independent review pass — no Must Fix items (guard placement is exhaustive: only two write sites for brands.site_id in the whole codebase, both now gated; org-scoping is sound; no ordering hazard with the other 409 guards). Pushed fixes for the Should Fix items and nits:

• Added a real-Postgres integration test (test/it/shared/tests/brands.js) covering cross-org baseSiteId on both create and update, using the existing ORG_1/SITE_3(ORG_2) fixtures — the unit suite's mocks alone didn't prove the guard survives a real round-trip. (Note: I couldn't run the Postgres IT suite locally — no Docker in this environment — so this needs a CI run to confirm; I did syntax-check and lint it.)
• Extended the unit-test mock harness to capture .eq() calls so the two guard tests now assert it queried with the actual siteId/organizationId pair, not just that some sites lookup happened.
• Documented brand_site_org_mismatch in the OpenAPI 409 responses for both POST and PATCH, matching the existing "one 409 description enumerates every code" convention.
• Renamed assertSiteBelongsToOrg's last param (brandName → brandLabel) since upsertBrand passes a name but updateBrand passes an id.

Full suite (16993 tests) and lint still green after merging in main's concurrent changes.