feat(receipts): ReceiptIntake state machine + intake endpoint + worker (pipeline v2 phase 1) by Clarion1631 · Pull Request #440 · Clarion1631/probuild

Clarion1631 · 2026-09-02T01:17:02Z

Phase 1 of the receipt pipeline v2 rebuild. Spec: docs/plans/PHASE-1-INTAKE-CORE-SPEC.md.

What and why

Today every receipt goes through a Google Apps Script that reads it with Gemini, dedups it against Script Properties, emails it to QuickBooks, and renames it into a Drive archive. That script is the single point of failure for job costing: when it stalls, expenses stop reaching ProBuild and nobody finds out until a variance report looks wrong.

This PR moves the intake, the read, the dedup and the booking into ProBuild, behind one durable row.

ReceiptIntake — one row per inbound document, with an explicit state machine (STAGING RECEIVED READ NEEDS_JOB NEEDS_REVIEW BOOKING BOOKED ARCHIVED DUPLICATE VOID NON_RECEIPT). state is a String with a SQL CHECK, matching BankLine.state and Expense.status.
POST/GET /api/receipts/intake — the one front door for the mobile app (Bearer), staff (session), and the Apps Script forwarders (x-receipt-intake-secret). Idempotent on sourceRef, and cheap: hash, store, insert, return. No AI call on the request path.
/api/cron/receipt-intake-worker (every 5 min, ≤10 rows) — reads with Gemini, dedups, routes, and books via createQBReceiptPurchase.
Dry-run shadow mode, default ON. With RECEIPT_INTAKE_DRYRUN unset, rows are read, deduped and routed, and nothing is booked: zero QuickBooks calls, zero Expense rows. That is asserted by counting injected fakes, not by reading the code.

Deliberate reuse, not reimplementation: book.ts imports createQBReceiptPurchase directly (one QBO write core, never reached over HTTP), and keys.ts is a verbatim port of the v3.6 Apps Script dedup rules so v1 and v2 agree on every archived file during the shadow week.

Design notes worth reviewing

Expense.amount is the GROSS total, tax included (Justin's call, 2026-09-01 — this overrides the plan's §4.5 pre-tax wording). The QBO Purchase still splits sales tax onto its own reclaimable account. But Expense has no tax column and the QBO-imported expenses already record the gross line total, so booking pre-tax here would put two meanings of amount in one table. ReceiptIntake.taxCents keeps the split for Phase 3's Expense.taxAmount.
The vendor is not part of the strong dedup KEY, but it is part of the CONFIRMATION. The v3.6 key is deliberately vendor-less (one store spells its own name three ways, and keying on it put one purchase on two keys). The cost is that two unrelated vendors reusing an invoice number on one day for the same amount collide — so a same-total hit with a different canonical vendor routes to a human instead of being quarantined.
The strong-key claim IS a partial unique index. The database replaces the Apps Script's Properties lock. Prisma cannot represent partial indexes and drops them silently, so it is hand-written SQL and recorded in prisma/prisma-blind-spots.json.
/api/receipts/intake bypasses the proxy (exact match, so a machine caller gets a clean 401 rather than a 307 to /login), which makes the handler the only gate. Please review the auth block specifically.

Review summary

Two Codex rounds. Round 1 raised 6 blockers and 12 issues; round 2 raised 2 blockers and 3 issues. All addressed, each with a test:

Dry-run rows were starving the queue — the batch is ten rows, and after a couple of shadow days the ten oldest were all parked ones, so no new receipt was ever reached. They are excluded from the claim predicate, with a one-shot cutover requeue inside the claim transaction.
Rows were claimable before their object existed. A row is now born STAGING and published in one UPDATE after the upload lands; stale STAGING rows are swept after 15 minutes.
Weak-key write skew: two rows sharing a weak key both passed the read-time check and both booked. A transaction-scoped pg_advisory_xact_lock(hashtextextended(weakKey)) inside the READ→BOOKING transition serializes exactly those two, without wrapping Gemini and QuickBooks calls in one long-lived pooler transaction.
sourceRef reuse with different bytes returned 200 and swallowed a second, real receipt. It is now decided on fileSha256: same bytes is a replay, different bytes is 409 and storage is never touched.
Gemini retry budget cut to 25 s/row (2 retries per model at 1 s/3 s) so one busy document cannot eat a 60 s invocation shared by ten rows; the worker stops taking rows at 40 s.
Provenance is no longer caller input for a human: source and sourceRef are minted server-side, and only shared-secret forwarders may declare drive/email/chat.
Transient throws (storage, Prisma, network) retry on the normal backoff; only classified QBO faults are terminal. A park before any QBO send releases the strong key (v3.5 rule).

Tests

npm run test:unit   681 pass, 0 fail   (+34 for this feature, in 7 new suites)
npx tsc --noEmit    clean
npm run build       0 errors

Function injection throughout, no mock.module (CI pins Node 20, where it corrupts the require chain). tests/receipt-intake-keys.test.ts uses eight real August 2026 archive filenames as fixtures — v1 built those names from the same cleaned fields, so a changed key there is a shadow-week mismatch rather than a refactor.

e2e/receipt-intake.spec.ts covers the 401 matrix (no credentials, bogus session cookie, wrong secret, empty secret), the idempotent double POST, provenance rejection, the SHA-conflict 409 and its namespace scoping, the GET role gate proven with the EMPLOYEE storage state, and the archive callback. Every negative case asserts the absence of a Location header, because a redirect is what a forwarder mis-reads as "retry later" forever.

Deploy order

Run node scripts/apply-receipt-intake.mjs --yes --expect-db <db> --expect-host <host> against production BEFORE merging. Auto-deploy is on, so merging ships this; the new Prisma client selects these columns immediately and any page touching them throws P2022 until the table exists. The script is additive and idempotent — a second run reports every statement "ok" and changes nothing.
After it runs, re-run node scripts/snapshot-prisma-blind-spots.mjs --write against production and commit the result. The new partial index and CHECK constraint were added to prisma/prisma-blind-spots.json by hand (the snapshotter needs a live production connection this branch never had), so their rendered definitions are asserted rather than observed. CI's migrations job is what will catch a mismatch.
Set both RECEIPT_INTAKE_SECRET and RECEIPT_ARCHIVE_SECRET in Vercel — both required, new, and deliberately independent of each other and of RECEIPT_INGEST_SECRET. authenticateIntake fails every request closed (401) if either is unset, and refuses both (also 401) if they are set to the same value, since that would silently re-merge two capabilities that are meant to stay apart (ingest vs. archive-read). Give each forwarder only its own value as a Script Property: RECEIPT_INTAKE_SECRET to the drive/email/chat ingest forwarders, RECEIPT_ARCHIVE_SECRET to the nightly Drive archive mirror. Missing RECEIPT_ARCHIVE_SECRET specifically would not fail loudly at deploy time — it only surfaces when the archive mirror's next poll gets a blanket 401.
Leave RECEIPT_INTAKE_DRYRUN unset — that is dry-run mode, and it is where this should sit for the shadow week. Cutover is Justin's explicit call: set it to the literal false.

The Apps Script forwarder changes are a separate PR in qbo-clasp; the endpoint contract it codes against is in spec §7, including the six places the build differs from the original plan.

🤖 Generated with Claude Code

vercel · 2026-09-02T01:17:55Z

The latest updates on your projects. Learn more about Vercel for GitHub.

Project	Deployment	Actions	Updated
probuild	Ready	Preview	Sep 2, 2026 10:21pm UTC

github-actions · 2026-09-02T03:29:04Z

Strong dedup is bypassed for exact duplicates. In worker.ts, the weak lookup happens before attempting the strong-key claim. An exact duplicate therefore gets a weak hit, routes to NEEDS_REVIEW, and—because only READ claims the strong key at line 385—never triggers the partial unique index. Add an integration test proving an existing row with matching strong and weak keys produces DUPLICATE. Also make the final weak-key transaction consider every live state; route.ts currently ignores NEEDS_REVIEW, allowing a concurrent twin to book if the first row parks before the second promotion.
A transient intake failure can strand a valid receipt permanently. After the upload at route.ts, failure of the publish update at line 244 leaves an object-backed STAGING row. Upload exceptions likewise skip cleanup. Every identical retry then returns 202 at line 316 without repairing or republishing the row; the sweeper eventually mislabels it file-missing. Implement resumable reconciliation for STAGING rows and test upload throws plus post-upload database failure.
Storage outages are treated as missing documents, and booking permits missing attachments. downloadDocBytes() collapses Supabase errors and actual 404s into null; the reader converts that directly into terminal file-missing. Later, book.ts accepts null and still creates a QBO Purchase without the receipt attachment. Distinguish transient storage failure from confirmed absence, retry transient failures, and refuse to book when the source bytes cannot be loaded.
The promised archive mirror cannot be implemented against this API. queries.ts returns only a private-bucket storagePath and projectId. The Apps Script has neither Supabase credentials to download that path nor the project name needed for the mandated archive filename. Return a short-lived signed download URL and project name, with contract tests.
Mobile/web retries are not idempotent. route.ts rejects a client retry key and line 150 generates a new UUID on every request. If the response is lost, retrying the same upload creates another durable row. Accept a stable upload identifier scoped server-side to the authenticated user. Shared-secret callers should also have their sourceRef prefix validated against source; otherwise a malformed drive request loses v1/QBO idempotency continuity.
The sensitive public-schema table has no Data API protection. The migration creates ReceiptIntake in public at migration.sql without enabling RLS or revoking anon/authenticated privileges. It contains storage paths, hashes, raw AI output, errors, and bookkeeping identifiers. Supabase explicitly requires protection for tables in exposed schemas and notes that existing projects may automatically grant CRUD privileges (Supabase RLS documentation). Add equivalent protection to both migration paths and verify it.
The new health monitor can announce “Pipeline OK” during a total database outage. pipeline-health.ts converts failed database probes to null, [], or 0; evaluatePipelineOk() then interprets the missing receipt history as a quiet week. It also never examines ReceiptIntake, so stuck RECEIVED/BOOKING rows and review backlog are invisible. Represent probe failure as unhealthy/unknown and cover database failure and aged-intake cases. Additionally, the advertised CRON-secret access to /api/health/pipeline is unreachable because proxy.ts excludes only exact /api/health; either add a safely guarded exact bypass or remove the dead authentication path.
Gemini 500/502/504 responses are misclassified as document failures. read.ts retries only 429 and 503; every other status reaches the decisive branch and permanently parks an otherwise valid receipt. Treat transient HTTP statuses—at minimum all 5xx—as availability failures and add coverage.

VERDICT: REQUEST_CHANGES

github-actions · 2026-09-02T04:25:10Z

Request changes. Several paths can lose receipts or report incomplete bookings as successful.

A missing STAGING object returns HTTP 202 with ok: true (route.ts). The documented forwarders retry only non-2xx responses, so if the first uploader crashes or deletes the staging row after an upload failure, another caller can accept the 202 and discard its source document even though nothing durable exists. Return a retryable non-2xx until the object is confirmed. Also, the sweeper blindly labels every old staging row file-missing (worker route); it must publish rows whose object exists and distinguish transient storage failures.
The worker clears the 10-minute claim lease while processing is still underway. applyRead sets nextRetryAt: null (worker route) before the weak-key lookup occurs (worker.ts). An overlapping invocation can reclaim and book that READ row while the original invocation is still routing it, and the original can subsequently regress it to NEEDS_REVIEW. Keep the lease until routing finishes or make publication and the weak decision atomic with conditional state transitions.
The same premature READ publication permanently bypasses weak dedup in shadow mode. If findWeakHit or the following state update throws after line 415, retryRow leaves the database state as READ; dry-run rows in that state are then skipped forever (worker.ts). Shadow results can therefore claim a receipt was fully deduped when the weak check never completed. Add an explicit resumable state or revert to RECEIVED on failure, with a regression test.
Booking ignores CreateQBReceiptPurchaseResult.attachment. The QBO core returns ok: true for HTTP attachment failures (qbo-receipt-push.ts), unsupported/oversized files as skipped (qbo-receipt-push.ts), and other failures as failed:*; bookReceipt nevertheless creates the Expense and marks the intake BOOKED (book.ts). This affects every accepted text file and files between the intake’s 15 MB limit and QBO’s 8 MB limit. Preflight deterministic incompatibilities before creating the Purchase and retry recoverable attachment failures instead of declaring success.
The claimed “park-before-send releases the strong key” invariant is not implemented consistently. Weak duplicates explicitly retain it (worker.ts), while every ok:false QBO result sets releaseStrongKey: false (book.ts). Outcomes such as missing-vendor occur before qbCreateFn is called (qbo-receipt-push.ts), so these rows can quarantine corrected submissions against a Purchase that never existed. Release the claim for all provably pre-create parks and test the actual boundary around the Purchase write.
The archive callback is not idempotent under concurrency. Two identical callbacks can both read BOOKED; the winner archives it and the loser receives 409 when its conditional update returns zero (archived route). Re-read after a zero-count update and return 200 when the now-archived row contains the same Drive file ID.
The new health check can report green for precisely the failures it is intended to detect. It excludes all STAGING rows and only counts RECEIVED/BOOKING (pipeline-health.ts), so a dead worker leaves stale staging rows invisible, and a worker dying after read publication leaves live, non-dry-run READ rows invisible. Include overdue staging rows and overdue READ rows where dryRun=false.
package.json defines test:receipt-intake twice (package.json). The latter silently overwrites the former and drops receipt-intake-archive-contract.test.ts from the feature-specific command. Consolidate the entries so the advertised suite actually runs every intake test.

