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.

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.

4 participants