fix(mercury): verify exact provider organization, account and canonical Finance tenant

[chitcommit]

Context

Existing ChittyConnect PR #343 cannot safely activate Mercury reconciliations until the ChittyFinance consumer has authoritative account attribution. Live read-only audit found 28/42 Mercury-labeled Finance accounts without external_id, including all 21 ARIBIA LLC - MGMT accounts, with an FC transaction booked to a conflicting Finance account. Names and last-four digits cannot be treated as binding evidence.

Scope

• Add global, bounded read-only lookup of exact external Mercury account IDs in existing ChittyFinance SystemStorage; two matches are an ambiguity, never first-wins.
• Add private GET /internal/chittyconnect/mercury/account-binding to verify existing tenant UUID, explicit canonical business identity, active account, exact provider account ID, and existing metadata for the Mercury organization ID. Preserve FC/CHIT organization boundaries despite shared canonical MGMT identity.
• Return explicit unbound for zero rows; reject duplicate, inactive, wrong-tenant, provider-org-provenance-missing, malformed, or query-failure cases. Never auto-provision a tenant, account, or transaction.
• Mutation tests prove read-only access, private service-binding isolation, wrong owner/provider rejection and fail-closed provider/Finance read failures.

Release gates

This PR is a preflight guard, not an ingest consumer, not a bookkeeping mutation and not evidence that three missing FC source IDs are reconciled. The ChittyConnect Finance /mercury-reconciliation/readiness and /consume endpoints must independently provide a real durable consumer before enabling PR #343 dispatch or registering CHIT/MNW webhooks. Production ledger writes stay operator-gated with exact source ownership, idempotency, audit receipt and rollback evidence. No new database, table, key store, binding, queue or provider write.

Existing work: ChittyFinance #203; chittyconnect-finance #21 and #22; ChittyConnect #343.

[coderabbitai]

Note

Currently processing new changes in this PR. This may take a few minutes, please wait...

⚙️ Run configuration

• Configuration used: defaults
• Review profile: CHILL
• Plan: Advanced
• Run ID: a8df5a5a-5d43-40d9-91c5-a4a9245cb919

---

📥 Commits

Reviewing files that changed from the base of the PR and between 1ff6229 and 40c5507.

---

📒 Files selected for processing (3)

• server/__tests__/internal-chittyconnect.test.ts
• server/routes/internal-chittyconnect.ts
• server/storage/system.ts

---

 ______________________________________________________________________________________________________________________________________________________________________
< Fix the problem, not the blame. It doesn't really matter whether the bug is your fault or someone else's - it is still your problem, and it still needs to be fixed. >
 ----------------------------------------------------------------------------------------------------------------------------------------------------------------------
  \
   \   \
        \ /\
        ( )
      .( o ).

✨ Finishing Touches

📝 Generate docstrings

• Commit to this branch
• Create a new PR

---

🧪 Generate unit tests (beta)

• Commit to this branch
• Create a new PR

---

• Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

---

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.}

[claude]

Review. A tight, fail-closed, read-only guard. Scope matches the description and the tests cover the main rejection paths. Notes by importance:

Issues / suggestions

1. Name fallback weakens the identity check ((identityInMetadata ?? t.name) !== canonicalBusinessIdentity). The PR says names can't be binding evidence, yet a tenant lacking metadata.canonicalBusinessIdentity is verified by name alone. If the intent is explicit canonical identity, require the metadata field and 409 when absent. If the fallback is deliberate, document it. The fallback path is untested.
2. The lookup isn't scoped to Mercury. getMercuryAccountBindingsByExternalId filters only on external_id. A non-Mercury account with the same external ID would cause a spurious ambiguity (409), or crowd out the real row under limit(2). Filter on institution in SQL or rename the method. The route-level check can stay as defence in depth.
3. getTenants() loads every tenant to find one. That is O(tenants) per call. A by-id lookup is cheaper and removes the exactTenants.length !== 1 handling.
4. Dead branch. rows.length > 2 can't happen because of .limit(2). Also confirm accounts.external_id is indexed, since the lookup is global.
5. The catch swallows errors silently. The sanitized 503 is right, but add a server-side log (no secrets) so failures can be diagnosed.
6. Naming. mercuryAccountUuid also validates org, tenant and Finance account IDs. uuidPattern would be clearer. Confirm no legacy non-UUID tenant IDs need to pass.

Test gaps

