feat(expenses): project + phase attribution, tax paid at source (pipeline v2 phase 3) by Clarion1631 · Pull Request #442 · Clarion1631/probuild

Clarion1631 · 2026-09-02T03:30:28Z

Receipt Pipeline v2, Phase 3. Spec: docs/plans/PHASE-3-ATTRIBUTION-SPEC.md (committed here).

What and why

Expense had no projectId. It reached a job only through its required estimateId, so eleven money-path readers each hand-rolled { estimate: { projectId } }, and a receipt could never be born knowing its job. Phase 3 gives an expense its own attribution and makes one module the answer to "whose job is this, and which phase?".

Schema — Expense.projectId (+FK SET NULL, +index), taxAmount, taxAtSource, taxSource, installedAtCustomer, taxDeductibleBase, needsTaxReview, costCodeSource, costCodeConfidence, updatedAt; ReceiptIntake.taxAtSource + installedAtCustomer. projectId is backfilled from estimate.projectId by the same idempotent UPDATE in both the apply script and the migration.
Writers — all seven now stamp the job they already knew, and any writer that sets a cost code records who decided it (capture = a person > ai/backfill > null). Nothing but a human edit rewrites a capture/manual code. costCodeSource is never read off a request body: provenance is something the server observes, not something a client asserts.
Tax provenance and review — taxSource records who decided the tax columns (ocr from the pipeline, manual from the PATCH). Booking never writes over a manual decision, including the decision that a receipt has NO tax, which is a null taxAmount and otherwise indistinguishable from "nobody looked". A QBO re-sync that moves the gross on a classified row sets needsTaxReview; clearing that flag takes an explicit taxReviewAck carrying both taxAmount and taxDeductibleBase, so a partial edit cannot silently certify figures nobody re-checked.
Readers — src/lib/expense-attribution.ts is the one resolver; every listed call site composes it. Identical output for existing data by construction, since the column is nullable and backfilled.
Backfill — node --import=tsx scripts/backfill-expense-attribution.ts, dry-run by default, prints before/after dollar coverage per job and writes the remainder to a CSV for Marge with a reason per row.
Tax report — /reports/tax-paid-at-source, the WA excise deduction, gated by financialReports on both the page and the CSV route.

Mobile is a separate repo and is untouched; the server contracts it needs (including overheadProjectId on /api/mobile/me) are built and recorded as an as-built table in spec §5.

Review summary

Two Codex rounds. The findings that changed the design, not just the code:

The QBO sync was overwriting estimateId on re-attributed rows. It kept a bookkeeper's projectId but still wrote the match's estimate back, leaving the expense on job B for every reader and on job A's estimate for cascade-delete and billing.
The estimate-refresh carve-out was dropped. My round-1 fix suppressed the estimate write only when the stored project and the incoming match disagreed, keeping "same job, newer estimate" as the sync's long-standing attach-to-the-active-estimate behaviour. Round 2 removed that carve-out. Attribution is now write-once: projectId and estimateId are the same fact said twice, so they are written together by one updateMany whose predicate is projectId: null, and never again. The carve-out bought a row following its job to a newer estimate and paid for it by making the rule conditional — which is exactly how the original bug got in. Re-pointing an estimate belongs to an explicit re-attribution path, not to an import. Recorded in spec §3.1.
The Company Financials page disagreed with itself. I had left the all-time groupBy(["estimateId"]) in place and called it "identical output". It was not: the ranking resolved a job through the estimate while the monthly series used the resolver, so re-attributed dollars ranked under one job and plotted under another. Now both use the resolver.
The backfill's item fallback could move a phase across jobs. Expense.itemId is ON DELETE SET NULL and was never scoped to the expense's own estimate, so a stored link can point at another job's line item. The item's project must now equal the expense's resolved project; anything else is skipped with reason item-outside-estimate and surfaced for a human.
{ costCodeSource: { notIn: [...] } } alone would have written nothing. SQL NULL NOT IN (…) is NULL, so every legacy row (source NULL) was excluded — presenting as "the rules matched nothing", which is plausible enough to go unnoticed. notHumanCodedExpenseWhere() carries an explicit NULL branch.
Money and dates: the tax report sums whole cents parsed from the Decimal's string form (no float), and computes period boundaries and month buckets in the company time zone — a receipt bought 30 September at 6pm Pacific is 1 October UTC and would land on the wrong excise return.
CSV: new src/lib/csv-safe.ts neutralizes formula-leading text (including behind invisible whitespace). Numbers are exempt and use a fixed-point formatter, so -12.50 stays a number and nothing emits exponent notation. The backfill's remainder CSV uses it too — its own escaper only quoted, and the vendor names in that file are OCR output. Flagged, not fixed: src/lib/sales-tax-report.ts has the same pre-existing gap in its own escapeCsv.
The backfill wrote no cost codes at all. Pass (a) filled projectId; pass (c)'s predicate still asserted the pre-fill value, so every legacy row pass (a) touched then matched nothing — an --apply that reported success and coded nothing. The test stub returned {count: 1} without mutating state, which is precisely why it went unnoticed. The stub is now stateful and honours predicates (including SQL's NULL NOT IN (…)).
"The cost code exists" is not a permission, and five writers treated it as one. The intake route, the worker's phase loader (it took a projectId and ignored it, offering every company code to the model), QBO suggestions, the manual expense edit and the backfill now all require the code to be a phase of that job.
The tax deduction no longer defaults to claimable. installedAtCustomer was defaulting to true for any non-overhead project, which turned "nobody looked at this" into a deduction on a state return — and a job receipt is just as likely to be consumables, tools, fuel or a service. WAC 458-20-102(12)(b) allows the cost of the articles actually resold. Silence is now NULL everywhere; the report counts only an explicit true. New Expense.taxDeductibleBase lets a bookkeeper allocate the resold portion of a mixed receipt, and the correction path is PATCH /api/expenses/[id], not the PUT on that route (PUT is gated by assertExpenseMutableOutsideQbo, which excludes every pipeline-booked row, i.e. exactly the rows the tax report is made of, and now rejects the tax fields by name). PATCH edits installedAtCustomer, taxDeductibleBase, taxAmount, taxAtSource and costCodeId only, validated 0 ≤ base ≤ amount − tax and 0 ≤ tax ≤ 12% against the row the request leaves behind. taxAtSource is unchanged — it stays the factual "tax was charged on this receipt".

Tests

npm run test:unit — 847 tests, 845 pass, 0 fail (2 pre-existing PG-dependent self-skips). npm run build — 0 errors. npx tsc --noEmit clean.

New: expense-attribution, expense-cost-suggest, apply-expense-attribution, backfill-expense-attribution, tax-at-source-report, tax-at-source-query, company-financials-spend-attribution, expense-phase-scope. The qbo-expense-sync suite is now wired into test:unit so the capture/manual no-overwrite guards actually run in CI.

Two guards were mutation-checked: reverting the charts ranking loop to e.estimate?.projectId fails 2 of its 3 cases, and reverting the backfill's post-fill predicate fails 2 of the backfill's.

Deploy order

Both schema scripts run against prod before merge, in this order. Migrations are not applied automatically here, so a merge ahead of them means P2021/P2022 on every page that touches the new columns (CLAUDE.md pre-deploy rule 2).

node scripts/apply-receipt-intake.mjs --yes --expect-db <db> --expect-host <host> — creates ReceiptIntake. This one first: apply-expense-attribution adds two columns to that table behind a to_regclass guard and skips them silently if the table is absent. Let its verification pass complete (columns, constraints, and the partial unique index checked by definition).
node scripts/apply-expense-attribution.mjs --yes --expect-db <db> --expect-host <host> — the Expense columns, the FK, the index, and the projectId backfill. Let its verification pass complete too; it asserts the FK by pg_get_constraintdef, not by name, and asserts zero expenses left unattributed against a known estimate project.

Both are additive and idempotent — a second run reports every statement ok and 0 rows updated.

Merge (auto-deploy ships main).
Optional, after review: node --import=tsx scripts/backfill-expense-attribution.ts (dry run) → Justin reads the coverage table and the remainder CSV → --apply. A re-run must then report 0 planned writes.

None of these scripts has been run anywhere yet.

🤖 Generated with Claude Code

vercel · 2026-09-02T03:30:33Z

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

Project	Deployment	Actions	Updated
probuild	Ready	Preview	Sep 2, 2026 9:58pm UTC

github-actions · 2026-09-02T03:44:38Z

Request changes. The green CI suite misses several production and accounting failures.

[P0] The documented deploy order will ship against a missing schema. This PR introduces ReceiptIntake, but the deploy instructions run only apply-expense-attribution.mjs. That script explicitly skips the intake columns when the table is absent (guard). Since migrations are not automatically applied here, the deployed worker will hit P2021/P2022. Require and execute: apply-receipt-intake first, then apply-expense-attribution, with both verification passes, before merge.
[P1] The advertised one-pass backfill skips its own cost-code writes. It first fills projectId (project pass), then attempts cost-code updates using the pre-fill predicate projectId: null (code pass). Every legacy row changed by pass one therefore fails pass two. The test stub always returns {count: 1} without mutating state, masking the bug. Use the post-fill/resolved project in the predicate or perform both changes atomically, and add a stateful test proving one --apply leaves zero planned writes.
[P1] Project phase scope is bypassed by multiple writers. The intake route persists any existing costCodeId without checking that it belongs to the selected project (intake write); the worker ignores its projectId argument and loads every active code (worker); QBO suggestions likewise load every active code (QBO sync); and manual edits validate only existence/activity (expense edit). These paths can assign phases absent from the job’s approved estimates. Apply resolveCostCode plus isCostCodeAllowedForProject consistently, including automated/backfill writes, and test cross-project and inactive-phase rejection.
[P1] The tax report can overstate a filing deduction by default. Any non-overhead project silently defaults installedAtCustomer to true (default); any positive AI-read tax becomes taxAtSource (worker); and the report deducts the entire pre-tax receipt (aggregation). Consumables, tools, services, and mixed receipts can therefore be claimed without an affirmative human allocation, and the expense edit route exposes no correction path. Washington’s rule permits the cost of the articles actually resold, not automatically the whole job-coded receipt (WAC 458-20-102(12)(b)). Default eligibility to unknown, add an authorized review/correction or deductible-base allocation, and cover mixed receipts.
[P1] The backfill CSV remains formula-injection vulnerable. Its private csvEscape merely quotes text (implementation), so attacker- or OCR-controlled vendor/project/description values beginning with =, +, -, or @ remain executable when Marge opens the CSV. Reuse csvCell/csvNumber from csv-safe.ts and add injection cases to the backfill tests.
[P1] A crash after storage upload permanently strands a valid receipt. After upload, publishing is one unguarded database update (publish). If it fails, replay returns perpetual 202 STAGING, while the sweeper later labels the row file-missing without checking storage (sweeper). Recover same-hash STAGING replays by verifying the object and conditionally publishing it, and test the upload-success/publish-failure boundary.
[P1] The QBO receipt endpoint’s timeout budget is internally impossible. It sets a 30-second function limit (route) while documenting sequential 20-second token-refresh and purchase-create deadlines; purchase creation can perform additional QBO calls too. Vercel can terminate the function before it returns the promised retryable 503. Use an end-to-end deadline comfortably below maxDuration, or raise maxDuration above the worst-case sequential budget and test it.

VERDICT: REQUEST_CHANGES

…tax deduction Codex PR #442 round 2. Items 6 and 7 are Phase 1 / Phase 0 code, fixed on their own branches. 2. THE BACKFILL WROTE NO COST CODES. Pass (a) filled `projectId`; pass (c)'s predicate still asserted the PRE-fill value, so every legacy row pass (a) touched then matched nothing — an `--apply` that reported success and coded nothing. The predicate now uses the resolved/post-fill project. The stub returned `{count: 1}` without mutating state, which is exactly why this passed: it is now stateful and honours predicates (including SQL's `NULL NOT IN (...)`), and a new test proves one `--apply` codes the row and a second dry run plans zero. Mutation-checked. 3. PHASE SCOPE. "The cost code exists" is not a permission, and five writers were treating it as one. The intake route, the worker's phase loader (it took a projectId and ignored it, offering every company code to the model), QBO suggestions, the manual expense edit and the backfill all now require the code to be a phase OF THAT JOB. 4. TAX POSITION. `installedAtCustomer` no longer defaults from the project — silence is NULL everywhere, including job-folder receipts. Defaulting it true turned "nobody looked at this" into a deduction on a state return, and a job receipt is just as likely to be consumables, tools, fuel or a service; WAC 458-20-102(12)(b) allows the cost of the articles actually RESOLD. The report still counts only an explicit true. New `Expense.taxDeductibleBase` (additive, in schema + migration + apply script) lets a bookkeeper allocate the resold portion of a MIXED receipt, and the expense PUT is the correction path — it accepts `installedAtCustomer` and `taxDeductibleBase`, validated 0 ≤ base ≤ amount − tax against the amount the request LEAVES on the row. `taxAtSource` is unchanged: it stays the factual "tax was charged here". 5. The backfill CSV used a private escaper that only quoted, leaving OCR'd vendor names executable. It now uses csvCell/csvNumber from csv-safe. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>

github-actions · 2026-09-02T04:13:11Z

[P1] The advertised tax correction path is unusable for pipeline receipts and under-authorized elsewhere. The PUT handler rejects any expense with qbPurchaseId before parsing the correction fields (route.ts), while every successfully booked intake expense receives one. Conversely, non-QBO tax fields can be changed by any authenticated user; there is no financialReports permission check. Add a permission-gated metadata-only update path that works for QBO-managed expenses, plus tests for finance access and field-crew rejection.
[P1] The tax report shifts persisted transaction dates by a day. Intake parses calendar dates as UTC midnight (worker.ts), but the report filters using company-midnight instants and buckets those UTC-midnight values through the company timezone (tax-at-source-report.ts). In Pacific time, 2026-10-01T00:00Z becomes September 30, and an October 1 row is excluded by an October 1 07:00Z lower bound. Treat expense dates consistently as date-only values and test actual UTC-midnight rows at quarter boundaries.
[P1] Invalid OCR tax can produce a negative or otherwise false filing deduction. The worker marks any positive tax as paid at source, and booking stores that raw value even when buildGroups rejects tax >= total as nonsense (book.ts). The report then computes receiptTotal - tax without validation (tax-at-source-report.ts). Additionally, lowering amount without also editing taxDeductibleBase can leave an existing allocation above the new ceiling. Enforce 0 < taxAmount < amount and the deductible-base invariant on every relevant write, preferably with database constraints as well as route validation.
[P1] The backfill still assigns phases without proving they belong to the job. If a project has no entry in allowedCodesByProject, allowed is undefined and the guard silently permits any globally active suggestion (backfill-expense-attribution.mjs). Worse, the allowed map is built from every coded estimate item, including draft and archived estimates (backfill-expense-attribution.mjs), unlike the shared project-phase resolver. Use the canonical eligible-phase logic and treat a missing/empty phase set as rejection.
[P1] Mobile and web intake retries are not idempotent. Authenticated callers are forbidden from supplying sourceRef, while the server generates a fresh UUID on every POST (route.ts). A timeout and retry therefore creates another intake row and potentially another QBO Purchase, contradicting the documented mobile:<uploadId> contract. Accept an opaque client retry key and namespace it server-side by authenticated user/source; test replay after a lost response.
[P1] Upload-success/publish-failure still permanently strands receipts. After storage upload, publishing is an unguarded database update (route.ts). A failure leaves STAGING; same-hash replays only return 202, and the sweeper blindly changes the row to file-missing without checking storage (receipt-intake-worker route). Recover by verifying the existing object and conditionally publishing it on replay or sweep, with a boundary-failure test.
[P1] The QBO endpoint still has an impossible timeout budget. It allows only 30 seconds (create route), while each QBO request may consume 20 seconds and purchase creation performs several sequential queries, entity lookups/creates, verification, purchase creation, and optional upload. A Vercel termination can occur before the promised retryable response. Introduce one end-to-end deadline below the function ceiling or raise the ceiling above the true worst case.
[P1] A QBO-sync race discards captured attribution and tax metadata. Booking treats an existing expense as complete and only links its ID (book.ts). If QBO sync imports the Purchase after QBO creation but before the intake transaction—or during a retry after database failure—the captured phase, provenance, tax, and installed-at-customer answer are never copied. Reconcile safe metadata onto the existing row under the documented human-over-AI precedence and test this race.

VERDICT: REQUEST_CHANGES

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

[P0] Intake receipt dates shift into the previous day—and sometimes the previous quarter. worker.ts:169 creates transaction dates at UTC midnight, and book.ts:350 copies that instant into Expense.date. tax-at-source-report.ts:391 then interprets it in the company timezone. In Pacific time, 2026-10-01T00:00Z becomes September 30 and is excluded from Q4. Preserve the calendar date using company-local noon or a date-only representation, with boundary tests through the actual booking/query path.
[P0] Unreviewed, nonsensical OCR tax can flow directly into a tax return. worker.ts:353 treats every positive OCR tax as factual without requiring tax < total. Although book.ts:143 rejects nonsense for the QBO split, book.ts:345 still stores the raw value, marks taxAtSource, and creates a Pending expense. The report at tax-at-source-report.ts:356 has no review-status condition and can calculate a negative deduction base. Validate tax against gross, require bookkeeper confirmation/review before filing inclusion, and provide a way to correct taxAmount/taxAtSource.
[P0] Any authenticated web user can alter tax-return data on any expense whose ID they know. expenses/[id]/route.ts:41 checks only for a session; it performs neither project authorization nor the financialReports permission check used by the report. The new tax fields and manual provenance therefore bypass the claimed bookkeeper boundary. Authorize access to the resolved project and restrict tax-return corrections to an appropriate financial permission.
[P1] Lowering an expense amount can leave an impossible deduction base. Validation at expenses/[id]/route.ts:119 runs only when taxDeductibleBase is included. A request that changes only amount can leave the existing base greater than amount - taxAmount, contradicting the stated invariant. Validate the resulting complete row whenever any dependent field changes, preferably backed by a database constraint.
[P1] Expense editing still accepts a line item from another estimate/project. expenses/[id]/route.ts:62 verifies only that itemId exists. It does not compare the item’s estimate/project with the expense’s resolved attribution, allowing corrupted links and incorrect or missing phase fallback. Scope the item lookup to the expense’s estimate/project.
[P1] Several cost-code writers still treat global existence as project permission. time-expense-core.ts:184 stores arbitrary costCodeId/costTypeId input and labels it permanently manual; receipt-ingest/route.ts:81 matches against every active company code and writes it without project-phase validation. Resolve the code server-side, derive its cost type, and require isCostCodeAllowedForProject in both paths.
[P1] The backfill’s phase permission check fails open for projects with no mapped phases. At backfill-expense-attribution.mjs:186, an absent allowedCodesByProject entry skips rejection and permits any globally matched code. Such an entry is absent precisely when a project has no eligible coded estimate items. Require allowed?.has(costCodeId) === true and test the empty/missing-set cases.
[P1] A failure after storage upload leaves intake retries unable to recover. The upload and publish update are separate at receipts/intake/route.ts:277. If upload succeeds but the STAGING → RECEIVED update fails—or upload cleanup cannot delete the row—a same-byte retry only returns 202 staging at route.ts:359. The worker eventually parks it for manual review despite the object existing. Reconcile storage and republish on idempotent retry, or provide an automatic stale-staging recovery path.
[P2] The committed as-built spec contradicts the implementation. PHASE-3-ATTRIBUTION-SPEC.md:298 says silence defaults true for customer jobs and false for overhead, while expense-attribution.ts:157 correctly preserves silence as null. This is the contract the separate mobile repository is supposed to consume; update every stale defaulting statement before it guides another implementation.

VERDICT: REQUEST_CHANGES

…e item links Codex PR #442 round 3. Items 1, 2 and 8 are Phase 1's and arrive on rebase. 3. [P0] PUT /api/expenses/[id] checked only that SOMEBODY was signed in — no project authorization, no permission. I added tax-return fields to that route, so any authenticated user who knew an expense id could edit the numbers on a state excise return. It now resolves the project and requires access to it (fail closed when there is none), requires `timeClock` to edit an expense at all, and requires `financialReports` on top for installedAtCustomer/taxDeductibleBase — "may edit this expense" and "may decide what the company deducts" are not the same authority. NOTE: DELETE on the same route has the identical pre-existing gap. Untouched here because it predates this PR; flagged for its own fix. 4. The deduction-base invariant is about the RESULTING ROW, not this request's fields. Validating only when taxDeductibleBase was sent meant a PUT that merely LOWERED amount could strand an existing base above the new pre-tax total — the same illegal state through the other door. 5. The item link is scoped to the expense's own estimate or its project. An existence check alone let an edit point at another job's line item, which then feeds the item->costCode fallback. 6. createExpenseCore stored an arbitrary costCodeId and stamped it "manual" (outranking every automated pass) with no project check; receipt-ingest v1 matched a Gemini category against every active company code. Both now go through resolveCostCode + isCostCodeAllowedForProject. RISK, flagged: a change-order expense whose code is not on a phase-eligible estimate item will now be rejected rather than silently miscoded. Correct, but it is a behaviour change on a live path. 7. The backfill's phase check failed OPEN when a project had no mapped phases — absent in exactly the case where we know its phases least. Now requires a positive answer; an unmapped project skips with reason "no-phases". 9. Spec §5 still documented the default-true/false toggle. Rewritten with the as-built tax position, since the mobile repo consumes that section. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>

github-actions · 2026-09-02T04:50:24Z

I found release-blocking correctness and security defects:

intake route breaks idempotency for mobile/web callers. It rejects every client sourceRef and generates a random UUID, so retrying an upload after a lost response creates another intake row and potentially another QBO purchase. Accept a stable client upload ID, namespace it server-side, and test replay through session and mobile auth.
tax report mishandles pipeline dates. The worker stores @db.Date values as UTC midnight, but the report filters and buckets them as instants in the company timezone. In Pacific time, 2026-07-01T00:00Z becomes June 30 and is excluded from a July 1 quarter boundary. Preserve calendar-date semantics or normalize storage consistently, with boundary tests using actual stored date-only values.
expense PUT cannot serve as the promised correction path. Pipeline-created expenses always have qbPurchaseId, so assertExpenseMutableOutsideQbo() rejects them before installedAtCustomer or taxDeductibleBase can be updated. For non-QBO expenses, a tax-only request also clears omitted vendor, date, description, and itemId at line 225. Permit local-only tax metadata edits on QBO-managed rows and preserve every omitted field.
booking persists raw OCR tax even when buildGroups() rejects it—for checks or when tax is greater than or equal to the receipt total. The report then claims that rejected tax. Persist/report only validated tax, or park invalid reads for review; add both invalid-tax cases to report-level tests.
intake publication is not recoverable when storage succeeds but the final database update fails. A retry hits the unique sourceRef, receives perpetual 202 STAGING, and never republishes the row; the sweeper later falsely marks the existing object file-missing. Make same-content STAGING replays verify the object and retry publication, with a fault-injection test.
ReceiptIntake migration creates a sensitive table in Supabase’s exposed public schema without RLS or explicit privilege revocation. It contains financial data, storage paths, hashes, and OCR output. Applying the Supabase security checklist, this must enable RLS and define appropriate policies—or explicitly revoke anon/authenticated access—in both the migration and apply script.
backfill does not enforce its claimed “phase of that job” rule. It loads coded items from every estimate, including draft/archived estimates and inactive codes, then builds allowedCodesByProject from that same unfiltered set. Use the canonical eligible-estimate/active-code phase rules for item fallback and suggestions, and test inactive and draft-only codes.
expense DELETE still authorizes only by “some user is signed in.” Any authenticated user who knows an ID can delete any non-QBO expense, despite PUT now checking permission and project scope. Apply equivalent timeClock and resolved-project authorization before deletion.

VERDICT: REQUEST_CHANGES

…; validated tax only Codex PR #442 round 4. Items 1, 2, 5 and 6 are Phase 1's and arrive on rebase. 8. DELETE had the same session-only gap PUT had — any authenticated user with an id could destroy any non-QBO expense on any job. Same gate as PUT now: timeClock, resolved project, fail closed when there is none. 3. The correction path could not reach a single row it was built for. PUT is guarded by assertExpenseMutableOutsideQbo, and every pipeline expense carries a qbPurchaseId — precisely the population the tax report reads. Split into a dedicated PATCH that edits ONLY installedAtCustomer, taxDeductibleBase and costCodeId. Those three are ProBuild-only bookkeeping: nothing syncs them to QuickBooks and nothing in QBO overwrites them, so the mutability guard does not apply. amount/vendor/date are refused there at any status. PUT keeps its guard, rejects the tax fields outright (a silent ignore would look like a successful correction), and is now a PARTIAL update — it used to null every field a request left out, so a tax-only edit erased the vendor, date and description. 7. allowedCodesByProject is built from the app's own phase-eligible set (PHASE_ELIGIBLE_ESTIMATE_WHERE, active codes only), not from every coded estimate item. The fail-closed check was looser than it claimed: a code from a draft or archived estimate counted as a phase of the job. The item->code fallback now passes the same gate, instead of bypassing it. 4. Booking persists tax ONLY when buildGroups accepted it. It was storing the raw OCR read even for a check or a nonsense tax >= total, with taxAtSource true — so a misread no human saw could be claimed on an excise return, and amount - taxAmount could go negative. The rejected value now lives only on ReceiptIntake.taxCents, which the report cannot read. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>

github-actions · 2026-09-02T05:13:48Z

This is not mergeable.

Critical — ReceiptIntake is exposed without RLS or revoked grants. The new public table contains OCR output, financial data, user/project IDs, hashes, and private storage paths, yet neither migration.sql nor the apply script enables RLS or revokes Data API roles. Existing Supabase projects commonly grant anon and authenticated access to new public tables by default. Route authentication does nothing against direct PostgREST access. Secure the table in both DDL paths and verify its ACL/RLS state. Supabase explicitly requires this for exposed schemas.
Critical — pipeline receipts are assigned to the wrong tax day. ReceiptIntake.txnDate is @db.Date, so Prisma returns UTC midnight. book.ts copies that directly into timestamp-valued Expense.date. The report then queries using company-midnight bounds and converts the value through the company timezone at tax-at-source-report.ts and tax-at-source-report.ts. In Pacific time, 2026-09-30T00:00Z becomes September 29. First-day receipts are excluded, and the rest are displayed one day early. The tests use fabricated real-time instants and never exercise the actual @db.Date → Expense.date path. Preserve the calendar date consistently and add a database-backed boundary test.
High — the advertised tax-correction path cannot correct tax. Booking explicitly says rejected OCR tax can be fixed through PATCH at book.ts, but PATCH’s allowlist at route.ts excludes both taxAmount and taxAtSource. A rejected or incorrect tax read is therefore permanently absent or wrong in the filing report. There is also no UI for the fields, while the spec incorrectly documents PUT at PHASE-3-ATTRIBUTION-SPEC.md, and PUT explicitly rejects them. Implement one usable, documented correction path—including tax amount/source—and test accepted, rejected, and corrected OCR cases.
High — the deduction invariant is race-prone and not enforced by Postgres. PATCH reads amount/taxAmount, validates the base, then performs a separate update at route.ts. A concurrent QBO sync can change amount between those operations, leaving taxDeductibleBase > amount - taxAmount; the report deliberately trusts that impossible row. Add database CHECK constraints or an atomic conditional write covering nonnegative tax and 0 ≤ taxDeductibleBase ≤ amount − taxAmount, with concurrent-update coverage.

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>

…gration, CAS Codex round 8. Rebased onto Phase 1 head e3da4a6 first. 1. /start and /finalize enforce the SAME capture rules as the inline door, through one shared validateCapturedPhase: a costCodeId is checked for existence, active-ness AND membership of the named project, and is refused outright when there is no project to check it against. Both persist installedAtCustomer as a tri-state with no default. The two-step path had no validation at all, so a crew phone could pin any active company code to a receipt on any job — and booking copies a captured code onto the Expense with provenance "capture", which no automated pass may correct. 2. bookReceipt's alreadyExists path FILLS the Phase 3 fields it left blank (projectId, phase, provenance, tax, installedAtCustomer) and only the blanks: costCodeSource capture/manual and an already-answered installedAtCustomer are a human's and are untouchable. If the existing Purchase is on a DIFFERENT job than the intake claims, nothing is filled and the row parks as NEEDS_REVIEW "attribution-conflict" — filling would be guessing which job is right, overwriting would move real money between jobs. 3. schedule-core (both spots), automation-events, the ai-review route and the manager receipt queue now label and roll up by the resolved job. 4. The tax PATCH writes under a compare-and-set on the values its validation depended on (amount, taxAmount, taxDeductibleBase) and answers 409 on a miss; the sync's write does the same and RE-PLANS once against a fresh read. Mutation-checked: dropping the predicate loses a bookkeeper's correction. 5. PUT refuses all five tax fields by name with the field in the response — a silent drop looks like a successful correction. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>

…ooking fill Rebased onto Phase 1 head 8297b46 (sealed uploads, SHADOW_QUARANTINE, claim fencing, project-scoped loadPhases — Phase 1's version kept for every shared hunk, and its stricter loadPhases supersedes mine). 1. E2E was RED because I made `companyTimeZone` a REQUIRED dependency, so every caller that builds its own set threw. It is optional now, defaulting to the shared resolver — a dependency exists to be overridden, not re-stated. 2. When a gross drop invalidates only the ALLOCATION, the row is flagged as well as cleared. A silent null still read as a valid deduction: installedAtCustomer was untouched and a null base means "the whole pre-tax total", so the report would have claimed MORE than the human allocated. 4. The already-booked fill is now one guarded `updateMany` per field (`costCodeId IS NULL`, `installedAtCustomer IS NULL`, source not capture/manual) instead of read-then-write. The read is inside the transaction but a PATCH can still land in the gap — and that PATCH is exactly the authority the fill must not overrun. Regression covers it. 5. /finalize on a non-STAGING retry applies late costCodeId/installedAtCustomer only where the row is still unanswered, and returns 409 late-fields-conflict when the retry carries a DIFFERENT answer to one already recorded. 6. The manager receipt queue selects the direct project and labels through resolveExpenseProjectLabel. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>

…t-scoped labor Codex round 9. Items 5 and 6 are start/finalize — Phase 1's files. 1. DELETE already authorized on the resolved job; a divergent-attribution test now pins it (the job it LEFT confers nothing, the job it is ON does). 2. The tax PATCH's CAS names projectId, estimateId and a new `Expense.updatedAt` row version. Access to the row was granted because of the project it was on, so a re-attribution landing in the gap means the permission check that let the request through was answered about a different job — 409 rather than a write. The column is added nullable, backfilled, then SET NOT NULL, so no DB-level default is left that `@updatedAt` does not declare. 3. New src/lib/expense-lock.ts: ONE `pg_advisory_xact_lock('expense:'||id)`, taken inside their transactions by the QBO sync, the tax PATCH and the booking fill. Per-column CAS stops a lost update but not a torn one, and the tax invariants span columns. The predicates stay — the lock orders writers that take it, the predicate protects against one that does not. After a CAS miss the sync re-plans and re-CASes; a still-contended row is LEFT ALONE, never unconditionally written, because the sync's facts survive to the next run and a discarded human answer does not. 4. The booking fill takes the same lock and pins `projectId` in every guarded predicate, so a re-attribution in the gap makes it match zero rows instead of writing a phase and a tax answer onto a job they were never about. 7. Labor coverage resolves its item fallback through the project-scoped map the expense side already used. Test fakes had to learn two real behaviours: the advisory lock is RE-ENTRANT within a transaction (serialising every call deadlocked the second one), and the apply-script parity test now selects the backfill by what it writes rather than by being the first UPDATE. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>

…kfill ordered Rebased onto Phase 1 head 8b332c7 first. Its shared reconcileLateFields and claim fence are kept; Phase 3's late fields fold INTO that helper (it already carried a note saying installedAtCustomer would land there) rather than being re-added alongside it, and a late costCodeId is validated against the job it is claimed for before reconciliation. 1. `Expense.updatedAt` gets `DEFAULT now()` in the migration, the apply script and Prisma (`@default(now()) @updatedAt`). The apply script runs against production BEFORE the build that knows the column, so for that window the OLD app is still inserting Expenses without it — NOT NULL with no default would have failed every receipt, manual entry and QBO-sync insert until the deploy landed. Order is asserted: nullable, backfill, DEFAULT, then NOT NULL. 3. `deleteExpense` (the single-expense server action) authorizes on the resolved job. It had its own copy of the bug the DELETE route had, and the earlier divergent test never touched this path. The new test does, and is mutation-checked: restoring the estimate read fails 2 of its 6 cases. 5. The backfill's cost-code writes run under the shared per-expense advisory lock and CAS on the row version their plan was computed from. This script's plan is the stalest of the four writers' — built for every row up front, applied over minutes — so it was the one still racing. A miss is counted and reported, never retried: the decision was about a state that no longer exists. Rows the project pass just filled are exempt from the version check, or the backfill would miss on a version it bumped itself. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>

@ts-nocheck

… the lock Rebased onto Phase 1 head 85d6af5. Its finalize authorization and exact-state late-field fencing are kept whole; Phase 3's `installedAtCustomer` folds into their reconcileLateFields and their authorizeLateFields replaces my duplicate phase check. 1/2. Confirmed on this branch and pinned by a new test: a late `projectId` at /finalize is refused when the session caller cannot see that job, the check is on the LATE project rather than the row's existing one, a late phase must belong to the EFFECTIVE project, and a secret forwarder skips the per-user check but not the phase rule. 3. The two data scripts imported TypeScript from src/, which plain `node` only resolves on 22.6+. They now run under `node --import=tsx` — and are renamed .mjs -> .ts, because tsx hands a .ts module to an .mjs file as CJS and the named imports fail outright. A `--help` path (no DB, no env) doubles as the CI smoke test that the documented command actually loads the import graph. @ts-nocheck keeps them exactly as unchecked as they were as .mjs; typing them properly belongs in its own change. 4. The backfill's cost-code pass now RE-READS each row under the per-expense lock and re-checks eligibility before writing, carrying the post-fill version into the CAS. The previous fix exempted rows the project pass had touched from the version check, which traded one hazard for another: an exempted row had no version guard at all. Tests cover a bookkeeper coding the row and a re-attribution, both mid-run. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>

…cost codes Codex round 12. Item 2 (finalize count/reauthorize) is Phase 1's. 1. The QBO suggester now reads the vendor and description off the PERSISTED row instead of taking them from the payload the sync just processed. Those differ exactly when the upsert refused the payload: an out-of-order webhook carries an older SyncToken, `isIncomingQboSyncTokenCurrent` correctly rejects it, and the suggestion then coded the row from a version of the purchase the database had just thrown away. The write is fenced on that row's `updatedAt` AND `qbSyncToken`, so a newer sync landing in the gap wins. 3. The backfill's project fill CASes on `projectId IS NULL AND estimateId = plannedEstimateId` — NULL alone did not say the derivation was still valid, and a row re-pointed at another estimate would have been stamped with the old estimate's project by the very pass that exists to get attribution right. The cost fill re-RUNS `planBackfill` over the freshly-read row under the lock and only writes if the answer is unchanged: eligibility was never the whole dependency, the vendor/description/item feed the decision too. 4. `suggest-expense-cost-codes.ts` loses `--apply` entirely. It was a second writer of `costCodeId` with none of the backfill's guarantees. Report-only now, through the canonical resolver and the project's phase-eligible codes, with csv-safe output; a test fails if a write or the flag returns. Test fakes learned one more real behaviour: `findMany` returns a SNAPSHOT, so a test can model "the row changed after the planner saw it". Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>

Phase 1's helper now returns `{status, body}` rather than a NextResponse, so the assertions read `.body` instead of awaiting `.json()`. Behaviour asserted is unchanged: the late project is the one authorized, and a late phase is checked against the EFFECTIVE project. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>

A re-sync only asked for review when the new gross BROKE an invariant (tax above the amount, or an allocation that no longer fits). An ordinary change breaks nothing and is just as capable of invalidating a human's answer: a $412.10 receipt re-syncing as $498.30 leaves $34.06 of recorded tax describing a purchase that no longer exists, and an installed-at-customer "yes" describing a different basket of goods. So any cent-level movement in the gross on a CLASSIFIED row (a tax amount, an installed-at-customer answer, or a hand allocation) now sets needsTaxReview. The classification itself is kept — it may still be right — and the report already excludes flagged rows, so the filing waits for a person rather than claiming a figure nobody re-checked. Unclassified rows are untouched, or every re-synced purchase would bury the ones that matter. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>

The route validated `Number(body.amount)` and persisted `parseFloat(body.amount)`. Those disagree: "10junk" validates as NaN, which passes every check that is not a comparison, and then persists as 10 — a $207.74 receipt quietly becoming a $10 one, with the deduction-base ceiling computed from a number nobody ever stored. `body.amount ? ...` also dropped a legitimate 0, so a receipt could not be zeroed. Now: one parse, rejected unless finite and >= 0, and that same value is both what the ceiling check uses and what is written. An absent key still means "leave it alone". Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>

…ction DDL Three round-13 items on the booking and rollout paths. taxSource ("ocr" | "manual"): a bookkeeper who decides a receipt has NO sales tax leaves a null taxAmount, which is indistinguishable from "nobody has looked" — so the next booking wrote an OCR figure straight over their answer. Booking stamps "ocr", the tax PATCH stamps "manual", and both fills now refuse to touch a manual row (with the explicit NULL branch SQL requires, or every legacy row would be excluded instead). PUT refuses the column by name. The existing-Expense fill takes the per-expense lock BEFORE the read it decides from, and re-reads the attribution after its guarded writes: an Expense that was re-pointed at another job while the fill ran now throws, which rolls the fills back with it, and the row parks as attribution-conflict rather than being marked BOOKED against somebody else's job. The apply script adds updatedAt WITH its default in one statement (a bare column left a window in which the OLD build's inserts landed NULL after the backfill had already passed, so SET NOT NULL could lose that race), keeps the old-shape repair as no-ops, and runs the whole DDL in a single transaction. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>

