Skip to content

fix(mercury): authoritative global source-ownership preflight in Finance - #203

Merged
chitcommit merged 3 commits into
mainfrom
fix/mercury-global-source-owner-20261012
Oct 12, 2026
Merged

chitcommit merged 3 commits into
mainfrom
fix/mercury-global-source-owner-20261012

Conversation

@chitcommit

@chitcommit chitcommit commented Oct 12, 2026 •

Copy link
Copy Markdown
Contributor

Production defect

The merged chittyconnect-finance source-owner preflight requires ChittyFinance SOT lookups across every tenant before FC/CHIT reconciliation. The deployed Finance read projection only exposed tenant-scoped transaction listings. Per-tenant getTransactionByExternalId cannot establish absence of an external Mercury ID in another tenant.

Changes

  • Read-only SystemStorage lookup by exact mercury:<id> external source identity across all tenant partitions, returning at most two owners.
  • Private GET /internal/chittyconnect/mercury/source-owner/:sourceIdentity through the existing ChittyConnect→Finance service-binding security boundary; no public access.
  • Exact owner {tenantId, financeAccountId} or owner:null on a complete empty result, 409 on duplicates, 503 on database failures/incomplete identity, 400 on malformed source identifiers.
  • Regression tests for source identity, zero/one/multiple matches, caller isolation and fail-closed behavior.

Guardrails

No new schema, database, queue, KV keys, credential bindings, financial writes, or public route. This patch does not implement the separate missing acceptMercuryReconciliationIntent RPC or claim production webhook repair. Connect must consume this read projection only after a valid Mercury activity gate. Existing seven webhooks remain untouched.

Validation

CI typecheck and focused server/__tests__/internal-chittyconnect.test.ts required. Confirm deployment and private broker visibility separately after merge. Do not assert end-to-end reconciliation until durable consumer is integrated and receipt verified.

Summary by CodeRabbit

  • New Features
    • Added a private lookup that identifies the ChittyFinance account associated with a Mercury transaction. It reports when there is no match and handles ambiguous or unavailable lookup results without exposing storage details.

@coderabbitai

coderabbitai Bot commented Oct 12, 2026

Copy link
Copy Markdown
Contributor

Review in Change Stack →

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: 3c27478d-77b6-4b7e-9dde-298aac29bda6

📥 Commits

Reviewing files that changed from the base of the PR and between 331b4b0 and 8d271ad.


📒 Files selected for processing (3)
  • server/__tests__/internal-chittyconnect.test.ts
  • server/routes/internal-chittyconnect.ts
  • server/storage/system.ts

 ___________________________________________________________________________
< This code is one line away from greatness-and 40 lines away from clarity. >
 ---------------------------------------------------------------------------
  \
   \   (\__/)
       (•ㅅ•)
       /   づ
✨ 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

Comment @coderabbitai help to get the list of available commands.

@chatgpt-codex-connector

chatgpt-codex-connector Bot commented Oct 12, 2026 •

Copy link
Copy Markdown

Codex Review Summary

This comment shows the latest Codex review activity on this pull request.

Review Status Commit Review trigger
📝 Code Review ✅ Completed 2026-10-12T05:40:30.184436Z 8d271ad PR opened
ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review" or "@codex security review".

Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings.

@claude

claude Bot commented Oct 12, 2026

Copy link
Copy Markdown

Review

Overall: a small, well-scoped, read-only change. Fail-closed handling is sensible, and the 404 test for the public host and wrong caller is good. A few things to look at before merge.

Performance

  • transactions.external_id has no index (database/system.schema.ts, the transactions table indexes cover tenant, account, date, property and COA only). The new cross-tenant WHERE external_id = ? is a sequential scan on the largest table, and it sits on the reconciliation preflight path. As the PR says "no new schema", an index (ideally partial, WHERE external_id LIKE 'mercury:%', or a plain btree) needs a separate, operator-gated db:push per CLAUDE.md. Please at least call this out or file a follow-up.

Correctness

  • Same-owner duplicates return 409. limit(2) returns two rows when the same (tenantId, accountId) booked the ID twice, and the route reports finance_duplicate_source_ownership. If that is intended (fail closed on any duplicate), fine. If the meaning is "conflicting owners", dedupe by owner first. No unique constraint backs this up either.
  • Dead branch: rows.length > 2 can't happen with .limit(2). It's harmless defensive code, but it also means the 503 "coverage" path is untested. owner with blank tenantId or financeAccountId is likewise untested. accountId may be nullable in the schema, so a legitimate row with a null account would become a 503. Please check that this is what you want.
  • The regex is duplicated in the route and in storage. Export a shared constant or validator. The accepted charset ([A-Za-z0-9-]) should also be confirmed against real Mercury IDs, which are UUIDs and so are fine, but if prefixed IDs ever contain _ they would 400.

Style

  • The new JSDoc was inserted between the existing "System-mode projection used by ChittyConnect…" comment and the /entities route it describes, so that comment is now orphaned from its handler. Move the new block above the old comment.
  • Per the CLAUDE.md convention, all DB access goes through system.ts, and it does here.
  • The bare catch {} swallows the error with no log. Consider logging a redacted error (without connection details) so a 503 is diagnosable.

Security

  • Gating relies on the existing internal-host and X-ChittyOS-Caller check, which the tests cover. Note that a header check alone isn't authentication, so confirm the service-binding boundary is what actually enforces access. The response leaks only tenant and account UUIDs to ChittyConnect, which is acceptable for this purpose.

Tests

  • Good coverage of found, missing, 400, 409, 503 and the public-host 404. Missing: the same-owner duplicate case, a null accountId, and a malformed-but-URL-encoded ID such as a path-traversal or oversized (257 character) value. There is no test of the actual Drizzle query. Storage is mocked, so the eq/limit(2) behavior isn't exercised.

@chitcommit
chitcommit merged commit 1ff6229 into main Oct 12, 2026
11 of 12 checks passed
@chitcommit
chitcommit deleted the fix/mercury-global-source-owner-20261012 branch October 12, 2026 05:38

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 8d271ad017

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

Comment thread server/storage/system.ts
Comment on lines +293 to +294
.where(eq(schema.transactions.externalId, externalId))
.limit(2);

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Badge Add an index for the global external-ID lookup

This new query filters only on transactions.external_id, but the transaction schema has indexes for tenant, account, date, property, and COA fields—not external_id. Consequently, each ownership preflight can require a full scan across every tenant, and the common owner: null case must scan the entire transaction table despite limit(2). Add an index on external_id (ideally scoped appropriately to Mercury IDs) before putting this lookup on the reconciliation path.

Useful? React with 👍 / 👎.

Comment thread server/storage/system.ts
Comment on lines +288 to +290
.select({
tenantId: schema.transactions.tenantId,
financeAccountId: schema.transactions.accountId,

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P1 Badge Validate the account belongs to the reported tenant

When an inconsistent transaction row exists, this projection combines transactions.tenant_id and transactions.account_id into a supposedly authoritative owner without verifying that the referenced account has the same tenant. Such rows are possible because account_id has only a standalone foreign key and createTransaction does not enforce the tenant/account pairing; the endpoint would therefore return a cross-tenant owner instead of failing closed, allowing reconciliation to attribute the Mercury source to a contradictory financial identity. Join the account on both ID and tenant, or otherwise reject mismatches.

Useful? React with 👍 / 👎.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant