Add invariant preflight CI gate#155
Conversation
Four findings from review of the invariant preflight CI gate PR:
- onetwo3d-ims-ejx [P2] CI runs preflight against a fresh empty DB —
code-correctness gate only, not data-correctness;
claims broader coverage than delivered
- onetwo3d-ims-ljw [P2] P8.1 product-lifecycle plan adds Group 21 but
doesn't spec migration from existing
ProductLifecycleStatus { ACTIVE, NOT_FOR_SALE,
ARCHIVED } enum or enumerate affected code paths
- onetwo3d-ims-dn2 [P3] P8.1 'incoming stock' check for auto-archive
spans 5+ tables; needs concrete query spec +
cron cadence + transaction-safe transition
- onetwo3d-ims-l7d [P3] No integration test for the script's actual
runScheduledInvariantCheck codepath; mocked
tests only
No P1 — the CI gate works as a code-correctness check, and the plan
addition (Group 21) is documentation only. The findings are about
scope clarity and spec completeness.
Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
OneTwo3D
left a comment
There was a problem hiding this comment.
PR #155 — Invariant preflight CI gate (Group 20 / QG1) + Group 21 plan addition
Two things in one PR:
- QG1 implementation: new
npm run invariant-check:preflightscript +lib/cron/invariant-check-preflight.tshelper +invariant-preflightCI job in the production-readiness workflow that boots a Postgres service, applies migrations, and runs the preflight. - Group 21 plan-only addition: P8.1 product-lifecycle statuses (
active,draft,eol,archived) and EOL sell-off semantics indocs/plan.md. No code for this.
The CI gate's structure is clean: pure helper (runInvariantCheckPreflight) takes a runCheck dependency injection, returns a structured { ok, failure, result }, defaults wire up the real runScheduledInvariantCheck with cron side effects disabled. Tests cover the helper's branching logic (happy path, critical findings, divergent summary/list, partial failure).
But the gate's coverage is narrower than the PR claims, and the plan addition introduces scope without fully specifying it.
4 findings filed in bd on this PR's branch (codex/ci-invariant-gate, commit 0219af2d).
Findings filed in bd
| Priority | bd ID | Title |
|---|---|---|
| P2 | onetwo3d-ims-ejx |
CI preflight runs against fresh empty DB — code-correctness only, not data-level |
| P2 | onetwo3d-ims-ljw |
P8.1 missing migration path from existing ProductLifecycleStatus enum |
| P3 | onetwo3d-ims-dn2 |
P8.1 "incoming stock" auto-archive query spans 5+ tables; needs concrete spec |
| P3 | onetwo3d-ims-l7d |
No integration test for the real runScheduledInvariantCheck codepath |
Why P2 on ejx and ljw
-
ejx— the original QG1 acceptance was "Synthetic data with a known invariant violation; assert CI fails." The PR's tests cover this on a mocked result, but the actual CI workflow runs against an empty migrated DB, where every invariant trivially passes. The gate catches code-level regressions (e.g., a refactor that makes a reporter throw on empty input) but does NOT catch data-level invariant violations — yet the PR description and the plan closure imply the latter. Worth making the scope explicit OR seeding a known-violating fixture in the CI step to genuinely deliver the acceptance criterion. -
ljw— the existingProductLifecycleStatus { ACTIVE, NOT_FOR_SALE, ARCHIVED }enum atprisma/schema.prisma:36-40needs an explicit mapping to the new{ active, draft, eol, archived }. The plan acceptance criteria mention 'each allowed and blocked transition' but the transition diagram is unstated. ExistingNOT_FOR_SALEcould plausibly map todraft(never published) OReol(post-publication withdrawal) — operators need a default policy.
Notes
- The CI workflow gating
if: needs.classify_changes.outputs.run_expensive == 'true'is the right shape — docs-only PRs skip the Postgres-backed job. disconnectDbwith the 5-second race +timeout.unref()is good defensive pattern for CI not hanging. Standard.- The "divergent critical summary/list" defensive test (
tests/cron/invariant-check-preflight.test.ts) is a nice touch — catches a class of bug where the count and the list could drift inrunScheduledInvariantCheck. - Adding Group 21 to the plan is the right home for the EOL discussion. The plan-only nature means no code review is needed for the implementation; the findings are entirely about pinning the spec before any implementation PR.
- All 4 bd issues tagged
pr-155for filtering:bd list --labels pr-155
Three findings on the new P8.2 plan addition (commit 65b726d — product supplier field and supplier-scoped reorder drafts): - onetwo3d-ims-tep [P2] P8.2's singular product.supplier field overlaps with existing SupplierProduct many-to-many relation; spec doesn't reconcile (supplement vs replace, migration, multi-supplier semantics) - onetwo3d-ims-2uu [P2] 'Latest placed PO wins' supplier rule doesn't handle emergency/backup POs, transition timing (PO_SENT vs received), rollback on cancellation, or multi-PO concurrent placement ordering - onetwo3d-ims-f91 [P3] 'Preserve forecast evidence on each line' has no schema home today; PurchaseOrderLine needs a new field, parallel table, or activity-log-only decision All P2/P3 — P8.2 is plan-only, no code yet. Findings are about pinning the spec before implementation starts. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
OneTwo3D
left a comment
There was a problem hiding this comment.
PR #155 — Second-pass review
Commit 65b726da added P8.2 to Group 21: product supplier field + supplier-scoped reorder drafts from forecasts. This commit doesn't touch code — purely a docs/plan.md addition that builds on P8.1 (the existing Group 21 entry).
The first-pass review (against ce4914e6) covered the QG1 invariant CI gate and P8.1 plan addition. Those findings (ejx, ljw, dn2, l7d) still apply.
The P8.2 addition introduces a new product-supplier relationship that doesn't reconcile with existing schema and has subtle workflow semantics that would benefit from explicit spec before implementation.
3 new findings filed in bd on this PR's branch (codex/ci-invariant-gate, commit 9b9bc28f).
Second-pass findings filed in bd
| Priority | bd ID | Title |
|---|---|---|
| P2 | onetwo3d-ims-tep |
product.supplier field overlaps with existing SupplierProduct many-to-many |
| P2 | onetwo3d-ims-2uu |
"Latest placed PO wins" doesn't handle emergency POs, transition timing, or rollback |
| P3 | onetwo3d-ims-f91 |
"Preserve forecast evidence on each line" has no schema home today |
Why the two P2s matter
tep — SupplierProduct (prisma/schema.prisma:943-958) already exists as a many-to-many between Product and Supplier with per-supplier metadata (supplierSku, lastUnitCost, leadTimeDays). P8.2 proposes a singular product.supplier field with auto-update from POs. The spec doesn't say:
- Whether the new field supplements
SupplierProduct(preferred-supplier pointer) or replaces it (loses multi-supplier metadata) - Backfill policy for existing products
- Multi-supplier products: do they oscillate per-PO, or does the auto-update opt out?
2uu — "Latest placed PO wins" is simple to implement but loses signal in real workflows:
- Emergency / backup POs silently overwrite the primary supplier
- "Placed" is ambiguous:
PO_SENT,PARTIALLY_RECEIVED, orRECEIVED? - No rollback when a PO is cancelled / returned (P now points at a supplier that never delivered)
- Concurrent multi-PO placement ordering not defined
Both findings are about pinning the spec before implementation. The cost of guessing the wrong interpretation is a redo of the relevant action paths + schema migration.
P3 — forecast evidence schema
f91 — the acceptance "lines carry enough forecast metadata to explain the suggested quantity" implies a schema addition that isn't enumerated. Three reasonable shapes (JSON field on PO line, parallel evidence table, activity-log only) have different ergonomics and queryability. Worth picking explicitly.
Notes on the QG1 part
No new code changes since ce4914e6 — first-pass findings on the CI gate (ejx, l7d) and P8.1 (ljw, dn2) still apply. Re-iterating the most actionable:
ejx: CI runs against empty DB → seed a known-violating fixture to genuinely deliver QG1's acceptanceljw: P8.1 needs explicit mapping from existingProductLifecycleStatus { ACTIVE, NOT_FOR_SALE, ARCHIVED }
Notes
- Group 21 now contains two planned items (P8.1, P8.2) with overlapping concerns: both touch the product schema, both interact with reorder forecast logic. Implementation should sequence P8.1 first (lifecycle states), then P8.2 (supplier field) —
2uu's rules need P8.1'seol/archivedstates to enforce the "EOL/archived excluded from draft generation" rule. - All 3 new bd issues tagged
pr-155for filtering:bd list --labels pr-155(now 7 total).
Four findings from review of the product lifecycle + supplier reorder implementation PR (P8.1 + P8.2 from PR #155's plan): - onetwo3d-ims-27w [P2] getReorderReport uses SupplierProduct[0] as primary supplier with preferredSupplier as fallback only — inverts the auto-update intent; 'latest placed PO wins' is invisible at the read path - onetwo3d-ims-ywu [P2] OPEN_WMS_ASN_STATUSES counts CREATE_PENDING / CREATE_IN_FLIGHT as firm incoming stock — stuck ASNs indefinitely defer EOL → ARCHIVED transition - onetwo3d-ims-3i3 [P2] updatePreferredSuppliersForPlacedPurchaseOrder writes no activity log — silent supplier reassignment, operator has no audit trail when 'why did the preferred supplier change?' is asked - onetwo3d-ims-zqe [P3] Third instance of \$executeRaw used for SELECT FOR UPDATE — same anti-pattern as PR #127. Worth a codebase sweep. Closed PR #155 P8.1 / P8.2 spec issues addressed in this PR: - onetwo3d-ims-ljw (migration path) → migration SQL maps NOT_FOR_SALE → EOL - onetwo3d-ims-dn2 (incoming stock spec) → structured getProductIncomingStock - onetwo3d-ims-tep (SupplierProduct overlap) → preferredSupplierId field, SupplierProduct kept - onetwo3d-ims-2uu (timing/rollback) → PO_SENT trigger, skipPreferred, preferredSupplierLocked, no rollback - onetwo3d-ims-f91 (forecast evidence) → reorderEvidence JSONB on PO line P2 weighting on the new findings because each leaves a load-bearing feature with a silent failure mode. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
Three new docs (greenfield): - help-docs/glossary.md — plain-English definitions of FIFO, COGS, RFQ, EOL, landed cost, preferred supplier, realised/unrealised FX, sub-ledger A1/A2/B, connection test gate, etc. The canonical source for term definitions; other articles link to it rather than re-defining. - help-docs/onboarding-walkthrough.md — step-by-step walkthrough of the setup wizard (Company → Currency → Integrations → Products). Covers what each step does, beginner tips, and a "recommended next steps" checklist for backup/cron/security/users that the wizard doesn't itself cover. - help-docs/troubleshooting.md — first-stop reference for common errors: login, inventory, sales/refund, purchasing/FX validation, integrations (connection test gate, tax fallback, FX pending queue), backups, cron rate limits, etc. Refreshes to existing docs (concentrating on recent-PR gaps): - help-docs/inventory.md — lifecycle status (DRAFT/ACTIVE/EOL/ARCHIVED) + preferred supplier auto-update + reorder evidence + EOL auto-archive (PR #156) - help-docs/sales.md — multi-currency FX, realised FX on settlement, tax inclusive/exclusive validation, refund warehouse-scoped idempotency, refunds without shipped source (PRs #135, #136, #142) - help-docs/purchasing.md — FX rate validation (10%/2% bands), PO cancellation cost-layer reversal, preferred supplier auto-update, supplier-scoped reorder drafts, landed-cost freightPoId attribution (PRs #127, #139, #143, #156) - help-docs/settings.md — backup hardening (manifest sidecar, 50MB cap, S3/SFTP remote, restore token), password policy, activity-log redaction, cron rate limits, invariant check (PRs #146, #148, #149, #152, #154, #155) - help-docs/woocommerce.md — connection test gate, tax fallback policy (block non-zero / warn zero-rated), FX pending retry queue, customer PDF download via WP helper plugin handoff (PRs #144, #152, #154) - help-docs/xero-sync.md — connection test gate, realised + unrealised FX, daily batch caps with deterministic split-batch refs (PRs #142, #149, #152) - help-docs/getting-started.md — link to new beginner docs; lifecycle intro; "where to find help" table - help-docs/README.md — articles index with sections (getting started, day-to-day, administration, integrations) Entry points and deployment: - README.md — link to new beginner docs; production env-var note (CRON_SECRET, RATE_LIMIT_BACKEND, backup cron) - docs/installation.md — added env vars RATE_LIMIT_BACKEND, DATABASE_RESTORE_MAX_FILE_BYTES, XERO_DAILY_BATCH_LIMIT, WC_PENDING_FX_ORDER_NOTIFY_THRESHOLD; new "Production Readiness Checklist" section covering env, crons, backup, integrations, security, monitoring The onboarding wizard itself is unchanged (audited but flagged for separate work in bd-mzs/a08/8o5/tan); these docs explain what the wizard does set up. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
* Implement product lifecycle supplier planning * chore(bd): file PR #156 review findings as follow-up issues Four findings from review of the product lifecycle + supplier reorder implementation PR (P8.1 + P8.2 from PR #155's plan): - onetwo3d-ims-27w [P2] getReorderReport uses SupplierProduct[0] as primary supplier with preferredSupplier as fallback only — inverts the auto-update intent; 'latest placed PO wins' is invisible at the read path - onetwo3d-ims-ywu [P2] OPEN_WMS_ASN_STATUSES counts CREATE_PENDING / CREATE_IN_FLIGHT as firm incoming stock — stuck ASNs indefinitely defer EOL → ARCHIVED transition - onetwo3d-ims-3i3 [P2] updatePreferredSuppliersForPlacedPurchaseOrder writes no activity log — silent supplier reassignment, operator has no audit trail when 'why did the preferred supplier change?' is asked - onetwo3d-ims-zqe [P3] Third instance of \$executeRaw used for SELECT FOR UPDATE — same anti-pattern as PR #127. Worth a codebase sweep. Closed PR #155 P8.1 / P8.2 spec issues addressed in this PR: - onetwo3d-ims-ljw (migration path) → migration SQL maps NOT_FOR_SALE → EOL - onetwo3d-ims-dn2 (incoming stock spec) → structured getProductIncomingStock - onetwo3d-ims-tep (SupplierProduct overlap) → preferredSupplierId field, SupplierProduct kept - onetwo3d-ims-2uu (timing/rollback) → PO_SENT trigger, skipPreferred, preferredSupplierLocked, no rollback - onetwo3d-ims-f91 (forecast evidence) → reorderEvidence JSONB on PO line P2 weighting on the new findings because each leaves a load-bearing feature with a silent failure mode. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com> * Fix product lifecycle review beads * Fix onboarding audit beads * Fix product lifecycle archive cron --------- Co-authored-by: OneTwo3D IMS <dev@onetwo3d.com> Co-authored-by: Claude Opus 4.7 <noreply@anthropic.com>
Three new docs (greenfield): - help-docs/glossary.md — plain-English definitions of FIFO, COGS, RFQ, EOL, landed cost, preferred supplier, realised/unrealised FX, sub-ledger A1/A2/B, connection test gate, etc. The canonical source for term definitions; other articles link to it rather than re-defining. - help-docs/onboarding-walkthrough.md — step-by-step walkthrough of the setup wizard (Company → Currency → Integrations → Products). Covers what each step does, beginner tips, and a "recommended next steps" checklist for backup/cron/security/users that the wizard doesn't itself cover. - help-docs/troubleshooting.md — first-stop reference for common errors: login, inventory, sales/refund, purchasing/FX validation, integrations (connection test gate, tax fallback, FX pending queue), backups, cron rate limits, etc. Refreshes to existing docs (concentrating on recent-PR gaps): - help-docs/inventory.md — lifecycle status (DRAFT/ACTIVE/EOL/ARCHIVED) + preferred supplier auto-update + reorder evidence + EOL auto-archive (PR #156) - help-docs/sales.md — multi-currency FX, realised FX on settlement, tax inclusive/exclusive validation, refund warehouse-scoped idempotency, refunds without shipped source (PRs #135, #136, #142) - help-docs/purchasing.md — FX rate validation (10%/2% bands), PO cancellation cost-layer reversal, preferred supplier auto-update, supplier-scoped reorder drafts, landed-cost freightPoId attribution (PRs #127, #139, #143, #156) - help-docs/settings.md — backup hardening (manifest sidecar, 50MB cap, S3/SFTP remote, restore token), password policy, activity-log redaction, cron rate limits, invariant check (PRs #146, #148, #149, #152, #154, #155) - help-docs/woocommerce.md — connection test gate, tax fallback policy (block non-zero / warn zero-rated), FX pending retry queue, customer PDF download via WP helper plugin handoff (PRs #144, #152, #154) - help-docs/xero-sync.md — connection test gate, realised + unrealised FX, daily batch caps with deterministic split-batch refs (PRs #142, #149, #152) - help-docs/getting-started.md — link to new beginner docs; lifecycle intro; "where to find help" table - help-docs/README.md — articles index with sections (getting started, day-to-day, administration, integrations) Entry points and deployment: - README.md — link to new beginner docs; production env-var note (CRON_SECRET, RATE_LIMIT_BACKEND, backup cron) - docs/installation.md — added env vars RATE_LIMIT_BACKEND, DATABASE_RESTORE_MAX_FILE_BYTES, XERO_DAILY_BATCH_LIMIT, WC_PENDING_FX_ORDER_NOTIFY_THRESHOLD; new "Production Readiness Checklist" section covering env, crons, backup, integrations, security, monitoring The onboarding wizard itself is unchanged (audited but flagged for separate work in bd-mzs/a08/8o5/tan); these docs explain what the wizard does set up. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
Summary
Validation
Risk areas