Two stale-input paths the guards did not cover. The project fill pinned `estimateId`, which proves the row never left its estimate and says nothing about where that estimate now lives: an estimate moved to another job between the plan and the write stamped every one of its expenses with the OLD project. The predicate now joins the estimate in the same statement, so the derivation has to still hold at write time. The cost-code fill re-planned under the lock, but against the minutes-old snapshot of item links and job phases — so an item re-coded, or a phase removed from the job, after the snapshot was invisible, and the pass wrote a code that was only correct in the past. It now re-reads that one item and that one job's phase list inside the same transaction, applying the same eligibility rules the snapshot query does. Tests cover all three: estimate moved, item re-coded, phase removed. Each was verified to fail with the corresponding guard removed. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>

needsTaxReview means a re-sync moved the gross out from under a whole tax classification, so any tax edit clearing it let a bookkeeper answering "yes, installed at customer" silently certify a tax amount and a deduction split they never looked at, and the row went straight back into the excise report. Clearing it now takes `taxReviewAck: true` carrying both taxAmount and taxDeductibleBase (installedAtCustomer stays optional: a null reads as unanswered and cannot overstate a deduction). A partial correction is still accepted and simply leaves the flag up. The modal shows a confirm checkbox on a flagged row and sends both figures with the ack, changed or not. Deletion also retires `taxSource` and counts it in the already-retired check, so a deleted purchase that a person had classified stops reporting a change forever while still claiming their provenance. Spec and PR body corrected: the backfill is `node --import=tsx scripts/backfill-expense-attribution.ts`, and the correction path is PATCH. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>

`--help` is the CI smoke test that this file still loads, and it ran a PrismaClient constructor at module scope — which throws without DATABASE_URL, so the check failed on CI while passing on any machine with a .env. The client is now built inside main, after the help branch returns. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>

…ll derives from Three round-15 items. An OCR tax read only had to clear "less than the total" to be booked as tax paid at source, so $90 of tax on a $100 receipt — a decimal point in the wrong place — went onto a state excise return as a $90 deduction nobody looked at. The bound the bookkeeper's PATCH already enforced now lives in expense-attribution.ts and both writers use it. The remedies differ because the situations do: PATCH refuses the request, booking cannot (the Purchase is already in QuickBooks), so it stores NULL, flags needsTaxReview, and keeps taxSource "ocr" — a machine looked and got an answer a person must replace. The sync's classification test now counts taxSource "manual" as evidence, and reads the column. A bookkeeper who decides a receipt carries NO tax leaves every other signal null, so that row — a human's explicit answer, now describing a different gross — was the one row a re-sync said nothing about. The backfill share-locks the rows its answers are DERIVED from (the estimate, the linked item, the job's phase rows) before taking the per-expense lock and reading. A predicate can catch a row that moved before the write; it cannot stop one moving during the read sequence that decides what to write. The project fill is now one expense per transaction so it can hold that lock, and a row re-pointed at an estimate or item the locks do not cover is skipped rather than written. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>

…lidation Four round-16 items. Provenance is per decision. `taxSource` governs the two tax FIGURES and is stamped "manual" only when the PATCH actually carries one, so answering the installed-at-customer question no longer claims a person supplied tax numbers, and clearing the tax back to blank leaves the column alone (a blank is an absence, not a decision, and locking on it would freeze the row out of the pipeline forever). `installedAtCustomer` is its own evidence: non-null means answered, booking fills only a null, and a manual tax figure no longer blocks a capture from answering a question nobody touched. The phase is re-asked a third time INSIDE the booking transaction, after share-locking the job's phase rows. The two earlier checks hold nothing still, so a phase deleted mid-write still reached job cost. A code that is no longer a phase parks the row: booking it posts money to a line the job does not have, booking it uncoded silently discards a captured phase, and the Purchase already exists. The lock helper is shared with the backfill so the two writers of a cost code take it in one order. Amounts are SIGNED. A refund is a negative expense and its tax comes back with it, so the rule is direction and magnitude: the tax matches the sign of the amount (zero always allowed) and never exceeds it. Encoded in the shared bound, in the PATCH (a sign mismatch is a 400 naming the reason, never a constraint violation surfacing as a 500), and in the CHECK, which is now dropped and re-added by name so a database carrying the old refund-refusing definition is corrected. `taxAtSource` tests for zero rather than "not positive", or every credit was refused. The company-financials all-time ranking is two grouped sums again - direct rows, and legacy rows resolved through their estimate - merged in memory. Correct but unbounded is still unbounded: it was fetching every expense ever, for five numbers. The two predicates are disjoint by `projectId: null`, which is the same precedence the row-by-row resolver applies. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>

…actional invariant Four round-17 items. ONE SIGNED MODEL FOR CREDITS. A return or vendor credit is a negative expense whose tax comes back with it, and half the pipeline still assumed money only goes out. The tax report summed `taxAmount > 0`, so every credit was dropped and the deduction went on claiming tax that had been refunded; the sync's invalidation compared `existingTax > amount`, and -4 > -50 is true, so it retired the classification on every credit it saw; the deductible-base CHECK demanded `base >= 0`, which made a credit unallocatable. All three are now sign-and-magnitude: the tax and the allocation point the way the money does and never exceed it. A reduced refund that can no longer carry its recorded tax is flagged for review with the tax nulled, never an aborted import. The base CHECK is dropped and re-added by name, in the migration, the apply script and the blind-spots snapshot. PROVENANCE. A PATCH carrying `taxAmount: null` is a bookkeeper saying there is no sales tax on this receipt, so it stamps `taxSource: "manual"` and booking will not write an OCR guess over it; an OMITTED key still leaves the column alone. `taxReviewAck` is now accepted only with both figures present, non-null and coherent with the amount, so an acknowledgement cannot certify an empty row back into the excise report. FINALIZE. The row read selects `installedAtCustomer`, the merge treats it as a captured field, and the publish CAS fences on it. It was the one path that could silently replace a tax answer: the merge saw no stored value, so a late `false` overwrote a captured `true`. PHASE VALIDITY IS NOW A TRANSACTIONAL INVARIANT. `assertPhaseOfProjectTx` locks Project, Estimate, EstimateItem and CostCode FOR SHARE in one fixed order and then answers on the caller's own transaction. Booking, the manual PATCH, finalize and the QBO suggester all use it, so an estimate archived or reassigned, or a cost code deactivated, between the check and the write can no longer be written into job cost. The phase data source also stops handing back deactivated codes, which the validation path had been trusting. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>

…ariant everywhere Six round-18 items. taxSource is now four explicit states — null (unreviewed), "ocr", "manual" (a person's figure) and "manual-none" (a person saying this receipt has no sales tax) — documented in the spec with a table, and composed through taxNotHumanDecidedWhere() rather than tested for by hand. An acknowledgement needs the taxAmount key: a figure, or an explicit null. Omitting it is a 400, because a request that says nothing about tax has nothing to certify. A blank deduction base is no longer a null with a remembered meaning: the server computes and stores amount - tax, sign intact. The modal derives taxAtSource from the figure through the shared rule, so a refund's negative tax no longer stores "no tax here" and drops the credit out of the excise report. ReceiptIntake now records WHO captured a phase: "user" for a signed-in person, "machine" for a shared-secret forwarder, derived from the caller at every door and never read off a body. Booking copies the distinction, so a Drive folder name books as a correctable "machine" phase instead of borrowing the authority of a person who picked it. Every Expense writer that sets a cost code now runs assertPhaseOfProjectTx inside its write transaction — the POST route, the legacy PUT, createExpenseCore and the Drive receipt ingest join booking, the PATCH and the QBO suggester — and a tripwire test fails when a new writer appears without it. The rollout script no longer swallows the CompanySettings query: only an ABSENT row falls back to the app default, and an unreadable one aborts rather than re-anchoring a whole table into a zone nobody chose. The re-anchor is idempotent by marker (attributionAnchoredAt), because the time-of-day predicate is not one for a company configured as UTC. The already-booked recovery fills a null receiptUrl and never replaces an existing one: a receipt nobody can open is the difference between a defensible deduction and a number in a spreadsheet. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>

… is re-resolved under lock Five round-19 items. A blank tax field meant two different things and the payload could not tell them apart. It now says: `{ taxAmount: null, taxKnown: false }` is "nobody has read it yet" — it stamps no provenance, keeps a review flag up, and is refused as an acknowledgement (TAX_UNKNOWN); `{ taxAmount: null, taxKnown: true }` is "I looked, there is no tax", recorded as manual-none. The modal asks which with a pair of radios when the field is empty, and refuses NaN or Infinity before serializing (JSON turns both into null, which the server would otherwise read as a deliberate "no tax"). The server refuses them too, because the modal is one caller of many. On a FLAGGED row an acknowledgement now needs both taxAmount and taxDeductibleBase present — each a figure or an explicit null. The flag says the whole classification is in doubt, and certifying one figure while staying silent about the other is the half-answer it exists to prevent. An expense with no projectId of its own answers through its estimate, and that estimate can be moved to another job mid-request. PATCH, PUT and DELETE now share-lock the estimate inside their transaction, re-resolve the job, re-check the actor against THAT job, and carry it in the write predicate — so a row that moved is refused (403 when the actor may not touch the new job, 409 when the row moved underneath them) rather than written under a stale permission. The QBO suggester does the same, so its phase check and its write agree about which job they are for. The new-expense form offers this job's phases instead of every active cost code in the company. The server refused everything else anyway; the picker was inviting a refusal. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>

github-actions · 2026-09-02T17:04:46Z

The diff is not ready. I found five correctness and security blockers:

The tax PATCH permits contradictory state. A request can set a positive taxAmount with taxAtSource: false, or turn the flag off while retaining existing tax. The report then silently excludes that tax. expense PATCH must enforce taxAtSource === (taxAmount > 0) or derive the flag server-side. Add tests for both contradictory cases and preferably enforce the invariant with a database constraint.
Marking tax “unknown” preserves stale human provenance. The taxKnown: false path writes taxAmount: null but leaves taxSource: "manual" or "manual-none" untouched because stampsTaxProvenance is false. Subsequent OCR/booking then refuses to populate tax forever. expense PATCH must clear the human provenance when a user explicitly revokes the decision, with regression tests starting from both human source values.
projectId and estimateId still are not written as one atomic fact. Both expense creation and time-expense creation validate the estimate before opening the transaction, allowing estimate reassignment to produce a cross-project pair. Existing receipt booking is worse: it can fill only projectId while retaining an unrelated estimateId. Lock and re-read the estimate inside the write transaction, revalidate itemId, and either write the attribution pair together or park/reject the record. Add reassignment-interleaving tests.
Several mutators authorize one project and later mutate by expense ID alone. Single/bulk deletion and change-order tagging can act on an expense after concurrent attribution moves it to a project the caller cannot access. Use a transaction with locked attribution resolution and include expenseStillOnProjectWhere(...) in every mutation predicate, as the API DELETE path already does.
The backfill’s claimed phase invariant is racy. The lock helper call omits the candidate costCodeId, so the cost-code row is never locked. A code can become inactive between validation and update, and the backfill will still assign it. Lock/assert the exact phase immediately before the update and add a concurrent-deactivation regression test.

I applied the repository’s Supabase/Postgres guidance when evaluating transaction locking and schema invariants. I could not rerun the suite because the workspace shell sandbox failed before command execution; these findings come from static review of the exact PR head.

VERDICT: REQUEST_CHANGES

…one pair Five round-20 items. `taxAtSource` was a second writable column saying what `taxAmount` already says, so the two could disagree: true with no amount is a claim about nothing, false with $16.55 on the row is a deduction silently dropped from the excise return. It is now derived server-side, refused outright when a client sends it (the modal no longer does), and a database CHECK makes the disagreement unrepresentable — normalised first, since a CHECK cannot be added to a table that already violates it. "Tax unknown" is a RETRACTION, not a no-op: it clears the provenance back to null along with both figures. A row left carrying "manual" with no human answer behind it locks the pipeline out of that receipt forever. `projectId` and `estimateId` are one fact said twice, and all three creators wrote them from two reads taken far apart. The expense POST route, createExpenseCore and booking's existing-expense fill now share-lock the estimate inside the write transaction, re-read the pair, revalidate that the line item is still on it, and write both halves together — booking parks as attribution-conflict rather than filling a project alongside a foreign estimate, which is an expense on two jobs at once. Single and bulk expense deletion and change-order tagging do the same locked re-resolve the API DELETE does, one row per statement so each carries its own attribution predicate: a batch authorized row by row was being mutated as a set with nothing holding those answers still. The backfill share-locks the candidate cost code and re-asserts `isActive` immediately before the update. Retiring a code is the company saying "stop putting money here", and this pass is the one writer with no human behind it. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>

github-actions · 2026-09-02T18:00:14Z

VERDICT: REQUEST_CHANGES

projectId/estimateId is still not an atomic, validated pair. QBO create/fill (src/lib/qbo-expense-sync.ts:910,930), AI parse (src/app/api/receipts/parse/route.ts:291), legacy ingest (src/app/api/integrations/receipt-ingest/route.ts:139), and new receipt booking (src/lib/receipt-intake/book.ts:956) write an estimate selected earlier without re-locking and confirming its current project. An estimate move can still create the exact split-job row this PR claims to eliminate. Resolve the pair with lockEstimateAttribution() inside each write transaction and add interleaving tests.
The legacy PUT validates against stale attribution. After re-resolving lockedProjectId, it calls assertPhaseOfProjectTx(..., resolveExpenseProjectId(expense), ...) at src/app/api/expenses/[id]/route.ts:320, using the pre-transaction project. If a fallback estimate moves from job A to B, an A-only code can be written onto job B and stamped manual. The similarly stale itemId check is not repeated in the transaction either. Validate both against lockedProjectId under lock.
The deployment procedure has a live-write gap. scripts/apply-expense-attribution.mjs:399 performs the date re-anchor and backfills before deployment while the old application remains capable of inserting new UTC-midnight, projectId = NULL expenses. Those rows arrive after the one-time updates and the optional attribution backfill does not re-anchor dates. Require an idempotent post-deploy rerun or provide a compatibility mechanism covering writes between DDL application and deployment.
The advertised coverage metric can count cross-job item links as attributed. scopedItemCostCodes() stores results by itemId alone (scripts/backfill-expense-attribution.ts:114), and measureCoverage() later resolves using that global map (:132). Once any legitimate job-A expense populates item I, a corrupt job-B expense pointing to I also receives its code in the metric. Key by project plus item, or resolve per row, and test this collision.
Pipeline receipt links break after client refreshes. getTimeExpenseData() converts stored receipt-intake:// references into signed URLs, but getExpenses() at src/lib/time-expense-actions.ts:366 returns raw rows. ExpensesTab uses the latter after tax saves and deletes, replacing working links with an unusable custom URI. Both loaders need the same serialization helper.
The inline intake endpoint bypasses the shared source-reference validation. At src/app/api/receipts/intake/route.ts:231-233, secret callers only need the correct namespace prefix, while /start uses decideSource() to enforce source-specific shape, length, and control-character rules. Inline requests can therefore create invalid or colliding QuickBooks identities and oversized unique-index values. Route both flows through decideSource() and add endpoint-level parity tests.

…tale checks answer about the locked job Codex round 21, findings 1-5. 1. FOUR writers still stamped `projectId` from a value read before their transaction: the QBO create and its catch-up fill, the AI parse, the Drive ingest, and the receipt booking's create path. Each resolved an estimate's job, did real work (a QBO round trip, a model call, a phase lookup), and then wrote both columns — so an estimate moved in that window produced an expense claiming two jobs at once, which `resolveExpenseProjectId` and every join through the estimate answer differently. All four now re-read the pair inside the write transaction through `lockEstimateAttribution`. The locked answer is the authority: the QBO paths write it (an unattributed row is honest, half a pair is not), the booking and the two routes REFUSE on a disagreement rather than guessing. A new tripwire in tests/expense-attribution-pair.test.ts fails when a future writer of the column appears without the re-read; its `data: { … }` detection matches braces rather than a character budget, because a fixed window read billing-core's invoice stamp as an attribution write. 2. The legacy PUT re-resolved the job under lock and then validated against the PRE-transaction one — the single value that re-resolve exists to distrust. Both checks now answer about `lockedProjectId`: the phase invariant, and the item link, which is re-asked inside the transaction through the new `itemBelongsToProjectTx` (it locks the item AND its estimate, because the link can break from either end). The pre-transaction item check stays as a fast fail and is documented as holding nothing. 3. The apply script cannot close the live-write gap in one pass — it runs BEFORE the new build, and the old build keeps writing NULL-projectId, UTC-midnight rows until it drains. The two backfills are now an exported subset with a POST-DEPLOY marker and a `--post-deploy` mode that re-runs only them. Both were already idempotent by predicate (`projectId IS NULL`; `attributionAnchoredAt IS NULL` AND a 00:00 time-of-day) — which is exactly the shape of the stragglers — and a test pins that they are a subset of the main run rather than a second copy that can drift. 4. The coverage metric keyed its item map by item id alone. The cross-job gate was applied when an entry was ADMITTED and then any other row pointing at that item read the same entry back — including the corrupt cross-job row the gate exists to exclude. Keyed by `projectId:itemId` now, so the scope is part of the lookup and the false positive is unrepresentable. 5. The expenses tab has two doors — the page's first render and the client refresh after a save — and only the first resolved `receipt-intake://` references to signed URLs. Every pipeline-booked receipt link died on the first tax edit. Both go through one `resolveReceiptUrls` helper now. Finding 6 (inline intake bypasses decideSource) is Phase 1 code and is routed to that builder. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>

github-actions · 2026-09-02T19:11:51Z

Request changes. The diff still has correctness failures in attribution, tax review, and rollout safety.

QBO sync can silently assign an expense to the wrong job. After matching an expense to a project/estimate, the create and fill paths re-read the estimate and accept its current project without verifying it matches the project used during matching (qbo-expense-sync.ts). If the estimate moves concurrently, the purchase is redirected to an unrelated job. Treat a missing or mismatched locked attribution as stale: abort, skip, or retry matching. Add a regression test that rejects—not blesses—this transition.
Receipt finalization is not idempotent when a phase is supplied. lateFields includes costCodeSource, but applyLateFields does not select that column (route.ts). The reconciliation loop consequently compares the requested source against undefined and returns 409 on an otherwise identical retry (late-fields.ts). Include provenance in every read, merge, and CAS guard, then test repeated finalization with the same phase.
The documented deployment sequence omits the migration script’s mandatory post-deploy pass. The script itself explains that the old build can create null attribution and UTC-midnight rows between the pre-deploy migration and rollout, and provides --post-deploy to repair them (apply-expense-attribution.mjs). The committed rollout plan stops after deployment and an optional attribution backfill (PHASE-3-ATTRIBUTION-SPEC.md); that optional backfill does not perform the date re-anchoring. Make the post-deploy invocation and verification mandatory in both the spec and deployment instructions.
The tax PATCH endpoint accepts non-number JSON values as money. Calling Number() without checking the input type means false, "", [], and numeric arrays can become valid zero/value amounts (route.ts). A flagged expense can therefore clear needsTaxReview with meaningless inputs. Require each supplied monetary field to be a finite JSON number or explicit null before conversion, and add rejection tests for coercible strings, booleans, and arrays.
The backfill’s transactional replan uses a stale snapshot of which projects are “In Progress.” scopedProjectIds is computed before processing (backfill-expense-attribution.ts) and reused inside the later locked replan without rechecking project status (backfill-expense-attribution.ts). A project closed during a long run can still receive an AI cost code, violating the declared scope. Re-read and validate project status under the transaction/lock or include it in the write predicate, with a status-transition test.

Local test execution was unavailable because the command sandbox failed during initialization; this review used the exact matching PR patch and head sources.

VERDICT: REQUEST_CHANGES

CI doesn't set DATABASE_URL so any test that imports a Prisma-dependent module fails with "DATABASE_URL is not set". Add the standard mock at the top of both attribution test files. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>

github-actions · 2026-09-02T20:50:28Z

QBO sync can silently attribute an expense to the wrong project. In qbo-expense-sync.ts, the create and legacy-fill paths replace the matched projectId with the project returned by lockEstimateAttribution() without checking that they agree. If an estimate is moved—or loses its project—between matching and the transaction, the purchase is imported onto the wrong job or left unattributed. Refuse/retry when the locked pair differs from the planned pair, and add interleaving tests for both create and fill paths.
The documented deployment procedure omits a required data-repair pass. apply-expense-attribution.mjs explicitly requires another --post-deploy run because old instances can keep creating null-project and UTC-midnight rows after the pre-deploy pass. The PR’s deployment order jumps directly from merge to an “optional” backfill. Make the post-deploy invocation mandatory after old instances drain and verify it reports zero remaining repairs; otherwise expenses can retain incorrect reporting-period dates.

VERDICT: REQUEST_CHANGES

…, and make the post-deploy pass mandatory Codex round 22 (REQUEST_CHANGES), 2 findings: 1. upsertQboExpense's create and legacy-fill paths wrote whatever project lockEstimateAttribution() returned, even when it disagreed with the project the purchase was matched and shaped for. write.projectId / plan.fill.projectId describe the job a purchase's vendor/description/ amount were classified against; silently swapping in a newer, disagreeing lockEstimateAttribution() answer landed a purchase classified for one job on another job's books. Both paths now compare the two and refuse the attribution write on a mismatch (skip-with-warning for create, skip-the- fill-only for the update path, since its tax/amount reconciliation is independent of attribution) — the next sync re-matches against the estimate's current project instead. tests/qbo-expense-sync.test.ts: the two tests asserting the old trust-the-lock-unconditionally behavior are rewritten to assert the refusal instead. 2. apply-expense-attribution.mjs's --post-deploy pass (closes the live-write gap while the old build drains) was documented as an "and again" nice-to- have. Its header now says plainly that skipping it leaves rows permanently unattributed with no error, and the script now verifies BOTH backfills report zero remaining rows (previously only the projectId one was checked) — the re-anchor gets the same "0 expenses left at UTC midnight" assertion. PHASE-3-ATTRIBUTION-SPEC.md goal 1 now states the post-deploy pass is mandatory and names the exact verify lines to look for. npm run build: 0 errors. tsc --noEmit: clean. test:expense-attribution (303 tests) and test:qbo-expense-sync (116 pass, 2 skipped — no local test DB) both green. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>

github-actions · 2026-09-02T21:25:35Z

I applied the repository’s Supabase/Postgres locking guidance. Four changes are required:

P1 — PATCH silently coerces invalid JSON into certified financial values. route.ts:599, route.ts:711, and route.ts:747 use Number(value). Consequently false, "", and [] become zero. A flagged expense receiving {"taxReviewAck":true,"taxAmount":false,"taxDeductibleBase":[]} passes validation, stores two zeroes, stamps manual provenance, and clears needsTaxReview despite containing no reviewed figures. Likewise, route.ts:811 treats every non-string costCodeId as null, silently clearing the phase. Require exact JSON types and add malformed-payload tests.
P1 — The claimed global lock order is not global. phase-invariant.ts:69 mandates Project → Estimate → EstimateItem → CostCode, but expense creation locks the estimate first at expenses/route.ts:171 and then enters that order at expenses/route.ts:181. The same inversion exists in time-expense-core, receipt ingestion, QBO suggestion, and PATCH. The backfill is explicit: backfill-expense-attribution.ts:439 locks Estimate and EstimateItem before calling the Project-first helper. This permits deadlocks against project deletion/FK actions or any Project-first updater. Refactor all callers to acquire one consistent order and test the SQL call sequence.
P1 — The supplied deployment procedure omits a mandatory production step. apply-expense-attribution.mjs:18 says the migration requires a second --post-deploy pass after the old Vercel build drains. Without it, expenses created during the deployment window remain null-attributed and UTC-anchored. The PR body instead ends with merge followed only by an optional attribution backfill. Update the authoritative rollout checklist and require the post-deploy verification to report zero unattributed and zero unanchored rows.
P2 — The committed contract contradicts the implementation and PR intent. PHASE-3-ATTRIBUTION-SPEC.md:358 says taxDeductibleBase is optional when acknowledging review, while route.ts:623 requires both keys on flagged rows, matching the PR body. This will cause clients built from the documented contract to receive unexpected 400s. Choose one contract and align the spec, API, and tests.

VERDICT: REQUEST_CHANGES

…d instead of coercing them Codex round 22 adversarial review, 2 findings fixed: 1. PATCH /api/expenses/[id] used `Number(value)` on taxAmount and taxDeductibleBase (the ack-review `coherent()` check, and both write paths), which silently coerces JSON garbage into a certified figure: `Number(false)`, `Number("")`, and `Number([])` are all `0`. A malformed or buggy client could book a $0 tax answer nobody actually entered. All four sites now require `typeof value === "number"` before touching it, and reject anything else with a 400. 2. The costCodeId write path only checked `typeof body.costCodeId === "string"` to decide the value, silently falling through to `null` (clear the cost code) for any other type — a number, boolean, array, or object. That treats a malformed request as an intentional attribution wipe. Now rejects a non-string, non-null costCodeId with a 400 instead of writing it. 3 new regression tests cover the coercible-to-zero values, the ack path's own coherent() check, and the costCodeId type guard. Also fixes docs/plans/PHASE-3-ATTRIBUTION-SPEC.md, which said taxDeductibleBase is optional when acknowledging a review — the code has always required both taxAmount and taxDeductibleBase together on a FLAGGED row (the flag means the whole classification is in doubt, and the two figures are the whole classification). The spec now matches. 2 findings from the same review are defended, not fixed: - the new Project->Estimate->EstimateItem->CostCode lock order in phase-invariant.ts is new infrastructure this PR introduces for the attribution path; pre-existing callers (e.g. expense creation's lockExpense) predate it and are out of scope for this PR. - the post-deploy procedure finding was already addressed in dc7bf48, which made the post-deploy pass explicitly mandatory in both the spec and the script's own header/verification output. npm run build: 0 errors. tsc --noEmit: clean. test:expense-attribution: 306 pass (303 + 3 new), 0 fail. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>

