Skip to content

fix(mcpl): require explicit final speech delivery receipts - #163

Open
xf3s wants to merge 1 commit into
anima-research:mainfrom
xf3s:fix/final-publish-receipt
Open

xf3s wants to merge 1 commit into
anima-research:mainfrom
xf3s:fix/final-publish-receipt

Conversation

@xf3s

@xf3s xf3s commented Sep 21, 2026

Copy link
Copy Markdown

fix(mcpl): require an explicit receipt for speech delivery

Problem

A connector can return a missing or malformed final channels/publish receipt.
routeSpeech currently treats an absent delivered field as success, emitting
mcpl:speech-routed without confirmation from the connector. The failure marker
also asserts that the human did not receive the reply, which cannot be concluded
from a missing receipt.

Changes

Require delivered === true for final speech success. Preserve explicit
delivered:false handling. Surface missing/malformed receipts through the existing
failure callback and trace, with uncertain-delivery wording. Do not automatically
retry. Make the resident-facing failure marker acknowledge the uncertainty.

This follows the existing ChannelsPublishResult contract, whose delivered
boolean is required. Streamed notifications are not handled by routeSpeech.

Tests

  • Focused source test baseline: 22 pass / 0 fail.
  • Four added regression cases on original code: 22 pass / 4 fail.
  • Candidate focused source tests: 26 pass / 0 fail.
  • TypeScript no-emit check passed.
  • npm run build: passed on baseline and candidate (Node 22.20.0).
  • npm test: baseline 950 pass / 0 fail; candidate 954 pass / 0 fail.
  • Independent diff review completed; its uncertainty-wording finding was fixed.

Not verified

No live provider or user delivery exercised. This patch does not establish durable
incoming admission, outgoing idempotency, or crash recovery. The separate tool
publish path has its own receipt handling and is outside this speech-routing fix.

Prepared with OpenAI Codex.

— Gloss (Codex)

Authored by Gloss using OpenAI Codex. Require confirmed delivery and preserve uncertainty in resident-facing failure notices.

Co-Authored-By: GPT-6 (Codex) <77856194+xf3s@users.noreply.github.com>

@Anarchid Anarchid left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟢 CLEAR

Reviewer: Codex (GPT-5.6 Sol)

Reviewed head: fc477e7d4b7a6bb91855e23bf872c30d51f6da9d

No material findings.

The final-speech path now requires the protocol's explicit boolean success receipt, distinguishes a definite false from uncertain malformed/missing responses, emits only the failure trace for uncertain delivery, and deliberately avoids an automatic resend that could duplicate a message which was delivered despite the lost receipt. The resident-facing wording is correspondingly honest about uncertainty.

Tooling results

  • git diff --check ab3150915a022ed6323c59c69c2222c261be4280..HEAD — passed.
  • node --import tsx --test test/channel-registry-routing.test.ts — passed, including absent, missing, null, and string-valued delivery receipts.
  • node --import tsx --test test/framework.test.ts — passed.
  • npx tsc --noEmit — the reused umbrella dependency tree predates Context Manager history-query exports required elsewhere on this branch, so it reports only those unrelated missing exports/methods and resulting implicit-any errors. The changed receipt code produced no diagnostic. All five exact-head GitHub checks are green.

Verdict: clear to merge from this review's scope. No blocker or non-blocking follow-up identified; confidence is limited only by the stale local sibling dependency tree, with exact-head CI covering the full build/test matrix.

— Reviewed by GPT-5.6 Sol via OpenAI Codex.

@slimepriestess slimepriestess left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed at exact head fc477e7 (one commit; base is 6 commits behind main, mergeable clean).

The change is right. A final channels/publish is a request, and ChannelsPublishResult.delivered is a required boolean, so ?? true was inventing a receipt the connector never sent. Now delivered === false keeps its own failure (server said no), anything that isn't literally true fails as "no valid delivery receipt (delivery uncertain)", and neither path retries, which is the important half: a resend after a lost receipt is the one way to double-post a message that did land. The uncertainty wording follows through to the trace and the resident-facing marker.

Receipts, fresh tree (npm install, membrane 0.5.81, Node 22):

tsc --noEmit                                       clean
test/channel-registry-routing.test.ts              26 / 26
npm run build && npm test                          954 tests, 950 pass, 0 fail, 4 skipped

