Skip to content

fix(flows): unbreak main — contract_renewal throws; rewrite the two tests #565 turned red - #566

Merged
yinlianghui merged 7 commits into
mainfrom
claude/flow-loop-conditions-and-recurrence
Jul 31, 2026
Merged

fix(flows): unbreak main — contract_renewal throws; rewrite the two tests #565 turned red#566
yinlianghui merged 7 commits into
mainfrom
claude/flow-loop-conditions-and-recurrence

Conversation

@yinlianghui

@yinlianghui yinlianghui commented Jul 31, 2026

Copy link
Copy Markdown
Collaborator

Description

main is currently RED. Both failures are tests #563 introduced, and this PR fixes them. It also fixes a live regression #565 left behind.

#565 landed the same CEL-envelope conversion this branch originally carried, using the P tagged-template helper — a cleaner notation, so the flow files here take main's version wholesale. This PR is now the remainder: the defect #565 made reachable, the two tests its merge turned red, and the guards that keep the class fixed.

Type of Change

  • Bug fix (non-breaking change which fixes an issue)

Related Issues

Follow-up to #495 / #563. Fixes the fallout of #565.

1. contract_renewal throws on every sweep (live regression on main)

#565 correctly wrapped the notice-window gate as a CEL envelope so it finally evaluates. That exposed a second defect sitting underneath:

timestamp(currentContract.end_date) <= daysFromNow(int(currentContract.renewal_notice_days))

end_date is a DATE field and arrives as YYYY-MM-DD, but CEL's timestamp() accepts only a full ISO 8601 datetime — it throws timestamp() requires a string in ISO 8601 format.

While the condition was a bare string it was never evaluated, so this sat latent. Making the envelope real makes it throw mid-sweep. Net effect on main today: contract_renewal still books nothing, having traded a silent no-op for an exception.

Fixed by appending T00:00:00Z. Verified load-bearing — reverting just that one change fails four contract_renewal tests:

× books a renewal task and notifies the owner inside the notice window
× honours each contract's own renewal_notice_days, not a shared constant
× opens a pre-filled renewal opportunity only when auto_renewal is on
× is idempotent across repeated sweeps within the same window

2. The two tests #565 turned red

Both came from #563 and both fired exactly as designed — they existed to go red the moment someone fixed the behaviour they pinned.

test why it failed fix
flow-scheduled → KNOWN DEFECT asserted the stagnation gate stays shut; #565 opened it, so it failed with the message written for this moment: "gate now opens — rewrite this test" rewritten to assert real behaviour
flow-record-changeopportunity_approval asserted typeof startCondition === 'string'; #565 made it an envelope accepts either form, checks a non-empty expression exists

The second one is worth calling out as a flaw in my original test: it pinned the notation rather than the thing that matters. A legitimate refactor should not have broken it. It now asserts what it always meant — that a gate exists and carries an expression.

The rewritten scheduled tests assert real behaviour for both sweeps: nudge task and notification created, repeated sweeps stay idempotent, the sweep re-arms once the previous stall task completes, and for renewals the task/notification/auto_renewal opportunity, the per-contract notice window, and no duplicate renewal deal.

3. task_recurrence overflowed month-end instead of clamping

Independent of the above. advanceDate stepped months with Date.setMonth, which rolls a nonexistent day forward: a task due Jan 31 spawned its next occurrence on Mar 3, drifting deeper into the following month every time and skipping February outright.

Month and year steps now move to the 1st before the arithmetic, then clamp: Jan 31 → Feb 28 (Feb 29 in a leap year), Mar 31 → Apr 30, Feb 29 2028 → Feb 28 2029.

advanceDate also moves inside the handler — it was the last module-level helper in src/objects/, and every sibling hook documents why that is unsafe (L2 hook bodies run body-only in the QuickJS sandbox, where module scope is not available).

Residual behaviour, asserted rather than left to be discovered: each occurrence is computed from the previous due date, not an anchor day, so a month-end series settles on the shorter day (Jan 31 → Feb 28 → Mar 28). Preserving the anchor needs a schema change; what this replaces drifted forward without bound, so clamping is a strict improvement.

What keeps this fixed

Two guards in test/flow-scheduled.test.ts:

  1. The engine asymmetry is pinned — a bare string still evaluates false where an envelope evaluates true. If that flips upstream, it surfaces as a deliberate review rather than a silent behaviour change.
  2. Every registered flow is walked, including nested loops, failing if any loop body reintroduces a bare string condition. It asserts the walk found something, so it cannot pass vacuously.

campaign_enrollment gains a runtime suite (eligibility, opt-out, cross-campaign dedupe, closed-campaign refusal) and leaves PENDING_FLOWS — now down to case_csat_followup and demo_bootstrap.

⚠️ Behaviour change

With both defects fixed, these sweeps start writing records they never wrote before:

  • The first run after deploy will nudge every currently-stalled deal and open renewal tasks for every contract already inside its notice window — a one-time burst proportional to the backlog.
  • The idempotency gates that stop a daily sweep from piling up duplicates have never actually run in production, because they were the broken part. Each is now covered by a repeated-sweep test asserting exactly one task / notification / opportunity across two consecutive sweeps.

Testing

Run on the merged tree (main incl. #565):

  • pnpm verify exits 0 — 382 passed / 21 files (2 skipped, pre-existing)
  • pnpm test:e2e11 passed against a cold database
  • pnpm typecheck, pnpm validate, pnpm lint all clean
  • Confirmed the timestamp() fix is load-bearing by reverting it (4 failures)
  • Confirmed both main-red tests fail on origin/main and pass here

Checklist

  • I have added a changeset (two — one per defect)
  • 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
  • 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

Note on CI

pull_request-triggered workflows have not started for any PR in this repo since ~04:27 UTC (push-triggered runs on main still work). The head commit here may therefore show no checks through no fault of the branch — worth a look independently of this PR.

claude added 2 commits July 31, 2026 04:17
…ert)

`applyConversionsToFlow` wraps a bare string `condition` into a
`{ dialect: 'cel', source }` envelope for a flow's TOP-LEVEL edges only — it
does not recurse into ADR-0031 control-flow regions (`loop.config.body`). A
bare string left in there falls through to the engine's legacy template path,
which substitutes `{var}` templates (there are none) and then string-compares
the leftover expression text:

    'existingStallTask == null'  ->  'existingStallTask' === 'null'  ->  false

The gate never opens, and the failure is silent in the worst way: the sweep
runs, queries correctly, selects exactly the right records, then does nothing.

  - opportunity_stagnation found every stalled deal and nudged nobody
  - contract_renewal never booked a renewal task, notification or opportunity
  - campaign_enrollment enrolled no leads at all

All loop-nested conditions in those three flows are now explicit envelopes.

contract_renewal's notice-window gate carried a SECOND defect that only became
reachable once the first was fixed: `timestamp(currentContract.end_date)`
throws "timestamp() requires a string in ISO 8601 format" because `end_date` is
a DATE field arriving as YYYY-MM-DD. It now appends T00:00:00Z. Verified
discriminating: a contract ending in 20 days is in window at notice 30 and 90,
out of window at 10.

Tests: the KNOWN DEFECT block that pinned the broken behaviour is replaced by
two guards — one pinning the engine asymmetry that makes the envelope necessary
(so an upstream fix surfaces as a deliberate review, not a silent change), and
one walking every registered flow to fail if any loop body reintroduces a bare
string. The stagnation and renewal suites now assert real behaviour including
their idempotency gates across repeated sweeps, and campaign_enrollment gains a
runtime suite and leaves PENDING_FLOWS.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_0127MccuEdXF7pxJFmaiD7UU
… month

`advanceDate` stepped months with `Date.setMonth`, which rolls a day that does
not exist in the target month FORWARD rather than clamping. A recurring task
due Jan 31 spawned its next occurrence on Mar 3 — drifting deeper into the
following month on every occurrence and skipping February outright. Feb 29 in a
leap year had the same shape.