• No test for an inactive account, an inactive tenant, or a non-Mercury institution. All are explicit branches.
• No test for conflicting provenance keys (e.g. mercuryOrganizationId matches but mercuryOrgId differs), or for case-insensitive org matching.
• Only organizationId is tried as malformed. Cover accountId and tenantId too.
• The ambiguity test uses stub rows like { id: 'another' }. Use full rows so it stays meaningful if validation order changes.

Security. Private-origin gating is tested, reads are bounded, errors don't leak internals, and there are no mutation paths. No concerns beyond item 1.

Nothing is blocking except possibly item 1, depending on intent.

[claude]

Review of #205

The design is sound: it is read-only and fail-closed, treats duplicates as ambiguity, and keeps error bodies generic (the test confirms private-db-secret doesn't leak). The route and the storage method follow the repo's storage-layer convention. I have no blocking concerns. Suggestions:

Correctness / robustness

1. Org provenance is weaker than the docs imply. server/routes/internal-chittyconnect.ts accepts mercuryOrganizationId, mercuryOrgId and providerOrganizationId, and passes if any are present and all agree. That is conservative, but nothing writes these keys today, so it's worth settling on one canonical key. Otherwise a later writer can introduce a second spelling that quietly changes behavior.
2. Tenant identity falls back to t.name when metadata.canonicalBusinessIdentity is absent. The PR says names can't be binding evidence. Names are admittedly the only identity for tenants without metadata. If that fallback is intentional, document it. Otherwise require the metadata field.
3. rows.length > 2 is unreachable because of .limit(2). It's harmless as a defensive check, but it reads as if the limit might not hold.
4. The storage query doesn't filter on institution. A non-Mercury account that shares the external ID will make the lookup return 409 "ambiguous" instead of being ignored. Failing closed is acceptable. Still, consider filtering to Mercury in SQL, or say in the doc comment that this is deliberate. The method is named getMercury… but isn't Mercury-specific.
5. The account id UUID check comes last and returns 503. Malformed stored data is a data-integrity problem, not an availability one, so 409 would be more accurate. It's also cheap to run earlier.

Performance

• storage.getTenants() loads every tenant on each call just to find one. A getTenant(id) lookup (one probably exists) would be cheaper and wouldn't depend on full-table coverage. Is there an index on accounts.external_id? The lookup is global, so one is needed to avoid a seq scan.

Tests
Covered well: success, origin spoofing, unbound, duplicate, wrong tenant, wrong org, missing metadata, tenant conflict, invalid IDs, and DB failure. Missing:

• an inactive account (isActive: false) and an inactive tenant
• a non-Mercury institution
• the mercury bank variant and mixed-case org IDs (the code lowercases them)
• conflicting values across the multiple org-metadata keys
• non-array tenants or rows returning 503
• the 400 path for a missing canonicalBusinessIdentity or one over 200 chars

The tests use mocked storage only. The SQL in getMercuryAccountBindingsByExternalId (filter plus limit) is untested. Consider one integration test if a test DB is available.

Minor

• The regex mercuryAccountUuid is used for org, tenant and account IDs. A more general name such as uuidPattern would fit better.

Overall this looks good to merge once the nits are considered.

[claude]

Review: Mercury account-binding preflight

Overall this is a tight, fail-closed, read-only design. It follows the repo conventions: DB access goes through SystemStorage, there are no mocks, and errors don't leak internals. I didn't run the tests or typecheck. This review is from reading the diff only.

What looks good

• The .limit(2) lookup treats duplicates as a conflict instead of taking the first row.
• The tenant UUID is checked against explicit metadata.canonicalBusinessIdentity, not the display name.
• Every ID is validated as a UUID before any DB read, and the tests assert no storage call happens on rejection.
• The 503 path logs only the error class, and the test checks the response body doesn't leak the DB error text.

Suggestions

1. Dead branch. rows.length > 2 (internal-chittyconnect.ts) can never be true, because the query has .limit(2). Either drop it or comment it as a defensive check against a storage-layer change. It also returns 503 while rows.length > 1 returns 409, which reads inconsistently.
2. No DB-level uniqueness. The guard only detects duplicate external_id values at read time. Consider a follow-up partial unique index on accounts(external_id) (where not null, and scoped by institution if needed) so duplicates can't be created. This needs the operator gate described in CLAUDE.md before running db:push.
3. Lookup isn't scoped to Mercury. getMercuryAccountBindingsByExternalId filters only on external_id, so a Wave or Stripe account that reuses the same string returns 409 as "ambiguous". That's safe, but the method name promises Mercury-only. Either filter on institution in SQL, or rename or document it. The institution allow-list is also applied only after the tenant check, in the route. Comparing 'mercury' / 'mercury bank' in the route is fragile, so consider a shared constant.
4. Metadata prerequisite. The endpoint depends on tenant.metadata.canonicalBusinessIdentity and account.metadata.mercuryOrganizationId. I saw nothing in this PR that populates them. Please confirm which existing writer or operator process sets them. Otherwise everything will 409 until a backfill happens, and the PR should say so.
5. Ordering and information. The tenant check runs before the account lookup. That's good for isolation, but a caller can distinguish finance_tenant_identity_conflict from finance_account_binding_conflict. That's acceptable for a private service-binding route. Just confirm the internal guard on this router is applied to the new path. The origin tests cover it, which is reassuring.
6. Test gaps.
   • There's no test for 'Mercury Bank' or case-insensitive org matching.
   • There's no test for several provenance keys that all agree (mercuryOrganizationId plus providerOrganizationId).
   • There's no test for a non-UUID account.id (the 503 branch).
   • There's no test for getTenant returning null, or for getTenant throwing.
   • The duplicate-row test uses id: 'another' with no tenant, so it would pass for the wrong reason if the ambiguity check moved after the account validation. Use realistic rows.
   • The tests mock storage, so the actual Drizzle query (where plus limit) is never exercised. A small integration test or query-shape assertion would help.
7. Nit. The response echoes organizationId from the query rather than the stored value. They are equal after the case-insensitive match, but returning the stored value (or lowercasing both) avoids case drift for downstream consumers.

No blocking security issues found. Items 1, 4 and 6 are the ones I'd address before merge.

[claude]

Review

Overall this is a well-scoped, fail-closed read-only guard. The tenant, institution, active-flag and org-provenance checks are sound, and the tests assert that the guard does not read ledger rows before the origin check passes. Notes below, roughly by importance.

Correctness / bugs

1. Case-sensitivity mismatch can turn a bound account into unbound. The UUID regex is /i, and the org comparison lowercases both sides. The storage lookup eq(accounts.externalId, externalId) is an exact, case-sensitive match. A caller sending an uppercase account UUID gets a 200 unbound for an account that is actually bound. The account.externalId !== externalAccountId check has the same problem. unbound is the signal that lets a caller go on to provision or reconcile, so this is not a safe failure direction. Lowercase externalAccountId, organizationId and tenantId right after validation, or reject non-lowercase input. Add a test for it.
2. The global lookup is not scoped to Mercury. getMercuryAccountBindingsByExternalId filters only on externalId. A non-Mercury account (Wave, Stripe, etc.) that happens to share an external ID, or a Mercury row from another tenant, makes the result a 409 "ambiguous". Failing closed is acceptable, but the method name promises Mercury. Either filter on institution in SQL (case-insensitively, to match the mercury / mercury bank list used in the route) or rename the method. Filtering in SQL also keeps limit(2) meaningful.
3. Dead branch. .limit(2) means rows.length > 2 can never be true. Change it to a check that is reachable, or drop it. The 503 "coverage unavailable" there currently has no test and no way to trigger.

Design / hardening

• The guard depends on the data having no duplicates, but nothing in the schema enforces it. Consider a follow-up with a partial unique index on accounts(external_id) where external_id IS NOT NULL (or on (institution, external_id)). Per CLAUDE.md, any db:push needs the operator gate and a snapshot, so keep it out of this PR.
• Org provenance accepts three metadata keys (mercuryOrganizationId, mercuryOrgId, providerOrganizationId). That is a reasonable fail-closed check, but write down which key is canonical so writers don't drift.
• getTenant runs before the account lookup, so any caller that passes the internal gate can probe whether a tenant UUID exists and is active (409 vs 200). This is low risk behind the private service binding. It is worth noting that the 409 error codes distinguish the failure reasons.

Tests

• Missing coverage: institution: 'Mercury Bank', the mercuryOrgId / providerOrganizationId aliases on their own, a non-UUID account.id (the 503 path), and the uppercase-UUID case from item 1.
• The storage method has no test of its own. The route tests mock it, so the SQL filter and limit are never exercised. A small test against the storage layer would cover that.
• The duplicate-rows fixture uses id: 'another'. It works, but a realistic row would be clearer.

Security

• Origin and service-binding isolation are covered. Error responses do not leak details, and logging records only the error class.

Item 1 should be fixed before merge. The others are suggestions.

[chatgpt-codex-connector]

You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard.