finding: a notify flow-run summary reports success/acted:1 while the delivery dead-lettered — the dead-letter is absent from the run summary

[huangyiirene]

Symptom

A ['push'] notification probe's flow-run summary reports status:'success', acted:1 while nothing was delivered. The dead-letter is visible in sys_notification_delivery (the push channel is unregistered — status:'dead', error: "channel 'push' not registered") but not in the flow-run summary — so an operator reading the run summary sees a successful action that in fact delivered nothing.

Root cause

Not located by the run. The flow-run summary's acted count (the selected/acted/skipped surfacing added by #4354) treats the notify node as acted on enqueue and does not reconcile against the delivery outcome recorded in sys_notification_delivery, so a dead-lettered delivery is still counted as a successful act. Split from the run's "Notes worth keeping" #6 as a real observability defect (the summary an operator reads disagrees with the durable delivery record). Fix spans the notify-node acted accounting and the delivery-outcome reconciliation (packages/services automation/messaging).

Stale-premise check: present on origin/main (the run summary does not reflect delivery dead-letters).

Reproduction

1. Boot without the push channel registered.
2. Fire a flow whose notify node targets ['push'].
3. Read the flow-run summary, then sys_notification_delivery.

Expected: the run summary reflects the failed/dead-lettered delivery (not acted:1 success). Actual: run summary status:'success', acted:1; sys_notification_delivery shows the delivery dead-lettered.

Source

Extracted from the QA run #7690 (framework 92f26f7, console 09987b680).

[claude]

Findings triage (grade): promoted — finding removed, pm:queue stays (resolves the filed dual pm-state).

Why queue: an operator-facing record that contradicts the durable record is a real observability defect, and both sides are named: the flow-run summary's acted accounting (the #4354 surfacing — e.g. service-automation/src/builtin/connector-nodes.test.ts:292 pins summary.acted) counts the notify node on enqueue, while the delivery outcome dead-letters in sys_notification_delivery (status:'dead', unregistered channel) with no reconciliation between them. Root cause is not yet pinned to a line, but the repro and the two disagreeing records make this dispatchable — locating the accounting seam is the first step of the fix, not a blocker to queueing it.

本评论来自分诊座位 Routine（#5474 试点），不构成认领。

---

Generated by Claude Code

[huangyiirene]

Claim: PM loop round 3
Session: session_01JVfWSQN9RDDGwdVSmMc29x
Branch: claude/issue-7747-notify-acted-dead-letter
Worktree: cloud session's own clone (mode:cloud)
Domain: domain:services
File surface: packages/services/service-automation/src/builtin/notify-node.ts (+ its test), and whatever the delivery-outcome read requires within packages/services.
Container & model: M (observability semantics — what acted should mean against an async delivery outcome), mode:cloud, model: opus
Serial constraints cleared: ✅ same-package predecessor #7742 (PR #7847) MERGED — serialization released; that diff touched plugin.ts's connector reconcile, not the notify node, so no interaction.

PM landing-site read (30-second grep, card said "not located"): notify-node.ts:264 sets metrics: { acted: 0 } and :293 sets metrics: { acted: Number(result.delivered) || 0 }, with the #4354 comment at :290 explaining the intent. So acted counts what the notify call reported as delivered at enqueue, and a channel that dead-letters afterwards (sys_notification_delivery.status: 'dead', e.g. unregistered push) never revises it. That is the seam — the dispatch names it as a PM hypothesis to verify, not as fact.

---

Generated by Claude Code

[huangyiirene]

{
  "issue": 7747,
  "premise_still_valid": true,
  "status": "draft_pr_open",
  "pr": "https://github.com/objectstack-ai/objectstack/pull/7875",
  "branch": "claude/issue-7747-notify-acted-dead-letter",
  "commit": "42d917d",
  "pm_hypothesis": {
    "verdict": "confirmed_and_sharpened",
    "pm_said": "notify-node.ts:293 `metrics: { acted: Number(result.delivered) || 0 }` records what the notify call reported delivered AT ENQUEUE, never revised after a dead-letter",
    "measured": "Correct in outcome, one level off in location. :293 CONSUMES the number; it is MINTED in MessagingService.emit(). `EmitResult.delivered` means two different things on the two emit paths: inline (P0) `fanOut()` has each channel's answer before emit() returns (an unregistered channel is already ok:false, delivered is terminal — this path never had the bug); outbox (P1, ADR-0030) `enqueueDeliveries()` counts ENQUEUED rows as `delivered` and the dispatcher decides the outcome afterwards. So `acted` is minted before any send attempt from a count labelled `delivered` that means `enqueued`, and nothing ever revisits it.",
    "corroborating_evidence_in_tree": [
      "messaging-service.ts:658 docstring: \"here `ok` means 'accepted for delivery' (enqueued), not yet delivered\"",
      "messaging-service.test.ts:311 pinned the conflation verbatim: `expect(result.delivered).toBe(2); // 2 enqueued (accepted)`",
      "dispatcher.ts:210 / :251 write the dead-letter (`dead: true`, \"channel '<c>' not registered\")"
    ]
  },
  "design_decision": {
    "chosen": "(c) — count only accepted deliveries toward `acted`; report a merely-enqueued delivery as `unmeasuredEffect`, the platform's existing qualifier for an effect it cannot count",
    "rationale": "The acceptance criterion is that the summary must not read as 'delivered' when sys_notification_delivery says 'dead'. `unmeasured` is already the platform's word for exactly this uncertainty and needs no new counter: the documented broken-sweep alert is `selected > 0 AND acted = 0 AND unmeasured = 0`, so a pending delivery suppresses the alert WITHOUT asserting success, and formatRunSummaryLine already prints `unmeasured=` with the comment that `acted=0` alongside it means 'cannot tell', not 'did nothing'.",
    "rejected_a_acted_zero": "Ruled out by the codebase itself — connector.zod.ts:613 'never `acted: 0`, which would claim it did nothing'. A blanket zero trips the broken-sweep alert on every healthy outbox-backed notify, which is precisely what the #4354 comment at notify-node.ts:290 was defending against.",
    "rejected_b_keep_acted_surface_outcome": "`acted_count` is a COLUMN and a highlightField on sys_automation_run and the documented alert reads it directly. Keeping the overstatement in the column and annotating it elsewhere leaves the lie where the operator actually looks.",
    "status_left_as_success": "Deliberate. The flow did everything it can do synchronously; failing it would let a channel registered a moment later retroactively break the flow (messaging-service-plugin.ts:212 documents late registration as supported). No synchronous wait on async delivery was added.",
    "blast_radius_note": "In outbox-backed stacks a notify run now reads `acted: 0, unmeasured: 1` where it read `acted: 1, unmeasured: 0`. That is the intended correction, and `unmeasured` keeps such runs out of the broken-sweep alert."
  },
  "changes": [
    "packages/services/service-messaging/src/messaging-service.ts — EmitResult gains `enqueued`; `delivered` narrowed to 'a channel ACCEPTED it' (terminal). Outbox path returns delivered:0 + enqueued:N; inline path returns enqueued:0. All 6 return sites updated.",
    "packages/services/service-automation/src/builtin/notify-node.ts — MessagingServiceSurface mirrors `enqueued?` (optional: structural mirror of a runtime-resolved service; absent reads as 'nothing in flight'). metrics: acted counts only delivered; unmeasuredEffect when enqueued > 0. Output gains `enqueued`.",
    "packages/services/service-automation/src/builtin/notify-delivery-outcome.integration.test.ts — NEW: the QA repro end-to-end.",
    "packages/services/service-automation/package.json + pnpm-lock.yaml — @objectstack/service-messaging as a TEST-ONLY devDependency (no cycle: messaging does not depend on automation; the notify node keeps its no-runtime-dep structural surface).",
    ".changeset/notify-acted-dead-letter.md — patch × 2."
  ],
  "test_evidence": {
    "asserts_on": "the two DURABLE operator-facing records — the folded run summary and the sys_notification_delivery/outbox row — never internal call counts",
    "why_real_stack": "wires the REAL MessagingService + MemoryNotificationOutbox + NotificationDispatcher behind the notify node; the defect lives in the seam BETWEEN enqueue and dispatch, so a fake answering emit() in one shot cannot express the disagreement at all",
    "cases": [
      "outbox + push unregistered → summary acted:0 unmeasured:1; row status 'dead' with \"channel 'push' not registered\"",
      "outbox + inbox registered → summary acted:0 unmeasured:1; row pending → 'success' after a dispatcher tick (stops the fix degenerating into 'unregistered channels are special')",
      "inline delivered → acted:1 unmeasured:0 (acted stays a measurement where the outcome IS known)",
      "inline + push unregistered → acted:0 unmeasured:0, a MEASURED zero, correctly alert-eligible"
    ],
    "reverse_verification": "On origin/main the durable-record assertions PASS (the dead-letter is real) while the summary assertion FAILS with `acted: 1, unmeasured: 0` — the finding exactly. 2 of 4 red before the fix, 4/4 green after."
  },
  "pins_reviewed": {
    "messaging-service.test.ts:311": "UPDATED deliberately — `delivered: 2 // 2 enqueued (accepted)` → `enqueued: 2, delivered: 0`. That comment WAS the conflation; the pin now states the distinction.",
    "messaging-service.test.ts:102": "extended with `enqueued: 0` so `delivered` is pinned terminal on both paths.",
    "connector-nodes.test.ts:292": "UNAFFECTED and green — it pins connector accounting (acted:1, unmeasured:0), not notify's.",
    "other_consumers": "none — repo-wide grep for EmitResult / .delivered finds only these tests and the notify node. plugin-approvals and plugin-audit await emit() without reading the result. No docs reference notify.delivered or EmitResult."
  },
  "gates": {
    "build_closure": "pass",
    "service_automation_suite": "pass — 77 files / 932 tests",
    "service_messaging_suite": "pass — 18 files / 206 tests",
    "check_docs_audit_scope": "pass",
    "changeset": "present (patch × 2, user-visible observability change)",
    "spec_regen": "n/a — packages/spec/src/** not touched",
    "engine_double_gate": "n/a — the new test adds no data-engine fake with update/delete verbs; its only fake is a send-only channel"
  },
  "open_questions": [
    {
      "q": "Worktree isolation: the designated branch was already checked out in this cloud container's primary clone, and `git checkout --` is blocked by the permission classifier, so the primary could not be moved off it.",
      "decided": "Created a proper dedicated worktree (/home/user/objectstack-7747) off origin/main under the local name `wt/issue-7747` and pushed it to the designated remote branch with an explicit refspec (`git push -u origin wt/issue-7747:claude/issue-7747-notify-acted-dead-letter`). The remote branch name and content are exactly as contracted; only the local branch label differs. The repo had no node_modules, so the worktree cost one pnpm install either way."
    },
    {
      "q": "Should `enqueued` be required on the notify node's MessagingServiceSurface?",
      "decided": "Required on EmitResult (the in-repo producer must be explicit), optional on the structural surface (a runtime-resolved, possibly third-party service; absent reads as 'nothing in flight', the only answer such a stack could honestly give)."
    }
  ],
  "followup_found_not_fixed": {
    "site": "packages/services/service-automation/src/builtin/http-nodes.ts:139",
    "detail": "`{ output: { deliveryId, enqueued: true }, metrics: { acted: 1 } }` for an outbox-enqueued HTTP delivery — the same class of overstatement against sys_http_delivery, which has its own `dead` status. Out of scope for #7747; worth its own card."
  }
}

---

Generated by Claude Code

[huangyiirene]

ACCEPT — PR #7875 (services seat review, session session_01JVfWSQN9RDDGwdVSmMc29x).

My dispatch hypothesis pointed at notify-node.ts:293; the dev confirmed the outcome and corrected the location one level down, which is the part that makes the fix right: :293 merely consumes the number — it is minted in MessagingService.emit(), where EmitResult.delivered means two different things on the two paths. Inline (P0) fanOut() has each channel's terminal answer before returning (that path never had the bug); outbox (P1, ADR-0030) enqueueDeliveries() counts enqueued rows as delivered and the dispatcher decides the outcome afterwards. So acted was minted before any send attempt, from a count labelled delivered that meant enqueued. The conflation was even pinned in-tree (messaging-service.test.ts:311, // 2 enqueued (accepted)) — that comment WAS the bug, and the PR turns the pin into a statement of the distinction.

What shipped: EmitResult gains enqueued, delivered narrowed to "a channel accepted it" (terminal, both paths); notify counts only delivered toward acted and reports a merely-enqueued delivery as unmeasuredEffect.

Review notes:

• The design choice is the strongest part. unmeasured is the platform's existing word for this uncertainty, so the fix needs no new counter and — critically — keeps healthy outbox notifies out of the documented broken-sweep alert (selected > 0 AND acted = 0 AND unmeasured = 0) without asserting success. Option (a) blanket-zero was ruled out by the codebase's own rule (connector.zod.ts:613, "never acted: 0, which would claim it did nothing") — a rejection argued from the repo rather than from taste. Option (b) was rejected because acted_count is a column and a highlightField the alert reads directly: annotating elsewhere leaves the lie where the operator looks. Endorsed.
• Leaving status: success is right and reasoned: a channel registered a moment later (a supported pattern per messaging-service-plugin.ts:212) must not retroactively fail a flow. No synchronous wait on async delivery — the one hard constraint in the dispatch, honored.
• Test shape is exactly what this defect required: real MessagingService + MemoryNotificationOutbox + NotificationDispatcher, because the disagreement lives in the seam between enqueue and dispatch — a fake answering emit() in one shot cannot express it. Four cases cover both paths in both directions (including "inline + unregistered ⇒ a MEASURED zero, correctly alert-eligible", which stops the fix degenerating into "unregistered channels are special"). Reverse verification is precise: on main the durable-record assertions pass while the summary assertion fails with acted: 1, unmeasured: 0 — the finding verbatim.
• Pin review done properly: the one updated pin is the one that encoded the conflation, connector-nodes.test.ts:292 correctly unaffected, repo-wide grep confirms no other EmitResult/.delivered consumers.
• Blast radius declared: outbox-backed notify runs now read acted: 0, unmeasured: 1. That is the intended correction, and the alert semantics hold.
• Worktree note (branch already checked out in the container's primary clone; dedicated worktree pushed with an explicit refspec so the remote branch name and content match the contract) — compliant, no guard bypassed.
• All 26 checks green; changeset ×2; test-only devDependency introduces no cycle.

Out-of-scope finding to file: http-nodes.ts:139 returns metrics: { acted: 1 } for an outbox-enqueued HTTP delivery — same overstatement class against sys_http_delivery, which has its own dead status. I'll file it unassigned rather than ride it here.

Marking ready and arming auto-merge now.

---

Generated by Claude Code