Repository navigation
Conversation
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
left a comment
There was a problem hiding this comment.
🟢 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-anyerrors. 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
left a comment
There was a problem hiding this comment.
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.
|
| if (delivered !== true) { | ||
| return fail(channelId, `server "${entry.serverId}" returned no valid delivery receipt for "${channelId}" (delivery uncertain)`); |
There was a problem hiding this 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.
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.| [{ | ||
| 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.`, |
There was a problem hiding this 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.
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.|
Verification receipt for Environment: isolated Node checkout and Chronicle stores under Observed on
On 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. |
fix(mcpl): require an explicit receipt for speech delivery
Problem
A connector can return a missing or malformed final
channels/publishreceipt.routeSpeechcurrently treats an absentdeliveredfield as success, emittingmcpl:speech-routedwithout confirmation from the connector. The failure markeralso asserts that the human did not receive the reply, which cannot be concluded
from a missing receipt.
Changes
Require
delivered === truefor final speech success. Preserve explicitdelivered:falsehandling. Surface missing/malformed receipts through the existingfailure callback and trace, with uncertain-delivery wording. Do not automatically
retry. Make the resident-facing failure marker acknowledge the uncertainty.
This follows the existing
ChannelsPublishResultcontract, whosedeliveredboolean is required. Streamed notifications are not handled by
routeSpeech.Tests
npm run build: passed on baseline and candidate (Node 22.20.0).npm test: baseline 950 pass / 0 fail; candidate 954 pass / 0 fail.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)