fix(validations): guard null-aborting predicates, unify end_after_start, drop duplicate revenue rule

[yinlianghui]

Description

Partial fix for #514, covering the three validation-declaration items only (3 — product half, 7, and 12). Every rule touched here is metadata that os validate accepts and that no test ever evaluated, so each failed silently: the rule did not error, it simply never fired.

Each item was re-verified against current main (5b88ad0) before being changed — #514 was written earlier and several of its entries have since been fixed by other PRs. All three below were still present.

Type of Change

• Bug fix (non-breaking change which fixes an issue)
• New feature (non-breaking change which adds functionality)
• Breaking change (fix or feature that would cause existing functionality to not work as expected)
• Documentation update
• Code refactoring
• Performance improvement
• CI/CD update

Related Issues

Refs #514 (items 3 — product half, 7, 12). This PR deliberately does not close the issue; the remaining items are out of scope.

Changes Made

• Item 3, product half — unguarded CEL predicates (product.object.ts). Strict CEL aborts on dyn<null> < int, so an unguarded comparison skips the rule rather than failing it. price_positive (record.list_price < 0) and cost_less_than_price (record.cost >= record.list_price) both lacked the != null guard that quote_line_item.unit_price_positive already models. cost is absent on every seeded product, so that warning had never evaluated on a single row. Both operands now guarded.
• Item 12 — end_after_start operator drift. The same rule had three spellings. crm_campaign used <, accepting a campaign that ends the day it starts while its own message promised "End Date must be after Start Date". crm_forecast.period_end_after_start also used <, with an "on or after" message contradicting its own rule name. crm_contract was already correct and is unchanged — it is the reference. All three now use <= in the violation predicate and say "must be after".
• Item 7 — duplicated revenue_positive. crm_account declared a validation saying "Annual Revenue must be positive" while account.hook.ts threw "must be greater than or equal to 0" for the same condition — disagreeing about whether zero is allowed, though both compare < 0 (it is). The duplicate declaration is removed and the hook remains the single enforcement point; it is also the tested one (test/hooks-runtime-sales.test.ts executes the handler, whereas the declaration was never evaluated by any test). The removal is documented in place, alongside the existing note about account_name_unique. P became unused in that file and was dropped from the import.

Seed-data impact

None. Adding a guard makes a previously-inert rule start firing, so this was checked per rule: no seeded product has a negative list_price or any cost at all, and no seeded campaign or forecast has end == start. No seeded row changes validity.

Scope note

Item 7's other half — the duplicate branch in account.hook.ts — is intentionally left in place. Removing it would mean editing a hook and an existing test file, outside this PR's declaration-only scope, and the hook is the enforcement point being kept. Also observed but not changed, as it belongs to no assigned item: product.price_positive's message still says "must be positive" while comparing < 0, the same wording/semantics mismatch item 7 flags on account.

Testing

• Unit tests pass (pnpm test) — 398 passed, 2 skipped, 23 files
• Linting passes (pnpm lint)
• Build succeeds (pnpm build)
• Manual testing completed
• New tests added

Full pnpm verify (validate && typecheck && lint && hygiene && build && test) exits 0. The two expression warnings the build prints are pre-existing on main (campaign_enrollment flow) and unrelated.

Tests were written first and confirmed failing on unmodified main — 6 failures, one per real defect, with crm_contract.end_after_start passing from the start as the reference implementation.

New file test/object-validation-predicates.test.ts (deliberately not appended to test/metadata-references.test.ts, which is a high-conflict file):

• a repo-wide sweep asserting every operand of every ordering comparison (<, <=, >, >=) in every object validation carries its own != null guard;
• the three date-range twins pinned to one operator and one wording;
• a single-enforcement-point check for annual_revenue, asserting the declaration is gone and the hook check is still there.

The sweep carries one documented exception — opportunity_line_item.unit_price_positive, the remaining half of item 3, tracked separately — plus a companion test that fails if that entry ever goes stale, so the exception list can only shrink.

Checklist

• I have added a changeset — .changeset/validation-predicate-guards-and-operator-drift.md
• My code follows the style guidelines of this project
• I have performed a self-review of my own code
• I have commented my code, particularly in hard-to-understand areas
• I have made corresponding changes to the documentation
• My changes generate no new warnings
• I have added tests that prove my fix is effective
• New and existing unit tests pass locally with my changes
• Any dependent changes have been merged and published

Additional Notes

The user-visible behaviour changes are narrow but real: a campaign or forecast whose end equals its start is now rejected where it was previously accepted, the two product rules begin firing on rows they used to skip, and a negative annual_revenue now reports the hook's message rather than the validation's.

---

Generated by Claude Code

[vercel]

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

1 Skipped Deployment

Project	Deployment	Actions	Updated (UTC)
hotcrm	Ignored		Jul 31, 2026 8:44am