Month and year steps now move to the 1st before the arithmetic, then clamp the
day to the target month's length: Jan 31 -> Feb 28 (Feb 29 in a leap year),
Mar 31 -> Apr 30, Feb 29 2028 -> Feb 28 2029.

The residual behaviour is asserted rather than left to be discovered: each
occurrence is computed from the previous due date, not from an anchor day, so a
month-end series settles on the shorter day (Jan 31 -> Feb 28 -> Mar 28) rather
than returning to the 31st. Preserving the anchor needs a schema change; what
this replaces drifted forward without bound, so clamping is a strict
improvement.

`advanceDate` also moves inside the handler. It was the last module-level
helper in src/objects/, and every sibling hook documents why that is unsafe: L2
hook bodies run body-only in the QuickJS sandbox, where module scope is not
available.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_0127MccuEdXF7pxJFmaiD7UU
@vercel

vercel Bot commented Jul 31, 2026

Copy link
Copy Markdown

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

1 Skipped Deployment
Project Deployment Actions Updated (UTC)
hotcrm Ignored Ignored Jul 31, 2026 6:51am

Request Review

@github-actions github-actions Bot added documentation Improvements or additions to documentation ci/cd dependencies configuration labels Jul 31, 2026
@yinlianghui
yinlianghui marked this pull request as ready for review July 31, 2026 06:35
claude added 3 commits July 31, 2026 06:40
The push that merged main after #563 landed (efb0707) delivered a
`synchronize` event — `Label Pull Request` and the Vercel check both ran on it
— but none of the `pull_request`-triggered workflows started, so the head
commit carried no CI result and the PR could not be evaluated. Empty commit to
re-run them; no content change.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_0127MccuEdXF7pxJFmaiD7UU
#565 landed the same CEL-envelope fix this branch carried, using the `P`
tagged-template helper from @objectstack/spec — a cleaner notation, so the flow
files take main's version wholesale. What remains here is the part #565 did not
cover, plus the two red tests its merge left on main.

main is currently RED. Both failures are tests this branch's parent (#563)
introduced, and both fired exactly as designed:

  - flow-scheduled: the KNOWN DEFECT case asserted the stagnation gate stays
    shut. #565 opened it, so the test failed with the message it was written to
    emit — "gate now opens — rewrite this test". Rewritten here to assert the
    real behaviour (nudge created, idempotent across sweeps, re-arms once the
    task completes).
  - flow-record-change: `opportunity_approval` asserted `typeof
    startCondition === 'string'`. #565 converted it to an envelope. The
    assertion was pinning the NOTATION rather than the thing that matters, so
    it now accepts either form and checks a non-empty expression exists.

Second defect #565 left reachable: `timestamp(currentContract.end_date)` throws
"timestamp() requires a string in ISO 8601 format" because `end_date` is a DATE
field arriving as YYYY-MM-DD. While the condition was a bare string it was
never evaluated, so this was latent; wrapping it as a real envelope makes it
throw mid-sweep. Appending T00:00:00Z fixes it — verified load-bearing by
reverting it, which fails 4 contract_renewal tests.

So on main as it stands, contract_renewal books nothing: the gate evaluates and
then blows up. That is a live regression, not a theoretical one.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_0127MccuEdXF7pxJFmaiD7UU
#565 landed the CEL-envelope conversion, so this branch no longer carries it.
What remains is the timestamp() ISO defect it left reachable, the two tests its
merge turned red on main, and the guards that keep the class fixed.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_0127MccuEdXF7pxJFmaiD7UU
@yinlianghui yinlianghui changed the title fix(flows): make loop-nested conditions evaluate; clamp monthly task recurrence fix(flows): unbreak main — contract_renewal throws; rewrite the two tests #565 turned red Jul 31, 2026
@yinlianghui
yinlianghui merged commit 5b88ad0 into main Jul 31, 2026
10 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants