fix(mercury): fail closed on missing durable reconciliation consumer

[chitcommit]

Confirmed production defect

Merged PR #201 is deployed and acknowledges organization-scoped Mercury events as 202 after writing an expiring FINANCE_KV marker, but no deployed consumer is known to drain webhook:mercury:org:* or drive the canonical chittyconnect-finance reconciliation adapter. This permits silent loss of bank activity.

Bounded containment

• Keep all seven legacy tenant-scoped Mercury receivers untouched. No provider, finance, secret or schema mutation.
• Continue native per-provider-org ChittyConnect→ChittySecrets signature verification.
• Require an authenticated private ChittyConnect RPC to return a broker-attested durable acceptance receipt, checked for organization and idempotency-key identity before acknowledging a valid organization-scoped event.
• Return retryable 503 if the method, its durable consumer, or receipt is absent, invalid, or unavailable.
• Require a new acceptance even if a legacy Finance KV marker exists. No 2xx from KV alone or best-effort asynchronous audit.
• Reuse current KV idempotency key only after actual durable acceptance. A receipt means dispatch only, never completed ledger reconciliation.
• Regression tests cover broker unavailable, malformed/non-durable receipts, broker exceptions, legacy markers, KV read/write partial failures, and success/duplicate behavior. Failures log only organization/event IDs and error class, never provider content.
• The receipt is trusted only to the degree that the private service binding and its future consumer implementation are trustworthy; these field checks do not independently prove an actual durable downstream write. The private RPC is not yet implemented, so this PR intentionally returns HTTP 503 in current production if an organization-scoped event arrives. Do not interpret this PR as end-to-end remediation.

Deployment gate

Do not migrate/register CHIT, MNW, or the other Mercury webhooks until the canonical chittyconnect-finance consumer, private broker method and ChittySecrets org-scoped custody are actually live and verified. Merging this fail-closed guard does NOT create the consumer or repair a webhook. Changes to prod financial data require separate operator gate.

Finance Operating Defaults: no new schema/store/ledger/checkpoint/tracker, no raw secrets, no financial writes. Run tests + typecheck + CodeQL and inspect diff before merge.

[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: a8196a6c-8a1d-4f83-ad88-2aab943a0cd0

---

📥 Commits

Reviewing files that changed from the base of the PR and between 18340da and 07412f1.

---

📒 Files selected for processing (3)

• server/__tests__/webhooks-mercury.test.ts
• server/books/webhooks.ts
• server/env.ts

---

 ________________________________________________
< I raised Series-B funding to review your code. >
 ------------------------------------------------
  \
   \   (\__/)
       (•ㅅ•)
       / 　 づ

✨ 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

The fail-closed approach is right. Keeping the seven legacy receivers untouched, and not letting a KV marker count as proof of dispatch, closes the silent-loss path. I didn't run tests or typecheck.

Issues

1. The receipt isn't independently verifiable. The PR description says the receipt must be "independently verifiable". The code only checks that fields match (accepted, durable, organizationId, idempotencyKey, non-empty receiptId). Those come from the same RPC response, so a buggy or stubbed broker that returns {accepted:true, durable:true, ...} would still get a 2xx. If the binding is trusted, say so in the comment. If not, add a signed or MAC'd receipt, or a lookup by receiptId.
2. No timeout on the broker RPC (server/books/webhooks.ts, the acceptIntent.call). A hung ChittyConnect call ties up the Worker until the platform limit. Mercury will probably time out and retry anyway. An explicit AbortSignal/Promise.race budget would give a deterministic 503.
3. The catch {} drops the error. There's no ledgerLog or console line for a broker exception, a missing method, or a non-durable receipt. Because these are now outage signals, log the error class and the org/event IDs (never the body) so the 503s can be alerted on.
4. Input shape isn't validated before use. dispatch?.accepted handles null, but a non-object return or a receiptId of a non-string type only works because of the typeof check. Fine, but a small Zod schema for the receipt would document the contract and match the "Zod for input validation" convention.
5. dispatchReceiptId is echoed in the 202 body and the audit log. This is probably fine, but confirm Mercury and the caller don't need it and that the ID isn't sensitive. The KV marker already stores it.
6. Partial-failure ordering. The broker accepts, then kv.put fails and the request returns 500. Mercury retries and the broker is called again, so the idempotency key has to make that a no-op. That's the intended design, but add a test that covers it, with the broker mock returning the same receipt twice.
7. The duplicate path now calls the broker on every redelivery. That is deliberate. Be aware of the added RPC load on replay storms. Reading KV first would not help, because the marker is no longer trusted.

Tests

• Coverage is good: no method, non-durable, wrong org, empty receipt, wrong key, a throw, and a legacy marker.
• The legacy-marker test asserts kv.put was called once, but that one call is the test's own setup put. It doesn't show the handler wrote nothing. Assert the handler's behavior instead, for example by capturing the call count after setup. Also assert the body error code (mercury_reconciliation_consumer_unavailable).
• Missing cases: accepted: false on its own, a null/undefined return, and a kv.put failure after acceptance.

Nits

• The comment edit "Never persist signed content, the mergePatch, or signing key" is vaguer than the original ("signed event body, mergePatch or signing secret"). I'd revert it.
• The inline type for dispatch duplicates the env.ts return type. Export a shared MercuryDispatchReceipt type.

Security

• No new secrets, schema or writes.
• Org binding and idempotency-key binding are enforced, which is good.
• Item 1 is the main gap.

Overall this is a reasonable containment patch. I'd address items 1, 3 and 6 before merge. The rest can follow.

[claude]

Review: fail-closed Mercury org webhook

Overall the change does what it says. It is a small fail-closed guard, it never acknowledges on KV alone, and it keeps the legacy receivers untouched. I'd merge it after the points below. I only read the diff; I didn't run the tests or typecheck.

What's good

• The receipt is checked for accepted, durable, org ID, idempotency key and a non-empty receiptId. Misbound receipts are rejected.
• Every failure path (missing method, broker throw, bad receipt, KV read, KV write) returns 503, so Mercury retries.
• Logs carry only org ID, event ID and error class. No payload or secrets are logged.
• The PR description says plainly that this is containment and not remediation.

Suggestions

1. Duplicate deliveries call the broker every time. Because dispatch happens before the KV read, each duplicate delivery makes a private RPC. This is intentional and the tests cover it. It does mean the KV marker no longer saves any work, and it only decides between 200 and 202. Consider noting this in a comment. Later, once the consumer exists, the KV read could be dropped or reduced to a metrics hint.
2. A KV-write failure after a durable acceptance returns 503. Mercury will retry and the broker must dedupe on idempotencyKey. That contract lives only in the env.ts JSDoc ("idempotent"). Say explicitly in the interface doc that repeated calls with the same key must return the same receipt and must not create a second job. The receiptId check can't enforce this.
3. Exposing dispatchReceiptId in the 202 body and the audit payload. It is probably harmless, since only the signed provider sees the body. If broker receipt IDs are meant to be internal, drop it from the HTTP response. Keeping it in the audit log is enough.
4. Type narrowing is weak. dispatch?.accepted is accessed on a value typed as non-nullable. The tests pass null, so the runtime check is right, but typing it as Receipt | null | undefined would make that visible. dispatch.organizationId and idempotencyKey may not be strings either, but strict !== handles that safely.
5. No timeout on the RPC. A hung service binding holds the webhook request until the Worker limits kick in. Consider a bounded Promise.race timeout that maps to the existing dispatch_unavailable 503.
6. Test gaps (minor).
   • There is no test for FINANCE_KV being absent. That branch already exists.
   • The receipt-rejection loop only asserts the 503 and no KV put. It doesn't assert that createTransaction and createAccount were not called.
   • typeof acceptIntent !== 'function' has coverage, but a non-function truthy value doesn't.
7. Repeated 503s are expected, and Mercury will probably disable the webhook after enough failures. The deployment gate in the description covers registration. It may be worth adding an alert on the consumer_unavailable log line so the failures are visible before then.

No security blockers found.

[claude]

Review of #202: fail-closed Mercury org webhook

The fail-closed design is sound, and the PR description is honest that this is containment only. It returns 503 in production until the private RPC exists. I found no blocking issues.

What looks good

• The receipt check covers accepted, durable, org ID, idempotency key and a non-empty receiptId, and it runs before any KV write.
• The typeof acceptIntent !== 'function' guard turns a missing RPC into a clean 503 instead of a TypeError.
• acceptIntent.call(verifier, ...) keeps this bound, which matters for RPC stubs.
• Logs carry only the org ID, event ID and error class.
• The tests cover the main failure modes: no method, a broker throw, each bad-receipt variant, KV read and write failures, and a legacy marker.

Suggestions (non-blocking)

1. KV is now only a replay hint, yet its failures turn a durable acceptance into a 503. Both kv.get and kv.put failures return 503 after the broker has already accepted the event. That causes extra retries, and each one re-calls the broker. The broker is idempotent, so this is safe, but it is wasted work and it makes KV a new availability dependency. Consider treating KV errors as best-effort after a valid receipt, or dropping the kv.get short-circuit entirely, since duplicates must be re-accepted anyway. If you keep the fail-closed behaviour, say in a comment that it is deliberate.
2. Duplicates get 200 and first deliveries get 202. The two paths also differ in the body fields they return: the duplicate response omits reconciliationRequired and dispatchReceiptId. Please confirm Mercury and any monitoring treat both as success. The duplicate path also writes no ledgerLog, so a broker re-accept leaves no audit trace.
3. The receipt check is a structural check only. The PR notes this. A short // TRUST BOUNDARY comment in webhooks.ts would stop a future reader from treating durable: true as verified.
4. The broker call has no timeout. A hung RPC holds the webhook request until the Worker limit. Consider AbortSignal.timeout or Promise.race, so a slow broker returns a quick 503.
5. Type safety. The inline dispatch type duplicates the Env return type. Use Awaited<ReturnType<...>>, or export a named MercuryDispatchReceipt type from env.ts. The dispatch?. check handles null at runtime, but the declared type doesn't allow null.
6. Tests.
   • Add a test for a missing FINANCE_KV (503, no broker call).
   • Add a test that the broker receives the exact idempotencyKey. The success test already does this, so only the verifyMercuryWebhookSignature failure paths are left uncovered.
   • Add a test for a receipt whose receiptId is a non-string.
   • The loop test would be easier to debug with it.each, because a failure there doesn't say which case broke.

Security: I found nothing new. No payload is persisted or logged, and the org ID comes from the verified result. Please confirm the handler compares the verifier's returned organizationId to the path parameter. I saw that only in the unchanged part of the file.

I didn't run the tests or typecheck. This review is from the diff only.

Approving in spirit, with suggestion 1 worth deciding on before merge.

[claude]

Review: fail-closed Mercury guard

Overall this is a sound, narrowly scoped containment. It fails closed, doesn't touch the legacy receivers, never logs payloads, and the tests cover the failure matrix well. I ran nothing locally; this is from reading the diff.

Worth addressing

1. Behavior change for non-envelope signed bodies. Before, a signed body that was unparseable or didn't match mercuryEventSchema returned 200 signature_only. Now it returns 503. The comment says Mercury's VerifyWebhook sends an event envelope, but the PR doesn't show evidence of that. If Mercury's registration or verification ping uses a different shape, webhook registration will fail and Mercury will retry or disable the endpoint. The deployment gate probably makes this moot for now. Still, please confirm against Mercury's docs or a captured payload, and add a test that pins the actual verification payload.
2. Unreachable KV read when it can't change the outcome. kv.get now runs after the broker call and only picks between 200 duplicate and 202. A KV read failure turns an already-durable acceptance into a 503, so Mercury retries and the broker is called again. That is safe because the call is idempotent, but a KV hiccup then costs an extra RPC. Consider treating a read failure as "not seen" and proceeding to the put, since the marker is only a hint. The test does not acknowledge a broker receipt if the KV replay read fails would need to change accordingly. If strictness is intended, a comment saying so would help.
3. Receipt validation is shape-only, as the PR admits. Consider validating the broker response with a small Zod schema (CLAUDE.md prefers Zod for validation) instead of the inline dispatch?.accepted !== true … chain. This also covers a non-object return such as a string, where dispatch.durable currently evaluates to undefined and is rejected safely. Also cap or format-check receiptId, since it is echoed in the response body and written to KV and the audit log.

Minor

• The local dispatch type duplicates the one in server/env.ts. Derive it with Awaited<ReturnType<...>> or export a named type.
• err.name is logged as errorClass. That is fine, but a custom RPC error could carry arbitrary names. Probably OK.
• Add a test that the broker is called with the right this. acceptIntent.call(verifier, …) is correct for RPC stubs, and a test asserting it would guard against a refactor that drops the .call.
• Add a test for a bad receipt on a duplicate delivery where the KV marker already exists. The legacy-marker test only covers the missing-method case.

Security and performance

• No new secrets or schema, and tenancy is unaffected. The idempotency key is derived server-side from the verified organizationId and event ID, and the receipt must echo it back, which is good.
• One extra RPC per delivery, including duplicates. That is intended and acceptable.

I'd approve once item 1 is confirmed.

[claude]

Review

The fail-closed approach is sound and the PR description is honest about its limits. The tests cover the failure matrix well: missing method, a throwing broker, each malformed receipt field, KV read and write failures, and a legacy marker. I didn't run the tests or typecheck.

Behaviour and risks

1. Production returns 503 for every org-scoped event after merge. The PR says so. Mercury will retry and may disable the webhook after repeated failures, so confirm no org-scoped webhook is registered before merging. The deployment gate is the only protection here.
2. KV is now redundant for dedupe. The broker is called on every delivery, duplicates included, and the KV get only picks between 200 and 202. If the broker is idempotent on idempotencyKey, as intended, the marker adds a failure mode (mercury_reconciliation_kv_unavailable) and a cost without adding safety. Consider reading KV only for the response shape, and treating a KV failure as non-fatal once the broker receipt is valid. A durable acceptance followed by a 503 only causes a harmless retry, but it is needless.
3. dispatch?.accepted is checked, but dispatch is typed non-nullable. The null test case works at runtime, and the code stays defensive. Typing the result as T | null | undefined would make that intent explicit.
4. The receipt check is limited to field identity. You acknowledge this in the description. Nothing here validates the shape or length of receiptId. It is echoed into the 202 body, KV and the ledger audit, so a cap on its length or charset would be cheap hardening against a misbehaving broker.
5. The broker call has no timeout. A hung RPC holds the request until the Worker limit rather than returning a clean 503. Consider an AbortSignal or Promise.race with a short deadline.
6. Logging. The logs carry only IDs and the error class, which is good. console.warn on a missing method will fire on every delivery while the consumer is absent, so it may be noisy. That's acceptable as an alert signal.

Minor

• The duplicate path skips ledgerLog. That seems intended, but a one-line comment would help.
• The 503 for an unparseable signed body means Mercury retries a permanently malformed payload indefinitely. That is defensible given the stated intent. A distinct, documented status or alert might be better than silent retry loops.
• The resourceVersion: number | null type in env.ts should be checked against mercuryEventSchema, so the contract and the schema agree.

Tests

The coverage is good. Two additions would help: a test that the broker is called with this bound to the verifier (acceptIntent.call(verifier, …)), and one for a receipt with whitespace-only receiptId. The trim() branch exists but I only see the '' case.

No security concerns found: the signature is verified before any parsing, no payload content is logged, and there are no financial writes.

Approve once the deployment gate is confirmed. Items 2 and 5 are worth considering before or soon after merge.

[chatgpt-codex-connector]

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-11T23:14:58.943457Z	07412f1	Draft marked ready

ℹ️ 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.