…ute.ts The previous commit's edit tool rewrote route.ts through a full-file string replacement, which normalized the file's pre-existing mixed CRLF/LF line endings (a known repo issue — see MEMORY.md "Apply script vs migration parity") to uniform CRLF. That turned a 4-site, ~40-line logic change into a 238-line diff with no functional difference, which would have obscured the actual fix from review. Reconstructed the file byte-for-byte against HEAD~2 (before either of these two commits), keeping every unchanged line's original EOL exactly as it was and using CRLF only for the genuinely new/changed lines. The diff against HEAD~2 is now the real ~40-line change. No logic changed. npm run build: 0 errors. tsc --noEmit: clean. test:expense-attribution: 306 pass, 0 fail. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>

github-actions · 2026-09-02T22:19:55Z

P1 — A deduction-base-only edit falsely makes OCR tax “manual.” The PATCH stamps taxSource: "manual" whenever either tax figure changes (route.ts), while booking refuses to fill tax on any human-sourced row (book.ts). Entering only taxDeductibleBase therefore permanently protects an untouched—or absent—OCR taxAmount. Make provenance field-specific, or only let taxAmount control taxSource; add a regression test for base-only edits followed by OCR fill.
P1 — QBO invalidation leaves an impossible provenance state. When the new gross cannot support the stored tax, the sync clears every tax value and raises needsTaxReview, but leaves the prior "manual"/"manual-none" source intact (qbo-expense-sync.ts). That contradicts the documented state where invalidated figures have taxSource = null, and blocks later automated rereads despite nobody standing behind the remaining values. Clear taxSource atomically whenever the figures are invalidated, and test both human source states.
P1 — The advertised deployment procedure omits its mandatory second pass. The apply script explicitly says old Vercel instances can create NULL-project and UTC-midnight rows after the pre-deploy migration and requires --post-deploy after they drain (apply-expense-attribution.mjs). The PR body’s rollout stops after the pre-deploy run and calls only the separate attribution backfill optional. Update the authoritative rollout instructions and require verification of both zero-leftover assertions after deployment.
P2 — The backfill’s headline coverage is not the variance page’s coverage. scopedItemCostCodes rejects item-derived attribution unless the code appears on a currently eligible estimate (backfill-expense-attribution.ts), while the actual variance reader deliberately includes coded items from draft and archived estimates as attribution-only rows (job-variance-db.ts). The reported before/after percentage can therefore undercount real variance coverage and cannot prove the stated >80% acceptance criterion. Separate strict write eligibility from measurement and calculate the metric using the exact variance item universe.
P2 — “Write-once” attribution has no correction path. Once QBO stamps a non-null projectId, subsequent syncs categorically refuse to change either attribution field (qbo-expense-sync.ts), while PATCH’s allowlist excludes both projectId and estimateId (route.ts). A bad fuzzy QBO match is therefore permanent through the application. Implement the promised authorized, atomic re-attribution path that moves both fields together, or explicitly scope and schedule it as a blocking dependency.

VERDICT: REQUEST_CHANGES

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

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

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

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

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

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

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

4d24b13

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

Clarion1631 and others added 18 commits September 2, 2026 09:26

Clarion1631 and others added 2 commits September 2, 2026 14:51

Navigation Menu

feat(expenses): project + phase attribution, tax paid at source (pipeline v2 phase 3) - #442

feat(expenses): project + phase attribution, tax paid at source (pipeline v2 phase 3)#442
Clarion1631 wants to merge 109 commits into
mainfrom
feat/phase3-attribution

Clarion1631 commented Sep 2, 2026 •

edited

Loading

Uh oh!

vercel Bot commented Sep 2, 2026 •

edited

Loading

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!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

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

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!

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