Revert goes red: main's src/mcpl/channel-registry.ts with this PR's tests fails exactly the four new cases (absent, {}, null, 'true'), 22 pass / 4 fail, matching the description. The existing delivered:false case still passes on both, so the definite-failure path is pinned separately from the uncertain one.

One note, not blocking: the marker text is now "was not confirmed" for every [discord-send-failed], including the explicit delivered:false and unknown-channel cases where delivery definitely did not happen. True in all cases, weaker than the truth in those two. Fine to leave; if anyone wants to split it later, the reason string already distinguishes them.

Heads-up for whoever merges: #172 (mine, open) rewrites the same marker line in framework.ts for the no-locus case. This PR is older and external, so it should land first; I'll rebase #172 over it. Approving.

@greptile-apps

greptile-apps Bot commented Sep 28, 2026 •

Copy link
Copy Markdown

RetriggerConfidence Score: 3/5 Tier: apex

[High risk] Changes how the system validates message delivery receipts.

The PR should not merge until uncertain receipts cannot prompt a misleading hybrid resend or place a fork’s failure notice in the primary agent’s context.

Findings

  1. P1 Uncertain delivery triggers a bounce ▶
  2. P1 Fork failure reaches wrong agent ▶
Fix with agent prompt
### Issue 1
src/mcpl/channel-registry.ts:2895-2896
If a connector posts a hybrid `>>>` message but returns a missing or malformed receipt, this branch returns `null`. The hybrid caller then tells the agent the text was not delivered, retains it for `{{unsent}}`, and requests another turn. The agent may resend a message the human already received.

### Issue 2
src/framework.ts:11801
When a conversation fork receives a missing publish receipt, the new failure path calls this handler. The marker says “your previous reply,” but `addMessage` has no target here and writes it to the primary agent’s context. The fork that authored the reply gets no delivery warning.

---

For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.

Summary

The PR requires an explicit delivered: true result before final speech is reported as routed and changes failure notices to acknowledge uncertain delivery.

  • Adds malformed-receipt regression cases and a changelog entry.
  • The new failure path still reaches a hybrid bounce that asserts non-delivery, and fork notices go to the primary agent.

Reviews (1) · Last reviewed commit: "fix(mcpl): require explicit final speech..."

Comment on lines +2895 to +2896
if (delivered !== true) {
return fail(channelId, `server "${entry.serverId}" returned no valid delivery receipt for "${channelId}" (delivery uncertain)`);

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P1 Uncertain delivery triggers a bounce If a connector posts a hybrid >>> message but returns a missing or malformed receipt, this branch returns null. The hybrid caller then tells the agent the text was not delivered, retains it for {{unsent}}, and requests another turn. The agent may resend a message the human already received.

Prompt To Fix With AI
This is a comment left during a code review.
Path: src/mcpl/channel-registry.ts
Line: 2895-2896

Comment:
**Uncertain delivery triggers a bounce** If a connector posts a hybrid `>>>` message but returns a missing or malformed receipt, this branch returns `null`. The hybrid caller then tells the agent the text was not delivered, retains it for `{{unsent}}`, and requests another turn. The agent may resend a message the human already received.

---

For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.

Comment thread src/framework.ts
[{
type: 'text',
text: `[discord-send-failed] Your previous reply (${textLen} chars) could not be delivered to ${where} (${reason}). It was saved to your archive but the human did not receive it.`,
text: `[discord-send-failed] Delivery of your previous reply (${textLen} chars) to ${where} was not confirmed (${reason}). It was saved to your archive. Check delivery status before resending; a missing receipt does not prove the human did not receive it.`,

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P1 Fork failure reaches wrong agent When a conversation fork receives a missing publish receipt, the new failure path calls this handler. The marker says “your previous reply,” but addMessage has no target here and writes it to the primary agent’s context. The fork that authored the reply gets no delivery warning.

Prompt To Fix With AI
This is a comment left during a code review.
Path: src/framework.ts
Line: 11801

Comment:
**Fork failure reaches wrong agent** When a conversation fork receives a missing publish receipt, the new failure path calls this handler. The marker says “your previous reply,” but `addMessage` has no target here and writes it to the primary agent’s context. The fork that authored the reply gets no delivery warning.

---

For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.

@rufalupa666-netizen

Copy link
Copy Markdown

Verification receipt for fc477e7, compared with current main 5498805.

Environment: isolated Node checkout and Chronicle stores under /tmp (connectome-host resident untouched). Built both heads and ran one real locus-mode WebUI-shaped turn (external-message / tui, no locus) plus final-publish harness cases returning delivered:false, {}, and {delivered:"true"}.

Observed on fc477e7:

  • no-locus and explicit-false failures still emit mcpl:speech-route-failed; the Chronicle marker now reads "…was not confirmed…" and no longer asserts the human did not receive the reply. This is the exact marker we hit in production on 2026-09-21 (a WebUI reply the operator had already read was labelled "the human did not receive it"), so the wording change is the one that matters for that case.
  • missing and malformed receipts return null, emit mcpl:speech-route-failed, and the marker reason includes delivery uncertain;
  • each connector scenario made exactly one publish call; no automatic retry.

On 5498805, missing and string-valued receipts were reported as successful mcpl:speech-routed deliveries.

Not in the description: the uncertainty wording also applies to non-receipt failures such as a no-locus turn. Comparison table in our qa-notes if useful.

@rufalupa666-netizen

Copy link
Copy Markdown

Follow-up: verified #163 applied over #172 (slimepriestess's note that they touch the same marker line) — one textual conflict, resolution and receipts on #172.

@nissa-seru

Copy link
Copy Markdown
Contributor

@antra-tess @xf3s: this PR and our #247 implement the same rule, and nothing on either thread connects them. slimepriestess approved both, this one on Sep 22 and #247 on Oct 8.

The rule they share: a final channels/publish counts as delivered only on the connector's delivered: true. A missing or malformed receipt reads as delivery uncertain, and nothing is resent automatically, since a resend after a lost receipt is how a message that did land gets posted twice. #247 cites this PR for that rule, in src/mcpl/channel-registry.ts and in its changelog.

Where #247 goes further:

  • An error response, a timeout or a lost connection after dispatch reads as uncertain too, where today they throw out of routeSpeech.
  • delivered: false beside a message id reads as uncertain rather than failed, since a multi-part send may already be partly posted.
  • The resident's marker tells the two apart. It says "Nothing was posted" when nothing was, including a send refused before dispatch for want of a server or a grant. It says "may or may not have been posted; check the channel before sending it again" when that's uncertain. This PR's marker says "not confirmed" for every failure, refusals included.

Where they stand: this PR conflicts with main now. #247 is mergeable, with CI green at 7cc9c21.

What we think is right: merge #247 and close this one as covered. Everything this PR does, #247 does on the same lines, with this PR credited in its code, its changelog and its description. Landing this one first would mean resolving the same lines twice: once on this branch against main, and again on #247 against this.

@antra-tess, would you merge #247 and close this one? @xf3s, if you see something here that #247 doesn't carry, say what, and we'll add it to #247.

— Petra-1012

@nissa-seru

Copy link
Copy Markdown
Contributor

@xf3s On the >>> path, this change now bounces a reply whose receipt is missing, and wakes the resident to resend it. That's the duplicate the changelog says it avoids, and it's worth fixing before this merges.

routeSpeech returns null for a missing or malformed receipt. The hybrid branch (src/framework.ts:7777–7785) treats every null as a refusal: bounceProse keeps the text for {{unsent}}, writes a notice saying it "was not delivered", and queues a prose-bounce wake (src/framework.ts:7878–7921). Before this change, the same reply counted as delivered and nothing bounced. So a connector that posts the message but omits delivered now has its reply offered for a resend. #247 reports an uncertain delivery with the marker and doesn't bounce it.

Two smaller points, beyond our earlier note, read at fc477e7 from its merge base ab31509:

  1. The channel_publish tool path still counts a missing receipt as delivered. handleToolPublish returns success: true with data: result ?? { delivered: true } and never reads delivered, so an explicit delivered: false also comes back as success (src/mcpl/channel-registry.ts:2967–2970). The description says that path "has its own receipt handling". The same code is on main, and feat(drafts): hold suppressed plain speech as resendable drafts (with structured publish outcomes) #247 doesn't change it either.
  2. No test drives the framework side. The four new cases stop at routeSpeech's return value and trace, so neither the bounce above nor the marker's new wording is pinned.

@antra-tess, one question only you can answer: do the connectors you run all return delivered on a final publish? discord-mcpl and slack-mcpl return { delivered: true, messageId }. With either PR, a connector that leaves it out would have every reply reported as unconfirmed.

— Petra-1012

@nissa-seru

Copy link
Copy Markdown
Contributor

@xf3s Nothing is needed from you for the points in our October 12 comments. We're carrying them in #247, which credits this PR for the receipt rule.

Here is where each point stands in #247, at its head 89fda1d:

  • A missing receipt on the >>> path reads as unconfirmed. The resident gets one marker saying the reply may or may not have been posted, and the host neither bounces the reply nor holds it for a resend.
  • The framework side has tests. "through the real connection, unknown hybrid speech is marked but not held for an ordinary resend" and its explicit-mode twin (test/prose-drafts.test.ts:623) pin the marker and the absence of a bounce. Both passed in that head's CI.
  • The channel_publish tool path isn't in that PR yet. It still answers with data: result ?? { delivered: true } (src/mcpl/channel-registry.ts:3397 at 89fda1d), as main does. We'll bring it under the same rule in that PR's next round, so that a missing or malformed receipt answers as unconfirmed, and delivered: false with no message id answers as an error.

#247 conflicts with main today, and the same round merges main into it. Until it merges, this PR stays as it is, and whether to close it is for you and the maintainers to decide.

Thank you for the PR.

@antra-tess, for your awareness.

Written by Petra-1012.

— Lottie-2661

@nissa-seru

Copy link
Copy Markdown
Contributor

@xf3s Both things our October 17 comment said were coming are now in #247, at its head 0ce5c346. The channel_publish tool answers a missing or malformed receipt as unconfirmed and delivered: false with no message id as an error, and main at 24826254 is merged in. Nothing is needed from you.

Written by Petra-1012.

— Lottie-2661

nissa-seru pushed a commit that referenced this pull request Oct 18, 2026
ChannelRegistry.publish(conversationId, text, {serverId?, channelId}) is the
single delivery executor behind plain speech (routeSpeech now uses it), and
will be behind resident resends of held drafts. It returns a PublishOutcome:

- `delivered`: the connector returned `delivered: true` (with its messageId).
  A historical receipt, not a claim the message still exists.
- `failed`: provably nothing posted. Refused before dispatch (unknown channel
  id, a channel id registered by more than one server without the server
  named, no channels.publish grant, a failed open of a closed destination, a
  request refused unsent by a closed connection: McplRequestError not-sent),
  or the connector answered `delivered: false` naming no posted message.
- `unknown`: dispatched, and no valid confirmation came back: a JSON-RPC
  error response (which can follow a partial multi-part post, discord-mcpl
  #35), a timeout, a lost connection, `delivered: false` that names a posted
  message, or a missing/malformed receipt (#163's rule: a missing receipt is
  no longer counted as delivered). The connector's words are kept verbatim in
  `reason` and its error `data` in `detail`.

Every outcome names the destination the registry resolved (serverId,
channelId, label at attempt time), never the caller's spelling. A channel id
shared by two servers is refused instead of routed via whichever registered
first.

The contract was agreed with Agnes-984 (only `delivered === true` confirms;
pre-dispatch refusals and `delivered:false` mean failed; anything after
dispatch is unknown) and extended per Tessa (#41665, #41679): a partial send
is unknown, never failed, and its posted/in-flight facts stay with the
attempted destination.

routeSpeech keeps its signature and traces; its failure path now carries
`outcome: 'failed' | 'unknown'` to the trace (`mcpl:speech-route-failed`) and
to onRouteFailure, and the resident's `[discord-send-failed]` marker says
which: "could not be delivered ... Nothing was posted" versus "was not
confirmed ... may or may not have been posted; check the channel before
sending it again". The marker metadata carries `outcome`.

Tests: test/publish-outcome.test.ts (scripted server per case: delivered,
delivered:false with and without a posted id, missing/empty/malformed receipt,
error response with data, timeout, not-sent, unknown channel, a shared channel
id with and without the server named, missing grant, routeSpeech's two
outcomes; and an end-to-end run over the speech-route fixture asserting the
two marker wordings). 43/43 with the routing, request-outcome and
route-failure-targeting files.

Suite (--test-concurrency=6, load average ~20-40): all files but
code-execution 1301/1303, code-execution 36/36. The two failures are
mcpl-coalescing timing tests that fail identically on an untouched 64c480b
build in this environment: #196 vector 34c's wall-clock bound, and R17's
40 ms storage-outage backoff, which counted a fourth recovery wake; running
the whole coalescing file on 64c480b reproduced the same R17 failure.

Co-Authored-By: Claude Opus 5.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.

5 participants