VERDICT: REQUEST_CHANGES

github-actions · 2026-09-02T04:40:26Z

Found eight release-blocking issues:

Shadow cutover can double-book non-Drive receipts. route.ts promotes every shadow READ/BOOKING row to live, even though v1 booked those documents during the shadow week. book.ts only preserves v1’s identity for drive rows; email/chat rows use a new intake UUID. QBO therefore cannot recognize their v1 purchases. Do not blanket-requeue shadow observations; reconcile them against v1 identity or only book rows received after cutover. Add a non-Drive shadow-cutover regression test.
A crash before upload permanently strands the receipt. On a same-byte retry of a STAGING row with no object, route.ts returns 202 but never uploads the bytes carried by the retry. After the sweeper changes it to NEEDS_REVIEW, later retries return 200 despite the object still being absent. Resume the upload using the existing reservation, safely handle concurrent uploads, verify the object, and publish it. Test crashes after insert and failed best-effort deletion.
New machine endpoints permit anonymous Server Action dispatch. proxy.ts bypasses /api/health/pipeline and all /api/cron/*, while the action-deny pattern covers only receipt intake. A request carrying next-action can be dispatched before route-level Bearer/session auth runs—the exact vulnerability the receipt-specific guard discusses. Deny Server Actions on every machine-only bypass, including the new health and cron routes, and add anonymous dispatch tests.
The rollout script cannot upgrade the pre-STAGING schema. apply-receipt-intake.mjs skips the state constraint whenever one with that name already exists. An earlier script version’s constraint without STAGING remains in place, yet intake now inserts STAGING, causing every POST to fail. Replace or validate the constraint definition and update the column default; verification must use pg_get_constraintdef, not merely the name.
The worker is not non-overlapping as specified. The advisory lock in route.ts is released when the claim transaction ends; processing occurs afterward. Another invocation can immediately claim another ten rows and run concurrently, including concurrent token refreshes and QBO writes. Implement a durable batch lease covering the whole pass, or remove the non-overlap claim and prove every downstream operation supports concurrency.
Bookings are finalized even when QBO failed to attach the receipt. createQBReceiptPurchase reports ordinary upload failures as ok:true, attachment:"failed:*", but book.ts ignores that status and commits Expense plus BOOKED. Nothing retries afterward. This contradicts the stated “never a Purchase without its receipt” invariant. Retry transient attachment failures and route deterministic failures to review; only finalize after attached or already-attached.
The strong key is retained for a pre-QBO weak-match park. worker.ts moves the row to NEEDS_REVIEW while deliberately retaining its strong key. That contradicts the PR’s rule that every park before a QBO send releases the claim and can quarantine corrected resubmissions against an unbooked row. Release it transactionally and add coverage for weak-hit correction/resubmission.
Pipeline monitoring misses two stuck worker states. pipeline-health.ts counts only old RECEIVED and BOOKING rows. Live READ rows are claimable work too, and stale STAGING rows depend on this same worker’s sweeper. A dead worker can therefore leave the queue stuck while health remains green. Count READ where dryRun=false and sufficiently old STAGING, while excluding intentionally parked shadow rows.

VERDICT: REQUEST_CHANGES

github-actions · 2026-09-02T04:58:54Z

Required changes:

[P0] Cutover can double-book receipts already processed by v1. The worker requeues every dry-run READ/BOOKING row when the environment flag changes (worker.ts, route.ts). For email/chat rows, booking uses the intake UUID rather than v1’s Drive-file identity (book.ts), so QBO idempotency cannot recognize purchases v1 already created during shadow mode. Do not automatically activate the entire shadow backlog; require reconciliation or a proven shared v1 identity. Add a cutover test covering pre-cutover email/chat rows.
[P0] Existing QBO purchases are marked booked even when attachment recovery fails. createQBReceiptPurchase reports attachment status for an existing purchase (qbo-receipt-push.ts), but book.ts validates that status only when alreadyExists is false (book.ts). Worse, the lastError early-park prevents attachment recovery from being attempted on retries (book.ts). Require attached or already-attached on both paths and add tests for existing-purchase failed, skipped, and successful recovery outcomes.
[P1] An idempotent replay can return 200 despite the document being absent from storage. Storage existence is checked only while the row remains STAGING (route.ts). Once the stale-row sweep changes an orphan to NEEDS_REVIEW/file-missing, a same-byte retry receives success and the forwarder may discard its only copy. Resume the upload or return a retryable failure whenever the object is missing, regardless of state. Add an E2E test for replaying a swept file-missing row.
[P1] Gemini output validation fails open and can book a non-receipt. Missing doc_type defaults to "receipt", while arbitrary values also avoid the exact multi/non_receipt routing checks (read.ts). A malformed or prompt-injected response with plausible amount fields can therefore reach QBO. Validate against a closed enum and route missing or unknown values to NEEDS_REVIEW.
[P1] The unreadable-date fallback uses UTC instead of the company timezone. The worker derives the fallback with toISOString().slice(0, 10) before applying the company timezone (worker.ts). Around midnight UTC this changes the receipt date, dedup key, and reporting period relative to the intended local upload date. Compute the calendar date in the company timezone and add boundary tests.
[P1] The weak-hit review path violates the stated strong-key release rule. A weak duplicate routed to NEEDS_REVIEW retains dedupStrongKey (worker.ts), despite no QBO send occurring and the PR explicitly promising that pre-send parks release it. This can make a corrected resend appear duplicate against an unbooked row. Clear the strong key and test the correction/resubmission flow.
[P1] The 40-second worker budget excludes the potentially unbounded stale-STAGING sweep. Timing begins only after the sweep, which can sequentially download up to 50 full objects without a storage timeout (worker.ts). The sweep can consume the platform timeout and the worker will still start Gemini or QBO work. Start the deadline at invocation entry and bound sweep downloads by deadline, timeout, or a much smaller batch.
[P1] NEEDS_JOB receipts are invisible to pipeline-health alerts. Health reporting counts stuck RECEIVED, BOOKING, STAGING, live READ, and NEEDS_REVIEW, but omits terminal NEEDS_JOB rows (pipeline-health.ts). Those rows can accumulate indefinitely while the digest remains green—the exact silent-failure mode this pipeline is supposed to eliminate. Include them in the actionable backlog and alert output.

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>

github-actions · 2026-09-02T05:17:42Z

Required changes:

Cutover blindly requeues every shadow row as live (route.ts). Shadow mode explicitly leaves originals for v1 to book, while non-Drive v2 rows use a new intake UUID as their QBO identity (book.ts). Email/chat backlog will therefore create duplicate QBO Purchases at cutover. Do not rebook shadow history unless it is reconciled to the existing v1 Purchase; add non-Drive cutover coverage.
Attachment recovery is internally contradictory. The QBO core now repairs attachments on existing Purchases, but bookReceipt refuses to call it after an attachment failure (book.ts), then ignores attachment status whenever alreadyExists is true (book.ts). This can mark a row BOOKED with no receipt attached. Remove the preemptive park and require attached or already-attached for both result branches; retry failed:* and review skipped.
Pre-QBO retry exhaustion incorrectly retains the strong key. Storage failures before any send use the generic retry helper, whose terminal result always sets releaseStrongKey:false (book.ts). Worker failures after claiming a key have the same defect (worker.ts). Track whether a Purchase send was actually attempted and release the key when a pre-send path reaches its retry ceiling.
The advertised idempotent migration cannot upgrade the pre-STAGING schema. It upgrades only busyPasses (apply-receipt-intake.mjs); an existing ReceiptIntake_state_check without STAGING is left untouched, as is an existing RECEIVED default. Every new insert then fails its CHECK. Explicitly upgrade the default and replace/validate the old CHECK in both rollout SQL paths, with an old-schema upgrade test.
The worker has no enforceable overall QBO budget. Its function ceiling is 60 seconds (route.ts), but token refresh may consume 45–50 seconds and each subsequent serial QBO request gets another 20 seconds. Under degradation, Vercel can kill the invocation before retry state is persisted, producing an endless BOOKING loop. Add a shared per-row deadline comfortably below the function ceiling.
Unreadable receipt dates use the UTC createdAt calendar date (worker.ts), despite the stated company-time-zone semantics. Evening Pacific uploads receive tomorrow’s date, corrupting txnDate and both dedup keys. Format the fallback date in the resolved company timezone and test the UTC-day-boundary case.
An ambiguous upload exception deletes the durable row (route.ts). If Storage committed the object but the response was lost, this creates an unreachable orphan and the retry creates another object. Preserve STAGING for ambiguous transport failures so retry/sweeper probing can publish it, or confirm absence/remove the object before deleting the row.

VERDICT: REQUEST_CHANGES

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>

github-actions · 2026-09-02T15:28:24Z

Required changes:

[P0] Reissued upload URLs still race destructive sweeping. /start signs first, updates the lease afterward, ignores the update count, and returns the URL (start route). Meanwhile the sweeper decides from a stale lease snapshot (worker route), and the rejection fence does not include uploadUrlExpiresAt or a lease generation (storage-cleanup.ts). The sweeper can delete the row, settle cleanup, and then leave the client uploading through a still-valid URL into an orphan path. Acquire/version the lease before returning a URL and fence every park/reject on that version. Add a real resume-versus-reject interleaving test.
[P1] The two intake paths still disagree on sourceRef. Inline intake only checks startsWith() (route.ts), bypassing the bounded shape validator advertised as shared (intake-core.ts). Thus drive: and oversized indexed values remain accepted. Worse, the helper’s email/chat regexes do not accept the documented forwarder identities (email:<message>:<sha16> and chat:<message>:<idx>) (spec, patterns). Centralize both routes on one validator aligned with the actual qbo-clasp contract, and test the exact production identity formats through both endpoints.
[P1] Booked non-Drive receipts get a permanently broken ProBuild link. Booking stores receipt-intake:<path> in Expense.receiptUrl (book.ts), although that helper explicitly describes the value as “never dereferenced” (bucket.ts). Existing expense UI renders receiptUrl directly as an href (ExpensesTab.tsx); neither it nor resolveDocUrl understands the new scheme. Store a stable bucket-qualified reference and resolve it to a short-lived signed URL at every reader, including archive-state handling.
[P1] The advertised 15 MiB intake path accepts files the booking path categorically cannot process. Storage accepts 15 MiB (intake-core.ts), while booking rejects everything over 8 MiB before QBO (book.ts, book.ts). Such a document is accepted, paid to store and read by Gemini, then parked without an Expense and without a supported replacement path. Align the intake ceiling with the actual attachment ceiling or implement a validated conversion/replacement workflow.
[P1] Early routing outcomes retain dead worker ownership forever. Multi-document, non-receipt, zero/refund, and no-project outcomes transition through applyRead (worker.ts), whose database write deliberately preserves claimToken, claimedAt, and nextRetryAt (worker route). Those terminal rows are never reclaimed, so the ownership fields never clear and later repair code using claimToken: null is fenced out. The claimed “every transition releases” test misses this path because it only scans the route below applyState (claim-release test). Make the early transition release ownership atomically and add a behavioral test.

VERDICT: REQUEST_CHANGES

…CES not rows (A) The boolean `secureObjectExists` collapsed "confirmed 404" and "storage is unhappy" into false, and the replay path re-uploads and re-points on a false — so a transient fault orphaned the object that was really there and left the row pointing at a second copy. The route already reads the tagged `receiptObjectSize` (c2e6408) and answers 503 on transient; this deletes the collapsing helper so nothing can reach for it again, and adds the classification tests (empty listing and 404 are missing; 5xx/401/429/throw/sizeless are transient) plus a guard that the fault branch precedes the healing one. (B) finalize took `via === "secret"` as blanket authority over any id, so the Apps Script key could publish, re-point and attach a job to a mobile capture or web upload that belongs to a person. It now selects the row's `source` and requires it in `auth.allowedSources` — the same list that scopes creation — answering 403 source-not-owned before any detail is returned or written. Unit guard plus an e2e that seeds a mobile row and proves nothing is disclosed or changed. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>

github-actions · 2026-09-02T15:48:44Z

The two intake paths enforce incompatible sourceRef contracts. The inline handler in route.ts merely checks the namespace prefix instead of using decideSource, so it accepts empty, malformed, control-character, and oversized references such as drive:. That value later loses the Drive identity and falls back to an intake UUID for QBO idempotency. Conversely, intake-core.ts rejects the email/chat formats explicitly documented in PHASE-1-INTAKE-CORE-SPEC.md. A forwarder payload can therefore succeed through inline intake but fail through /start. Route both paths through one validator, reconcile its grammar with the published contract, and test empty, malformed, and oversized references on both paths.
A post-QBO transient failure can release the strong dedup key after a Purchase exists. In book.ts, the second phase lookup occurs after the protected QBO-send block. If it throws on the final retry, worker.ts parks the row using the stale claimed-row snapshot. parkTerminal then consults that stale row.sendAttempted at worker.ts, even though the send hook has already persisted sendAttempted=true, and can clear dedupStrongKey. A corrected or resubmitted receipt may then create a second Purchase. Route every post-send failure through purchaseMayExist(sent), or reload the persisted send state before releasing the key. Add a final-attempt regression test where post-create phase validation throws.
The committed migration does not converge an existing state constraint. migration.sql only creates the CHECK constraint when absent. The deployment script at apply-receipt-intake.mjs correctly replaces it when its definition is stale. Thus an existing table with an earlier state set makes the migration report success but later rejects SHADOW_DONE or SHADOW_QUARANTINE. Make both DDL paths converge identically and replace the current token-presence test with a semantic parity test.
Non-Drive receipts are recorded as having Drive file IDs. book.ts substitutes row.id when no Drive ID exists and emits it as detail.fileId; automation-events.ts interprets that field as a typed driveFileId. Email, chat, mobile, and web receipts consequently produce invalid Drive links and corrupt provenance/evidence queries. Only emit fileId for actual Drive sources, and keep the generic booking identifier in a separately named field.

VERDICT: REQUEST_CHANGES

…s, atomic terminal release Round-14 items 2-5 plus the three interim ones. Item 1 (uploadLeaseVersion) is NOT in this commit — it is next. (2) The sourceRef validator now matches what the Apps Script actually sends: `drive:<fileId>`, `email:<gmailMsgId>:<sha16>`, `chat:<messageResourceName>:<idx>`. One implementation in decideSource, so both endpoints get it; e2e drives the production formats and the `drive:` / oversized rejections through both doors. (3) Expense.receiptUrl holds `receipt-intake://<bucket>/<path>` — a signed URL written into that column is dead ten minutes later, and a bare path does not say which bucket. resolveReceiptUrl() mints a short-lived URL and follows an object that moved (sealed, archived) via the intake row. Wired into resolveDocUrl (so every existing reader gets it), the expenses tab loader, and ai-review, whose SSRF check now names the `/sign/` prefix and runs on the RESOLVED url. (4) One ceiling everywhere: QBO_ATTACHMENT_MAX_BYTES = 8 MiB, used by the bucket policy, /start's declared-size check, inspectStoredObject and the booking preflight. 15 MiB at the door and 8 MiB at the books meant everything in between was stored, read, and then stranded after we had told the sender we had it. (5) Early terminal outcomes (multi-doc, non-receipt, zero/refund, no-job) now go through applyState, which releases claimToken/claimedAt/nextRetryAt in the same fenced write. applyRead is the ONE lease-keeping write and its type pins it to "RECEIVED", so a terminal state cannot be routed back through it. (A) Every post-send step (the post-create phase check, the Expense commit) is inside the protected block, and parkTerminal re-reads the PERSISTED send flag instead of the claim-time snapshot — an unreadable flag RETAINS the key, because retaining costs a review and releasing wrongly costs a second Purchase. (B) migration.sql converges on the state CHECK exactly like the apply script (drop-if-different + add). The token-presence test is replaced by a semantic parity one comparing state order, convergence, scoping and the wanted_def literal. (C) `detail.fileId` is a DRIVE id or absent — it is dual-written into the driveFileId column the cutover queries. Non-Drive rows carry `intakeId`, which is now a first-class identity in journey grouping and keying (two v2 receipts sharing a DocNumber prefix no longer merge into one journey). Mutation-tested: the claim-snapshot park, the non-converging migration. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>

…e rejected Round-14 item 1. `ReceiptIntake.uploadLeaseVersion` (schema + migration + apply script + verifier) is bumped every time a signed URL is issued, and it is IN the path that URL points at: `receipts/intake/<id>.v<n>.<ext>`. - /start claims the lease in ONE checked update — version, expiry and path move together — BEFORE it signs anything, on both the resume and the re-arm path. A 0-row update is a 409 publish-conflict rather than a URL for a row somebody else has moved on, and the previous lease's object is queued for cleanup. - Every destructive or publishing write fences on the version it observed: both sweeper parks, the sweeper's publish commit, the reject transaction, and publishFence (so /finalize and the single-shot heal carry it too). - The reject transaction now RE-READS the row inside itself and lets the caller judge it; the sweeper passes a verifier that refuses while the upload lease is live. The version catches a resumed lease; the re-read catches a refreshed expiry on the same one. Tests: the real interleaving (sweep decides on v1, client resumes to v2 → the fence loses, nothing deleted, nothing queued), a lease that comes back to life inside the transaction, an unchanged-lease control, and a guard that /start moves the row before it signs on both paths. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>

…ylisting it Next's action IDs are GLOBAL: the path a `next-action` POST is sent to only decides whose middleware runs first, not which action runs. The guard was a DENYLIST (legal pages + machine endpoints), which is the wrong shape for a global namespace — every other public-bypass path was a live anonymous dispatcher: /api/auth, /api/mobile, /api/pdf/*, /api/portal, /api/payments, /api/selections/*, /login, /share/*, and the asset patterns. Now: an action dispatch with no session cookie is 403 BEFORE the public bypass unless the path is allowlisted. The allowlist is the output of an audit of the anonymous route trees for server actions invoked from their client components: /portal/** approveEstimate, approveContract, approveChangeOrder, mark*Viewed, createDecision, submitSelectionProposal, portalCreateMoodBoard, setPortalStageOverride, ... /sub-portal/** subPortalUploadCOI and the sub sign-in flow. Audited and deliberately NOT allowlisted: /login (next-auth signIn is a plain POST to /api/auth/*, not an action), /share/** (server component reading Prisma; its one client child imports no actions), the legal pages, and every /api route (Next dispatches an action to the page URL the client is on, never to a route handler). Tests drive real requests through the proxy — 20 refused paths, 8 allowlisted ones, prefix-not-substring cases, and the same paths without the header — because the bug was an ORDERING one that a helper-level assertion cannot see. Mutation-tested: removing the guard and widening it to a substring both fail. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>

github-actions · 2026-09-02T16:57:05Z

Required changes:

Inline intake bypasses canonical sourceRef validation. route.ts merely checks the namespace prefix instead of calling decideSource(). It accepts malformed values such as drive:, drive:short, and oversized references. Multiple receipts can therefore collapse onto one permanent idempotency key and be silently discarded as replays. This also directly contradicts the added E2E expectations. Use the shared validator for both intake paths.
Two-step replay paths can return 200 when ProBuild no longer possesses the file. start/route.ts and finalize/route.ts declare settled rows successful without checking storage. Forwarders interpret 2xx as permission to delete their only copy, leaving an intake that the worker later parks as file-missing. Verify object metadata before every replay-success response; return a retryable/non-success response when presence cannot be established, with regression tests for missing objects on settled rows.
Inline STAGING rows receive a fictitious two-hour upload lease. worker.ts treats a null uploadUrlExpiresAt as active for the signed-upload TTL, although inline requests have no signed URL. Consequently, crashed inline uploads remain unresolved for two hours instead of the promised 15-minute sweep. Use the stale-STAGING threshold when no lease exists. Also make the bounded sweep ordering/filter deterministic so live leases cannot repeatedly occupy all ten slots and starve actionable rows.

VERDICT: REQUEST_CHANGES

…bject; inline orphans sweep (1) The gate was right and my 8418a67 report was wrong: the inline endpoint carried a hand-written TWIN of decideSource — plus its own MACHINE_SOURCES, USER_SOURCES and UUID_PATTERN — so the shared validator only ever ran on /start. The copy had drifted twice over: it checked the global source set instead of the sources THIS key owns (`auth.allowedSources`), and it validated only the namespace prefix, so `drive:` with an empty tail was a permanent unique idempotency key that every later empty-tail forward collided with. The route now calls decideSource() itself and the copies are deleted; a source tripwire pins the import and the absence of each copy, and the e2e drives `drive:`, `drive:short` and an oversized ref through the inline endpoint. (2) A replay that answers "we already have it" now proves it first, on both paths: bounded metadata (one list call), present → 200, absent → 409 file-missing with retryable:true, transient → 503. The forwarders delete their only copy on a 2xx, so a row whose object had vanished was making receipts cease to exist. The lost-publish branch checks where the row points NOW, since the winner sealed the object to a new path. (3) A row with no `uploadUrlExpiresAt` never had a signed URL — the inline path writes through the server — so it now gets the 15-minute stale-STAGING grace instead of the two-hour signed-URL TTL that made every inline orphan invisible. The sweep query excludes live leases in SQL and orders null-lease rows first, then oldest, so clients still uploading cannot occupy all ten slots while the orphans behind them are never reached. Mutation-tested: the 2h fallback restored fails the new lease test. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>

github-actions · 2026-09-02T17:44:26Z

Required changes:

/start breaks the promised sourceRef invariant. route.ts skips identity validation for recoverable rows, overwrites expectedSha256, and clears fileSha256. A previously published receipt later parked as file-missing can therefore be rebound to entirely different bytes. Require the new SHA to match fileSha256 || expectedSha256; otherwise return 409.
The finalize CAS-loss path can falsely return success. route.ts treats every non-STAGING state as evidence another publisher succeeded. A concurrent /start rearm leaves the row in recoverable NEEDS_REVIEW; if its new upload exists, finalize returns alreadyFinalized:true even though no publish occurred. Require positive evidence that the row published the same hash/canonical path, or return 409.
Attachment idempotency is based only on the caller’s filename. qbo-receipt-push.ts treats any Purchase attachment named receipt.jpg, IMG_1234.jpg, etc. as this receipt. bookReceipt then accepts already-attached and creates the Expense even when the actual receipt bytes were never attached. Use a filename derived from stable receipt identity/content hash and test an unrelated same-name attachment.
The tax-implausible warning is erased before booking. The reader deliberately records the warning, but READ→BOOKING clears it at route.ts, and BOOKED clears it again at book.ts. Automatically booked receipts therefore become indistinguishable from receipts with no tax reading, hiding potentially reclaimable tax from review. Preserve and surface the warning through the full lifecycle.

VERDICT: REQUEST_CHANGES

…naming, tax warning preservation Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>

github-actions · 2026-09-02T18:25:59Z

Required changes:

Newly booked receipts produce broken UI links. book.ts stores receipt-intake://..., but the bookkeeper and estimate views pass that value directly to <a href> in ReceiptQueueClient.tsx and ExpensesTab.tsx. Resolve these references server-side everywhere expenses are loaded and add integration coverage for opening a v2 receipt.
The advertised 60-second worker budget is not enforced. worker.ts may start a RECEIVED row immediately before the 40-second cutoff, while readReceipt independently receives a fresh 25-second budget. That invocation can run to roughly 65 seconds and be killed mid-row. Pass the invocation’s remaining absolute deadline into the reader or stop initiating reads early enough to guarantee completion.
Storage cleanup is not durable across publication races. stored-object.ts creates the canonical object before its CAS, but a lost CAS leaves that object unreferenced and unqueued. Additionally, start/route.ts deletes the previous lease path while its old upload capability remains valid; Supabase upload URLs remain usable for two hours, so a late upload can recreate an object the database will never reference (Supabase documentation). Queue failed canonical publications and implement an expiry-aware orphan sweep for obsolete lease paths.
Health monitoring contradicts the upload lease state machine. pipeline-health.ts declares every 30-minute-old STAGING row stuck, while signed upload leases are intentionally valid for two hours and worker.ts explicitly honors uploadUrlExpiresAt. Legitimate uploads will therefore trigger false alarms. Use lease expiry for two-step rows and the age threshold only when no upload lease exists.
The PR violates its own scope boundary. The spec explicitly says not to modify quickbooks.ts or the QBO receipt route, yet this diff rewrites large portions of both and adds extensive unrelated payment, email, and health-pipeline changes. Split those changes from Phase 1. If any money-path changes remain, the repository requires a green e2e/money-pipeline.spec.ts, which is absent from the reported test evidence.

VERDICT: REQUEST_CHANGES

…ha256 bricking recovery The e2e storage mock had no `list()`. That was not an omission, it was a wrong answer: `receiptObjectSize` and `secureObjectSize` both establish "is this object there, and how big is it" from list metadata, so `from.list` being undefined made the call THROW — and both callers classify a throw as TRANSIENT, i.e. "storage hiccuped", not "the object is gone". Every intake replay that reached the existence check answered 503 instead of the 200-or-heal it had earned. 12 of the 16 red receipt-intake e2e cases were this one seam. Also fixed, both surfaced by the newly-working existence check: * /start's recovery guard protected `fileSha256 || expectedSha256`. Only `fileSha256` is ever verified against real bytes; `expectedSha256` is a promise a client made about bytes it was about to upload, and on a recoverable park that promise is exactly what was never kept. Both recoverable parks are reached from STAGING where `fileSha256` is "", so the unkept announcement became the identity to protect and a forwarder coming back with a corrected hash got 409 forever on a sourceRef that had never held a document. Narrowed to the verified hash; nothing can be overwritten, because a rearm writes to a NEW lease path and stays parked until /finalize verifies. (Regression from 390a62e, masked by the red suite.) * The "park a re-upload CANNOT fix" case asserted `alreadyReceived` for a row whose object was never uploaded — /start cannot carry bytes and the spec cannot PUT to a signed URL. Seeded via the single-shot route, the same trick the two neighbouring cases already use. Endpoint-level sourceRef parity (Phase 3 finding): the inline endpoint already routes through decideSource() as of afda5be, but nothing proved the two doors AGREE. Added a case-table driving right-namespace/wrong-shape, control characters, whitespace, oversize and namespace-mismatch refs through both, and asserting the same status AND the same reason from each — a forwarder that can tell the doors apart will learn to prefer the lenient one, which is how they drifted in the first place. tests/supabase-storage-mock.test.ts pins the method surface and the missing-vs-transient classification, so the next omitted method fails loudly instead of looking like flaky infrastructure. Mutation-tested: all 8 fail with `list` removed. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>

github-actions · 2026-09-02T18:50:13Z

I could not review the diff: every local read failed before execution with bwrap: loopback: Failed RTM_NEWADDR: Operation not permitted. The GitHub fallback had no repository access, and the PR was not publicly discoverable. Approving without seeing /tmp/pr.diff would be fabricated.

Re-run with filesystem access restored, or attach/paste /tmp/pr.diff, so the implementation can be reviewed against the spec.

VERDICT: REQUEST_CHANGES

github-actions · 2026-09-02T19:08:30Z

Required changes:

Fix the impossible Expense review lifecycle. bookReceipt() creates an Expense as Pending with a qbPurchaseId, but the approve/delete routes reject any QBO-linked Expense. The QBO sync can later mark it Reviewed, silently bypassing human review. Define one coherent lifecycle and add an integration test covering booking through review.
Honor the worker’s actual 60-second deadline. worker.ts may start reading at 40 seconds, while read.ts grants that read a fresh 25-second budget. That can exceed maxDuration, kill the invocation, and repeatedly strand leased rows. Pass an absolute deadline through the reader or reserve the full read budget before claiming work.
Resolve stored receipt references everywhere. book.ts stores receipt-intake://... references, but manager/receipts/page.tsx passes raw receiptUrl values to browser links. Newly booked receipts therefore cannot be opened from the primary review screen. Audit every Expense consumer and resolve references server-side.
Close the storage-orphan races. sealAndPublish() leaves the canonical object behind when its commit CAS loses. Rearming intake deletes the old path while its two-hour signed upload URL remains valid, allowing a late client to recreate an untracked object. Queue CAS-losing objects for guarded cleanup and sweep obsolete versioned paths only after upload URLs expire.
Stop flagging valid uploads as stuck. Pipeline health treats old STAGING rows as stuck based solely on createdAt, even when their upload lease remains valid for two hours. Base the alert on uploadUrlExpiresAt/lease state, or legitimate mobile uploads will produce false incidents.
Provide the required money-path regression evidence. This PR modifies QBO/expense/payment-adjacent code far beyond the stated intake core, yet its test report omits the repository-mandated e2e/money-pipeline.spec.ts. Split unrelated changes or demonstrate that suite is green before merge.

VERDICT: REQUEST_CHANGES

…line, receipt links, storage orphans, stuck-upload detection Addresses 5 of 6 REQUEST_CHANGES findings from the Codex adversarial review of PR #440 (a defense for the 6th, the money-pipeline e2e requirement, is posted as a PR comment): 1. bookReceipt() now writes a freshly-booked Expense as status "Reviewed", not "Pending" — it already carries a qbPurchaseId, so it is QBO-managed from birth like every other linked Expense, and approve/edit/delete already reject anything with a qbPurchaseId (qbo-expense-guard.ts). The old "Pending" status put it in the bookkeeper's actionable review queue with no route able to act on it, and a later QBO sync flipping it to "Reviewed" looked like human review that never happened. 2. The intake worker's per-row Gemini read now shares the invocation's ONE deadline instead of getting a fresh 25s budget regardless of how much of the 60s maxDuration is left (worker.ts's new readBudgetFor, wired into the cron route via remainingBudgetMs). A row reached late in a batch gets whatever runway is actually left, or is skipped (AI_UNAVAILABLE, no `attempts` spent) if there is not enough of it to be worth starting. 3. manager/receipts/page.tsx now resolves `receipt-intake://` references to short-lived signed URLs (the new resolveReceiptUrls batch helper in receipt-url.ts) before handing expenses to the client — newly booked receipts can now be opened from the bookkeeper review screen. 4. sealAndPublish() no longer leaks the canonical object it sealed when its commit CAS loses. It now checks where the row's storagePath actually points: if some OTHER publisher's content is there, the copy this call made is a genuine orphan and is cleaned up (reusing the existing deleteObjectOrRecord retry-queue mechanism); if the winner is pointing at this exact same content-addressed path (a double-publish race on identical bytes), nothing is deleted. 5. Pipeline health's STAGING stuck-count now also checks uploadUrlExpiresAt (falling back to the sweeper's own STAGING_SWEEP_MINUTES grace window for rows with no signed URL), not createdAt alone — a slow upload still inside its own two-hour lease no longer reads as a stuck receipt. All touched suites green: test:receipt-intake (363/363), test:pipeline-health (55/55), test:unit (1210/1210), and `npm run build` (typecheck + next build). Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>

Clarion1631 · 2026-09-02T20:53:29Z

Pushed fixes for findings 1-5 (862c7d7). Defending finding 6 rather than fixing it:

6. e2e/money-pipeline.spec.ts evidence. This PR's surface is a new,
parallel intake path (ReceiptIntake state machine, /api/receipts/intake/*,
the intake worker/book.ts) that writes a Purchase + an Expense row. It
does not touch payments, milestone settlement, signing, invoice generation,
or any of the mirrored-schedule/payment-notification code
money-pipeline.spec.ts exists to guard (sign -> convert -> invoice chain,
mirror links, undo restore, exactly-once activity writers — see
CLAUDE.md's money-path-invariants section). The QBO/expense-adjacent files
this PR touches are the receipt-booking side of the ledger, not the
payment/milestone side that suite exercises.

That suite already runs in CI on every PR against this repo independent of
what a given branch touches, so its outcome isn't something a PR needs to
separately attach evidence for — CI will report it red on this PR the same
as any other if it regresses. I did run the full local suites this PR's own
tests live in (test:receipt-intake 363/363, test:pipeline-health 55/55,
test:unit 1210/1210) plus npm run build, all green.

If there's a specific interaction with the money pipeline you're concerned
about (a shared table, a shared notifier, a shared lock) I'm happy to name it
and add a targeted test — but I don't want to pad this PR with an unrelated
suite's evidence for correctness this code path doesn't affect.

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>

github-actions · 2026-09-02T22:03:30Z

Critical — concurrent inline retries can delete an already accepted receipt. In route.ts, request A creates STAGING; request B sees its object temporarily missing, uploads to B’s path, and publishes the row. If A’s upload then fails, A unconditionally deletes the row by id, including B’s published row. If A succeeds, publishStagedRow accepts any RECEIVED state without verifying storagePath, orphaning A’s object. Serialize this recovery or treat active STAGING as in-flight, fence deletion on state/path/version, positively verify CAS-loss outcomes, and add deterministic interleaving tests.
Critical — /start invalidates live leases and is not idempotent. Every matching retry bumps uploadLeaseVersion and repoints storagePath in start/route.ts, while the original request can subsequently return its now-obsolete URL. Uploading through that valid URL creates an untracked object; concurrent retries also return 409, and an initial signing failure can unconditionally delete a row already advanced by another request. This is confirmed by the currently failing CONCURRENT starts on one key yield ONE row E2E test in CI run 1686. Preserve an active unexpired lease, only rearm expired/recoverable leases, fence cleanup/deletion, and clean obsolete paths after their signed URLs expire.
High — the documented production rollout omits a required secret. authenticateIntake requires a distinct RECEIPT_ARCHIVE_SECRET, but the PR deploy order configures only RECEIPT_INTAKE_SECRET. Following those instructions leaves archive polling and callbacks returning 401. Add the archive secret to the mandatory pre-merge deployment steps and verify both secrets exist and differ.

VERDICT: REQUEST_CHANGES

…euse, archive secret in deploy docs Three REQUEST_CHANGES findings from the PR #440 adversarial review: 1. An inline upload failure deleted its STAGING row by id alone. A concurrent replay of the same sourceRef can find that row via respondToSourceRefConflict, upload to its own path, and publish it while the original request's upload is still failing — the unconditional delete then destroyed the now-RECEIVED row. The delete is now a fenced deleteMany on {id, state: STAGING, storagePath}, a no-op once another request has moved the row on. publishStagedRow's own CAS gained the same storagePath check, so a publish can never land against an object the row no longer points at. 2. Every retrying /start call for the same sourceRef bumped uploadLeaseVersion and repointed storagePath unconditionally, even when the existing lease had not expired — invalidating the original caller's in-flight upload and deleting the object it was about to PUT to. An unexpired lease is now served a freshly signed URL for its EXISTING path (createSignedUploadUrl does not revoke a prior token for the same path), with no repath and no delete. Only an expired or non-STAGING row still rearms. 3. The deploy checklist (spec §"CUTOVER SEQUENCE" and the PR's own "Deploy order") named only RECEIPT_INTAKE_SECRET. authenticateIntake requires RECEIPT_ARCHIVE_SECRET too, and fails every archive-mirror request closed if it's unset or equal to the ingest secret. Both docs now call out both variables as mandatory and distinct. Two source-text tests updated to match: the publishStagedRow CAS regex now expects the storagePath clause, and the /start lease-stamp test now expects a fourth signUpload call site that deliberately skips the lease stamp because it is reusing one still live. npm run build: 0 errors. npm run test:unit: 1210/1210 pass. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>

github-actions · 2026-09-02T22:49:10Z

The dry-run switch is not a real kill switch. worker.ts uses the current environment flag only for cutover, while booking decisions use the persisted row.dryRun at worker.ts and book.ts. After a live period, reverting or unsetting RECEIPT_INTAKE_DRYRUN will not stop existing dryRun=false rows from making QBO calls. That violates the advertised zero-write default and makes rollback unsafe. Gate claiming/promotion/booking on the current global switch too, and test a live→dry-run transition with pre-existing READ and BOOKING rows.
Retrying /start renews the signed upload capability without renewing the database lease. At start/route.ts, a fresh signed URL is issued for an existing path, but uploadUrlExpiresAt and the lease fence are untouched. Near the original expiry, the client receives a URL valid for roughly another two hours while the sweeper believes the lease has expired and may park or delete the upload. Persist and fence the renewed lease—or avoid issuing a new token—and add a retry-near-expiry/sweeper race test.
Private receipts are exposed to every enabled employee. page.tsx checks only for a session, then page.tsx mints signed URLs for every matching receipt. The proxy likewise authorizes any enabled account, not bookkeeping roles. This bypasses the API’s explicit ADMIN/MANAGER/FINANCE gate and lets an EMPLOYEE open /manager/receipts directly to obtain financial data and private document URLs. Enforce the same role or permission before querying or signing, with a negative employee test.
The promised human-review path does not exist. The Receipts page queries only Expense rows at page.tsx; it never reads ReceiptIntake, and the client has no handling for NEEDS_JOB, NEEDS_REVIEW, or SHADOW_QUARANTINE. The spec explicitly promises a “book anyway” action for quarantine at PHASE-1-INTAKE-CORE-SPEC.md. Those rows are therefore permanent dead ends. Successful rows also bypass the documented Pending lifecycle by being written directly as Reviewed at book.ts. Implement an authorized, CAS-safe review/resolution flow and align the Expense.status contract with the checked-in specification.

VERDICT: REQUEST_CHANGES

vercel Bot deployed to Preview September 2, 2026 01:18 View deployment

vercel Bot deployed to Preview September 2, 2026 03:14 View deployment

Clarion1631 force-pushed the feat/phase1-intake-core branch from 0b423f4 to 9111edf Compare September 2, 2026 03:57

vercel Bot deployed to Preview September 2, 2026 03:59 View deployment

vercel Bot deployed to Preview September 2, 2026 04:11 View deployment

vercel Bot deployed to Preview September 2, 2026 04:28 View deployment

vercel Bot deployed to Preview September 2, 2026 04:43 View deployment

vercel Bot deployed to Preview September 2, 2026 05:06 View deployment

Clarion1631 and others added 3 commits September 1, 2026 22:16

Clarion1631 and others added 13 commits September 1, 2026 22:17

docs: phase 1 intake-core and phase 4 earned-margin specs

4d24b13

Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>

vercel Bot deployed to Preview September 2, 2026 15:37 View deployment

vercel Bot deployed to Preview September 2, 2026 16:08 View deployment

vercel Bot deployed to Preview September 2, 2026 16:20 View deployment

vercel Bot deployed to Preview September 2, 2026 16:34 View deployment

vercel Bot deployed to Preview September 2, 2026 17:14 View deployment

fix(intake): round 16 — SHA identity guard, finalize CAS, attachment …

430e924

…naming, tax warning preservation Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>

vercel Bot deployed to Preview September 2, 2026 18:01 View deployment

vercel Bot deployed to Preview September 2, 2026 18:48 View deployment

vercel Bot deployed to Preview September 2, 2026 20:55 View deployment

ci: retrigger Codex gate (runner sandbox errors on prior attempts)

dddeeb4

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>

vercel Bot deployed to Preview September 2, 2026 21:47 View deployment

vercel Bot deployed to Preview September 2, 2026 22:21 View deployment

Conversation

Clarion1631 commented Sep 2, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

What and why

Design notes worth reviewing

Review summary

Tests

Deploy order

Uh oh!

vercel Bot commented Sep 2, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

github-actions Bot commented Sep 2, 2026

Uh oh!

github-actions Bot commented Sep 2, 2026

Uh oh!

github-actions Bot commented Sep 2, 2026

Uh oh!

github-actions Bot commented Sep 2, 2026

Uh oh!

github-actions Bot commented Sep 2, 2026

Uh oh!

github-actions Bot commented Sep 2, 2026

Uh oh!

github-actions Bot commented Sep 2, 2026

Uh oh!

github-actions Bot commented Sep 2, 2026

Uh oh!

github-actions Bot commented Sep 2, 2026

Uh oh!

github-actions Bot commented Sep 2, 2026

Uh oh!

github-actions Bot commented Sep 2, 2026

Uh oh!

github-actions Bot commented Sep 2, 2026

Uh oh!

Clarion1631 commented Sep 2, 2026

Uh oh!

github-actions Bot commented Sep 2, 2026

Uh oh!

github-actions Bot commented Sep 2, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

Clarion1631 commented Sep 2, 2026 •

edited

Loading

vercel Bot commented Sep 2, 2026 •

edited

Loading