Skip to content

fix(email): dry-run send no longer strands approved pipeline jobs - #25

Merged
oratis merged 2 commits into
mainfrom
claude/fix-dryrun-strands-pipeline
Aug 9, 2026
Merged

fix(email): dry-run send no longer strands approved pipeline jobs#25
oratis merged 2 commits into
mainfrom
claude/fix-dryrun-strands-pipeline

Conversation

@oratis

@oratis oratis commented Aug 9, 2026

Copy link
Copy Markdown
Owner

Summary

The last outstanding item from the 2026-08 review's follow-up list (the chip filed while writing #12's tests).

With no mail provider configured, the email.send worker took a dev fallback that returned early — before the pipeline_jobs sync, the thread row and the event. The contact reached status='sent' while pipeline_jobs.stage stayed at 'send' forever: the Pipeline page showed the job stuck mid-flight, "Sent / Monitoring" never incremented, and the thread view was empty.

That's every provider-less deployment — which is what CI is, and what a fresh self-host is before Resend is wired up.

The branch now skips only the provider call. Everything downstream runs with a synthetic { success: true, messageId: null, provider: 'dry-run' }, so the flow completes exactly as it would with a provider, minus the send: stage advances to 'monitor', the outbound email_replies row exists, and the 'sent' event carries dryRun: true so nobody mistakes it for a real delivery. Provider ids stay NULL because nothing was sent.

Tests that pinned the old behavior

Both the integration test and the Playwright spec asserted the bug as a deliberate canary — the spec's header said in as many words that it should be updated if the dry-run branch ever learned to sync the pipeline row. It has; both now assert the corrected flow.

The stage-gate assertion also stopped hardcoding "send" in the rejection message: the worker races it ('send' while queued, 'monitor' once the handler finishes), so which stage the second approve reports is timing, not behavior.

Verified in a browser against a provider-less server

Approving moved Awaiting Review 1 → 0 and Sent / Monitoring 0 → 1, the stage track advanced to its final node, and the row reads "Sent". DB after:

pipeline_job: {"stage":"monitor","email_sent_at":"2026-08-09 15:03:33","smtp_message_id":null}
contact     : {"status":"sent","sent_at":"2026-08-09 15:03:33","provider_message_id":null}
thread rows : 1 | events: [{"event_type":"sent","payload":{"provider":"dry-run","dryRun":true,...}}]

Test plan

  • npm test 656/656
  • Playwright 5/5 (both approve specs updated)
  • Browser verification above

🤖 Generated with Claude Code

oratis and others added 2 commits August 9, 2026 23:04
With no mail provider configured, the email.send worker took a dev
fallback that returned early — before the pipeline_jobs sync, the thread
row and the event. So the contact reached status='sent' while
pipeline_jobs.stage stayed at 'send' forever: the Pipeline page showed
the job stuck mid-flight, "Sent / Monitoring" never incremented, and the
thread view was empty. That is every provider-less deployment — which is
what CI is, and what a fresh self-host is before Resend is wired up.

The branch now skips only the provider call. Everything downstream runs
with a synthetic { success: true, messageId: null, provider: 'dry-run' }
result, so the flow completes exactly as it would with a provider, minus
the send: stage advances to 'monitor', the outbound email_replies row
exists, and the 'sent' event carries dryRun: true so nobody mistakes it
for a real delivery. Provider ids stay NULL because nothing was sent.

The integration test and the Playwright spec both pinned the old
behavior as a deliberate canary — the spec's header said in as many
words that it should be updated if the dry-run branch ever learned to
sync the pipeline row. Both now assert the corrected flow. The
stage-gate assertion also stopped hardcoding "send" in the rejection
message: the worker races it, so which stage the second approve reports
is timing, not behavior.

Verified in a browser against a provider-less server: approving moved
Awaiting Review 1 → 0 and Sent / Monitoring 0 → 1, the stage track
advanced to its final node, and the row reads "Sent". DB after:
stage='monitor', email_sent_at set, 1 thread row, 1 event with
dryRun: true.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Review follow-up on this PR. The fix is right, but it removes the only
accidental signal that a deployment has no mail provider: jobs used to
strand at pipeline stage 'send', which at least looked wrong. Now the
flow reports complete success — contact 'sent', stage 'monitor', a thread
row, a 'sent' event — and the dry-run branch itself logs nothing at all.

That matters beyond dev because nothing fails fast on a missing provider:
server/email.js isConfigured() is a plain boolean check and no boot guard
consults it. A production revision that lost RESEND_API_KEY would report
every outreach as delivered, with `provider: 'dry-run'` buried in the
email_events payload as the only tell.

Adds a log.warn naming the three things it looked for and the contact it
is about to mark sent without sending. No behavior change — the synthetic
result, the bookkeeping and the dryRun labelling are untouched.

Verified: node --test server/__tests__/{pipeline-send-integration,email-jobs}.test.js
→ 18/18. Full suite 678/678 on this branch merged onto main (serialized;
the 4 SQLITE_BUSY failures on a default parallel run are the known shared
influencex.db flake and reproduce on main alone).

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
oratis added a commit that referenced this pull request Aug 9, 2026
…lake

Review follow-up on this PR. The numbers went stale during review: #23
(+9 tests, +1 file) and #24 (+7, +1) merged while this branch sat open,
so "656 / 69 files" was already wrong by the time it could land — the
exact failure mode the PR exists to fix.

Measured on main at 7df4183: 678 tests across 71 files. Corrected in all
five places (CLAUDE.md ×2, memory.md §5.2 / §7.3 / Last-reviewed footer,
which also still said "post #20").

Also documents why a clean checkout can show 2-5 red files: `npm test` is
`node --test`, which runs files concurrently against the one shared
influencex.db at the repo root, so writes collide and report
`{ code: 'SQLITE_BUSY' }` on a rotating cast of files. It reproduces on
main with no changes applied, and the triage step is a serialized re-run
(--test-concurrency=1 → stable 678/678). Without this written down the
next session reads the flake as its own regression — which is what the
rest of this PR is trying to prevent.

Verified: 678/678 serialized on main, and on main + #25 + this branch.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
@oratis

oratis commented Aug 9, 2026

Copy link
Copy Markdown
Owner Author

Review: approving — fix is correct and complete, one gap closed on top

Traced the whole email.send handler rather than the diff in isolation, because the change's safety rests on what the shared success path does after the branch. It holds up: everything the old dry-run UPDATE did is preserved downstream —

old dry-run UPDATE where it happens now
status='sent', sent_at success path, non-follow-up branch
send_error=NULL same statement
send_attempts+1, last_send_attempt_at the pre-send increment

So no field regresses, and stage='monitor' + thread row + event are net-new. The atomic claim from #10 is upstream of the branch and untouched, so dry-run still can't double-send — confirmed by the existing double-approve test still passing.

Verified locally (not just trusting CI): merges clean onto main at 7df4183, and 678/678 server tests pass with --test-concurrency=1.

⚠️ On CI runs of this branch you may see 2–5 red files with { code: 'SQLITE_BUSY' }. That is not this PR: npm test runs files concurrently against the single shared influencex.db at the repo root, and it reproduces on main with no changes applied. Serialized re-run is green. I've documented this in #26 so it stops being re-diagnosed every time.

One real gap — fixed in 0a71092

The fix removes the only signal that a deployment has no mail provider. Jobs used to strand at stage='send', which at least looked broken; now the flow reports complete success and the dry-run branch logs nothing at all.

That reaches production, because nothing fails fast on a missing provider — server/email.js:29 isConfigured() is a plain boolean and no boot guard consults it:

return gmailEnv || !!process.env.RESEND_API_KEY || !!(process.env.SMTP_HOST && ...);

A revision that lost RESEND_API_KEY would report every outreach as delivered, with provider: 'dry-run' buried in the email_events payload as the only tell. I added a log.warn naming the three things it looked for and the contact it's about to mark sent without sending. No behavior change.

Minor, not blocking

The dry-run email_replies row is indistinguishable from a real outbound one except resend_email_id IS NULL — the dryRun label lives on the event, not the thread row. Fine for the stated goal (a non-blank UI), worth knowing if anything ever audits threads for real delivery.

Merging.

@oratis
oratis merged commit 79aaa4c into main Aug 9, 2026
5 checks passed
@oratis
oratis deleted the claude/fix-dryrun-strands-pipeline branch August 9, 2026 15:15
oratis added a commit that referenced this pull request Aug 9, 2026
* docs: 修正入门文档里已经失效的运维事实

排查一份 brand-voice embedding 的 bug 报告时发现问题本身已经在 #20
(`0c5323b`)修掉了,但顺手核对上下文的过程中撞上好几处文档与现状不符 ——
每一处都会让下一个会话走一段冤枉路,所以单独收一个 docs PR。

改了什么:

- **prod DB 口令来源**:CLAUDE.md 和 memory.md 有 5 处说密码"在 .env 里",
  但这台机器上根本没有 .env(只有 .env.example)。全部改为从 Secret Manager
  取:`gcloud secrets versions access latest --secret=DATABASE_URL`。
- **测试数字**:CLAUDE.md 同时写着 377 和 234,memory.md 写 234 —— 实际是
  656 个服务端测试 / 69 个文件、~1 秒。前端测试也不再是"4 个组件测试"或
  "还没有",是 13 个 vitest 文件 + 5 条 Playwright(3 个 spec)。
- **Sentry / OTEL**:memory.md §5.4 还写着"没有(Sprint 1 待加)",与同文件
  §6 已关闭表和 CLAUDE.md 自相矛盾。两者早已接入,只是要配 DSN / OTLP 才上报。
- **brand_voices 生产现状**:借这次机会连 prod 只读查了一次 —— 表 0 行。
  也就是说 embedding 写入路径坏了这么久没有造成数据损失,不需要 backfill。
  这条结论写进 memory.md §6,省得以后有人再问一遍。
- 新增一条"仍然成立":embedding 只在 `POST /api/brand-voices` 创建时写,
  目前没有 update 路由所以无害,但以后加编辑接口必须重新 embed。

验证:纯文档改动,未动任何代码路径;`npm test` 656/656 通过。

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>

* docs: correct this PR's own test counts, and record the SQLITE_BUSY flake

Review follow-up on this PR. The numbers went stale during review: #23
(+9 tests, +1 file) and #24 (+7, +1) merged while this branch sat open,
so "656 / 69 files" was already wrong by the time it could land — the
exact failure mode the PR exists to fix.

Measured on main at 7df4183: 678 tests across 71 files. Corrected in all
five places (CLAUDE.md ×2, memory.md §5.2 / §7.3 / Last-reviewed footer,
which also still said "post #20").

Also documents why a clean checkout can show 2-5 red files: `npm test` is
`node --test`, which runs files concurrently against the one shared
influencex.db at the repo root, so writes collide and report
`{ code: 'SQLITE_BUSY' }` on a rotating cast of files. It reproduces on
main with no changes applied, and the triage step is a serialized re-run
(--test-concurrency=1 → stable 678/678). Without this written down the
next session reads the flake as its own regression — which is what the
rest of this PR is trying to prevent.

Verified: 678/678 serialized on main, and on main + #25 + this branch.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>

---------

Co-authored-by: Claude Opus 5 <noreply@anthropic.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant