fix(qbo): fetch timeouts on every QuickBooks call + pipeline health/digest (pipeline v2 phase 0) - #438
fix(qbo): fetch timeouts on every QuickBooks call + pipeline health/digest (pipeline v2 phase 0)#438Clarion1631 wants to merge 52 commits into
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
VERDICT: REQUEST_CHANGES |
VERDICT: REQUEST_CHANGES |
VERDICT: REQUEST_CHANGES |
VERDICT: REQUEST_CHANGES |
VERDICT: REQUEST_CHANGES |
|
The new monitoring can still report green while payment synchronization is failing.
VERDICT: REQUEST_CHANGES |
VERDICT: REQUEST_CHANGES |
Bare fetch() has no timeout, so today's Intuit API outage hung every QB call until Vercel killed the function at its maxDuration (60s on the receipt-push create, 120s on the payments cron). qbTimedFetch wraps fetch with AbortSignal.timeout (QB_FETCH_TIMEOUT_MS, default 20s) and rethrows our own deadline as QBTimeoutError carrying the URL path only (no query string, no tokens). A caller-supplied signal is combined via AbortSignal.any and still wins; every other error passes through unchanged. Routed through it: exchangeQBCode, refreshQBToken, qbFetch, qbQuery, plus the five other direct fetches in quickbooks.ts (payment link read, payment delete, invoice delete, purchase CDC, invoice send), the Attachable multipart upload in qbo-receipt-push.ts, and the QBO temp-URL attachment download in qbo-receipt-attachments.ts. Both attachment call sites already treat a throw as a non-fatal "failed:<name>". Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Real local http server, not mock.module — mock.module corrupts the require chain on Node 20, which CI pins. Covers: a never-responding server becomes QBTimeoutError; the message carries the path but neither the query string nor a token; success and request init pass through; a connection refusal is NOT relabelled a timeout; a caller's own abort is NOT reported as a QBO outage; QB_FETCH_TIMEOUT_MS drives the default and a garbage value falls back instead of breaking every QB call. Wired into test:unit so CI runs it. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
A QBTimeoutError from either the token fetch or the purchase create is now
audited as reason "qbo-timeout" and answered 503 {ok:false, retry:true,
reason:"qbo-timeout"}. Non-200 is what makes the Apps Script retry on its
next pass, which is right for an outage — a terminal ok:false would send
the receipt down the email fallback for a failure that fixes itself.
Retrying is safe even if a timed-out create actually landed: it carries a
QBO requestid idempotency key and the docNumber pre-check returns
already-exists.
maxDuration 60 -> 30. Two 20s QB deadlines plus the DB work fit; the whole
point is that we now fail long before the ceiling.
Every other branch is unchanged.
Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
One summariser (src/lib/pipeline-health.ts) behind two surfaces, so the on-demand check and the digest can never disagree about whether the pipeline is OK: - GET /api/health/pipeline — Intuit status, last QBO purchase sync, last receipt booked, 24h receipt counts by status, bank-ledger high-water mark, 24h error count. - GET /api/cron/pipeline-digest (0 14 * * *, 7am Pacific) — emails the plain-text summary to PIPELINE_DIGEST_TO (default jadkins@) and, when BOT_HEALTH_CHAT_WEBHOOK is set, posts the same text to Google Chat. Sends every morning: a digest that only arrives on failure is indistinguishable from one that stopped running. Verdict rules, unit-tested: a degraded Intuit indicator or any error in 24h fails; an UNREACHABLE Intuit status page does not (a third party's downtime is not evidence of ours); a gap between 48h and 7d fails because traffic was flowing and stopped, while no pushes in 7d is ok with a note because a quiet week is quiet, not broken. Every read degrades to null/unknown on its own rather than throwing — a health check that 500s during an outage tells you nothing. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Codex blocker 1. fetch() resolves as soon as headers arrive and streams the body afterwards, so a deadline firing mid-body rejected out of res.json() as a raw AbortError — past the wrapper's header-phase catch. The receipt route then classified an outage as a generic transient failure (500) instead of the 503 qbo-timeout it is. The signal stays attached (the deadline must still cut off a stalled body); the returned Response is proxied so json/text/arrayBuffer/blob/ formData translate OUR abort into QBTimeoutError with the same path-only message. Getters and clone() run against the real Response. A caller's own abort during the body read stays a plain error. Codex blocker 7: replaced the AbortSignal.any fallback, which used the caller's signal ALONE and so silently disabled the deadline on any runtime lacking it, with a manual combiner that keeps both live. Tests: headers-then-stall body -> QBTimeoutError; text/arrayBuffer the same; proxy preserves status/headers/clone; caller abort mid-body stays plain; and three cases with AbortSignal.any deleted from scope. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
…eadline Codex blockers 2 and 3. maxDuration back to 60 on qbo-receipts/create: a healthy push does a lot of SERIAL QBO work (lookups, customer/vendor ensures, account verify, create, attachment upload) and the refresh alone is now allowed 45s. 30 would have started killing legitimately slow pushes. qbTimedFetch is what makes the outage case fail fast; the ceiling is only the backstop. refreshQBToken is not safely retryable — Intuit rotates the refresh token during the exchange, so a timeout may already have burned the stored token while we never saw its replacement. Mitigated two ways: its own deadline (QB_REFRESH_TIMEOUT_MS, default 45s, capped at 50s to stay under the route ceiling), and a distinct diagnosable message naming the stranded-token risk. Still a QBTimeoutError, so route classification is unchanged. Persistence order untouched. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Codex blocker 4. Two ways the verdict could report OK while knowing
nothing:
1. A failed DB probe degraded to null/0 and sailed into ok:true — an
unreachable database read as "nothing wrong", which is the most
dangerous output this file can produce. Every probe now carries its own
{status: "ok"|"error"}, and any probe error forces ok:false with reason
probe-failed:<name>. The fallback value is never read as evidence: a
failed stuck probe reports probe-failed, not "0 errors", and the digest
prints "unavailable (probe failed)" rather than a number.
2. "No receipts in 7d -> ok with a note" never expired, so a permanently
dead pipeline reported OK forever. Removed. Now ok:false with reason
no-receipts-72h when the last booked push is older than 72h (or there
is none), and the digest prints how long the silence has actually been
so a human decides whether it is expected.
`reasons: string[]` replaces the single note and is empty exactly when ok.
Judgment call, flagged: an UNREACHABLE Intuit status page still does not
by itself fail the check — it is a third party whose downtime is not
evidence of ours, and failing on it would cry wolf on every statuspage
hiccup. It reports status:"error"/indicator "unknown" and is flagged in
the digest body; our real outage signal is the QBTimeoutError count in
`stuck`. Say the word and I will make it hard-fail.
Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Codex issues 5, 6 and 8. The copied `if (process.env.VERCEL_ENV && ...)` shape only enforced the secret where VERCEL_ENV happened to be set — an all-negative env gate that fails OPEN anywhere it is not (self-hosted, container, drifted preview). New src/lib/cron-auth.ts: authentication required everywhere except an explicit NODE_ENV === "development", and a MISSING CRON_SECRET rejects rather than waving traffic through. Comparison is timingSafeEqual with a length check first (it throws on unequal lengths). /api/cron/pipeline-digest uses isCronAuthorized (dev bypass); /api/health/pipeline's Bearer branch uses hasCronSecret, which has NO environment escape hatch, and its staff-session branch is unchanged. Issue 8: vercel.json is strict JSON and cannot hold a comment, so the note that `0 14 * * *` is 7 AM PDT / 6 AM PST (and shifts an hour across DST, since Vercel cron is UTC-only) lives in the cron route's doc comment. The schedule is unchanged. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
… values Codex round 2. 1. raceAbortSignals latches WHICH signal aborted first, in the handler, and attribution reads that instead of inspecting callerSignal.aborted after the fact. The old check lost a real race: deadline fires, caller aborts a moment later, both read aborted by the time the catch runs, and a genuine outage was reported as a caller cancellation (500, not 503). Handlers are named and every listener is removed once the race is decided, so nothing stays attached to a caller signal that outlives the call. 2. The response proxy also wraps bytes() where the runtime has it, and clone() now returns a recursively wrapped Response. Streamed reads via .body/getReader() still surface the raw abort, noted in a comment — no QBO caller streams (the one getReader() in src is on a user-supplied receipt URL with its own controller, not a QBO response). 3. normalizeTimeoutMs floors to a positive integer and falls back to the default on anything non-finite or < 1, so AbortSignal.timeout can never receive a fraction. 4. Comment only: the route's 60s ceiling can preempt a late refresh, so the stranded-token message is best effort. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
…y-exists Codex gate P1 #1 and #2. parseJsonOrNull replaces `res.json().catch(() => null)` at every QBO body read (vendor create, purchase create, attachment upload, payment create, invoice memo read, invoice send). That catch turned a body-phase QBTimeoutError into "QBO returned no body" — a generic 500 instead of the retryable 503 qbo-timeout, and on the attachment path it could report a successful "attached" for an upload whose response never arrived. Only genuine parse errors resolve to null now; a timeout is rethrown. The already-exists branch no longer returns before the upload. That branch is normally reached because the FIRST attempt's response was lost after QBO committed the Purchase, so its receipt was stranded with no image and every retry took the same early return. It now re-checks the Attachable (by purchase id, filtered to Purchase links) and uploads when missing. Idempotent via a shared deterministic FileName; the result carries attachment: attached | already-attached | skipped | failed:<reason>, which the route now records for both ok branches. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
…reshness Codex gate P1 #3, #4 and P2 #5. runProbe wraps each health probe in a 5s deadline. A throwing query was already handled; a query that never SETTLES was not, so a wedged database hung the health check until the platform killed it — for the cron that means a silent morning with no digest. A timeout reports {status:"error", reason:"timeout"} and forces ok:false like any other probe failure. Digest delivery is no longer best effort: email and Chat run independently under Promise.allSettled with their own 10s deadlines (one failing or hanging can no longer cost the other), and an email that is not accepted returns 500 {ok:false, reason:"email-not-accepted"} so the failure shows in Vercel's cron history instead of a 200 nobody reads. Chat stays optional. The route gains a DI seam so this is testable. lastReceiptPushAt counts status "created" only. "already-exists" is an idempotent re-push of a receipt created earlier, so counting it refreshed the freshness clock with nothing new in the books — a bot stuck retrying one old file looked like a healthy pipeline indefinitely. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
…th; real email check
Codex gate round 3, all four P1s.
1. getFreshQBTokens swallowed a refresh QBTimeoutError and returned STALE
tokens, so the caller spent another full QBO deadline and the 60s ceiling
was still reachable. The policy moved to refreshTokensOrFallBack (what
getFreshQBTokens now calls, same real defaults) and rethrows a timeout.
CHOICE: the stale-token fallback is KEPT for non-timeout failures — that
is the existing intent (an ordinary refresh error can still leave the old
access token valid); only timeouts propagate.
2. /api/health/pipeline was intercepted by NextAuth because the proxy only
bypassed the exact /api/health path, so headless Bearer checks were
redirected to /login. Added an exact-match bypass in both the pattern and
the matcher; the route self-authenticates. No descendant inherits it.
3. email.ts returns {success:true} on a missing RESEND_API_KEY, so the digest
could report emailed:true and 200 while delivering nothing — the failure
disguised as good news. isEmailDeliveryConfigured() fails closed in
production, in the digest route only; email.ts is unchanged for every
other caller. Chat still posts.
4. An attachment QBTimeoutError was turned into failed:QBTimeoutError on an
ok:true response, which the Apps Script treats as FINAL — the Purchase
kept a missing receipt forever and the existing-Purchase recovery never
ran. It now propagates so the route answers 503 and the next pass
attaches. Non-timeout attachment failures keep failed:<reason> + ok:true.
Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
…t it) CI's unit job failed where local passed: "a refresh TIMEOUT propagates" resolved instead of rejecting. Cause is module identity, not logic — under Node 20's CJS/ESM interop quickbooks.ts loaded twice, so the class the test threw was not the class quickbooks-payments.ts compared against and `instanceof` was false, sending the timeout down the stale-token fallback. This is not a test artifact: bundler chunk duplication does the same thing in production, and the failure mode is every timeout branch in the codebase silently taking the non-timeout path — the exact misclassification the deadline work exists to prevent. isQBTimeoutError() accepts either the real class or any Error carrying name === "QBTimeoutError" (set as a class field on every instance), and now backs all six checks: parseJsonOrNull, refreshQBToken, refreshTokensOrFallBack, both attachment paths, and the receipt route. Tested against a foreign duplicate class, and pinned against over-matching. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
…losed on ambiguous creates The issuance-identity work (durable per-issuance keys, payload hashing, stranded-issuance reconciliation, the breakQBInvoiceLink remote-state rework) outgrew Phase 0 and collected seven open gate items of its own. It now lives on feat/qbo-invoice-issuance-identity as a stacked draft PR, and Phase 0 goes back to a shape that can ship. REMOVED from Phase 0 (all of it preserved on the stacked branch): - qbIssuanceKey / qbIssuancePayloadHash columns, their migration and apply script — Phase 0 needs NO schema change again - ensureIssuanceKey, issuancePayloadHash, issuanceMatchesPayload, clearIssuanceIfUnlinked, reconcileStrandedIssuance, reconcileIssuedInvoice - the createQBMilestoneInvoice requestid/idempotencyKey and the extra fields it returned for reconciliation - the progress-billing issuance and drift changes - the breakQBInvoiceLink rework (back to main's shape) REPLACED BY one small fail-closed guard, using the EXISTING qbSyncError column: when an invoice create ends with an unknown outcome — a timeout, or a transport failure after the request went out — the row is marked "ambiguous-create" and the send path refuses to re-send while that marker is present, with a typed error telling the operator to check QuickBooks and clear it via the existing unlink flow (which already nulls qbSyncError). A business refusal (4xx) is NOT ambiguous: QuickBooks answered "no" and created nothing, so those rows stay freely re-sendable. Milestones and progress billings both. That is strictly weaker than the issuance work — it prevents the duplicate bill rather than recovering from it — but it is correct, needs no schema, and does not block Phase 0. KEPT in Phase 0: the whole deadline/budget effort, gate item 6 (one deadline through payment creation and estimate/invoice sync, with cumulative-budget tests) and item 7 (qbo-maintenance reporting the run rather than the request). Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
VERDICT: REQUEST_CHANGES |
Codex gate round 27, items 1 and 2. Items 3 and 4 are NOT done — see the report. 1. The pay-link read is a remote call, and a timeout there abandoned a real, created invoice: the row still said unlinked, so the next send made a second one. qbInvoiceId is now persisted immediately after the create returns, under the same content-snapshot CAS the final write uses, BEFORE the pay-link fetch. A pay-link failure then leaves a linked row marked "paylink-pending" for the maintenance sweep to finish — the invoice is correct, only the convenience link is missing, so it is a success rather than something the operator must fix. This also required repinning the final link claim: it demanded `qbInvoiceId: null`, which after the early write would miss every time and compensate away the invoice it had just created. It now pins the id we wrote. 2. A process killed between the POST and the link write left NO trace, so the next send saw a clean row and created a duplicate. The row is now CAS-claimed `qbSyncError: null -> "create-in-flight"` BEFORE the request goes out; losing that CAS refuses the send (someone else is already sending). A definitive 4xx refusal releases the claim, since QuickBooks created nothing and the row is freely re-sendable; an unknown outcome promotes it to "ambiguous-create". The marker write is no longer swallowed — if it cannot be written the send aborts, because an unwritten marker is exactly the invisible-crash case it guards. An in-flight marker older than 5 minutes is reported as an unknown outcome rather than an active peer. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
…uards Codex gate round 27, items 1 and 2, applied to the second create path. The milestone push was fixed in 4a57a43; progress billings still carried the old shape, so the same two duplicate-bill routes were live for them. - The row is CAS-claimed `qbSyncError: null -> create-in-flight` BEFORE the POST goes out, and the write is no longer swallowed: an unwritten marker is exactly the invisible-crash case it guards, so failing to write it aborts the stage. Losing the CAS refuses (a peer is already staging). - A definitive refusal (4xx) releases the claim, since QuickBooks created nothing and the billing is freely re-stageable. An unknown outcome promotes the claim to `ambiguous-create` and records an automation event. - The returned QBO id is persisted BEFORE the pay-link fetch. That read is a second remote call, and a timeout there used to abandon a real invoice — the row still said unlinked, so the next stage created a second one. The row is now left `paylink-pending` for the maintenance sweep to finish, which is a success rather than something the operator must fix. - Compensation is now scoped to the window where the row is NOT yet linked. Deleting the invoice after the link write would strand a Staged row pointing at nothing. ProgressBilling has no `updatedAt`, so an in-flight marker has no readable age. It blocks either way; the resolver (next commit) is how it clears. Tests drive the real function against a fake ProgressBilling table and a fake QuickBooks, so the claim/release/park decisions are exercised rather than restated: pay-link timeout, marker write fails -> no POST, lost CAS -> refuse, definitive refusal releases, unknown outcome parks, lost link claim compensates. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Both create paths now link the QBO invoice BEFORE fetching its pay link, so a timeout on that second read leaves a correct, linked row whose only missing piece is the convenience link. Nothing finished those rows. `sweepPendingPayLinks` runs at the end of the existing sync-payment-options sweep, under the same route deadline, over both rails (PaymentSchedule and ProgressBilling). A fetched link is written and the marker cleared under a CAS pinned to the id and the invoice we just read, so a concurrent unlink or re-stage wins instead of being overwritten. It stops on any connection-level failure, same rule as every other QBO loop here: during an outage the next row fails identically at a fresh 20s. That stop rule needed a pay-link read that can tell "QuickBooks answered, no link" from "QuickBooks did not answer" — `getQBInvoicePaymentLink` collapses both into null, so a 503 storm would have cleared every pending marker without fetching anything. `readQBInvoicePaymentLink` throws instead. The existing helper is unchanged; only the sweep uses the strict one. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
|
Required changes:
VERDICT: REQUEST_CHANGES |
Codex gate round 27, item 3, plus the follow-up that a parked row is not an unlinked row. The row was parked, every send path refused it, and the error told the operator to "clear the QuickBooks link" — but `breakQBInvoiceLink` rejected exactly that state (there is no link to break), and progress billings had no clear operation at all. Worse, a parked row's `qbInvoiceId` is null, so every money guard that asked only that question let it be deleted, repriced, re-split or swept into a progress billing while a real, collectible invoice sat in QuickBooks. - `resolveAmbiguousInvoiceCreateCore` (src/lib/qbo-ambiguous-create.ts) asks QuickBooks by DocNumber under a RouteDeadline and acts only on an unambiguous answer: exactly one invoice carrying OUR PrivateNote is adopted under a CAS (left `paylink-pending` so the sweep fetches the link); zero clears the marker ONLY on an explicit "confirmed-none"; more than one, or an unreachable QuickBooks, refuses and writes nothing. DocNumber is not unique in QuickBooks, so the PrivateNote is what proves an invoice is ours — and both it and the DocNumber now come from shared helpers used by the create path itself, so the resolver cannot drift out of matching what we make. - Permission is narrower than `invoices`: ADMIN or FINANCE (`canResolveAmbiguousCreate` in access-rules.ts). Every outcome writes an automation event carrying the actor and their stated reason. - `breakQBInvoiceLink` routes the null-id parked state here instead of rejecting it. Progress-billing delete refuses with a typed `QBResolveRequiredError`. - One shared predicate `isQboInvoiceLinkedOrPending` now backs the milestone delete guard, the re-split in-flight check, the rebalance content check, the progress-billing inclusion check and its AUTO-SPLIT claim. The marker vocabulary moved to a PURE module (qbo-create-markers.ts) so the invoice editor can share it without pulling Prisma into the browser bundle. - UI: a "Resolve in QuickBooks" button on a parked milestone, behind confirm(), with a second confirm for the "QuickBooks has nothing" assertion. The badge now reads QB unconfirmed / link pending rather than mislabelling both as "voided", and delete is hidden while a row is parked. Note recorded rather than papered over: neither PaymentSchedule nor ProgressBilling has an `updatedAt` column and this PR ships no schema change, so the resolve action's optimistic-concurrency token is a fingerprint of (qbSyncError, qbInvoiceId) — the two fields the decision depends on — and the write's CAS pins the same pair. Tests: 11 end-to-end resolver cases against a fake Prisma and a fake QuickBooks (link, confirmed-none, DocNumber-collision, multiple, unreachable, wrong role, stale token, not-parked, progress billing, in-flight, blocked delete), plus guard tests for the delete and progress-billing paths and a source tripwire that fails if a new bare-`qbInvoiceId` guard appears with no marker in sight. Both were mutation-checked by deleting the guard. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Three failure classifications that all made the same mistake — reading "we
could not ask" or "who are you?" as a settled, terminal answer.
1. `getQBInvoicePaymentLink` returned null for EVERY failure, so null meant
both "this invoice has no pay link" and "QuickBooks never answered". Now
null means only the former; 401/403 throw a typed QboHttpError and
408/429/5xx/our own deadline throw retryable. The create paths already
distinguish those: a transient failure keeps `paylink-pending` for
`sweepPendingPayLinks` (named in the comments, not "a sweep"), an auth
failure surfaces. An already-linked milestone whose link read fails
transiently is now marked pending too, instead of ending up linked, linkless
and unmarked with nothing to retry it.
2. `POST /api/integrations/qbo-receipts/create` answered 401/403 with
`qbo-fault` + 200 via its deterministic-4xx rule. That is right for a
business refusal and wrong here: nothing is wrong with the receipt, the
connection is broken. It told the bot to give up and book by email, for
every receipt, silently. Now 401/403 and a stranded/unpersistable token
refresh answer 503 `{ok:false, retry:true, reason:"qbo-auth"}`. The
pre-existing test that asserted 403-is-terminal was rewritten against a real
business refusal (400), which is the case that rule was carved out for.
3. The payments cron returned HTTP 200 while carrying `runFailed:true`, so an
outage that skipped every row read as a clean hourly sync in Vercel's cron
log. Now 503 with the same body plus `retry:true`.
Health/digest: a new optional `qboAuth` probe counts `qbo-auth` events in 24h
and reports `quickbooks-reconnect-needed` with a digest line naming the fix.
"Automation errors (24h): 3" does not tell anyone to reconnect QuickBooks. The
probe is optional on the snapshot type so older callers and tests are unchanged,
and a probe that cannot run reports `probe-failed:qboAuth` rather than reading
as "no auth failures".
Tests: pay-link null-vs-typed-failure across 401/403/408/429/5xx, an auth
failure on the progress-billing stage surfacing while the row stays linked,
route-level 401/403 and token-stranded/persistence cases, the business-4xx
control, the cron's 503, and the health reconnect reason (present/zero/absent/
probe-failed).
Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
…he wall
Codex gate round 27, item 4. Bounding the individual QuickBooks calls was not
enough: `billing-core.ts` fetched tokens with no shared budget, then caught
failures per milestone and carried on. During an outage each row spent a fresh
20s deadline against the same wall, so a handful of rows still ran the action to
the platform ceiling and it was killed with nothing reported — the original
defect, one level up.
- `BILLING_QBO_BUDGET_MS` (45s) is created once at the entry of
`resendInvoiceCore`, `sendMilestoneInvoicesCore` and
`updatePendingMilestoneAmountsCore` (each also accepts one from a caller) and
is threaded into every QuickBooks call in both their loops: the token refresh,
the invoice status read, the milestone push, the pay-link read, the invoice
probe, the compensating delete, the re-stage push.
- Each loop checks the budget between rows and stops on a connection-level
failure — `isSharedQboWall`: our deadline, a 429/5xx, a 401/403, a stranded
token. A per-row business refusal is still recorded and the loop continues.
Rows never attempted are reported as such ("not attempted"), never as
failures, so the operator knows what to retry.
- The rebalance preflight aborts rather than committing on a partially verified
set: running out of budget half way means the remaining staged invoices were
never checked for payments.
- `POST /api/quickbooks/sync` ran a four-call serial chain (refresh, customer
ensure, service-item ensure, document sync) entirely unbounded. It now has a
50s budget carried through all of them, an explicit `maxDuration = 60`, and
answers 503 + retry on an outage instead of 500. `syncEstimateToQB` and
`syncInvoiceToQB` take an optional deadline for that.
Tests: the stop-rule classification (7 wall errors vs 3 per-row refusals), the
"not attempted" wording, the cumulative-budget arithmetic that makes the rule
necessary (20 rows x 20s stops inside the 60s ceiling), and a source tripwire
that each of the three loops still takes a shared deadline, checks it between
rows, and consults the wall — mutation-checked by deleting one loop's break.
Full run: npm run test:unit 1052/1052, npm run build clean.
Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
VERDICT: REQUEST_CHANGES |
…link Gate on 392356b, four items. 1. A successful compensating delete now releases the provisional link. Both rails record `qbInvoiceId` BEFORE the pay-link fetch, so by the time compensation runs the row may already point at the invoice being deleted; deleting without clearing left it linked to a document that no longer exists — the poller kept probing it, the portal offered a dead pay link, and the next send refused because the row "already had" an invoice. One shared `compensateAndUnlink` step does delete-then-CAS-clear (pinned to the exact id we wrote, so a concurrent settle or re-stage wins instead of being trampled), on milestones and progress billings alike, restoring a billing to Draft. A FAILED delete deliberately keeps the link: the invoice is still collectible and a row pointing at it is how a human finds it, and the error now says so. 2. The recovery identity is immutable and carries no schema: the in-flight marker holds it. `create-in-flight:<docNumber>|<privateNote>` (and `ambiguous-create:<same>`) is written in the claim CAS before the POST, and the resolver reads the identity back off the marker instead of recomputing it. It had to stop recomputing: the docNumber is the milestone's POSITION in its schedule and the note carries the project and milestone names, so deleting an earlier milestone or renaming the project made recovery query QuickBooks for a document we never created, find nothing, and offer to release a row whose real invoice was sitting there collectible. A marker with no identity (legacy or corrupt) stays blocked with a typed `identity-unknown` refusal — confirmed-none cannot clear it, because "I cannot ask" must never become "there is none". Parsing lives in one helper; the release, promote and provisional-link CASes are all pinned to the exact marker we wrote, and the guards that matched a fixed marker list now match by prefix. 3. `getQBInvoicePaymentLink` returns null only when a valid Invoice lacks an InvoiceLink. A body that will not parse, or a 200 carrying no Invoice, is a typed transient error — a truncated or proxy-mangled response says nothing about whether a link exists, and the sweep would have cleared the marker on it. 4. The maintenance sweep pages through schedules by id with a cursor instead of one `take: 200` slice that silently ignored the 201st row run after run, and `ok` is now false if any row failed OR anything remains. When the deadline or an outage stops it, the response carries `truncated: true` and a `remaining` count read from the database after the last row it actually finished. Tests (1065/1065, build clean): marker compose/parse round trip incl. legacy, corrupt and pipe-bearing notes; identity unchanged when a sibling is deleted and the project renamed; identity-unknown refusing both decisions without ever querying; compensation clearing the provisional link, keeping it on a failed delete, and not trampling a row that moved on; malformed pay-link body keeping the marker through the real sweep; and the maintenance route driven end to end over a fake Prisma and a fake QuickBooks for pagination past 250 rows, row failure, early stop with a remaining count, and a 404 staying a finding. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
VERDICT: REQUEST_CHANGES |
…ential routing Addresses all 8 Codex gate findings on PR #438: - P0: confirmed-none can no longer clear a create-in-flight marker while the claim may still be live (embeds a claim timestamp in the marker and refuses under CREATE_IN_FLIGHT_STALE_MS or with no readable age) - P0: link-write CAS in progress-billing.ts and quickbooks-payments.ts now pins the exact in-flight marker the caller owns, not just Draft/unlinked/ unchanged content - P1: parked progress billings can no longer be edited (isBlockedByAmbiguousCreate guard added to updateProgressBillingCore) - P1: QboPurchaseFaultError and the attachment upload/lookup paths now route 401/403 to qbo-auth instead of qbo-fault/qbo-unavailable/terminal failure - P1: getQBInvoiceStatus throws a typed error for 401/403/429/503 instead of answering null, so sendMilestoneInvoicesCore stops the batch on a shared wall - P1: findQBInvoicesByDocNumber refuses (QBResultSetTruncatedError) when the 20-row page cap is hit, and the resolver treats it as ambiguous - P1: payment preflight now classifies 401/403 as qbo-auth so pipeline-health's digest counts it toward the reconnect alert - P2: compensateAndUnlink falls back to clearing by the owned in-flight marker when the row never carried qbInvoiceId, so a confirmed delete always releases the claim Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
…module-identity fix) tsx on Node 20 can resolve @/ alias imports and ../src/ relative imports as separate module cache entries, breaking instanceof checks. Added isQBNotConnectedError() type guard matching the existing isQBTimeoutError() pattern. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
VERDICT: REQUEST_CHANGES |
…classification Extends the milestone rail's issuance-hash protection (a create that lands, loses its post-create CAS, then fails to compensate leaves a real QuickBooks invoice for stale money with a matching identity) to the progress-billing rail: composeCreateMarker/parseCreateMarker now carry an optional hash field, and resolveAmbiguousInvoiceCreateCore refuses to link an otherwise-matching invoice whose issuance hash has moved or was never recorded. Also fixes the one remaining bare `instanceof QboPurchaseFaultError` check in isQboAuthFailure() (qbo-receipts/create/route.ts) to the name-based dual-check pattern already used everywhere else in this module (isQBNotConnectedError, isQboAttachmentAuthError, qboHttpStatus) — an instanceof-only check on this class can miss the same error loaded via a second module instance and silently fall through to the terminal business-fault branch instead of the retryable qbo-auth one. Verification against a Codex review handed down for this PR found the other 6 flagged findings already resolved on this branch by earlier commits (f5f5a31 marker liveness/link ownership/credential routing, 422e16e recovery identity/compensation release, 392356b route deadline, 4181cb5 credential-vs-document refusal): create-in-flight staleness is gated on the marker's own embedded claim time (not a nonexistent updatedAt column), progress-billing/milestone link CAS writes already pin qbSyncError to the owned in-flight marker, updateProgressBillingCore/deleteProgressBillingCore already block on isBlockedByAmbiguousCreate, compensateAndUnlink already falls back to clearing by marker when qbInvoiceId is null, getQBInvoiceStatus already throws typed errors for shared failures instead of returning null (and sendMilestoneInvoicesCore already stops the batch on them via isSharedQboWall), and classifyPreflightFailure already routes 401/403 to the qbo-auth reason family that pipeline-health's probe counts. Confirmed with a full read of the current code and a green test run — see PR conversation for the finding-by-finding trace. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
|
Required changes:
VERDICT: REQUEST_CHANGES |
…delete-wall classification, receipt push ambiguity Four fixes from the PR #438 adversarial review: - composeCreateMarker now carries the timestamp on BOTH create-in-flight and ambiguous-create markers, and callers promoting a marker pass the ORIGINAL claim time through unchanged. The resolver's liveness cooldown now applies to ambiguous-create too, so an operator can no longer clear a promoted marker with confirmed-none before the original request has had time to land in QuickBooks. - qbo-maintenance's sync-payment-options sweep persists its resume cursor via automationSettingCursorStore (same pattern as the payments/pay-link sweeps), instead of reinitializing to null every invocation — a budget cutoff or outage no longer re-walks the same leading rows forever while the tail starves. - deleteQBPayment/deleteQBInvoice now distinguish an authoritative 404 ("already gone", returns false) from a shared QBO failure (401/403/429/5xx, now thrown via qboResponseError) so the billing-core rebalance loop's isSharedQboWall check can see the outage and stop, instead of reading it as an ordinary per-row refusal and grinding through every remaining row at full cost. - The QBO purchase-create path now raises QboRetryableError instead of a generic Error when a 2xx response is empty/malformed or carries neither a Purchase nor a Fault — the create's outcome is genuinely unknown, and the idempotent requestid makes a retry safe, so the route now answers a retryable 503 instead of a terminal push-failed/500. Defended (no change): the non-idempotent creates in syncEstimateToQB/ syncInvoiceToQB are pre-existing behavior outside this PR's scope — the retry:true this PR's route advertises covers transient network failures (the timeouts this PR addresses), not idempotent replay of a create. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
VERDICT: REQUEST_CHANGES |
…etes, unlink discard, abort propagation Codex round-31 review of PR #438 returned REQUEST_CHANGES with 5 findings; this fixes 4 and defends 1 (bare-create retry safety, unchanged pre-existing behavior outside this PR's scope). - qbo-create-markers identity: composeCreateMarker's privateNote is now truncated to QB_PRIVATE_NOTE_MAX_LEN (4000, the same cap createQBMilestoneInvoice applies) before it goes into the marker, in both progress-billing.ts and quickbooks-payments.ts. Previously an untruncated identity could never match the truncated PrivateNote QuickBooks actually stored, so resolveAmbiguousInvoiceCreateCore would report "none found" on a real invoice and a confirmed-none clear would let a duplicate through. - deleteProgressBillingCore / deleteInvoiceMilestoneCore: the final delete is now a deleteMany reasserting the same deletable-state predicates (status/qbInvoiceId/qbSyncError, etc.) instead of an unconditional delete-by-id. The create-in-flight marker CAS write happens outside the delete's transaction lock, so a concurrent stage/push landing its claim in the read-to-delete window used to get silently wiped out along with the row — orphaning a real QuickBooks invoice. A missed CAS now throws instead. - progress-billing.ts stageProgressBillingToQuickBooksCore: the link-write catch block now surfaces compensationUnlinkFailed (already computed by compensate(), previously read only by the sibling branch below it) instead of re-throwing the bare original error, so a "deleted but not unlinked" outcome is reported instead of silently discarded. - quickbooks.ts qboResponseError / parseJsonOrNull: both now re-throw a caller-originated AbortError (name-based check) before falling through to status/malformed-body handling, so a caller's own cancellation propagates as itself instead of being converted into a fabricated HTTP or parse error. npm run build passes (0 errors). Targeted test run (progress-billing-stage, qbo-ambiguous-create, qbo-parked-row-guards, qbo-payments-outage, billing-qbo-deadline, qbo-maintenance-sweep): 154/154 pass. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
VERDICT: REQUEST_CHANGES |
Phase 0 of the receipt-pipeline rebuild: stop the QuickBooks hang, add a pulse
Plan:
docs/plans/RECEIPT-PIPELINE-V2-PLAN.md(also adds the Phase 1 and Phase 4 specs).Why
On 2026-09-01 Intuit's QuickBooks API had a major outage. Every ProBuild route that calls QuickBooks hung to its Vercel ceiling (60 s on
qbo-receipts/create, 120 s on the payments cron) becausesrc/lib/quickbooks.tsused barefetch(). The Apps Script receipt bot saw 504s, gave up after 6 passes, and parked files as "MAYBE in QuickBooks". Nobody was told: bot alerts had been going to a dead Telegram chat since Aug 20.What
qbTimedFetchinsrc/lib/quickbooks.ts: 20 s deadline on every Intuit call (QB_FETCH_TIMEOUT_MS), 45 s for token refresh (QB_REFRESH_TIMEOUT_MS). Our deadline surfaces asQBTimeoutError(path only, never a token) for headers and body reads (json/text/arrayBuffer/blob/bytes/clone). Caller aborts stay plain errors; manual signal combiner whenAbortSignal.anyis absent. Also routed the attachment upload and download fetches.POST /api/integrations/qbo-receipts/create: aQBTimeoutErrorlogsreason: qbo-timeoutand returns 503{ok:false, retry:true}so the Apps Script retries next pass instead of burning its attempts.maxDurationstays 60.GET /api/health/pipeline(staff withfinancialReports, or BearerCRON_SECRET): Intuit status page, last purchase sync, last booked receipt push, 24 h event counts, last bank line, error count. Every probe reportsok|error; any probe failure or no booked receipt in 72 h →ok:falsewithreasons[]. No auto-green.GET /api/cron/pipeline-digestat 14:00 UTC (7 AM PDT / 6 AM PST): plain-text summary emailed toPIPELINE_DIGEST_TO(default jadkins@) and posted toBOT_HEALTH_CHAT_WEBHOOKif set (existing chat.googleapis.com allowlist). Newsrc/lib/cron-auth.ts: constant-time Bearer compare, missing secret rejects,NODE_ENV=developmentis the only bypass.Review
Two Codex rounds. Round 1 blockers (body-read timeouts escaping the wrapper, 30 s maxDuration, refresh-token ambiguity, false-green health) fixed. Round 2: no critical findings; the medium ones fixed. Then 13 rounds of the repo's CI Codex gate (each round smaller): payments-cron budget and heartbeat, attachment retry semantics, parse-vs-transport classification, an end-to-end RouteDeadline threaded through every QBO call, probe-failure and stale-heartbeat handling.
Tests
npm run test:unit1065/1065,npm run test:qbo-receipt-push88/88,npm run buildclean (0 errors).New test files:
tests/qb-timed-fetch.test.ts,tests/pipeline-health.test.ts,tests/cron-auth.test.ts,tests/progress-billing-stage.test.ts,tests/qbo-ambiguous-create.test.ts,tests/qbo-parked-row-guards.test.ts,tests/billing-qbo-deadline.test.ts,tests/qbo-maintenance-sweep.test.ts(all wired intotest:unit).Round 27 follow-ups (the duplicate-bill and deadline items)
Progress billings now carry the milestone rail's guards (
src/lib/progress-billing.ts): the row is CAS-claimedqbSyncError: null -> create-in-flightbefore the POST (a failed marker write aborts; a lost CAS refuses), a definitive 4xx releases the claim, an unknown outcome parks itambiguous-create, and the returned QBO id is persisted before the pay-link fetch so a timeout there can no longer abandon a real invoice. Compensation is scoped to the window where the row is not yet linked.sweepPendingPayLinksfinishes rows leftpaylink-pendingon both rails, inside the existingqbo-maintenancesync-payment-optionssweep, under the same route deadline, CAS-guarded, and stops on a connection-level failure.resolveAmbiguousInvoiceCreate(src/lib/qbo-ambiguous-create.ts+ the action inactions.ts) is the recovery the old error message promised: a bounded QuickBooks query by DocNumber, acting only on an unambiguous answer — exactly one invoice carrying our PrivateNote is linked, zero clears only on an explicitconfirmed-none, multiple or unreachable refuses and writes nothing. ADMIN/FINANCE only, audited with actor + reason.breakQBInvoiceLinkroutes the null-id parked state here instead of rejecting it, and progress-billing delete is blocked while parked. UI: a "Resolve in QuickBooks" button on the parked milestone.A parked row is not an unlinked row. One shared predicate
isQboInvoiceLinkedOrPending(pure modulesrc/lib/qbo-create-markers.ts) now backs the milestone delete guard, the re-split in-flight check, the rebalance content check, and the progress-billing inclusion check + AUTO-SPLIT claim, with a source tripwire against new bare-qbInvoiceIdguards.A refused credential is not a refused document.
getQBInvoicePaymentLinkreturns null only for "no link exists" and throws typed failures otherwise; the receipt route answers 401/403 (and stranded/unpersistable token refreshes) with 503qbo-auth+ retry instead of a terminalqbo-fault; the payments cron answers 503 whenrunFailed; health/digest gained aquickbooks-reconnect-neededreason that names the fix.One
RouteDeadlineper entry through the billing send/resend/re-stage loops andPOST /api/quickbooks/sync, with the loops stopping on a connection-level failure and reporting untried rows as "not attempted".Recovery identity lives in the marker, not in a recomputation.
create-in-flight:<docNumber>|<privateNote>is written in the claim CAS before the POST, and the resolver reads it back rather than re-deriving it — the docNumber is the milestone's POSITION in its schedule and the note carries the project and milestone names, so deleting an earlier milestone or renaming the project used to make recovery ask QuickBooks about a document we never created. A marker with no identity (legacy or corrupt) stays blocked with a typedidentity-unknownrefusal thatconfirmed-nonecannot clear.A compensating delete releases the provisional link (
compensateAndUnlink, both rails, CAS-pinned to the id we wrote); a failed delete deliberately keeps it so a human can find the invoice.The maintenance sweep pages by cursor instead of one 200-row slice, and
okis false if any row failed or anything remains (truncated+remaining).Known gap, recorded rather than papered over: neither
PaymentSchedulenorProgressBillinghas anupdatedAtcolumn, and this PR ships no schema change. The resolve action's optimistic-concurrency token is therefore a fingerprint of(qbSyncError, qbInvoiceId)and the write's CAS pins the same pair;isStaleInFlighthas no timestamp to read on a real row, so it always reports "unknown outcome" (fail-closed, and it only affects what the operator is told).Scope note
This PR is the ProBuild half of Phase 0. Rerouting the Apps Script and Hermes bot alerts off the dead Telegram target is configuration on Justin's machine and in the Apps Script project (set
GOOGLE_CHAT_WEBHOOK_URLviasetChatWebhook(); Hermes jobs already deliver locally and are read by the Picard bridge watch), not code in this repo. Until that is done, real-time bot failures surface only through this PR's 7 AM digest and/api/health/pipeline.Deploy notes
No schema change. Optional env vars:
BOT_HEALTH_CHAT_WEBHOOK,PIPELINE_DIGEST_TO,QB_FETCH_TIMEOUT_MS,QB_REFRESH_TIMEOUT_MS.🤖 Generated with Claude Code
Scope
The QuickBooks invoice issuance identity work (durable per-issuance keys,
payload hashing, stranded-issuance reconciliation, the
breakQBInvoiceLinkremote-state rework, and the
qbIssuanceKey/qbIssuancePayloadHashcolumns)was split out of this PR into a stacked draft: #445. It had grown past the
scope of "stop the QuickBooks hang" and carries seven open gate items of its own.
What Phase 0 keeps in its place is a small fail-closed guard on the existing
qbSyncErrorcolumn: an invoice create whose outcome is unknown (timeout, or atransport failure after the request went out) parks the row as
ambiguous-create, and the send path refuses to re-send until a human haschecked QuickBooks and cleared it via the existing unlink flow. Weaker than the
issuance work — it prevents a duplicate bill rather than recovering from one —
but correct, and it needs no schema.