diff --git a/.beads/issues.jsonl b/.beads/issues.jsonl index c48277d2..1dbeb96a 100644 --- a/.beads/issues.jsonl +++ b/.beads/issues.jsonl @@ -5,6 +5,7 @@ {"_type":"issue","id":"onetwo3d-ims-3i0","title":"COGS recognition deferred to nightly Xero batch — null cost-layer snapshots until Group B","description":"ShipmentLine.costLayerSnapshot is only populated by the daily Xero Group B batch. Until that runs, interim P\u0026L and margin reports are wrong. A failed batch leaves COGS unrecognized indefinitely. Manual SHIPPED transitions (outside the allocation flow) crash the next batch run.\n\nRefs:\n- app/actions/allocation.ts:1089 (confirmAllocations creates ShipmentLine without snapshot)\n- lib/connectors/xero/daily-sync.ts:124-126 (Group B throws if snapshot is null)\n- lib/connectors/xero/daily-sync.ts:950 (snapshot only populated here, hours later)\n\nFix direction: take the snapshot at the SHIPPED transition, not in the batch. Make the batch idempotent against pre-existing snapshots.","status":"closed","priority":0,"issue_type":"bug","assignee":"OneTwo3D IMS","owner":"dev@onetwo3d.com","created_at":"2026-04-25T09:55:51Z","created_by":"OneTwo3D IMS","updated_at":"2026-04-25T10:31:49Z","started_at":"2026-04-25T10:31:49Z","closed_at":"2026-04-25T10:31:49Z","close_reason":"Verified: not a bug. costLayerSnapshot IS populated on ShipmentLine at the SHIPPED transition (allocation.ts:1341-1350) — the agent confused snapshot *consumption* by the daily batch with snapshot *creation*. Snapshots are written synchronously during transitionShipmentStatus (allocation.ts:1277), inside the same tx as the FIFO consumption + StockLevel decrement. The daily-sync.ts:124-126 throw is a defensive guard that fires only if a shipment was somehow marked SHIPPED outside the standard path — and there is no such path in the codebase (grep confirms allocation.ts is the only writer of status:SHIPPED on Shipment).","labels":["code-review","cogs","o2c","sync-xero"],"dependency_count":0,"dependent_count":0,"comment_count":0} {"_type":"issue","id":"onetwo3d-ims-ahu","title":"No realised FX gain/loss on payment, no AR/AP revaluation","description":"Pay a EUR bill weeks later at a different rate — settlement variance is silently dropped. Sales invoices in non-base currency are never revalued. No grep hits for 'realised', 'fx gain', 'fx loss' anywhere in the codebase.\n\nImpact: P\u0026L missing FX gains/losses for every multi-currency settlement; AR/AP balances drift away from underlying foreign value over time; month-end reconciliation to Xero will fail.\n\nRefs:\n- app/actions/sales.ts:2690-2800 (payment matching is foreign-amount-only)\n- app/actions/purchase-orders.ts:2620-2720 (markBillPaid)\n\nFix direction: at payment, compute (foreign × rate-now) - (foreign × rate-on-invoice), book to FX gain/loss account. Add a periodic revaluation job for open AR/AP.","notes":"Verified: claim holds up. Grep for 'realised', 'fx gain', 'fx loss' returns 0 hits in the codebase; payment matching at sales.ts:2690-2800 and markBillPaid at purchase-orders.ts:2620-2720 reconcile by foreign-currency amount only, never compute the GBP settlement variance.\n\nTwo distinct features needed:\n1. Realised FX on payment: when a foreign bill/invoice is settled at a different rate than booked, post the difference to a P\u0026L FX gain/loss account.\n2. AR/AP revaluation: at month-end (or on demand), revalue open foreign-currency AR and AP to current rate, post unrealised FX gain/loss to a B/S contra account, reverse on the next revaluation.\n\nBoth depend on the FX policy decision in onetwo3d-ims-a8u — the booking rate must be stable to compute the variance. Schedule together.\nRe-checked 2026-04-25 during a COGS/Tax/FX workflow analysis. The implementation appears to have landed since this issue was filed:\n\n- lib/accounting-fx.ts:32-105 computes realised FX gain/loss on payment settlement (threshold 0.01 base; AP/AR control vs FX gain/loss leg).\n- lib/accounting-fx-revaluation.ts + /api/cron/accounting-fx-revaluation handle daily unrealised FX revaluation of open AR/AP, with prior-day reversal.\n- lib/connectors/xero/settings.ts exposes xero_realised_fx_gain_loss_account / xero_unrealised_fx_gain_loss_account and the xero_sync_realised_fx_journal / xero_sync_unrealised_fx_journal toggles.\n\nThe original 'grep returns 0 hits' verification is no longer accurate. Recommend either closing this with a link to the implementing commit or narrowing scope to whatever (if anything) is still missing — e.g. payment matching at sales.ts:2690-2800 / purchase-orders.ts:2620-2720 should be re-verified to confirm it now invokes the realised FX path.","status":"open","priority":0,"issue_type":"bug","owner":"dev@onetwo3d.com","created_at":"2026-04-25T09:55:51Z","created_by":"OneTwo3D IMS","updated_at":"2026-04-25T20:48:07Z","labels":["accounting","code-review","fx"],"dependencies":[{"issue_id":"onetwo3d-ims-ahu","depends_on_id":"onetwo3d-ims-a8u","type":"blocks","created_at":"2026-04-25T12:38:45Z","created_by":"Jan","metadata":"{}"}],"dependency_count":1,"dependent_count":0,"comment_count":0} {"_type":"issue","id":"onetwo3d-ims-a8u","title":"FIFO cost layer not reconciled with supplier invoice (FX/landed cost)","description":"Cost layers are created with the PO line's base-cost at receipt, but the bill can arrive later with a different FX rate or unit cost. There is no reconciliation, so the FIFO layer (and therefore COGS) is permanently in a different base currency than AP.\n\nRefs:\n- app/actions/purchase-orders.ts:1827-1837 (layer create on receipt uses unitCostBase from PO line)\n- app/actions/purchase-orders.ts:2398-2401 (invoice FX rate looked up at invoice date, not PO date)\n- app/actions/purchase-orders.ts:3360-3370 (recalculateDirectLandedCosts uses primary PO unitCostBase, ignores invoice FX)\n- app/actions/purchase-orders.ts:2157-2158 (returns reverse PO-line cost, not billed cost)\n\nImpact: AP/inventory mismatch in base currency; balance sheet does not tie out for foreign-currency suppliers; landed-cost allocations are skewed when invoice FX != PO FX.\n\nDecision needed: pick a single FX policy (PO-rate, receipt-rate, or invoice-rate for the layer; book the FX delta as gain/loss) and retrofit. See related issues for AR/AP revaluation and realised FX on payment.","notes":"Verified: claim holds up. Cost layer cost basis (purchase-orders.ts:1827-1837 receipt path) uses the PO line's unitCostBase (computed at PO creation time from the PO's fxRateToBase). Invoice creation (purchase-orders.ts:2398-2401) re-resolves FX rate at invoice date. The two are never reconciled, so AP and inventory base-currency totals diverge for foreign-currency suppliers when PO date and invoice date differ.\n\nDecision needed (finance team):\n- Policy A: lock FX at PO issuance (current FIFO behaviour). Invoice rate unused for valuation; FX delta booked at invoice as gain/loss.\n- Policy B: lock FX at receipt (current FIFO behaviour matches if PO rate == receipt-day rate); invoice rate booked separately to AP, FX delta at receipt vs invoice booked as gain/loss.\n- Policy C: lock FX at invoice; FIFO layer gets retroactively corrected when bill arrives (most accurate, hardest to implement — requires re-walking COGS for shipments that consumed the layer between receipt and invoice).\n\nRecommend Policy A for simplicity unless finance team is doing accrual revaluation. Pair with onetwo3d-ims-ahu (realised FX gain/loss) for a complete fix.","status":"open","priority":0,"issue_type":"bug","owner":"dev@onetwo3d.com","created_at":"2026-04-25T09:55:50Z","created_by":"OneTwo3D IMS","updated_at":"2026-04-25T10:31:53Z","labels":["code-review","cogs","fx","p2p"],"dependency_count":0,"dependent_count":1,"comment_count":0} +{"_type":"issue","id":"onetwo3d-ims-2it","title":"Invoice PDF Content-Disposition filename now uses random nonce instead of invoice id (PR #146)","description":"PR #146 changes the invoice PDF download filename from the invoice id to the token's nonce:\n\n```diff\n- 'Content-Disposition': `inline; filename=\"invoice-${safeInvoiceFilenameId(id)}.pdf\"`,\n+ 'Content-Disposition': `inline; filename=\"invoice-${safeInvoiceFilenameId(verification.payload.nonce)}.pdf\"`,\n```\n\n(`app/api/invoices/[id]/route.ts` around line 165 of the diff)\n\nThe nonce is `randomBytes(16).toString('base64url')` — a 22-character random string like `aB3xY7zP9_4kQrmTuVwXyZ`.\n\nUser impact: instead of downloading `invoice-INV-2025-001.pdf`, the user gets `invoice-aB3xY7zP9_4kQrmTuVwXyZ.pdf`. Files in the downloads folder have random-looking names that don't match the invoice they represent.\n\n**Why this matters:**\n1. **UX regression** — users can't identify invoice files at a glance; finance teams downloading dozens of invoices can't filter/sort by invoice number.\n2. **No security benefit** — the URL path `/api/invoices/{id}?token=...` already exposes the invoice id; the user clicked a link knowing what invoice they wanted. Hiding the id in the filename adds zero security.\n3. **Likely accidental** — the change isn't mentioned in the PR description's bullet points. The other PDF changes (token binding, TTL) are explicit; this filename change reads as collateral damage from the same edit.","acceptance_criteria":"- Revert the Content-Disposition filename to use `safeInvoiceFilenameId(id)` (the invoice id), not the nonce\n- Add a test that asserts the Content-Disposition header includes the invoice id, not the nonce\n- If there was a deliberate reason to use the nonce (e.g., preventing concurrent downloads from clashing), document it explicitly in a code comment AND update the PR body","notes":"Filed during PR #146 review. Highest priority of the findings because it's a likely unintentional UX regression that ships out of an otherwise good security PR.","status":"closed","priority":1,"issue_type":"bug","assignee":"Jan","owner":"dev@onetwo3d.com","created_at":"2026-06-08T23:07:42Z","created_by":"Jan","updated_at":"2026-06-08T23:21:02Z","started_at":"2026-06-08T23:12:31Z","closed_at":"2026-06-08T23:21:02Z","close_reason":"Restored invoice-id Content-Disposition filenames and updated route tests to assert sanitized invoice ids are used instead of token nonces.","labels":["code-review","pr-146","security","ux"],"dependency_count":0,"dependent_count":0,"comment_count":0} {"_type":"issue","id":"onetwo3d-ims-6p5","title":"Tighten account balance freshness plan group","description":"Verified docs/plan.md suggested groups 1 and 2 are already implemented, marked their plan items complete, and implemented group 12/P5.3 by requiring a previous-day opening GL balance snapshot by default.","status":"closed","priority":1,"issue_type":"task","assignee":"Jan","owner":"info@onetwo3d.co.uk","created_at":"2026-06-08T21:52:04Z","created_by":"Jan","updated_at":"2026-06-08T21:57:22Z","started_at":"2026-06-08T21:52:11Z","closed_at":"2026-06-08T21:57:22Z","close_reason":"Implemented P5.3 account-balance freshness and updated docs/plan.md completion status for verified completed groups.","dependency_count":0,"dependent_count":0,"comment_count":0} {"_type":"issue","id":"onetwo3d-ims-xin","title":"WC tax fallback block-policy allows fallback to zero-rate even when destination should be taxed (PR #144)","description":"PR #144's `shouldBlockWcTaxRateFallback()` (lib/connectors/woocommerce/sync/order-import.ts) blocks WooCommerce imports only when the fallback tax rate value is \u003e 0:\n\n```ts\nexport function shouldBlockWcTaxRateFallback(lines: WcTaxRateFallbackLine[]): boolean {\n return lines.some((line) =\u003e line.taxRateValue \u003e 0)\n}\n```\n\nThe original bd issue `onetwo3d-ims-xx4` (now closed) was about tax-rate fallback being silent on WC import. The fix correctly blocks imports when the fallback uses a non-zero default rate. But the inverse case is still silent:\n\n**Scenario:** A WC order ships to Germany (DE), 19% standard VAT. The DE/STANDARD tax rate exists in IMS but the WC tax rate is unmapped. The IMS resolver falls back through (country+category → country+STANDARD → global+category → order default). If the order default is a 0% rate (e.g., for a tenant that has a misconfigured zero-rated default), the resolver returns rate 0 — and `shouldBlockWcTaxRateFallback` does NOT block because `taxRateValue === 0`.\n\nResult: a 19% DE order is imported with 0% tax. Understated VAT, silent.\n\n**Why this matters:** This is the same class of bug as the original `xx4`: silent tax misconfiguration produces wrong VAT. The fix only addresses the over-rated direction.","acceptance_criteria":"- Add a second blocking condition: when the destination country has any active non-zero tax rate configured for the product category and the resolver fell back to a different (zero or otherwise non-matching) rate, block the import\n- Alternatively: require that the resolver's matched rate ID actually matches the destination jurisdiction (countryCode == destCountry) — block on any fallback regardless of rate value\n- Add a focused WC import test for the zero-rated fallback case where the destination should have been non-zero\n- Update the Sales Settings fallback widget to also surface zero-rate fallback events","notes":"Identified during PR #144 review. The PR addressed bd onetwo3d-ims-xx4 in the over-rated direction but the under-rated direction (silent zero-rate import for a non-zero-rated jurisdiction) is the more dangerous failure mode because under-reported VAT is harder to detect than over-reported VAT.","status":"open","priority":1,"issue_type":"bug","owner":"info@onetwo3d.co.uk","created_at":"2026-06-07T23:44:43Z","created_by":"Jan","updated_at":"2026-06-07T23:36:43Z","labels":["code-review","connectors","pr-144","vat"],"dependency_count":0,"dependent_count":0,"comment_count":0} {"_type":"issue","id":"onetwo3d-ims-xjj","title":"Tax snapshot refresh has a read-then-write race against status transitions (PR #144)","description":"PR #144 adds `refreshMutableDocumentTaxSnapshotsForRate()` (lib/tax/document-tax-snapshot-refresh.ts) which is called transactionally from `updateTaxRate()` when a TaxRate is edited. The function:\n\n1. `findMany` for SalesOrderLines whose `order.status IN [DRAFT, PENDING_PAYMENT]`.\n2. Loops through each line and calls `salesOrderLine.update()` plus header `salesOrder.update()` to apply the new rate.\n\nThe findMany result is materialized at read time. If a SalesOrder transitions from DRAFT → PROCESSING (or PENDING_PAYMENT → PROCESSING) between the findMany and the update, the function silently rewrites tax fields on an order that should now be immutable.\n\nThe transaction surrounding it doesn't take a SELECT FOR UPDATE on the orders, so concurrent writers can sneak in.\n\nSame gap exists in `refreshMutablePurchaseOrderTaxSnapshots` (status set `[DRAFT, RFQ_SENT, QUOTE_RECEIVED]`).\n\n**Why this matters:** Tax rate edits typically happen during operator setup, but a user shipping an order at the same moment as another user editing the tax rate can cause the shipped order's persisted tax to silently change. Hard to detect post-hoc.","acceptance_criteria":"- Either (a) add SELECT FOR UPDATE on the SalesOrder/PurchaseOrder rows before re-checking status and applying updates, or (b) re-check the order status inside the update by using a conditional update (`update where: { id, status: { in: MUTABLE_STATUSES } }`) and counting affected rows\n- Add a concurrency test that exercises the race: start the tax refresh transaction, transition the order to PROCESSING from another connection, assert no update lands on the now-immutable order\n- Document the chosen safety mechanism in lib/tax/document-tax-snapshot-refresh.ts","notes":"Identified during PR #144 review. The risk is bounded by how rarely operators edit a TaxRate concurrently with order processing, but the failure mode is silent — tax fields are denormalized onto orders/lines, so a stale-write here changes accounting numbers without an audit trail beyond the tax_rate_snapshot_refresh activity log entry (which doesn't include which lines were touched).","status":"open","priority":1,"issue_type":"bug","owner":"info@onetwo3d.co.uk","created_at":"2026-06-07T23:44:35Z","created_by":"Jan","updated_at":"2026-06-07T23:36:24Z","labels":["code-review","concurrency","pr-144","tax"],"dependency_count":0,"dependent_count":0,"comment_count":0} @@ -21,8 +22,13 @@ {"_type":"issue","id":"onetwo3d-ims-5oh","title":"Allocation snapshots editable between A2 staging and Group B run","description":"resetAllocationAccountingIfStaged() only triggers when shipmentJournalDate is set — i.e. after Group B has stamped the order. Between A2 staging and Group B, allocations can be edited and the next batch will use stale snapshots. Shipment-line snapshots are also not cleared on reset (only allocation snapshots are).\n\nRefs:\n- app/actions/allocation.ts:42, :74-77\n\nFix direction: lock allocations on A2 staging and require an explicit unstage that clears both allocation- and shipment-line snapshots.","status":"closed","priority":1,"issue_type":"bug","assignee":"OneTwo3D IMS","owner":"dev@onetwo3d.com","created_at":"2026-04-25T09:57:53Z","created_by":"OneTwo3D IMS","updated_at":"2026-04-25T10:17:44Z","started_at":"2026-04-25T10:17:43Z","closed_at":"2026-04-25T10:17:44Z","close_reason":"Verified: not a bug. resetAllocationAccountingIfStaged (allocation.ts:42-79) is called inside every allocation-mutation tx (e.g. deallocateOrder line 937). When inventoryAllocatedDate is set: (a) blocks edits if any shipment has shipmentJournalDate (Group B already ran) — line 55-64; (b) otherwise clears inventoryAllocatedDate AND OrderAllocation.costLayerSnapshot so A2 rebuilds on next batch. ShipmentLine snapshots are intentionally NOT cleared because they reflect the actual SHIPPED state and should be preserved. The 'stale snapshots in next batch' claim is incorrect.","labels":["code-review","cogs","sync-xero"],"dependency_count":0,"dependent_count":0,"comment_count":0} {"_type":"issue","id":"onetwo3d-ims-jgi","title":"StockLevel.quantity drifts from sum of cost-layer remainingQty","description":"StockLevel.quantity is mutated directly in several places and is not derived from the cost layers. Partial transaction failures or layer adjustments can leave the two out of sync. There is no reconciliation job.\n\nRefs:\n- lib/stock.ts:166-172, :193-197 (direct StockLevel mutations)\n- lib/transfers.ts:558-559 (transfer mutations)\n- lib/cost-layers.ts:181-197 (layer mutations not synced back)\n\nFix direction: either make quantity a derived view (sum of layers) or add a reconciliation job + alert when they diverge.","status":"closed","priority":1,"issue_type":"bug","assignee":"OneTwo3D IMS","owner":"dev@onetwo3d.com","created_at":"2026-04-25T09:57:53Z","created_by":"OneTwo3D IMS","updated_at":"2026-04-25T10:17:45Z","started_at":"2026-04-25T10:17:44Z","closed_at":"2026-04-25T10:17:45Z","close_reason":"Verified: not a real bug. StockLevel.quantity and cost-layer sums are mutated together inside the same Prisma $transaction at every site (allocation.ts:1303-1310 + 1327-1329, manufacturing.ts:684-705 + 722-738 + 780-798, transfers.ts:405-414 + 556-578, stock.ts:689-756, purchase-orders.ts receipts). Postgres ACID prevents partial commits. Drift is only possible via out-of-band DB writes, which no app fix can prevent. A reconciliation job would be defensive observability, not a bug fix — open a separate feature request if desired.","labels":["code-review","fifo","inventory"],"dependency_count":0,"dependent_count":0,"comment_count":0} {"_type":"issue","id":"onetwo3d-ims-0r8","title":"FIFO consumption race: candidate read happens before per-row lock","description":"lib/cost-layers.ts:62-95 — findMany reads the candidate cost-layer set, then takes per-row locks. Two concurrent shippers can read the same candidate set before either acquires a lock, then both decrement the same layer.\n\nImpact: physical stock corruption — over-consumption, negative remainingQty, broken FIFO ordering.\n\nVERIFY: confirm whether the lock is actually `SELECT ... FOR UPDATE` on the same row IDs and whether the surrounding $transaction is at Serializable isolation. If not, switch to Serializable or take the row locks before/inside the candidate read (e.g. `SELECT ... FOR UPDATE SKIP LOCKED`).","status":"closed","priority":1,"issue_type":"bug","assignee":"OneTwo3D IMS","owner":"dev@onetwo3d.com","created_at":"2026-04-25T09:57:52Z","created_by":"OneTwo3D IMS","updated_at":"2026-04-25T10:31:50Z","started_at":"2026-04-25T10:31:49Z","closed_at":"2026-04-25T10:31:50Z","close_reason":"Verified: not a bug. consumeFifoLayers (lib/cost-layers.ts:62-95) implements the textbook lock-then-re-read pattern: (1) findMany candidate IDs, (2) lockCostLayers acquires SELECT...FOR UPDATE on those IDs, (3) re-reads the locked rows via findMany on id IN [...] for fresh remainingQty values, (4) decrements based on the post-lock state. The explicit comment at line 69-70 documents this: 'Re-read after taking row locks so concurrent consumers cannot both act on the same pre-lock remainingQty snapshot and double-decrement a layer.' The agent missed the re-read.","labels":["code-review","concurrency","fifo","verify"],"dependency_count":0,"dependent_count":0,"comment_count":0} -{"_type":"issue","id":"onetwo3d-ims-gyk","title":"No daily cron creates account balance snapshots — tightened freshness window will fail in production (PR #145)","description":"Codex adversarial review of PR #145 (P5.3) identified:\n\nThe PR tightens the snapshot freshness window from 7 days to 1 day (previous-day required). But snapshot creation depends on a manual UI sync button (`app/(dashboard)/sync/xero-client.tsx:352`) and `app/actions/xero-sync.ts` — no scheduled cron handler exists in `app/api/cron/` for account balance snapshots specifically.\n\nConsequence: tenants that don't run a manual sync every day — or any fresh-install tenant — will have no previous-day snapshot. With this PR merged, every GL period-movement query will either return null (current behaviour) or fail loudly (if bd-304 is addressed).\n\nThis is a deployment dependency gap: the code tightens the contract without the operational mechanism to satisfy the contract.\n\n**Why this matters:** A tenant that never manually clicks Sync will see their COGS variance / GL period reports break silently (current) or loudly (after bd-304). Either way, the report is unavailable until someone clicks Sync — which they may not realise is required.\n\n**Reproduction:** Fresh tenant; no manual sync performed; invoke a GL COGS variance report for yesterday's period — no previous-day snapshot exists; report shows the user notice / fails.","acceptance_criteria":"- Add a scheduled cron handler (e.g. `app/api/cron/account-balance-snapshot/route.ts`) that creates the previous-day snapshot daily\n- Wire the cron entry into docs/installation.md / docs/architecture.md cron table\n- Add a fallback in the domain function: if no snapshot exists for asOf, fetch it on-demand from Xero (with explicit error if the fetch fails)\n- Document the cron dependency in CHANGELOG.md so tenants upgrading know to verify the cron is wired up\n- Add a startup / preflight assertion that fails if the cron is unscheduled in production","notes":"Filed during Codex review of PR #145. Codex severity P2; this is the classic 'tighten the contract without the operational backbone' rollout risk.","status":"closed","priority":2,"issue_type":"bug","assignee":"Jan","owner":"dev@onetwo3d.com","created_at":"2026-06-08T22:16:10Z","created_by":"Jan","updated_at":"2026-06-08T22:30:15Z","started_at":"2026-06-08T22:28:15Z","closed_at":"2026-06-08T22:30:15Z","close_reason":"Fixed in PR #145: plan complete markers now name concrete tests; account-balance period movement throws typed missing-snapshot errors; COGS GL reporting fetches missing Xero snapshots on demand; daily account-balance snapshot cron is added, documented, registered, and covered by tests.","labels":["accounting","code-review","cron","deployment","pr-145"],"dependency_count":0,"dependent_count":0,"comment_count":0} -{"_type":"issue","id":"onetwo3d-ims-304","title":"Account balance period movement returns null instead of failing loudly when previous-day snapshot is missing (PR #145)","description":"Codex adversarial review of PR #145 (P5.3 — \"Account balance opening up to 7 days stale\") identified:\n\nThe domain function `getAccountBalancePeriodMovement` (`lib/domain/accounting/account-balance-snapshots.ts:192`) returns `null` when the previous-day opening snapshot is missing, instead of throwing a typed error or returning a discriminated failure result. The report layer converts that `null` into a user notice, but the domain boundary does not satisfy the plan's acceptance criterion:\n\n\u003e \"Period movement query without a snapshot on dateFrom; assert behaviour (either fetch or fail loudly, not silently fall back).\"\n\n`null` from a domain function is silent fall-back, not loud failure — every caller has to remember to check, and a future caller that forgets will see `null` propagate into a report or aggregation as zero.\n\nThe accompanying test (`tests/domain/accounting/account-balance-snapshots.test.ts:221`) asserts the return value is `null` — pinning the silent-fallback behaviour as the expected contract rather than the loud failure the plan required.\n\n**Why this matters:** A future refactor that drops the report-layer null check (or a new caller that consumes the return value directly) would silently produce zero movements / wrong period balances. The pin makes it a feature, not a workaround.","acceptance_criteria":"- Domain function returns a discriminated union ({ success: true, ... } | { success: false, reason: 'missing_previous_day_snapshot', ... }) OR throws a typed `MissingSnapshotError`\n- Update tests to assert the failure path explicitly (rejection or discriminated failure)\n- Update report layer to handle the typed failure and continue emitting the user notice\n- Plan.md P5.3 entry's Tests pointer names the specific test (not just the file)","notes":"Filed during Codex review of PR #145. Codex severity P2; agreed because the user-visible behaviour (report shows a notice) is correct, but the contract at the domain boundary is wrong and the test pins it that way.","status":"closed","priority":2,"issue_type":"bug","assignee":"Jan","owner":"dev@onetwo3d.com","created_at":"2026-06-08T22:15:54Z","created_by":"Jan","updated_at":"2026-06-08T22:30:15Z","started_at":"2026-06-08T22:28:15Z","closed_at":"2026-06-08T22:30:15Z","close_reason":"Fixed in PR #145: plan complete markers now name concrete tests; account-balance period movement throws typed missing-snapshot errors; COGS GL reporting fetches missing Xero snapshots on demand; daily account-balance snapshot cron is added, documented, registered, and covered by tests.","labels":["accounting","code-review","pr-145","silent-failure"],"dependency_count":0,"dependent_count":0,"comment_count":0} +{"_type":"issue","id":"onetwo3d-ims-5z1","title":"Add product end-of-life feature to plan","description":"Add a planned feature to docs/plan.md for an End of Life product flag: product page checkbox, reorder forecast exclusion, filtering, and automatic archival once all warehouse stock is zero and no incoming stock remains.","status":"closed","priority":2,"issue_type":"task","owner":"info@onetwo3d.co.uk","created_at":"2026-06-08T23:25:14Z","created_by":"Jan","updated_at":"2026-06-08T23:26:47Z","closed_at":"2026-06-08T23:26:47Z","close_reason":"Added P8.1 to docs/plan.md for product End of Life checkbox, reorder forecast exclusion, EOL filtering, and automatic archival once stock and incoming supply are exhausted.","dependency_count":0,"dependent_count":0,"comment_count":0} +{"_type":"issue","id":"onetwo3d-ims-eda","title":"Password policy common-password list contains 7 entries — order-of-magnitude too small (PR #146)","description":"PR #146 adds `lib/security/password-policy.ts` with:\n\n```ts\nconst COMMON_PASSWORDS = new Set([\n 'password',\n 'password1',\n 'password12',\n 'password123',\n 'password123!',\n 'adminadmin123!',\n 'onetwoinventory1!',\n])\n```\n\n7 entries. The intent is the right one (block obviously-bad passwords) but the list is way too small to materially improve security:\n\n- The well-known SecLists 'common-passwords-top-10000' has 10k entries — none of which (except the seed entries above) are caught.\n- Real attackers use lists with 100k–1M entries.\n- The RockYou leak alone contains 14M entries.\n\nEffect: `'Letmein123!'`, `'Qwerty123!'`, `'P@ssword123'`, `'Welcome2024!'` — none caught by this list. Each meets all the structural requirements (length, uppercase, number, symbol) and passes validation.\n\n**Why this matters:** The plan item P6.6 specifically calls for stronger password requirements. A 7-entry common-password list satisfies the letter (some check exists) but not the spirit (real common passwords are blocked) of the requirement.","acceptance_criteria":"- Either:\n - **Option A** (preferred): integrate HaveIBeenPwned Pwned Passwords k-anonymity API — submit first 5 chars of SHA1 hash, get list of suffixes, check locally. Catches actual breached passwords without sending the full password anywhere.\n - **Option B**: bundle a static list of top 10k common passwords (e.g., from SecLists). Lower fidelity than HIBP but better than 7.\n - **Option C**: keep the 7-entry list AS A FLOOR plus dictionary checks (no plain English words, max repeated characters, etc.)\n- Add tests asserting the policy rejects common attack-list passwords (e.g., 'Welcome2024!', 'Qwerty123!', 'P@ssword123')\n- Also add (probably worth a separate issue): max password length (e.g., 256) to prevent bcrypt DoS via long inputs","notes":"Filed during PR #146 review. The implementation works as written, but the protection is illusory for real attacker password lists.","status":"closed","priority":2,"issue_type":"bug","assignee":"Jan","owner":"dev@onetwo3d.com","created_at":"2026-06-08T23:08:33Z","created_by":"Jan","updated_at":"2026-06-08T23:21:03Z","started_at":"2026-06-08T23:12:32Z","closed_at":"2026-06-08T23:21:03Z","close_reason":"Expanded password commonness checks with denylisted common roots, keyboard sequences, repeated-character detection, max length, and tests for common attack-list examples.","labels":["code-review","passwords","pr-146","security"],"dependency_count":0,"dependent_count":0,"comment_count":0} +{"_type":"issue","id":"onetwo3d-ims-quf","title":"Invoice PDF token max TTL hard-capped at 10 minutes with no admin override (PR #146)","description":"PR #146 sets:\n\n```ts\nconst DEFAULT_INVOICE_PDF_TOKEN_TTL_SECONDS = 10 * 60\nconst MAX_INVOICE_PDF_TOKEN_TTL_SECONDS = 10 * 60\n```\n\nBoth default and max are 10 minutes. The old values were 3-day default and 30-day max. The 30-day max was clearly too lax, but 10-min max means:\n\n1. **No admin override possible** — if a tenant needs longer TTL for a specific workflow (e.g., finance team scheduled an invoice review for tomorrow), there's no env or settings configuration that allows it.\n2. **Email-an-invoice-link workflows break** — operator emails themselves an invoice link from desktop, opens it on phone 15 minutes later → expired.\n3. **Batch download flows break** — opening multiple invoices, leaving tabs in background while doing other work, then downloading → first tabs expired.\n\nCombined with the IP-binding fragility (`onetwo3d-ims-qv1`), the link is effectively single-use within ~5 minutes from same device, same network. That's appropriate for the highest-security path but not configurable for tenants with more relaxed threat models.","acceptance_criteria":"- Make max TTL configurable via env var (e.g., `INVOICE_PDF_TOKEN_MAX_TTL_SECONDS`) with a sensible production default\n- Document the env var in `.env.example` and `docs/installation.md`\n- Keep the 10-minute DEFAULT (most operators won't override) but allow tenants to raise the cap when needed\n- Consider: separate TTL for 'fresh download from invoice page' (short, 10min) vs 'recovery from emailed link' (longer, 1h-24h)","notes":"Filed during PR #146 review. The PR's stated rationale ('shortens max/default TTL to 10 minutes') is sensible for the default; the lack of any override path is the operational risk.","status":"closed","priority":2,"issue_type":"bug","assignee":"Jan","owner":"dev@onetwo3d.com","created_at":"2026-06-08T23:08:19Z","created_by":"Jan","updated_at":"2026-06-08T23:21:04Z","started_at":"2026-06-08T23:12:32Z","closed_at":"2026-06-08T23:21:04Z","close_reason":"Added INVOICE_PDF_TOKEN_MAX_TTL_SECONDS with a 24-hour default cap, documented it, and covered configurable max TTL in tests.","labels":["code-review","operational","pr-146","security"],"dependency_count":0,"dependent_count":0,"comment_count":0} +{"_type":"issue","id":"onetwo3d-ims-qv1","title":"Invoice PDF token IP-binding is operationally fragile for legitimate users (PR #146)","description":"PR #146 binds invoice PDF tokens to both the session and the client IP. The IP binding breaks token validity for legitimate users in several realistic scenarios:\n\n1. **Mobile users**: a user opens the invoice list on home WiFi, then their phone switches to cellular (different IP), then they tap the invoice link → token rejected with `wrong_ip`.\n2. **Corporate NAT / load balancers**: enterprise networks rotate the apparent client IP on subsequent requests through different egress routes → token rejected mid-session.\n3. **VPN reconnect**: VPN clients drop and reconnect with a new IP regularly → broken links.\n4. **Roaming / hotspot handoff**: mobile networks shift IPs every few minutes on long-running pages.\n5. **Tab restored after device sleep**: IP may have changed.\n\nThe threat model the binding addresses is 'shared link from a stolen URL'. But session-binding alone already prevents this for non-authenticated attackers (they don't have the session cookie). For authenticated attackers who do have the session, IP binding helps marginally but at significant operational cost.\n\n**Why this matters:** The PR description says PDF links 'are no longer generated into WooCommerce customer notes because the hardened token now requires current session/IP binding'. That's an explicit acknowledgment that the binding is too strict for non-interactive use. But the same strictness applies to interactive users on mobile/VPN/corporate networks.\n\n**Refs:**\n- `lib/invoice-pdf.ts` — `bindingHash('clientIp', ...)` in signing and verification\n- `app/api/invoices/[id]/route.ts` — `getInvoicePdfTokenBinding` extracts clientIp from `getClientIp(request.headers)`","acceptance_criteria":"- Decide on the threat model explicitly: is IP binding worth the operational cost given session binding is already enforced?\n- If yes (keep IP binding): document the operational caveats in user-facing help docs ('if you switch networks, refresh the invoice list to get a new link')\n- If no (drop IP binding): keep session binding; remove the `ipHash` field and `wrong_ip` reason\n- Consider middle ground: bind to /24 subnet rather than exact IP — catches most legitimate IP changes (mobile carrier subnet stays similar), still blocks attackers from another network\n- Add a 'PDF link expired or invalid — please request a new one from the invoice page' user-facing error that operators see when the binding fails (currently the route returns 404 / 403 which is opaque)","notes":"Filed during PR #146 review. The same trade-off was implicitly acknowledged by the PR's own removal of WC customer-note links. Worth making the trade-off explicit rather than discovering it via support tickets.","status":"closed","priority":2,"issue_type":"bug","assignee":"Jan","owner":"dev@onetwo3d.com","created_at":"2026-06-08T23:08:02Z","created_by":"Jan","updated_at":"2026-06-08T23:21:05Z","started_at":"2026-06-08T23:12:32Z","closed_at":"2026-06-08T23:21:05Z","close_reason":"Removed exact client-IP token binding while retaining authenticated-session binding, and added a clearer invalid/expired-link recovery message.","labels":["code-review","operational","pr-146","security"],"dependency_count":0,"dependent_count":0,"comment_count":0} +{"_type":"issue","id":"onetwo3d-ims-76n","title":"Implement production plan group 13 security hardening","description":"Why this issue exists: docs/plan.md group 13 is the next unimplemented production-readiness batch after groups 1-12. It covers P6.2 supplier portal tenant boundaries, P6.4 invoice PDF token leakage mitigation, P6.5 generic WooCommerce connector errors, P6.6 stronger password requirements, and P6.8 activity-log metadata redaction. What needs to be done: implement the security hardening batch with focused tests, mark the covered plan items/group complete, run validation, commit, push, and open a PR against development.","status":"closed","priority":2,"issue_type":"task","assignee":"Jan","owner":"info@onetwo3d.co.uk","created_at":"2026-06-08T22:41:02Z","created_by":"Jan","updated_at":"2026-06-08T22:57:44Z","started_at":"2026-06-08T22:41:06Z","closed_at":"2026-06-08T22:57:44Z","close_reason":"Implemented production plan group 13 security hardening: supplier boundary assertions, session/IP-bound invoice PDF tokens, generic WooCommerce config errors, stronger password policy, and activity-log secret redaction coverage.","dependency_count":0,"dependent_count":0,"comment_count":0} +{"_type":"issue","id":"onetwo3d-ims-gyk","title":"No daily cron creates account balance snapshots — tightened freshness window will fail in production (PR #145)","description":"Codex adversarial review of PR #145 (P5.3) identified:\n\nThe PR tightens the snapshot freshness window from 7 days to 1 day (previous-day required). But snapshot creation depends on a manual UI sync button (`app/(dashboard)/sync/xero-client.tsx:352`) and `app/actions/xero-sync.ts` — no scheduled cron handler exists in `app/api/cron/` for account balance snapshots specifically.\n\nConsequence: tenants that don't run a manual sync every day — or any fresh-install tenant — will have no previous-day snapshot. With this PR merged, every GL period-movement query will either return null (current behaviour) or fail loudly (if bd-304 is addressed).\n\nThis is a deployment dependency gap: the code tightens the contract without the operational mechanism to satisfy the contract.\n\n**Why this matters:** A tenant that never manually clicks Sync will see their COGS variance / GL period reports break silently (current) or loudly (after bd-304). Either way, the report is unavailable until someone clicks Sync — which they may not realise is required.\n\n**Reproduction:** Fresh tenant; no manual sync performed; invoke a GL COGS variance report for yesterday's period — no previous-day snapshot exists; report shows the user notice / fails.","acceptance_criteria":"- Add a scheduled cron handler (e.g. `app/api/cron/account-balance-snapshot/route.ts`) that creates the previous-day snapshot daily\n- Wire the cron entry into docs/installation.md / docs/architecture.md cron table\n- Add a fallback in the domain function: if no snapshot exists for asOf, fetch it on-demand from Xero (with explicit error if the fetch fails)\n- Document the cron dependency in CHANGELOG.md so tenants upgrading know to verify the cron is wired up\n- Add a startup / preflight assertion that fails if the cron is unscheduled in production","notes":"Filed during Codex review of PR #145. Codex severity P2; this is the classic 'tighten the contract without the operational backbone' rollout risk.","status":"open","priority":2,"issue_type":"bug","owner":"dev@onetwo3d.com","created_at":"2026-06-08T22:16:10Z","created_by":"Jan","updated_at":"2026-06-08T22:16:10Z","labels":["accounting","code-review","cron","deployment","pr-145"],"dependency_count":0,"dependent_count":0,"comment_count":0} +{"_type":"issue","id":"onetwo3d-ims-304","title":"Account balance period movement returns null instead of failing loudly when previous-day snapshot is missing (PR #145)","description":"Codex adversarial review of PR #145 (P5.3 — \"Account balance opening up to 7 days stale\") identified:\n\nThe domain function `getAccountBalancePeriodMovement` (`lib/domain/accounting/account-balance-snapshots.ts:192`) returns `null` when the previous-day opening snapshot is missing, instead of throwing a typed error or returning a discriminated failure result. The report layer converts that `null` into a user notice, but the domain boundary does not satisfy the plan's acceptance criterion:\n\n\u003e \"Period movement query without a snapshot on dateFrom; assert behaviour (either fetch or fail loudly, not silently fall back).\"\n\n`null` from a domain function is silent fall-back, not loud failure — every caller has to remember to check, and a future caller that forgets will see `null` propagate into a report or aggregation as zero.\n\nThe accompanying test (`tests/domain/accounting/account-balance-snapshots.test.ts:221`) asserts the return value is `null` — pinning the silent-fallback behaviour as the expected contract rather than the loud failure the plan required.\n\n**Why this matters:** A future refactor that drops the report-layer null check (or a new caller that consumes the return value directly) would silently produce zero movements / wrong period balances. The pin makes it a feature, not a workaround.","acceptance_criteria":"- Domain function returns a discriminated union ({ success: true, ... } | { success: false, reason: 'missing_previous_day_snapshot', ... }) OR throws a typed `MissingSnapshotError`\n- Update tests to assert the failure path explicitly (rejection or discriminated failure)\n- Update report layer to handle the typed failure and continue emitting the user notice\n- Plan.md P5.3 entry's Tests pointer names the specific test (not just the file)","notes":"Filed during Codex review of PR #145. Codex severity P2; agreed because the user-visible behaviour (report shows a notice) is correct, but the contract at the domain boundary is wrong and the test pins it that way.","status":"open","priority":2,"issue_type":"bug","owner":"dev@onetwo3d.com","created_at":"2026-06-08T22:15:54Z","created_by":"Jan","updated_at":"2026-06-08T22:15:54Z","labels":["accounting","code-review","pr-145","silent-failure"],"dependency_count":0,"dependent_count":0,"comment_count":0} {"_type":"issue","id":"onetwo3d-ims-jox","title":"Tax snapshot refresh is N+1: per-line update inside a loop (PR #144 follow-up)","description":"PR #144's `refreshMutableSalesOrderTaxSnapshots` and `refreshMutablePurchaseOrderTaxSnapshots` (lib/tax/document-tax-snapshot-refresh.ts) loop through every affected line:\n\n```ts\nfor (const line of lines) {\n const next = calculateSalesLineTaxSnapshot({ ... })\n await client.salesOrderLine.update({ where: { id: line.id }, data: { ... } })\n // accumulate delta into Map\n}\nfor (const [orderId, delta] of deltas) {\n await client.salesOrder.update({ where: { id: orderId }, data: { increment: ... } })\n}\n```\n\nFor a TaxRate change affecting 500 DRAFT order lines across 50 orders, that's:\n- 500 individual `salesOrderLine.update` round-trips\n- 50 `salesOrder.update` round-trips\n- All inside a single Prisma `$transaction` (long lock hold)\n\nThe whole operation runs synchronously in `updateTaxRate`, which is a settings page action — operator sees the form spin while 500+ queries run.\n\n**Why this matters:** Tax rate edits are infrequent but blocking. For a tenant with substantial draft queues (e.g., subscription-based recurring orders staged in DRAFT), this turns a settings change into a multi-second outage of the tax-rate edit endpoint AND holds locks on every affected row.","acceptance_criteria":"- Replace per-line update with a single `updateMany` (or raw SQL UPDATE) where possible — most lines just need rate/tax fields rewritten using the same formula\n- For cases where per-line computation is required (e.g., inclusive vs exclusive), batch into smaller chunks (~50 lines per round-trip) with explicit progress\n- For tenant safety, add a row-count threshold: if more than N affected lines, defer to a background job rather than blocking the UI\n- Add a perf test asserting that a 500-line refresh completes in under T seconds","notes":"Identified during PR #144 review. The function works correctly; this is a perf/scale concern not a correctness one. Bumping priority above P3 because settings-page latency is operator-visible and the transaction lock duration affects concurrent users.","status":"open","priority":2,"issue_type":"chore","owner":"info@onetwo3d.co.uk","created_at":"2026-06-07T23:44:58Z","created_by":"Jan","updated_at":"2026-06-07T23:37:54Z","labels":["code-review","perf","pr-144","tax"],"dependency_count":0,"dependent_count":0,"comment_count":0} {"_type":"issue","id":"onetwo3d-ims-dmi","title":"Mutable tax statuses include RFQ_SENT/QUOTE_RECEIVED — supplier-facing quotes silently re-rated (PR #144)","description":"PR #144's tax snapshot refresh treats these PurchaseOrder statuses as mutable:\n\n```ts\nconst MUTABLE_PURCHASE_TAX_STATUSES = ['DRAFT', 'RFQ_SENT', 'QUOTE_RECEIVED'] as const\n```\n\nDRAFT is unambiguous — nothing has been sent. But:\n\n- **RFQ_SENT**: an RFQ document has been generated and (typically) emailed to the supplier. The supplier's response is based on the tax rate IMS sent.\n- **QUOTE_RECEIVED**: the supplier has responded with quoted prices, presumably with their own tax expectations.\n\nIf an IMS operator edits a TaxRate while RFQ or quote responses are in flight, the IMS-side tax fields silently change, but:\n\n1. The supplier's RFQ PDF (already sent) still shows the OLD rate.\n2. The supplier's quote (if received) doesn't account for the new rate.\n3. The IMS operator sees the new rate without realizing the supplier is operating on the old one.\n\n**Why this matters:** When the PO is eventually issued, the IMS tax fields don't match the supplier's quotation. Discrepancies surface only at invoice reconciliation, often weeks later.\n\nThe original bd issue context (PR #144's intent) was to keep MUTABLE statuses limited to truly editable documents. RFQ_SENT and QUOTE_RECEIVED arguably aren't editable in this sense — the document has crossed a supplier boundary.","acceptance_criteria":"- Decide with finance/operations: are RFQ_SENT and QUOTE_RECEIVED considered IMS-mutable for tax purposes, or have they crossed a 'document sent to counterparty' boundary that locks tax fields?\n- If the latter (recommended), restrict MUTABLE_PURCHASE_TAX_STATUSES to just ['DRAFT']\n- If the former, document this explicitly in the helper's JSDoc with the operational caveat\n- Same review for MUTABLE_SALES_TAX_STATUSES — PENDING_PAYMENT means the customer has been quoted/invoiced; refreshing tax silently there is also boundary-crossing","notes":"Identified during PR #144 review. Worth treating as a product/finance decision rather than a code bug — the implementation is consistent with the stated MUTABLE_*_STATUSES sets, but the choice of which statuses count as mutable for TAX purposes specifically deserves explicit finance sign-off because the field is denormalized onto persisted documents.","status":"open","priority":2,"issue_type":"bug","owner":"info@onetwo3d.co.uk","created_at":"2026-06-07T23:44:53Z","created_by":"Jan","updated_at":"2026-06-07T23:37:20Z","labels":["code-review","p2p","pr-144","tax"],"dependency_count":0,"dependent_count":0,"comment_count":0} {"_type":"issue","id":"onetwo3d-ims-rdy","title":"PENDING FX retry queue dedupe relies on error-message prefix matching (PR #144)","description":"PR #144's `recordPendingFxOrder()` and `retryPendingWcOrdersWaitingForFx()` (lib/connectors/woocommerce/sync/order-import.ts) both look up the queue via:\n\n```ts\nerrorMessage: { startsWith: 'Missing GBP FX rate' }\n```\n\nString-prefix matching on a serialized error message is fragile:\n\n1. The error message format is owned by the FX resolver, not the queue. PR #142 already changed FX-related error messages once.\n2. The base currency is hard-coded in the prefix ('Missing GBP FX rate'). If a tenant operates in EUR base currency, this prefix never matches and the queue never deduplicates or retries.\n3. Future i18n / message-text edits silently break dedupe → orders queue infinitely without retry.\n\nSame anti-pattern flagged in PR #129 inline #2 (Prisma error.message fallback). The fix there was to trust `error.meta.target` instead of string matching.\n\n**Why this matters:** The dedupe is the only mechanism preventing the queue from accumulating multiple PENDING entries per missing-FX order. A broken dedupe means the threshold-based admin notification (`WC_PENDING_FX_ORDER_NOTIFY_THRESHOLD = 5`) fires spuriously, and `retryPendingWcOrdersWaitingForFx()` processes the same WC order N times until one succeeds.","acceptance_criteria":"- Add a structured discriminator: store a `reason: 'missing_fx_rate'` marker in the shoppingSyncLog payload (already present!) and use that for dedupe/retry lookups instead of errorMessage prefix\n- The retry function already calls `isQueuedWcOrderPayload` to validate the payload shape — reuse the same shape check for the dedupe query\n- Either filter the shoppingSyncLog query by a payload-JSON predicate, or add a separate column/index for the reason discriminator\n- Add a test for the multi-base-currency case (e.g., EUR base) to prove the fix isn't GBP-specific","notes":"Identified during PR #144 review. Closely related to onetwo3d-ims-xin (also PR #144). The dedupe-via-message-prefix anti-pattern is recurring in this codebase — also flagged in PR #129 review. Worth a sweep to find other instances.","status":"open","priority":2,"issue_type":"bug","owner":"info@onetwo3d.co.uk","created_at":"2026-06-07T23:44:48Z","created_by":"Jan","updated_at":"2026-06-07T23:37:01Z","labels":["code-review","connectors","fragile-string-match","fx","pr-144"],"dependency_count":0,"dependent_count":0,"comment_count":0} @@ -48,7 +54,8 @@ {"_type":"issue","id":"onetwo3d-ims-ggg","title":"Invoice/bill edits after Xero push are silently lost","description":"Once accountingInvoiceId is set, queueSalesInvoiceForOrder returns immediately. Even if it didn't, the Xero idempotency key is built from the internal sync-log entry ID, not invoice content, so Xero would dedupe content changes.\n\nRefs:\n- app/actions/sales.ts:1310 (early return on accountingInvoiceId)\n- lib/connectors/xero/sync-processor.ts:35 (idempotency key from sync log entry ID)\n- lib/connectors/xero/sync-processor.ts:50-65, :73 (same on bill side)\n\nFix direction: idempotency key = hash(content); on edit, queue an UPDATE op (Xero supports invoice updates pre-AUTHORISED).","notes":"Verified: claim holds up. queueSalesInvoiceForOrder at sales.ts:1310 short-circuits when accountingInvoiceId is set, and lib/connectors/xero/sync-processor.ts:35 builds idempotency keys from the internal sync-log entry ID rather than invoice content. So edits to a SalesOrder after Xero push are silently dropped on the floor — no UPDATE path exists.\n\nFixing this needs design work, not just a code patch:\n1. Schema: add SALES_INVOICE_UPDATE / PURCHASE_INVOICE_UPDATE entries to AccountingSyncType enum.\n2. Edit detection: SalesOrder line/discount/tax mutations need a hook that enqueues an UPDATE sync log when accountingInvoiceId is already set.\n3. Processor: branch in sync-processor.ts to PUT the invoice (Xero supports invoice updates while DRAFT or AUTHORISED, blocked once PAID).\n4. Idempotency: use a content hash (lines+totals+date) so re-edits generate fresh keys.\n5. UX: surface 'invoice in Xero is stale' in the UI when an edit happened post-push but the UPDATE is queued.\n\nRecommend: keep open, schedule as a focused 1-2 day task with finance team alignment on which fields trigger an UPDATE.","status":"open","priority":2,"issue_type":"bug","owner":"dev@onetwo3d.com","created_at":"2026-04-25T09:57:55Z","created_by":"OneTwo3D IMS","updated_at":"2026-04-25T10:28:38Z","defer_until":"2026-07-07T23:29:57Z","labels":["code-review","sync-xero"],"dependency_count":0,"dependent_count":0,"comment_count":0} {"_type":"issue","id":"onetwo3d-ims-qnu","title":"Refund of an unshipped order throws with unhelpful error","description":"Refund COGS reversal hard-fails if there are no shipment/allocation snapshots. The error doesn't distinguish 'no shipments exist' from 'snapshots missing' from 'snapshots stale'.\n\nRefs:\n- app/actions/sales.ts:2162-2214\n\nFix direction: detect the no-history case explicitly and either short-circuit (refund without COGS reversal) or surface a clear error to the user.","status":"closed","priority":2,"issue_type":"bug","assignee":"OneTwo3D IMS","owner":"dev@onetwo3d.com","created_at":"2026-04-25T09:57:55Z","created_by":"OneTwo3D IMS","updated_at":"2026-04-25T10:10:57Z","started_at":"2026-04-25T10:09:34Z","closed_at":"2026-04-25T10:10:57Z","close_reason":"Refund cost-reversal helpers now: (1) short-circuit with [] when the line has no shipment/allocation history (pre-fulfillment refunds), (2) throw clear messages when history exists but is insufficient — distinguishing 'never fulfilled' from 'stale snapshot'","labels":["code-review","o2c","refunds"],"dependency_count":0,"dependent_count":0,"comment_count":0} {"_type":"issue","id":"onetwo3d-ims-v30","title":"Cancelling a partially shipped order orphans PACKED shipments and stock","description":"When status is set to CANCELLED, all PENDING/PICKING/PACKED shipments are deleted but stock is not returned. SHIPPED shipments remain, so the order cannot be fully refunded — and the deleted PACKED shipment's stock is permanently lost.\n\nRefs:\n- app/actions/sales.ts:1467\n\nFix direction: generate RETURN_INBOUND stock movements for non-SHIPPED shipments before deletion, OR forbid CANCELLED on partially shipped orders.","status":"closed","priority":2,"issue_type":"bug","assignee":"OneTwo3D IMS","owner":"dev@onetwo3d.com","created_at":"2026-04-25T09:57:54Z","created_by":"OneTwo3D IMS","updated_at":"2026-04-25T10:14:17Z","started_at":"2026-04-25T10:14:17Z","closed_at":"2026-04-25T10:14:17Z","close_reason":"Verified: not a bug. Cancel flow at sales.ts:1456-1483 (a) calls deallocateOrder which releases reservedQty via applyAllocationReservationDelta('release'); (b) only deletes PENDING/PICKING/PACKED shipments — none of which decrement physical stock (FIFO consumption + StockLevel.quantity decrement happens only at SHIPPED transition in allocation.ts:1303-1310, 1327-1329); (c) blocks CANCELLED on fully SHIPPED orders. No stock is lost. Partially shipped orders end up in CANCELLED status with SHIPPED lines preserved for refund processing — that's intentional, not a leak.","labels":["code-review","o2c","state-machine"],"dependency_count":0,"dependent_count":0,"comment_count":0} -{"_type":"issue","id":"onetwo3d-ims-9x6","title":"Plan items newly marked Complete in PR #145 lack test pointers / verifying-commit refs (P0.1-P0.5, P2.1, P2.2, P2.4)","description":"Codex adversarial review of PR #145 identified:\n\nMultiple `Status: Complete` markers were added in this PR's `docs/plan.md` changes (P0.1–P0.5, P2.1, P2.2, P2.4) without:\n1. Replacing the future-tense test text (\"Add an integration test...\") with an actual test file path\n2. Including a `Reference:` line pointing to the implementing/verifying commit SHA\n\nThis matches the \"Complete marker rests on inspection\" anti-pattern called out earlier in this rollout (see bd onetwo3d-ims-8c7 for the P1.3/P1.5/P4.2 sweep).\n\n**Why this matters:** The plan.md is the canonical audit document for the production-readiness rollout. \"Complete\" markers backed only by PR-body assertions (rather than test pointers) are unverifiable by future contributors. Three months from now, someone auditing the plan won't know whether P0.1 was actually fixed or just claimed-fixed.\n\n**Concrete findings from PR #145's docs/plan.md hunk:**\n- P0.1 marked Complete — original Tests line still says \"Add an integration test that hits /api/auth/totp-setup...\"\n- P0.2 marked Complete — original Tests line still future-tense\n- P0.3, P0.4, P0.5 — same pattern\n- P2.1, P2.2, P2.4 — same pattern","acceptance_criteria":"- For each newly-marked-complete item, either:\n - Replace the future-tense Tests line with the actual test file path + test name (e.g. `tests/security/totp-setup-route.test.ts::\"rejects when TOTP already configured\"`), OR\n - Add a `Reference:` line naming the implementing commit SHA + brief verification note\n- Audit the consolidated section's checkbox list to confirm every checked item also has a pointer\n- Same sweep needed for PR #143's bd-issue closures (hyz, a8u, ahu) — those PRs closed bd issues with prose close_reasons but the plan.md entries may not have test pointers","notes":"Filed during Codex review of PR #145. Codex severity P3 (process/cosmetic) — the markers don't break code, but they degrade the audit value of the plan.md document. Related: bd onetwo3d-ims-8c7 covers the same sweep for P1.3/P1.5/P4.2.","status":"closed","priority":3,"issue_type":"chore","assignee":"Jan","owner":"dev@onetwo3d.com","created_at":"2026-06-08T22:16:28Z","created_by":"Jan","updated_at":"2026-06-08T22:30:15Z","started_at":"2026-06-08T22:21:36Z","closed_at":"2026-06-08T22:30:15Z","close_reason":"Fixed in PR #145: plan complete markers now name concrete tests; account-balance period movement throws typed missing-snapshot errors; COGS GL reporting fetches missing Xero snapshots on demand; daily account-balance snapshot cron is added, documented, registered, and covered by tests.","labels":["docs","plan-followup","pr-145"],"dependency_count":0,"dependent_count":0,"comment_count":0} +{"_type":"issue","id":"onetwo3d-ims-d1f","title":"PDF token audit treats wrong_session and wrong_ip as ordinary verification failures — security signal lost (PR #146)","description":"PR #146 adds new `wrong_session` and `wrong_ip` reasons to `PdfTokenVerificationFailureReason`. The audit logger (`auditInvoicePdfTokenAttempt`) treats them the same as any other verification failure — they get logged but not distinguished as potential token-sharing / theft signals.\n\nThese two failure modes are interesting because they specifically indicate:\n- `wrong_session`: a valid token from a previous auth session is being presented. Could be a re-authenticated user using a bookmarked link (benign) or a stolen token (malicious).\n- `wrong_ip`: a token signed for IP A is being presented from IP B. Could be a network change (benign per `onetwo3d-ims-qv1`) or a token shared/leaked (suspicious).\n\nAggregated patterns matter:\n- `wrong_session` for the SAME user repeatedly → likely benign (user re-authed)\n- `wrong_ip` from many different source IPs for tokens belonging to the SAME user → token shared in a public Slack channel\n- `wrong_session` + `wrong_ip` from a different user agent → likely stolen\n\nCurrent audit logs each attempt at the same level, with no aggregation, no rate-based alerting.","acceptance_criteria":"- Add a separate higher-severity activity log entry (`level: 'WARNING'` instead of `'INFO'`) for `wrong_session` and `wrong_ip` reasons\n- Include user-agent in the audit row metadata so investigators can spot UA mismatches\n- Add a dashboard / settings widget that surfaces recent `wrong_session` / `wrong_ip` events grouped by invoice id, similar to the tax-rate fallback widget added in PR #144\n- (Optional) Add a rate-based alert: if more than N `wrong_*` events for the same invoice id in T minutes, notify admins","notes":"Filed during PR #146 review. Lower priority because the audit logging happens; this is about extracting more value from it. Operational maturity, not correctness.","status":"closed","priority":3,"issue_type":"chore","assignee":"Jan","owner":"dev@onetwo3d.com","created_at":"2026-06-08T23:08:49Z","created_by":"Jan","updated_at":"2026-06-08T23:21:03Z","started_at":"2026-06-08T23:12:33Z","closed_at":"2026-06-08T23:21:03Z","close_reason":"Token audit metadata now includes user-agent and session-mismatch rejections are flagged as suspicious WARNING events; wrong_ip no longer exists after removing IP binding.","labels":["code-review","observability","pr-146","security"],"dependency_count":0,"dependent_count":0,"comment_count":0} +{"_type":"issue","id":"onetwo3d-ims-9x6","title":"Plan items newly marked Complete in PR #145 lack test pointers / verifying-commit refs (P0.1-P0.5, P2.1, P2.2, P2.4)","description":"Codex adversarial review of PR #145 identified:\n\nMultiple `Status: Complete` markers were added in this PR's `docs/plan.md` changes (P0.1–P0.5, P2.1, P2.2, P2.4) without:\n1. Replacing the future-tense test text (\"Add an integration test...\") with an actual test file path\n2. Including a `Reference:` line pointing to the implementing/verifying commit SHA\n\nThis matches the \"Complete marker rests on inspection\" anti-pattern called out earlier in this rollout (see bd onetwo3d-ims-8c7 for the P1.3/P1.5/P4.2 sweep).\n\n**Why this matters:** The plan.md is the canonical audit document for the production-readiness rollout. \"Complete\" markers backed only by PR-body assertions (rather than test pointers) are unverifiable by future contributors. Three months from now, someone auditing the plan won't know whether P0.1 was actually fixed or just claimed-fixed.\n\n**Concrete findings from PR #145's docs/plan.md hunk:**\n- P0.1 marked Complete — original Tests line still says \"Add an integration test that hits /api/auth/totp-setup...\"\n- P0.2 marked Complete — original Tests line still future-tense\n- P0.3, P0.4, P0.5 — same pattern\n- P2.1, P2.2, P2.4 — same pattern","acceptance_criteria":"- For each newly-marked-complete item, either:\n - Replace the future-tense Tests line with the actual test file path + test name (e.g. `tests/security/totp-setup-route.test.ts::\"rejects when TOTP already configured\"`), OR\n - Add a `Reference:` line naming the implementing commit SHA + brief verification note\n- Audit the consolidated section's checkbox list to confirm every checked item also has a pointer\n- Same sweep needed for PR #143's bd-issue closures (hyz, a8u, ahu) — those PRs closed bd issues with prose close_reasons but the plan.md entries may not have test pointers","notes":"Filed during Codex review of PR #145. Codex severity P3 (process/cosmetic) — the markers don't break code, but they degrade the audit value of the plan.md document. Related: bd onetwo3d-ims-8c7 covers the same sweep for P1.3/P1.5/P4.2.","status":"open","priority":3,"issue_type":"chore","owner":"dev@onetwo3d.com","created_at":"2026-06-08T22:16:28Z","created_by":"Jan","updated_at":"2026-06-08T22:16:28Z","labels":["docs","plan-followup","pr-145"],"dependency_count":0,"dependent_count":0,"comment_count":0} {"_type":"issue","id":"onetwo3d-ims-lvs","title":"Cost-layer revaluation audit log stores previous + patched + delta — 3x storage overhead (PR #144)","description":"PR #144's `recordCostLayerSnapshotRevaluation()` (lib/cost-layers.ts) writes an activity log row per snapshot revaluation with metadata containing:\n\n```ts\nmetadata: {\n ...\n changedEntries: params.changedEntries, // entries that changed\n previousSnapshot: params.previousSnapshot, // FULL old snapshot\n patchedSnapshot: params.patchedSnapshot, // FULL new snapshot\n}\n```\n\nFor a ShipmentLine with 50 cost-layer entries where 1 entry changes, the activity log row stores:\n- 1 changedEntry (the delta)\n- 50 entries in previousSnapshot\n- 50 entries in patchedSnapshot\n\nSame data stored ~3x. For a landed-cost adjustment touching 100 shipment lines, that's ~300x the snapshot data in activity_logs.\n\n**Why this matters:** The original bd issue `onetwo3d-ims-imn` (now closed by this PR) asked for an audit trail — but didn't specify the audit needed to store the full snapshot. The changedEntries field alone reconstructs the delta. Both full snapshots are operationally useful for one-off forensics but bloat the activity_logs table for routine landed-cost revaluations.\n\nactivity_logs is a hot table (other reports, dashboard queries, daily aggregations read it). Bloat here propagates.","acceptance_criteria":"- Decide retention/granularity trade-off:\n - Option A: store only changedEntries (the delta) — reconstruct full snapshots from delta + current shipment_line.costLayerSnapshot on demand\n - Option B: store previousSnapshot only (since patchedSnapshot equals shipment_line.costLayerSnapshot after the update)\n - Option C: keep current (3x) but document the trade-off and add a sweeper that compacts old revaluation rows after N days\n- Add a row-size assertion test that proves a 50-entry snapshot doesn't produce a 50KB activity_log row\n- If retention is bounded (e.g., 90 days), this becomes self-limiting and current shape is fine — but should be explicit","notes":"Identified during PR #144 review. Lower priority because storage is cheap and audit trails are valuable — but worth knowing before the activity_logs table hits a size that affects query performance.","status":"open","priority":3,"issue_type":"chore","owner":"info@onetwo3d.co.uk","created_at":"2026-06-07T23:45:04Z","created_by":"Jan","updated_at":"2026-06-07T23:37:37Z","labels":["audit","code-review","pr-144","storage"],"dependency_count":0,"dependent_count":0,"comment_count":0} {"_type":"issue","id":"onetwo3d-ims-8c7","title":"Clean up stale test path pointers in docs/plan.md (P1.3, P1.5, P4.2)","description":"Several `**Status:** Complete` plan items in docs/plan.md retained their original `**Tests:**` lines, which point at file paths that don't exist on origin/development.\n\nStale pointers found during PR #140 review audit:\n\n| Item | Plan claims | Actually exists |\n|---|---|---|\n| P1.3 | `tests/refund-service.idempotency.test.ts` | Path doesn't exist. Adjacent helper-level tests live in `tests/domain/sales/refund-service.test.ts` and `tests/woocommerce-refund-sync.test.ts`. |\n| P1.5 | `tests/refund-service.revaluation.test.ts` | Path doesn't exist. Revaluation tests are in `tests/domain/sales/refund-service.test.ts` (lines 703, 759, 815). |\n| P4.2 | 'Complete two production orders same day; assert FIFO order matches completedAt' (no file path) | No such integration test. Helper-level coverage only in `tests/domain/manufacturing/manufacturing-action-inputs.test.ts`. |\n\n**Why this matters:** Anyone auditing the plan to verify a completed item follows a broken link, gets nothing, and either re-implements the test (waste) or assumes coverage exists (silent regression risk).","acceptance_criteria":"- Update docs/plan.md P1.3 Tests line to point at actual test file + test name\n- Update docs/plan.md P1.5 Tests line to point at the three landed cost revaluation tests in refund-service.test.ts\n- Either fix P4.2 Tests line (after the P4.2 follow-up issue lands integration test) OR remove the prose acceptance criterion and replace with reference to the helper-level test\n- Audit remaining `**Status:** Complete` items for similar stale pointers","notes":"Two follow-up issues filed for P1.3 and P4.4 missing integration tests; this issue is just the docs cleanup. P1.5's tests do exist, just in a different file than the plan claims — fix the pointer.","status":"open","priority":3,"issue_type":"chore","owner":"dev@onetwo3d.com","created_at":"2026-06-07T18:09:01Z","created_by":"Jan","updated_at":"2026-06-07T18:09:01Z","labels":["docs","plan-followup"],"dependency_count":0,"dependent_count":0,"comment_count":0} {"_type":"issue","id":"onetwo3d-ims-4nf","title":"Compound tax / reverse charge / EC sales / OSS not supported","description":"Each line carries a single taxRateId/taxRatePercent. Use cases not currently expressible:\n- Compound taxes (e.g. VAT + provincial surcharge)\n- Reverse charge B2B (zero-rated outbound, customer self-accounts)\n- EC sales / OSS reporting buckets\n- Multi-tax stacking for jurisdictions with both federal and state tax\n\nLower priority than the per-line fallback bugs (xx4, hyz) but worth tracking. UK-only sellers won't notice; expansion to EU OSS or the US would expose this.\n\nFix direction:\n- Schema: a join table linking SalesOrderLine/PurchaseOrderLine to multiple TaxRate rows with order/compound flags.\n- Resolver: extend pickTaxRate to return a list of applicable rates with compound rules.\n- Connector mapping: confirm Xero TaxComponent / QB ItemBased tax codes can carry compound rates.","status":"open","priority":3,"issue_type":"feature","owner":"dev@onetwo3d.com","created_at":"2026-04-25T20:46:03Z","created_by":"OneTwo3D IMS","updated_at":"2026-04-25T20:46:03Z","defer_until":"2026-09-05T23:29:10Z","labels":["vat"],"dependency_count":0,"dependent_count":0,"comment_count":0} diff --git a/docs/plan.md b/docs/plan.md index 927364c4..13bde039 100644 --- a/docs/plan.md +++ b/docs/plan.md @@ -450,6 +450,32 @@ These are silent-corruption risks where the failure mode is "the numbers are wro --- +## Phase 8 — Product lifecycle controls + +### P8.1 — Product end-of-life flag and automatic archival +- **Files:** `prisma/schema.prisma`, `app/(dashboard)/products/**`, `app/actions/products.ts`, `lib/domain/inventory/replenishment-reports.ts`, product/report filter APIs, scheduled inventory/product maintenance job. +- **Problem:** Products that should no longer be bought or replenished can still appear in reorder forecasts. Operators also need a way to find end-of-life products and have them leave the active catalogue once remaining stock is exhausted. +- **Fix:** + 1. Add a boolean `endOfLife` field to `Product` with a default of `false`. + 2. Add an "End of life" checkbox on the product create/edit/detail flow next to SKU/EAN/MPN and other product metadata. + 3. Exclude `endOfLife = true` products from reorder forecasts/replenishment recommendations by default. If a report supports advanced filters, expose an explicit "include end-of-life products" option rather than silently mixing them in. + 4. Add product-list and relevant report filters for `endOfLife` so operators can find active, end-of-life, and archived/end-of-life products. + 5. Add an automatic archival path: when an end-of-life product has zero stock in every warehouse and no incoming stock remains, set its status to `ARCHIVED`. +- **Acceptance:** + - Product create/edit persists the checkbox and shows the current value on reload. + - End-of-life products do not appear in reorder forecasts unless the user explicitly opts into including them. + - Product list/report filters can show only end-of-life products. + - Automatic archival does not run while any warehouse has positive on-hand stock, reserved stock that needs fulfilment, open inbound purchase-order quantity, pending transfer receipt, or pending manufacturing output for the product. + - Once all warehouse stock is zero and no incoming stock exists, the product is archived and an activity-log entry records the reason and evidence counts. +- **Tests:** + - Product action/UI test: toggling `endOfLife` persists and renders correctly. + - Reorder forecast test: an end-of-life product with low/zero stock is excluded by default and included only when the filter requests it. + - Filter test: product list/report filters can return end-of-life products without leaking archived products unless requested. + - Auto-archive tests: archive when stock is zero and no incoming exists; do not archive when any warehouse stock, open PO quantity, pending transfer receipt, or pending manufacturing output exists. + - Activity-log test: auto-archive writes evidence metadata without sensitive payloads. + +--- + ## Quality gates — tests + invariants These are not a separate implementation phase except for the invariant CI gate. They are rules that apply to every PR in the plan. @@ -546,6 +572,7 @@ This reduces the plan from 45+ tiny PRs to roughly 16-20 coherent PRs. Split any 18. **Integration settings test gate:** P7.5. 19. **Sidebar cleanup:** P2.3, CR5. 20. **CI invariant gate:** QG1 plus QG2's regression-test convention. +21. **Product lifecycle controls:** P8.1. After each PR: - Run `npm run validate` and `npm run validate:db`.