feat(WS-26d-autolead): auto-lead from inbound email, OFF behind CRM_AUTO_LEAD (D5) - #403
Open
vjvarada wants to merge 4 commits into
Open
feat(WS-26d-autolead): auto-lead from inbound email, OFF behind CRM_AUTO_LEAD (D5)#403vjvarada wants to merge 4 commits into
vjvarada wants to merge 4 commits into
Conversation
…ind a flag that is still off
The last demo-path slice. With CRM_AUTO_LEAD off — which is how it ships and how
it stays until the owner flips it (work_plan.md §6 (b)) — this branch changes no
runtime behaviour at all. That is not a caveat, it is done-when 2: the flag is
read at the CALL SITE in process_new_mail, so the OFF state enters no CRM code
and issues no CRM query. A gate that lived inside the step would have satisfied a
careless test while opening a database session on every sync cycle of every
mailbox to discover it had nothing to do. An AST assertion pins the gate's
position, because the runtime sentinel cannot see a refactor.
THE SEAM IS ALSO REACHED BY DEEP RESYNCS, AND THAT IS THE WHOLE DESIGN.
process_new_mail is the one place scheduler, manual sync and webhook all funnel
through — which is why the ticket chose it — but resync_account runs a ~1-year
all-folder backfill and then fires the same hook, a newly connected mailbox's
first sync is deep by the same heuristic, and neither stamps rules_held_back_at.
A candidate query of "everything classified" would therefore mint a lead per
unknown sender across a year of mail the moment a second mailbox connects, each
born zoho_dirty and queued for the LIVE Zoho tenant within one 600s cycle
(D-CRM-9), with no confirmation card anywhere on a scheduler hook and no delete
tool to take them back.
So the step keeps a per-account two-timestamp cursor (migration 157,
crm_auto_lead_cursors). activated_at is stamped ONCE on the first ON-state run
and never advanced: `received_at > activated_at` is the backfill discriminator,
and it says mail that ARRIVED before auto-lead existed for this account mints
nothing no matter when a resync gets around to classifying it. processed_watermark
is the ordinary incremental cursor. Both apply together. The test that matters
seeds a year-old classified backlog and asserts zero leads; deleting the first
predicate turns it red.
DEDUP IS A SELECT GUARD PLUS IN-BATCH DE-DUPLICATION, NEVER ON CONFLICT.
crm_leads has no unique constraint on email — idx_crm_leads_email is a plain
index, only zoho_id is UNIQUE — so the shape the ticket originally prescribed
could not have fired at all. The cross-invocation race (two concurrent syncs of
one account reading the same watermark) is ACCEPTED and recorded: the cost is one
visible, hand-deletable duplicate, and the fix that suggests itself is a UNIQUE
index on a column where 1,516 imported rows may already carry duplicates —
a deploy-blocking constraint of exactly the shape migration 148 had to defuse.
The in-batch half is asserted as one unknown-sender probe per ADDRESS rather than
"one lead": the fake shares a dict, so "one lead" would stay green with the dedup
deleted while production, where those two sessions are genuinely concurrent, minted
two.
"NEVER A COLLEAGUE" IS TWO GATES BECAUSE IT IS TWO QUESTIONS. sender_scope fails
SAFE to "external", which is the wrong direction when the consequence is a lead
row for your own CFO in a live tenant, so it is necessary and not sufficient. The
second gate is the internal-domain list, normalised through resolve_org_domains —
and that is where it earns its place rather than restating the first: sender_scope's
own extra-domain arm only lstrip('@')s its input, the exact divergence runner.py
documents, so a colleague on a second company domain that somebody typed as an
address is external to gate 1 and internal to gate 2. Deleting gate 2 mints him a
lead, and a named test says so.
The first activity is metadata and never content: type='system' — deliberately
outside sync_zoho's `type IN ('note','task')` push predicate, so the subject and
sender stay inside the native CRM even though the lead itself is queued for the
tenant — with an EMPTY body and no snippet. The candidate SELECT does not read
body_text or snippet at all: the projection is the privacy boundary, and nothing
downstream can leak a body it was never handed. The exclusion is asserted against
push_activities' own statement text, not against the constant beside it.
The lead goes through records.create_record, never raw SQL: _resolve_status, the
owner_email default, validate_source and mark_dirty_on_insert all live only
there, and only the last of the four is visible in the row afterwards. lead_name
is left to compute_lead_name over a display name stripped BEFORE it is split.
Placement: routes/crm/auto_lead.py, not the email package. What it does is write
a CRM record — it owns the cursor table, it uses the CRM's own write path, its
flag is a CRM owner gate. It registers no routes and, like broker_handlers.py, is
not imported from routes/crm/__init__.py. It IMPORTS the automation package's
public identity primitives rather than copying them: D-CRM-4 declined to import
another package's private helper, and a third copy of "is this person a
colleague?" is the drift that rule exists to prevent.
_crm_fakes.py gained one reader — jsonb `@>` containment. Without it the "have we
ever emailed them" probe was invisible to the fake, which answered "yes" for
every message in Sent regardless of recipient, and the already-known-contact case
would have been a test of nothing. Same lesson as the four readers WS-26d-email
had to add.
Tests: tests/unit/test_crm_auto_lead.py, 52 cases, every done-when named. Seven
mutants run red and reverted: the flag check, the activated_at predicate, the
watermark advance, the internal-domain gate, type='system', the service write
path, and the in-batch dedup.
BUILT, NOT FLIPPED, NOT DEPLOYED. R4 sweep in this same change: crm_app.md status
header + the WS-26d-autolead ticket + §9.0's D5 row, work_plan.md's WS-26 row and
§6 gate (b) — whose "the settings field does not exist yet" note is now retired —
plus the gateway and infra AGENTS.md.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…on number somebody else had claimed Diff-review repair round. Both P1s needed a cursor that had actually been running to see, which is why the first cut passed its own seven mutants. P1-1 — activated_at guarded the wrong thing. It stops a deep resync, and it does nothing at all about a flag that was on, turned off for four weeks, and turned back on: the cursor still carried day-1's anchor, so the first ON cycle minted the entire OFF window in one batch. The reviewer measured 27 of 27, each born zoho_dirty and each pushing unattended into the live tenant. The anchor now means the CURRENT ON epoch: a gap in the step's own runs beyond REANCHOR_GAP_SECONDS (3600, six scheduler periods) re-stamps the cursor, mints nothing from the gap, and says so at WARNING with gap_seconds. Fail-closed both ways — an OFF window and a real outage each skip their backlog, because a missed lead is hand-creatable and visible in the mailbox and 27 unattended tenant pushes are neither.⚠️ DEVIATION, with its reason: dormancy is measured on a THIRD column, last_run_at, not on processed_watermark as ruled. The watermark tracks MAIL, not runs, and the literal predicate has two production failures. A mailbox that is merely quiet over a weekend carries a 60-hour-old watermark while the step has run faithfully every 600s — it would be re-anchored, and Monday's first message, the one this whole feature exists to catch, falls before the new anchor and mints nothing. Every Monday. And a poison head message holds the watermark still ON PURPOSE under P1-2 below; a watermark-based test would re-anchor past it after an hour and quietly undo the stall that was supposed to stay visible. last_run_at is stamped on every cycle including empty and stalled ones, so "quiet" and "not running" stay different facts. The ruling's constant, log key and behaviour on a genuine OFF window are unchanged. Both named tests are in the file, including the control that a quiet-but-running mailbox is never re-anchored. P1-2 — a failure advanced the cursor over the work it lost. This step opens a SECOND session per lead through create_record while holding the batch's own, so pool exhaustion fails many candidates at once; the old code advanced past all of them. Measured: 3 candidates, 3 errors, watermark advanced, three leads gone for good. The watermark now advances over the contiguous PREFIX that actually wrote its leads and stops at the first that raised. Later successes in the same batch are simply re-considered next cycle, which is free — the third unknown-sender step finds the lead they already created. A held cursor logs sync.auto_lead_stalled at WARNING EVERY cycle, because a held cursor and a quiet mailbox both create nothing and only the level can tell them apart; that trades silent loss for a visible stall on a genuinely poison head message, deliberately, fail-closed toward the CRM. The counter-case matters as much: a failed first ACTIVITY does NOT hold the cursor. The lead is already committed, so the message would be skipped on re-consideration and the activity never retried — holding for it would stall forever on work that cannot be redone. It is counted separately as activity_errors, and `created` now increments the moment the lead commits rather than after its activity: the row exists and will push either way, and a cycle logging created=0 beside a queued lead sends whoever reads it looking in the wrong place. P2-4 — the Sent probe inherited @>'s case-exactness. The owner wrote to Asha@AcmeRobotics.com; she replied from asha@acmerobotics.com and was minted as a cold lead. This module now asks with EXISTS over jsonb_array_elements and lower(). _maybe_block_cold is left alone on purpose: it is the email package's predicate with its own blast radius. The fake learned to READ the new shape — and the hard part was stripping it, since its inner lower(recipient->>'email') = :address is exactly what _JSONB_LOWER_CMP matches, which would have filtered the outer rows on a column they do not have. P2-6 — cfo@mail.fracktal.in passed both colleague gates. Gate 2 is now suffix aware, anchored on a leading dot so notfracktal.in is still a prospect. P2-5 — the cap had no tiebreak. ORDER BY rules_processed_at, id; fetch cap+1; and when the cap falls INSIDE a group sharing one timestamp, defer the whole group. Only then — deferring the last group unconditionally would shrink every capped batch by one message for nothing. Latent in production, where the rules runner stamps one transaction per message. Also: lead_name and the activity subject are clipped (120/500, with a marker). Nothing upstream bounds a display name, and it becomes a column every list, board card and Zoho push carries. MIGRATION RENUMBERED 157 → 158. Open PR #399 claims 157, and two migrations sharing a number replay in filename order against the wrong schema. The ladder carries a deliberate reservation gap until #399 lands; the header names it, the test finds the file by content, and the contiguity assertion is replaced by the uniqueness one — which is the property that actually protects the ladder and the only one this branch can hold. The step's import moved inside the gated branch so the OFF state does not load routes/crm on the mail path. The predicate stays above the gate on purpose: auto_lead_enabled is the flag's ONE definition, and reading settings.crm_auto_lead in the hook would make two places responsible for agreeing what the flag means. The import-inside-try divergence from the five sibling steps is now recorded rather than left to be rediscovered. 52 → 73 tests; 7 → 13 mutants red and reverted, one of them precision-checked (reverting the Sent probe must not redden the lower-case already-emailed case, or the mutant broke it rather than narrowing it). _crm_fakes gained fail_on(after=N), because where in a batch a failure lands IS the property under test. Still BUILT, NOT FLIPPED, NOT DEPLOYED. R4 in the same change: the ticket's cursor paragraph rewritten to the epoch-anchor semantics, done-when 8 and 9 added, the as-built block's deviation list grown to seven, plus work_plan's WS-26 row and §6(b) — which now tells an owner that turning the flag off is a stop, not a pause that accumulates. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…amp, don't reset My own P1, introduced by the previous round's fix, and it was worse than the bug it fixed: instead of over-minting once after a four-week outage, it silently dropped a lead every single night. The premise I built that fix on was false. I assumed this step runs once per scheduler period, so a stale last_run_at meant "the service was down". It does not run per period: email_ingestion/scheduler.py:463-472 reads `synced` off the sync result and fires the hook ONLY when a sync actually persisted mail. A mailbox with no new mail does not run this step at all, so last_run_at freezes on a quiet mailbox exactly like the watermark does — which means the cycle that trips the dormancy test is ALWAYS the cycle carrying the message that woke it. Measured consequence: no mail 22:00 to 07:30, a cold prospect writes at 07:30:50, the sync persists it, the step runs for the first time in nine hours at 07:31:05, gap > 3600 so it re-anchors everything to 07:31:05 and returns early — and the triggering message's received_at is fifteen seconds the wrong side of the anchor its own arrival created. Excluded forever. Every night, every weekend, on exactly the message this feature exists to catch. The fix is to clamp rather than reset, and to never discard the triggering batch: activated_at moves forward to now - REANCHOR_GAP_SECONDS instead of to now, and the cycle proceeds into the batch normally. The candidate WHERE then does the work by itself — received_at > activated_at excludes the OFF window while admitting everything from the last hour, which is where the waking message always is. The watermark is deliberately untouched on re-anchor: OFF-window mail is already excluded by the anchor whatever its rules_processed_at says, and moving it would skip the triggering batch a second way. Documented residual, now asserted deliberately rather than discovered later: mail received in the final hour before the flag comes back on IS minted, up to one capped batch of it. That is the price of never dropping the waking message, and it is bounded. §6(b) tells an owner about it before they flip, with the advice to flip at a quiet moment if the tail matters. The guarding test was green on fiction — it seeded last_run_at = now - 300s directly, a state the scheduler cannot produce, so it could not see any of this. Tests now drive a movable clock and express the lull the way the scheduler does: by NOT CALLING the step. The Monday-morning case runs an evening cycle, advances nine and a half hours with no invocation at all, then delivers the 07:30 message and asserts it is minted in the same cycle that re-anchors. Also corrected the docstrings that carried the false premise — the constant's comment claimed "six scheduler periods", which the wiring does not provide, and the third-column rationale claimed the step "runs faithfully every 600s" on a quiet mailbox, which it does not. last_run_at is still the right clock, but for a narrower reason than I wrote: both it and the watermark freeze on a mailbox receiving nothing, and what separates them is a cycle that ran and found no candidates — which is also the state a deliberate stall holds the watermark in. Migration hardening: CREATE TABLE IF NOT EXISTS is a no-op against a database that already has the table, so it cannot add a column to one. A scratch DB that applied this file while it was still the two-column version would keep the old shape and fail every cycle. Added the guarded ADD COLUMN IF NOT EXISTS, a COALESCE backfill (SET NOT NULL refuses otherwise), and SET NOT NULL — all no-ops on a fresh database. Keeping _JSONB_CONTAINS in _crm_fakes: it is what makes mutant k expressible, and that mutant is the only thing standing between the case-folding Sent probe and a silent revert to case-exact containment. 73 -> 75 tests; 13 -> 15 mutants red and reverted, the two new ones being the reset-instead-of-clamp and the early-return that discarded the batch. R4 in the same change: the cursor paragraph's re-anchor block, done-when 8, as-built decision 6 (which now names its own earlier false premise rather than quietly replacing it), work_plan's WS-26 row and the §6(b) owner note, both AGENTS.md. Still BUILT, NOT FLIPPED, NOT DEPLOYED. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…n we reverted Comments only — no executable line changed in this commit. The header of 158_crm_auto_lead_cursor.sql was written for the reset-to-now re-anchor and never updated when that became a clamp, so it contradicted the code twice. It repeated the false premise the code and the spec now explicitly disown — that the step "has run faithfully every 600s" on a quiet mailbox, which is why re-anchoring one would drop Monday's first message — and it claimed all three timestamps are re-stamped to now and the gap's backlog mints nothing. None of that is true: only activated_at moves, it moves to now - GAP, processed_watermark is deliberately left alone, and the final gap-width IS minted. That matters more here than in most comments. This block is the canonical description of crm_auto_lead_cursors — it is what the next person to touch dormancy logic reads before they touch it, and a stale comment beside correct code is worse than no comment, because it is evidence. The rewritten header now describes the clamp: which column moves and which deliberately does not, why clamping rather than resetting (the step runs only when a sync PERSISTED mail, so the cycle that detects the gap always carries the message that woke it), the measured failure the reset caused, and the accepted residual stated as a residual. Second fix, same understatement in four places: "up to one capped batch of it" understates the residual. The cap defers rather than drops, so the tail drains ACROSS cycles — 7 messages with a cap of 3 is 3 then 3 then 1, not 3 and the rest lost. The bound is on TIME, not volume: one gap-width of mail, drained across however many cycles the cap takes. Corrected in the migration header, the _reanchor_if_dormant docstring and work_plan §6(b) as asked, and in two places the review did not name but which carry the same sentence — crm_app.md §9's residual paragraph, which is the canonical spec statement of it, and the docstring of the test that asserts the bound. Leaving those two saying something the other three now contradict is the same failure this commit is fixing.⚠️ Two of the five files are .py, and both diffs are entirely inside docstrings — `git diff` on them shows only comment text. No assertion, no logic, no test behaviour changed, which the re-run confirms: 592 / 24 / 421 across the three blocks, unchanged. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What
WS-26d-autolead (D5 — the last demo-path slice, spec §9.0): auto-mint CRM leads from inbound email. Ships OFF behind
CRM_AUTO_LEAD(new settings field, default False); the flip is §6 owner gate (b) and has not been touched. Newroutes/crm/auto_lead.pyruns as the sixth try/except-isolated step inprocess_new_mail(last, after auto-archive — mail the user's own rules archived never becomes a lead), gated lexically outside the step (AST-pinned: with the flag off, zero calls, zero queries, zero imports of the step module). Migration 158: per-account cursor table (157 is left for open PR #399 — the header names it; the parallel-session collision trap, dodged unilaterally).Why the design looks the way it does
This slice writes to a live-syncing CRM unattended — no confirmation card exists on a scheduler hook, so the safety boundary is the OFF default, the flip gate, and the candidate arithmetic. Three successive review gates each caught a different member of the same defect family, all measured against the fake DB, none speculative:
activated_atanchor — mail received before auto-lead was active mints nothing, by construction.process_new_mailonly fires when a sync persisted mail, so every >1h lull tripped dormancy and the triggering message was excluded forever. The guarding test had been green on a hand-seeded state the scheduler cannot produce. Fix: clamp — the anchor moves tomax(last_run_at, now − 1h), nevernow, and the re-anchor cycle runs its own batch. Quiet-weekend mail mints; OFF-window/outage backlog stays out; the residual (the final hour before an ON flip mints, drained across capped cycles) is asserted as a deliberate bound, not left implicit.Other rulings recorded in the spec's as-built block: watermark advances over the contiguous successful prefix only (a transient failure holds the cursor and retries — WARNING-stalls visibly — instead of silently dropping up to 200 leads; a poisoned activity write deliberately does not hold, since the lead exists and dedup would stall forever); the first activity is
type='system'metadata-only (outside the Zoho push predicate, pinned against the actual statement text; subject + sender + ids in meta, body never); Sent-probe case-folding (a correspondent you emailed asAsha@…who replies asasha@…is not a lead); subdomain-aware colleague gate; in-batch dedup with the no-unique-index decision documented (the migration-148 lesson); name/subject clipping against attacker-controlled display names.Chain evidence
test_crm_auto_lead.pyalone), full-suite collection clean, six mutations re-run independently; found the feat(WS-27o…t): recurrence · dependencies · cards · calendar · timeline · search — parity backlog closed #399 migration collision that prompted the 158 renumber.For the owner, before ever flipping the flag (§6 gate (b))
sync.auto_lead_stalledWARNING means the cursor is deliberately holding on a failure — leads are deferred, not lost.Deferred (recorded): the fake's
_orderedasserts the, idtiebreak as SQL text only (latent-only property);schema.generated.sqluntouched (owner chore, needs live DB).🤖 Generated with Claude Code