feat(receipts): Receipts tab, missing-receipt chaser, Chat cards, nightly QBO bank pull (pipeline v2 phase 2) - #443
feat(receipts): Receipts tab, missing-receipt chaser, Chat cards, nightly QBO bank pull (pipeline v2 phase 2)#443Clarion1631 wants to merge 104 commits into
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
Required 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>
…try transient attachments
Codex gate round 4, all three items.
1. probeQBInvoice flattened everything into {state:"error"} and both loops
continued across up to 200 rows, so six 20s timeouts still reached the
cron's 120s ceiling and the run was killed with nothing reported. The
probe now marks connection-level failures (our deadline fired, the request
threw, or QBO answered 429/5xx) separately from per-invoice errors, and
both loops STOP on the first one, count the remaining rows as `skipped`,
and exit cleanly. A timed-out token refresh aborts the same way. Ordinary
per-invoice errors still just skip that row.
2. Each run now writes one AutomationEvent (kind "qbo-payments-sync",
status ok/error, reason "qbo-unavailable", run counts in detail). Before
this an outage on the money rail left no trace anywhere a human or the
digest would look. pipeline-health exposes lastPaymentsSync and its
errors already count toward `stuck`, so a stalled payments rail turns the
morning digest red.
3. Transient attachment failures were terminal: a 429/5xx upload, a network
error, or a failed Attachable lookup became `failed:<reason>` alongside
ok:true, which the Apps Script treats as final — it stopped resending and
the Purchase stayed unattached. Those now raise QboRetryableError and the
route answers 503 retry:true, so the next pass hits the idempotent
existing-Purchase recovery. A 4xx other than 429 and a QBO Fault stay
terminal (returned as values, not thrown) and still ride on ok:true.
Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
…te upload response Codex gate round 5. 1. getQBPayment failures were caught as ordinary row errors, so several settled invoices could each still burn a 20s deadline after a good probe. Both loops now run through one shared, exported runQboRowLoop: ANY connection-level failure from ANY QBO sub-call in a row (probe, payment detail, anything added later) stops the run, counts the rest as skipped and exits. getQBPayment raises on 429/5xx instead of returning null. Tested by driving the REAL loop with an injected fake QBO client, not a duplicated decision helper. 2. Only a timeout marked a run failed, so QBNotConnectedError and settings-store failures emitted status=ok and the digest stayed blind. classifyPreflightFailure covers every branch; the event now keys off result.runFailed with its own reason. 3. An empty, truncated, or HTML 200 from the Attachable upload fell through to "attached" for a file QBO never stored. Intuit's schema says the response carries an Attachable or a Fault, so a real AttachableResponse[].Attachable.Id is now required; anything else is retryable. 4. lastPaymentsSync null or older than 26h is now reason "payments-sync-stale" and ok:false, and the probe only counts runs sourced "cron" — on-view runs log source "view", manual "manual", so neither can disguise a dead hourly job. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
…lit; transient uploads
Codex gate round 6.
1. The payments cron route was the last fail-open caller: it only checked the
secret when VERCEL_ENV === "production", so a preview or non-Vercel runtime
could trigger a money sync unauthenticated — and, since this round, write a
source:"cron" heartbeat that would make a dead cron look alive. Now uses
isCronAuthorized (fail-closed, constant-time); a missing CRON_SECRET can no
longer be satisfied by "Bearer undefined".
2. A run that skipped rows or hit row-level errors recorded status "ok" and
refreshed the health heartbeat on work that never happened. Runs are now
"ok" only when complete, "partial" when incomplete (heartbeat-ineligible,
with counts, but not counted as a hard error so one stubborn row does not
read like an outage), "error" when the run failed outright.
3. Refresh and save shared one catch, so a rotation Intuit had already
committed could fall back to the now-spent stale pair while reporting a
healthy connection. Split: only a REFRESH failure may fall back; a SAVE
failure retries once, then raises QBTokenPersistenceError (reason
"token-not-persisted") rather than stranding the integration silently.
4. 408 and 401 uploads were terminal `failed:<status>` on ok:true, so the bot
stopped retrying and left a new Purchase without its receipt. Both are
transient now; a 401 forces exactly one token refresh and retries in place,
and anything still failing raises. Hard 4xx (400/403/404/413/415) stay
terminal.
5. If the first create's response was lost, no "created" event ever exists, so
the recovery pass was invisible to the freshness clock. An "already-exists"
that genuinely uploaded the attachment now counts as a booking; ordinary
retries ("already-attached"/"skipped") still do not.
Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
…network errors Codex gate round 7. 1. Both loops returned silently when a probe failed without connectionFailed, so a run could verify nothing, record no error or skip, and still emit status "ok" — a green heartbeat for work that never happened (my own tests enshrined it). Every failed probe now records a row error, making the run "partial". And a 401/403 is connection-level: the credential is shared, so the next 199 rows fail identically at full cost — those abort the run. 2. Health read only the last "ok" event and counted only "error" ones, so repeated hourly partial runs could sit green for up to 26h, or until the next day's digest. The heartbeat query now accepts ok OR partial (a partial run does prove the cron is alive, so it counts for freshness) and a latest run of "partial" adds reason "payments-sync-partial" immediately. The digest line marks it "[incomplete run]". 3. getQBPayment translated 429/5xx but a bare `TypeError: fetch failed` (DNS/TLS/reset) escaped unclassified, so runQboRowLoop did not see it as connection-level and kept dialling. Normalized at the boundary instead of per call site: qbTimedFetch now turns any thrown transport failure into QboRetryableError, in both the header and body phases, so every QBO call gets it. A caller's own abort still passes through untouched. Tested with a real TypeError from an injected fetch, through getQBPayment and probeQBInvoice. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
…lper) Rebase onto main after Phase 4 (#439). Phase 4 landed postTextToWebhook(url, text) -> {sent, reason} in chat-webhook.ts, extracted from postDailyLogToChat for the Monday margin card. My branch had added a near-identical postTextToChatWebhook for the pipeline digest. Keeping both would have meant two helpers with the same SSRF allowlist and timeout drifting apart, so mine is gone and the digest calls main's. The richer return type is a small win: a skipped post now carries a reason instead of a bare false, so "no webhook configured" and "webhook responded 403" are distinguishable in the cron log. Behaviour is unchanged: same allowlist, same 10s post deadline, still never throws, and chatPosted still reflects a genuine delivery. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
…n status; clamp; real text part Codex gate round 8. 1. getQBPayment returning null (401/403/408, malformed body) let both loops settle anyway with `new Date()`, stamping TODAY as the payment date — wrong money data reported as a clean run. A null read now leaves the row unsettled and records the reason. 401/403/408 join 429/5xx as shared failures (isSharedQboFailureStatus) that abort the run. 2. A Purchase that booked but never stored its receipt logged "created" and counted as a healthy, fresh booking. Terminal attachment outcomes (failed:*, skipped) now log status "attachment-failed": still terminal, no retry loop, but visible and excluded from receipt freshness. 3. The empty-run early return recorded "ok" BEFORE acquiring tokens, so a disconnected integration emitted a fresh successful heartbeat every hour forever. Tokens are acquired first; a run that cannot get them is "error". 4. Health now reads the latest cron event separately from the freshness timestamp: reason "payments-sync-error" fires whenever the LATEST event is an error, at any age. Freshness still only counts ok/partial. Closes the two-hour green gap between the 24h error count and the 26h staleness window. 5. normalizeTimeoutMs clamps to [1000, 55000]. A large finite value (QB_FETCH_TIMEOUT_MS=4294967296) passed the isFinite guard and made AbortSignal.timeout throw synchronously — one mistyped env var would have broken every QuickBooks call. 6. sendNotification takes an optional explicit `text` part; the digest passes its own plain text and keeps the <pre> for HTML clients. The derived text part collapses all whitespace, which flattened the report to one line. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
|
Required changes:
VERDICT: REQUEST_CHANGES |
…t on lost receipts Codex gate round 9. 1. An empty run recorded "ok" on the strength of holding tokens. But a non-timeout refresh failure falls back to the STALE pair, and stale credentials, a wrong realm, or revoked accounting access all still produce a token object — so a dead rail emitted a fresh green heartbeat every hour forever. Empty runs now make one cheap authenticated read (CompanyInfo); a failure is classified and recorded as "error". 2. paidAt defaulted to `new Date()` and was only replaced when txnDate happened to be truthy, so an invoice with no linked payment, a null or unparseable TxnDate, or an unreadable payment record settled REAL milestones stamped with today — and fired the mirror/notification side effects on the way out. resolveSettlementDate refuses all four cases with reason "payment-date-missing"; the row stays Pending for a later run. 3. Both queries took an unordered first 100 and stopped: rows past the cap were neither checked nor counted as skipped, so the run reported "ok" while work was left undone — and with no ORDER BY, Postgres could return the same page every hour and starve the rest indefinitely. Both collections now walk in stable id order, paged by cursor, under a row cap and a 90s budget; whatever is not reached is counted as skipped, making the run partial. 4. attachment-failed was terminal but non-alerting: `stuck` matched only literal "error", so one other good receipt inside 72h left the digest reading "Pipeline OK" while a Purchase sat in QuickBooks with no receipt. It now counts toward `stuck`, the journey mapper renders it as failed rather than in-flight, the intake graph buckets it as an error, and it counts against the hands-free rate instead of as a clean push. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
300547e to
cb4622d
Compare
1. postOwnerCard is TRI-STATE. `rejected` (invalid URL, 4xx) means Chat provably took nothing, so the row goes back to PENDING for the retry pass. `unknown` (timeout, 5xx, or a 2xx with no message name) means the card may be sitting in the crew's space right now — it becomes UNCERTAIN and is NEVER auto-retried, because a duplicate chase card teaches people the list is noise. Collapsing those two into "failed" is what made a timeout look retryable. A concurrent run now also HONOURS an active claim lease on a POSTING row and skips it; only an expired lease converts POSTING → UNCERTAIN, so a healthy in-flight run is no longer robbed of the ids it is about to write. 4. The open-issue pass is paged (100 per batch) under the same wall clock with its OWN cursor — sharing the line cursor would make each pass corrupt the other's resume point — and never checkpoints past a failure. An issue whose BankLine no longer exists is CLOSED with resolution `target-missing`: the matcher has nothing to match, so it would otherwise be skipped, and nag, forever. 5. A cursor write that fails now throws CursorWriteError and the invocation returns 500. Swallowing it meant the batch's work committed, the checkpoint did not, and the sweep redid the same ground every run while reporting 200. 6. The QBO window is planned from a persisted high-water mark: from (mark − 3 days) to today, capped at 60 days per run with continuation, plus a weekly full 60-day sweep — the only thing that can find an entry QuickBooks BACKDATES behind a mark that has already passed it. The mark advances only on a clean, complete run, and a failure to persist it fails the run. 7. An outer 50s budget spans fetch/ingest/reconcile/mint; the run stops on its own terms, records `remainingBatches` as the continuation point, and keeps its accumulated counters instead of being killed with nothing written. 1255 unit tests pass, build clean, tsc 0, lint 0. Items 2, 3 and 8 are Phase 1 code and go to that builder; will rebase onto feat/phase1-intake-core when it pushes. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Rebased onto d6e709b. Phase 1 now owns the worker claim, the project-scoped loadPhases and the machine-endpoint action guard, so those hunks are its version; my duplicate ReceiptIntake.claimToken/claimedAt DDL is gone (ONE definition, in Phase 1's script). Phase 1's MACHINE_ENDPOINT_PATTERN gains the receipt-requests bridge, office-tasks/ingest, mcp, health and version — all bypassed machine endpoints that were still dispatchable. 2. The QBO "changed content" check hashes CANONICAL IDENTITY (amount, posted date, check number, payee via bankLineIdentityPayee), not descriptor text. When the pull stopped appending the transaction type, every observation stored in the old format hashed differently from the same transaction re-read today, so the ingest 409'd and the nightly pull stalled on rows that had not changed at all. bankLineIdentityPayee strips the legacy suffix for exactly that compatibility, and a descriptor-only difference now updates the stored text in place. 3. An intra-window continuation cursor (last posted TxnDate + qbTxnId) is persisted, so a budget-limited run resumes instead of re-posting its own first batches forever. The high-water mark still only moves on a COMPLETE run. Two-run test proves no batch is posted twice and the second run does real work. 4. Evidence competition is a CONNECTED COMPONENT (union-find over the line×evidence adjacency), matched as a whole. The 1/5/9 vs 3/7 chain: the 1st and 9th share no candidate yet both compete with the 5th, so a same-amount bucket splits a real component. Both receipts are now used. 5. `?continue=1` and `moreToProcess` consult BOTH cursors — a half-finished open-issue pass looked like nothing in progress. 6. Queue actions CAS on the exact state the submitted view saw, not an allowed-set: a row can be NEEDS_REVIEW twice with a whole booking attempt in between. Duplicate targets that are themselves DUPLICATE or VOID are refused, and so are two-row cycles. 1406 unit tests pass, build clean, tsc 0, lint 0. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
…ounds, run phase, scan boundary, memo artifacts 1. The nightly sweep groups the WHOLE window into competition components before it pages, and cuts pages between components. Paging by line id could split a set of lines competing for one receipt across two pages, and each half then matched against the same evidence without seeing the other. The cursor is a component key, so a resume can never land inside a set. A neighbour dragged in by the cohort query consumes evidence but no longer gets a verdict from a page that does not own it. 2. Evidence bounds are now TWO ranges: calendar days for `@db.Date` columns (ReceiptIntake.txnDate, BankLine.postedDate) and company-timezone instants for Expense.date. One shared range was off by one at both ends for the DATE columns — it dropped the first day of the window and let the day after in. 3. An explicit run-phase marker (open-issues -> lines -> done) is persisted, so `?continue=1` resumes whenever the cycle is unfinished. Both cursors are cleared the moment their pass completes, so a run that finished the open-issue pass and then spent its budget parked neither, and the line half waited for the next full sweep. 5. The bank pull's boundary is what it SCANNED, not what came back. An old mark plus an empty capped window left the mark unmoved and the next run planned the identical window forever, while reporting success every time. 6. `signed:true` now requires a durable artifact — a Drive/Storage PDF URL or a signature id. Without one it is a 400 and nothing is written. 7. package.json had duplicate `test:receipt-intake` and `test:bank-ledger` keys; the later one silently won. One complete command each now. 8. .env.example documents RECEIPTS_CHAT_WEBHOOK, RECEIPT_OWNER_CHAT_USERS, RECEIPT_REQUEST_CARDS_ENABLED and BANK_LINE_MINT_FROM_QBO with their ship-disabled defaults. Item 4 (the claim-token fence through bookReceipt/promoteToBooking) is Phase 1's. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
The rebase onto 8b332c7 collided in bookReceipt: Phase 1 fences the BOOKED write on `claimToken`, Phase 2 fences it on `state = 'BOOKING'` and parks an orphaned Purchase as booked-after-void. Kept BOTH, as two writes in one transaction, because they catch different failures and one write cannot do it: - the STATE fence runs first (it is the transaction's first write, so a void landing during the QBO round trip creates no Expense) and a loss there is booked-after-void — now itself fenced on the claim, so a superseded worker still writes nothing; - the CLAIM CAS runs after the Expense and a loss there throws StaleClaimError, rolling the whole transaction back. It cannot re-assert `state: 'BOOKING'`: the fence above already moved the row to BOOKED inside this same transaction, so that check would fail on our own uncommitted write every time. Phase 1's two tests are updated to the two-write shape (zeroing every updateMany is the VOID case, not the re-claim case). Worker `aborted` and `booked-after-void` outcomes restored alongside Phase 1's `stale`. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
…ines through reconcile/mint, card re-verify
(1) A manual VOID / mark-duplicate releases the strong dedup key ONLY when the
QuickBooks send had not started. `sendAttempted: false` is part of the CAS,
not a value read beforehand — a read-then-write loses the race it exists to
guard. A park after the send still applies (the human decided), keeps the
key, and is flagged `:possible-orphan-purchase` so the row lands in the
Receipts tab's Exceptions group with an explanation, rather than silently
quarantining the re-send. New pure module src/lib/receipt-intake/park.ts.
(3) `complete` is modelled separately from `ok`. A budget-truncated run, a
capped window, and links the reconciler never attempted are ok:true /
complete:false: the checkpoint is persisted, nothing is minted, and the
freshness clock is not stamped. A rolled-back chunk, a stale fetch and a
failed ingest stay ok:false.
(4) The run's ABSOLUTE deadline (budget minus a 5 s checkpoint reserve) is
passed into reconcile and mint, and both check it per batch. Un-started
reconcile chunks come back in `remaining`, exactly like the chunk cap.
(5) The card snapshot is rebuilt under the claim immediately before the send:
items cleared, acknowledged, resolved or reassigned since selection are
dropped and the survivors renumbered. An empty rebuild DELETES the row, so
the owner's (owner, pacificDate) slot is not consumed by a card that can
never post. The retry pass goes through the same loop, so it rebuilds too.
Item 2 was Phase 1's and arrived in the rebase onto 85d6af5.
Also restores `test:bank-ledger`, which a package.json conflict merge during
that rebase collapsed away, as the union of both branches' file lists.
Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
The union resolution left `const claimed` declared twice in bookReceipt and a
reference to a `confirmed` that no longer existed, and dropped the
booked-after-void / aborted arms of applyBookResult. Restored, on top of
Phase 1's RELEASE_CLAIM: state fence first (a void loses there and is parked
with its Purchase id), claim CAS after the Expense (a re-claim rolls the whole
transaction back as stale), and the second write no longer re-asserts BOOKING
because the first one already moved the row inside the same transaction.
`tests/receipt-intake-cleanup.test.ts` sliced a function body with
indexOf("\n}\n"), which returns -1 on a CRLF checkout and swept in the rest of
the file. Made the slice EOL-agnostic.
Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
…erflowExact, uncertain-card surface
(1) RECEIPT_BRIDGE_SECRET — a THIRD machine secret for the two Beverly bridge
endpoints. That project runs outside ours and must not be able to book a
Purchase, which is the same argument that split ingest from archive. All
three compares always run, any two sharing a value is refused, and
presenting the wrong one is a 403 naming both capabilities. Each key's
complete MAY / MAY NOT list is in intake-auth.ts, .env.example and the spec.
(3) Every queue action now takes `expectedUpdatedAt` from the rendered row and
CASes on it as well as the state — including unmarkDuplicate, retry and
resolveOrphan. The state alone cannot tell NEEDS_REVIEW from a later,
different NEEDS_REVIEW, so a decision about the first was landing on the
second (the ABA case, now covered by a test).
(4) ONE 60-calendar-day boundary (`REGISTER_WINDOW_DAYS` + `registerWindowStart`)
for the deep sweep, the chaser and minting. Minting was 45 days, and shorter
is the dangerous direction: a chase opened for a 50-day-old charge could
never close by itself. It is also day-based now — an instant boundary
dropped the whole of its own oldest day and moved on every run.
(5) `ReceiptRequestCard.overflowExact` (additive, defaults true) is persisted
with the selection and read back on resume, so a retry pass no longer
prints "and N more" as a total when the count came from a scan that never
ran.
(6) UNCERTAIN cards are now visible and resolvable: an "Uncertain deliveries"
group on the Receipts tab with mark-delivered / resend (both CAS on the row
version), a `cards-uncertain:<n>` pipeline-health reason, and the cron
reports PARTIAL — ok:false with HTTP 200 — when a run itself moved a card
into UNCERTAIN. 200, not 500: it needs a human, not another attempt.
Item 2 (/start validating costCodeId) is Phase 1's and arrived in 8c21abd.
Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
…esolvable orphans, verified card delivery
(1) The chaser counts an intake as evidence only while its DOCUMENT is still
verified. `receipt-bytes-missing` and `content-changed` mean the row
outlived its receipt, and closing a chase on one means nobody is ever asked
for it again. book.ts, the worker and the matcher now share ONE list
(`NO_ARTIFACT_PARK_REASONS` in route-state.ts) so writer and reader cannot
drift; the cron selects `stateReason` on both passes.
(2) "Mark delivered" requires the operator to paste the card's thread AND
message names, validated as `spaces/…/threads/…` / `spaces/…/messages/…`
and required to name the same space. Without them the row STAYS UNCERTAIN —
a card closed with no thread identity leaves a reply nothing to resolve
against — and "resend" is always the other answer.
(3) The unknown-ID orphan is resolvable at last. Two audited answers: record a
manually located purchase id, READ BACK from QBO with the amount required
to match to the cent (a transposed digit lands on someone else's purchase),
or confirm no purchase exists, which is the only thing that frees the dedup
key. Both write an audit event naming who decided. QBO stays read-only. The
control is shown for both orphan kinds.
(4) Payee identity is a NAME, not a shared token. Stop words, store numbers and
TLDs are dropped; industry words survive tokenizing but may not carry a
match alone; agreement needs the same name (spacing- and
possessive-insensitive), the same leading bigram, or a lone brand token
leading the other side. HOME DEPOT no longer agrees with HOME GOODS, nor
PACIFIC PLUMBING with PACIFIC SUPPLY, nor ACME with ZENITH HARDWARE — while
LOWES #02516 still agrees with Lowe's Home Improvement.
(5) The PR body and a new spec §9 operator checklist state that
RECEIPT_BRIDGE_SECRET is a new, required, distinct secret needed in TWO
places (Vercel and the Apps Script bridge). The "existing, reused" claim is
gone.
Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
…es, descriptor refresh reaches the line, closure walk, locked mark-duplicate
(1) resolveUnknownOrphan CASes on the WHOLE orphan predicate — permitted state,
the unknown-orphan reason, sendAttempted, no recorded purchase id, no live
claim, and the row version. Re-checking in JavaScript proves what was true a
moment ago; the UPDATE proves what is true at the write. A BOOKED or
ARCHIVED row is not in the permitted set at all, so a direct call against
one changes nothing. A miss is a typed `not-an-unknown-orphan` refusal.
(2) A located purchase must also carry THIS receipt's `[gtr-file:<fileId>]`
marker. Amount alone is weak — this business posts several purchases for the
same amount to the same vendor in a week — and every Purchase the pipeline
creates says which document it came from. No marker, or another file's, is
refused with its own reason.
(3) The ingest descriptor refresh now moves the QBO-minted BankLine's canonical
descriptor (and its derived payee) in the SAME transaction, narrowed to a
QBO-owned, unmatched line carrying that one observation. Without it the
observation refreshed to the text carrying `C#8516` while the line kept the
tail-less copy, so the chaser resolved those charges to `office` and the
crew was never asked.
(4) The OCC recompute walks the ±4-day link rule to closure instead of querying
a fixed ±8-day window, then reduces to the component containing the seed. A
fixed window is wrong in both directions at once; matching a FRAGMENT is how
the recompute disagreed with the batch. Capped at 200 lines with a typed
abort that leaves the chase OPEN — returning [] would close it because we
could not look.
(5) markReceiptIntakeDuplicate is one transaction: both rows locked FOR UPDATE
in id order (one statement, so there is no window between the two locks),
the original re-validated under the lock, then the write. Concurrent A→B /
B→A now yields exactly one success instead of a cycle neither receipt can
escape.
Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
…elivered cards leave a trace, bankPull is probed
(1) planParkWrites clears claimToken/claimedAt in the same write. The fence
already let a park through when the worker's lease had expired, but it left
the dead token on the row — and resolveUnknownOrphan's predicate requires
`claimToken: null`, so the orphan that park had just created could never be
resolved by anybody. The unknown-orphan control is now hidden on rows that
already carry a purchase id, whose path is "mark resolved".
(2) The open-issue pass expands each target by walking the link rule to closure
instead of one fixed ±8-day query. Its page is an arbitrary set of old
issues, not a component, so a chain reaching further than the fixed span was
matched as a fragment — a different answer from the one the line pass
reaches for the same rows. A component that will not load costs that line
its verdict (reported undecided), never a close.
(3) resolveUncertainCard("delivered") records the card on its issues inside the
same transaction, through the one writer the cron uses (now
lib/receipt-card-history.ts). Without it a hand-resolved card left no thread
for a reply to resolve against, and its items still read as NEVER CARDED —
so never-carded-first ordering put them at the front of tomorrow's card and
the crew was asked twice.
(4) readBankPullState runs inside the existing Promise.all as a probe. It was an
unprobed await afterwards whose own catch returned `{enabled:false}` — which
reads as "the pull is switched off", i.e. as health — and a hung database
held the whole check open past every other deadline. Now a failure is
`probe-failed:bankPull`, and the staleness rule only runs on a successful
read.
(5) The apply script's verifier checks overflowExact's type, nullability and
default, and a parity test asserts every column the script creates on
ReceiptRequestCard is verified — a column that is applied but unchecked is
how a nullable variant reaches production reading as verified.
Also realigned two book tests with Phase 1's send fence, which now catches a
mid-flight void through `markSendAttempted` (state + claim in one CAS) instead
of the separate pre-send read.
Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
…rds wait for the chase, delivery+history are atomic
(1) Line-pass components sitting within one link of the window's edge are
loaded to closure; the interior keeps the cheap query. Grouping over 60 days
makes a component whole WITHIN the window and says nothing about what sits
just past either end, so an edge component was a fragment — and a fragment
allocates evidence differently from the whole.
(2) A plan now carries its component's version (newest updatedAt across its
issues and candidate intakes, plus their counts, because a max() cannot see
a DELETE). It is re-read immediately before anything is applied, and a
change replans the whole component rather than committing a verdict drawn
from a world that no longer exists. Assignment is a property of the SET, so
a memo signed on the charge NEXT to this one changes this line's answer
without touching this line — the per-write fresh read could never see that.
Bounded at 3 replans; giving up leaves the chase OPEN and reports it.
(3) The cards cron refuses to select until the sweep has stamped a completed
cycle for today. Mid-cycle the open set is half-reconciled — answered items
not yet closed, missing ones not yet opened — and selection claims the
owner's whole day, so a card built from it is the only card they get. The
stamp rides the existing phase-marker row as JSON (a bare phase string still
parses, as "no completion"). Refusal is ok:false with HTTP 200 and nothing
consumed. The retry pass is exempt: it never selects.
(4) The POSTED write and the thread record now commit in one transaction — a
lost history CAS throws and takes the delivery back with it, because a card
marked posted whose items carry no thread is one nobody can answer. Plus a
bounded repair pass in the same cron that re-records history for recent
POSTED cards whose issues lack it; nothing else would ever fix those.
Also removes BookDependencies.readState, dead since Phase 1's send fence took
over the mid-flight-void guard, along with its stale comment and supplier.
Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
… input, truncated mints, verified memo artifacts, our-space threads
(2) refreshQboDescriptors takes BANK_LINE_IDENTITY_LOCK before it reads or
writes rawDescriptor/normalizedPayee. Minting and statement adoption plan
under that lock precisely so no two writers see different versions of an
identity; the refresh rewrote both columns outside it, so an adoption
planned from the old payee could commit against the new one, match nothing,
and mint a second canonical line for a transaction that already had one.
(3) The component version now fingerprints all four planner inputs: issues,
intakes, the component's bank lines (count + newest updatedAt + descriptor
hash) and the receipt-bearing expenses in the window (count + a hash of
(id, hasReceipt)). Lines are re-read BY AMOUNT AND SPAN, because an id list
drawn from the plan cannot see a line that just arrived. Expense carries no
updatedAt column at all — only createdAt and qbSyncedAt — so a timestamp
cannot see a bookkeeper attaching a receipt to an existing expense; hashing
the flag is the only fingerprint that catches it.
(4) mintFromQbo returns {complete, remainingCursor}; the pull propagates it, so
a mint stopped by its batch cap or the deadline makes the run complete:false
and the route never stamps the freshness clock. The cursor is persisted on
the window state, which is what makes a backlog that is not draining visible
at all — a cursor that never moves is a stuck mint, and without it that
looks exactly like a quiet week.
(5) The answers route verifies the memo instead of trusting its shape: `pdf_id`
is required and its Drive metadata is read (bounded, no download, no mock
fallback) before anything is written. Found records pdfId+pdfUrl and clears;
Drive saying "no such file" (or trashed) is 422 and terminal; Drive
unreachable is 503 with retry, because "we could not check" must never be
recorded as "it checked out". `signature_id` is no longer accepted.
(6) parseChatDelivery requires the thread and message to name OUR space, derived
from RECEIPTS_CHAT_WEBHOOK or given by RECEIPTS_CHAT_SPACE. A well-formed
pair from another room is the dangerous shape: it passes every syntactic
check, marks the card delivered, and points the bridge at replies that can
never arrive. Unconfigured is refused, not waved through.
Item 1 (the proxy next-action allowlist) is Phase 1's file and went there.
Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
…he Drive credential a stated prerequisite
(1) Every component's verdicts now commit in ONE transaction that first takes
FOR UPDATE on its ReviewIssue rows and the candidate ReceiptIntake rows (id
order, one statement each — the same discipline mark-duplicate uses),
recomputes the component's fingerprint from those LOCKED rows plus the bank
lines and expenses re-read inside the same transaction, and aborts the whole
component if anything moved. A fingerprint checked outside a transaction
only narrowed the window; the sibling could still move between the check and
the writes, and a per-issue write could commit half a plan — "answered AND
chased" on the same component. The lifecycle's own transaction is flattened
onto this one so its writes join the same unit.
(2) The signed-memo probe's credential is now a stated, checkable prerequisite.
`ensureDriveAuth()` also honours CompanySettings.googleDriveRefreshToken —
the admin connect flow already stored it there, but loadToken() only ever
read the local file or GMAIL_REFRESH_TOKEN, so connecting Google changed
nothing for Drive on Vercel. pipeline-health gained a `driveCredentials`
probe reporting `drive-not-configured`, and the answers route names that
case separately from a transient outage (503 `drive-not-configured` vs
`artifact-unverifiable`) because it will not fix itself. .env.example and
the spec's operator checklist now state exactly what production needs.
Also re-points isMachineOnlyBypass at Phase 1's new allowlist (0962751) rather
than keeping a second list that would drift, and rewrites the two proxy tests
that encoded the old denylist model.
Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
e86d451 to
6e30986
Compare
Required changes
VERDICT: REQUEST_CHANGES |
…r halt, card retry, constraint convergence, health probe, owner CAS 1. Every replan reloads the issue snapshot through one shared loader. A replan happens BECAUSE something moved, and the usual something is a memo signed on a sibling — which lands in openIssues/resolvedIssueKeys/detailsByKey and nowhere else. Retrying with the run-start snapshot replanned against the state that was already stale, so attempt two reached attempt one's verdict and opened a chase for a charge somebody had just answered. 2. The evidence fence is now actually exclusive: each component takes a per-component advisory lock (row locks cover rows that EXIST — they cannot exclude a second sweep reading the same Expense rows or inserting a new competitor), and the fingerprint covers every decision-driving field. Expense amount, date and vendor decide which line an expense can answer, and Expense has no updatedAt column at all; intakes are hashed on state, reason, total, date and vendor for the same reason. 3. Any failure on an open-issue page stops the checkpoint. An orphan close that threw was counted and then stepped over, so a later `?continue=1` could finish the pass and clear the cursor — stranding that issue permanently, nagging with a target nothing can answer. 4. The retry pass selects when no claim exists for an owner, and scans so it has something to select from. The sequence that lost a whole day was the ordinary one: 14:30 finds the chase unfinished and claims nothing, the chase completes at 15:00, and the 16:30 retry refused to select because there was no row to re-post. 5. Both CHECK constraints converge on their DEFINITION via pg_get_constraintdef on the owning table, replacing a mismatch. IF-NOT-EXISTS-by-name is not idempotent for a constraint, only silent: a row from an earlier revision keeps its definition forever while every re-run reports "ok". The idempotency test now allows a constraint DROP only as part of re-adding the same name, and still forbids every data-destroying form. 6. pipeline-health probes the chaser's own marker and reports `chaser-stale:<hours>h` past 26h (or `never`). A stalled chaser was invisible: the cards cron answers 200 with `skipped:"chaser-incomplete"`, which nobody sees unless they read cron logs — and everything on the Receipts tab is downstream of that sweep. 7. setMissingReceiptOwner takes the version the page RENDERED and CASes on it. Reading the current version at click time guarded against nothing an operator cares about: the row can be cleared, reopened and rewritten in between, and the read would pick up whatever it had become — so an assignment aimed at the charge somebody was looking at landed on a different question with the same id. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
|
I applied the repository’s Supabase/Postgres concurrency and migration guidance when evaluating the transaction fences and DDL.
VERDICT: REQUEST_CHANGES |
…undecided completion, TZ, artifact link, constraint quoting Codex adversarial review of PR #443 returned REQUEST_CHANGES with 8 findings; fixes 6, defends 2 (freshness fence gap and QBO cursor limit are documented, acceptable eventual-consistency tradeoffs). - Fingerprint: BatchLine now carries BankLine.updatedAt so the planned and in-transaction component fingerprints agree; previously a brand-new unmatched line (no issue, no intake) had an empty planned `newest` against a populated locked one, so every attempt replanned and the one case the sweep exists for was never chased. - Atomic transaction: applyReceiptRequestPlan takes an `abortOnError` option, set true for the per-component transaction, so a second lifecycle write failing throws and rolls back the first instead of leaving a half-applied allocation. - Undecided != completed: sweepPhaseAfter now blocks the "done" phase (and the chaserCompletedAt stamp) on contended components, not just exhaustion/errors, so an unreconciled issue set can no longer be reported as a clean cycle. - Timezone: expense day keys are derived via dayKeyInTimeZone against the resolved company zone instead of a UTC slice, matching the company-local evidence window boundaries. - Artifact link: the bridge only stores the caller's pdf_url when driveFileIdFromUrl(pdf_url) matches the verified pdf_id; otherwise it stores the probed webViewLink, so a durable-looking but unrelated link can no longer be recorded as evidence for the wrong file. - Migration/apply-script parity: both guarded DO blocks compare pg_get_constraintdef output with quotes and spaces stripped from both sides, so a camelCase column's quoted definition no longer triggers a needless DROP/re-ADD on every run. 483/483 receipt-requests tests pass; npm run build is clean. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
|
Request changes. The advertised concurrency and artifact-integrity guarantees are not actually met.
VERDICT: REQUEST_CHANGES |
…gerprint covers evidence identity Codex round-22 review on PR #443 found two real gaps: 1. Contended components (processBatchWithReplan exhausted its 3 replans) reported errors === 0, so both the open-issue pass and the line pass checkpointed their cursor past a page that was never actually reconciled. A component racing a concurrent edit could get silently skipped forever instead of retried next run. Both loops now stop cursor advancement on `pageContended > 0` the same way they already do on `pageErrors > 0`. 2. evidenceUnitKey() folds an Expense and a ReceiptIntake into one receipt via qbPurchaseId/expenseId, but componentVersionOf() never hashed those fields and the transactional re-read never selected them. A qbPurchaseId or expenseId changing between plan and commit could let the fingerprint match while the two verdicts actually evaluated different evidence groupings. Both now flow through the hash and the re-read select. Updated the source-text guard tests (receipt-round20-fixes, receipt-component-closure, receipt-requests-bridge) that literally matched `if (pageErrors > 0) break;` to match the extended condition. Full test:receipt-requests suite (483 tests) and npm run build both pass clean. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
… the evidence fence Codex adversarial review on PR #443 found a third gap after the round-22 contended-cursor and fingerprint-identity fixes already on this branch: `groupCompetingLines` joins a same-amount line into a component whenever it lands within COMPETING_LINE_ADJACENCY_DAYS (4 days) of an edge, but the transactional BankLine re-read that recomputes the component fingerprint under lock only looked ±RECEIPT_MATCH_DATE_SLOP_DAYS (2 days) beyond the component's span — the evidence fence, not the join fence. A bank line landing 3-4 days past an edge after the initial scan is a real new competitor for that component, but the re-read's date filter could never select it, so the line count in the fingerprint could never change to catch it: the fingerprint would silently match a plan that no longer reflects the true competition set. Added a second, wider range (`joinRange`, ±COMPETING_LINE_ADJACENCY_DAYS from the component's own date extremes — the tight bound for "any line that could join this component") and pointed the transactional `tx.bankLine.findMany` re-read at it instead of the evidence-width `componentRange`. The evidence-window filters (`intakeInWindow`, `expenseInWindow`) are untouched — evidence genuinely can only land within ±2 days of a line, that part was already correct. Updated the matching source-text guard test in receipt-sweep-marker.test.ts that literally asserted the old `componentRange.calendar` re-read. Full test:receipt-requests suite (483 tests) and npm run build both pass clean. Left probeDriveFile() as-is: the review flagged it for checking existence only, not PDF MIME/provenance/target fingerprint. The memo bridge is a separate system that owns issuing correctly-signed affidavit PDFs; this step only confirms the bridge finished its work (file exists, not trashed). The bridge secret gates who can create these files and the bridge enforces template/signature/target binding on issuance — adding a redundant check here would couple this consumer to the bridge's internal format for no real safety gain. A provenance/fingerprint check, if ever needed, belongs in the bridge's issuance pipeline. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
|
Request changes. Five material gaps remain.
VERDICT: REQUEST_CHANGES |
… of skipping A claimed row whose itemsJson parsed to zero items previously just continued, leaving the claim in place. The unique (owner, pacificDate) constraint then blocked any replacement for the rest of the day, so every later run repeated the same no-op claim. Delete the row under the claim token instead, freeing the slot for a fresh selection. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
|
Three blocking correctness issues remain.
I treated the first two as blockers under the repository’s Supabase/Postgres concurrency and integrity guidance. VERDICT: REQUEST_CHANGES |
What
Receipt Pipeline v2, Phase 2 (
docs/plans/PHASE-2-QUEUE-AND-MEMOS-SPEC.md), plus the prerequisite the spec flagged as its own risk 1./automation(?tab=receipts) — six groups overReceiptIntakeplus open missing-receipt issues, behind the samefinancialReportsgate as the register. Row actions are server actions insrc/lib/actions.ts, each with the permission gate and a compare-and-swap onstate.MISSING_RECEIPTjoins the closed reason-code set;src/lib/receipt-requests.ts(pure) opens exactly oneReviewIssueper unmatched bank debit older than 3 days, auto-closes on match, reopens on unmatch. Nightly at 13:00 UTC.affidavit-threads.json/chat-job-answers.jsoncontract. Ships behindRECEIPT_REQUEST_CARDS_ENABLED.No PDF is ever emailed. A test rejects a mail-helper import in any Phase 2 module.
Why
The chaser's truth is
BankLine, which until now only filled from monthly statement imports plus a laptop-runscripts/post-qbo-register.mjs. A "3-day" chase was in practice a 30-day one. The pull closes that, and per Justin's decision 3 (the QBO bank feed is bank truth) QBO register rows can now mint canonical lines themselves.Documented minting limitation
BANK_LINE_MINT_FROM_QBOdefaults OFF, and thesrc/lib/bank-line-mint.tsheader states why:amountCentsis immutable by trigger — a human resolves it;Minting is paired with adoption: a statement line matching a QBO-minted line exactly attaches to it and flips
sourceOfRecordtoSTATEMENTrather than minting a twin. Both write paths takepg_advisory_xact_lock(hashtext('bank-line-identity'))and plan inside that transaction.Review history
Three Codex rounds, all findings addressed on this branch.
Round 1 (6 blockers, 5 real issues). Server-Action bypass on the machine endpoints; freshness (minting); card idempotency; signed memos reopening nightly; void/mark-duplicate racing the worker; thread retention. Plus: qbTxnId content comparison, one-to-one evidence assignment, the query window, the retry list, and card-selection starvation.
Round 2 (5 blockers, 7 real issues). The action guard had to cover every machine bypass and run before the dev bypass; minting had to be cardinality-aware and lock-protected; the card outbox needed a token CAS with
cards[]written before posting; the resolution merge had to re-read per issue; theBOOKEDwrite had to be fenced. Plus the schema/index parity, evidence-unit folding, evidence date coverage, the owner scan's exit condition, legacycardseeding, a telemetry counter that never fired, and column-type verification.Two holes were found by tests while writing them, not by review: the card resume path let a concurrent run re-post an in-flight card (fixed with a claim lease, then the token CAS), and the first evidence-unit key did not fold the email-fallback path.
Tests
npm run test:unit— 938 pass, 0 fail (8 new suites wired in, plustest:receipt-requests).npm run build— compiled successfully,tsc0 errors, eslint 0 errors.BOOKINGnever booking;booked-after-voidparking the orphaned Purchase.e2e/receipt-requests.spec.ts— ADMIN sees every group; both bridge endpoints answer JSON 401 (never a/loginredirect) for anonymous, bogus-session-cookie, and wrong-secret callers.Nothing was run against production.
Deploy order
Both scripts are additive and idempotent, and must run before merge (CLAUDE.md pre-deploy rule #2 — the new build selects these columns immediately):
node scripts/apply-receipt-intake.mjs --yes --expect-db <db> --expect-host <host>node scripts/apply-phase2-receipt-queue.mjs --yes --expect-db <db> --expect-host <host>Then enable flags one at a time, and only after step 2.
Env vars a human must set
RECEIPTS_CHAT_WEBHOOK— incoming webhook forspaces/AAQAKhvMYtg. Unset ⇒{skipped:"no-webhook"}, fails soft.RECEIPT_OWNER_CHAT_USERS—{"CJ":"users/…","Richard":"users/…"}. Still needs collecting; a wrong id locks that owner out of signing their own memo.RECEIPT_REQUEST_CARDS_ENABLED— leave unset for a shakedown week. Turn Beverly's own asks off in the same step.BANK_LINE_MINT_FROM_QBO— leave unset until step 2 above has run.RECEIPT_BRIDGE_SECRET— NEW, REQUIRED, and DISTINCT from the other two receipt secrets. It gates both bridge endpoints (/api/automation/receipt-requests/threadsand.../answers); presentingRECEIPT_INTAKE_SECRETorRECEIPT_ARCHIVE_SECRETthere is a 403, not a fallback. It must be set in two places: Vercel production, and the Apps Script bridge that sends thex-receipt-intake-secretheader. Change both together — until they agree, the bridge is refused and signed memos stop being recorded. Beverly's bridge runs outside our repos, which is exactly why its key must not be able to book anything.RECEIPT_INTAKE_SECRET,RECEIPT_ARCHIVE_SECRET,CRON_SECRET— existing. The intake and archive keys are not reused for the bridge; each key's complete MAY / MAY NOT list is insrc/lib/receipt-intake/intake-auth.tsand.env.example, and any two of the three sharing a value is refused at runtime.The full sequence, including which order to flip the flags in, is the operator checklist in §9 of
docs/plans/PHASE-2-QUEUE-AND-MEMOS-SPEC.md.Companion changes (neither is in this repo)
mirrorReceiptRequestThreads()and an answers forwarder.Until both ship, photo and job-name replies work;
sign Nis recorded but no sign card appears.🤖 Generated with Claude Code