Skip to content

fix(chat): surface silent contract failures in session store and delivery - #5554

Open
AronSwan wants to merge 1 commit into
loopx-project:mainfrom
AronSwan:fix/5462-chat-store-contracts
Open

AronSwan wants to merge 1 commit into
loopx-project:mainfrom
AronSwan:fix/5462-chat-store-contracts

Conversation

@AronSwan

@AronSwan AronSwan commented Oct 4, 2026 •

Copy link
Copy Markdown
Contributor

Goal

Surface three silent contract failures in the session store and manager return delivery (Refs #5462). Conflict detection lives in the delivery layer per the direction in #5462, not in the store's generic replay path (thanks to the CI round for catching that first version's mistake — the store's replay-idempotence semantics are intentional and unchanged now).

  1. Delivery-time payload conflict detection: the two delivery loops compare the six semantic payload fields (role/text/turn_id/origin/attachments/goal_draft, per-field normalization: missing column folds into the canonical empty value) against the transcript row before delivering; a divergence records the existing explicit_unverified status with the typed manager_return_payload_conflict error code (no new status words; a retry can never fix a payload mismatch). append_message keeps its intended semantics: replaying an explicit message_id returns the existing row.
  2. Transport shape validation: _resolve_delivery_sender (attempt-aware callable or bare callable) resolves the sender before invoking; an unusable transport records return_transport_unavailable (registered in DELIVERY_ERRORS) with the existing retry semantics.
  3. Channel namespace documentation: omitting channel_id pins the exact goal.<goal_id> channel — documented on the three session-resolution methods and anchored by a test.

The checked-in project registry I/O manifest is regenerated via scripts/generate_project_registry_io_manifest.py for the final call-site shape (0 unclassified direct sites).

LoopX Area

loopx/chat_store.py, loopx/capabilities/manager_context/roundtrip.py, loopx/semantics/project_registry_io_manifest_v1.json (Python-only; the conversation_scope path through TS is read-only).

Implemented against

#5462 direction Implementation
Message identity conflict semantics (direction 1) Delivery-time six-field normalized comparison + explicit_unverified/manager_return_payload_conflict terminal record; store replay path unchanged
Delivery transport validation + send_with_attempt recognition (direction 2) _resolve_delivery_sender + return_transport_unavailable (registered in DELIVERY_ERRORS) + pre-invoke failure recording
Channel namespace documentation (direction 3) Docstrings on latest_session / resumable_session_candidates / session_candidates + behavior anchor test

Semantic inventory

  • New module-private helpers in roundtrip.py (_resolve_delivery_sender, payload-comparison helper); DELIVERY_ERRORS gains return_transport_unavailable and manager_return_payload_conflict (so reply_status maps them instead of redrawing).
  • No store-API changes, no new status words, no Enum/Literal/schema additions, no TS changes; project_registry_io_manifest_v1.json regenerated with the official generator.

Regression coverage

Five families (synthetic fixtures only): positive retry without fake receipt; attempt-only transport still delivers; channel-scoped transcript-only completion (explicit channel_id pin asserted); payload conflict records terminal state without retry (both delivery loops covered separately); transport-unavailable records retry state. Plus replay-idempotence anchoring (test_generated_message_id_skips_transcript_deduplication path), normalization boundaries (missing column vs None vs [] vs value), and constructor recovery after a divergent replay.

Author Declaration

This PR was prepared by a model_agent — GLM, by Z.ai. Human (repository owner) reviewed and approved the changes before submission.

Known limitation (disclosed)

chat_loopx_mode.py's wake pump has a broad except that would also fold a payload conflict into "retrying"; reaching it requires a double fault (planner writes a divergent row through that path). We left it untouched to keep this PR focused; happy to handle it in the shared layer per the direction in #5462.

@AronSwan
AronSwan requested a review from huangruiteng as a code owner October 4, 2026 05:26
@AronSwan

AronSwan commented Oct 4, 2026

Copy link
Copy Markdown
Contributor Author

We triaged the CI failures against the latest run on main (37139797259):

  • 71 of the failing tests are identical to upstream main — pre-existing, not introduced by this PR.
  • 5 are ours, and one of them is a real design mistake on our side: test_generated_message_id_skips_transcript_deduplication fails because we placed the payload-conflict check inside append_message's generic replay path. The base semantics there are intentionally "replay with the same message_id is ignored" — our check turned that into a raise. The conflict check belongs in the delivery layer, which is also the direction you described in Developer ergonomics: three undocumented silent-failure contracts in ChatSessionStore / drain (repro + production incidents) #5462 (fix at the shared conversation/delivery layer, not a manager-specific workaround).
  • The other three are checked-in registry/census updates we missed (project_registry_io_manifest, semantic vocabulary, goal-instance binding inventory). Those tests need npm dev dependencies, which we hadn't installed locally — our coverage gap.

We're preparing a fix that moves the conflict detection into the delivery loops and regenerates the checked-in inventories, and we'll push it to this branch. Sorry for the noise on the first run.

…very

Three contract gaps from loopx-project#5462, Python-only, each mapped to a direction the
maintainer named; conflict detection lives in the delivery layer per that
direction, not in the store's generic replay path.

- message identity conflicts merged silently on explicit message_id replay
  during return delivery, so retries that changed content left no account;
  the delivery loops now compare the six semantic payload fields
  (role/text/turn_id/origin/attachments/goal_draft, with per-field
  normalization: a missing column folds into the canonical empty value)
  against the transcript row before delivering, and a divergence records
  the existing explicit_unverified status with the typed
  manager_return_payload_conflict error code instead of folding into
  transport retry — a retry can never fix a payload mismatch. The store's
  own replay path keeps its intended idempotent semantics (replay returns
  the existing row).
- an invalid external sender raised TypeError mid-loop and was folded
  into the broad except with an unreadable error code; the two delivery
  loops now resolve the sender shape before invoking
  (_resolve_delivery_sender: attempt-aware callable or bare callable) and
  record return_transport_unavailable (registered in DELIVERY_ERRORS)
  with the existing retry semantics.
- omitting channel_id pins the exact goal.<goal_id> channel; that
  namespace rule is now documented on the three session-resolution
  methods and anchored by a test.

The checked-in project registry I/O manifest is regenerated via
scripts/generate_project_registry_io_manifest.py for the final call-site
shape (0 unclassified direct sites). No new status words: the conflict
terminal reuses the existing explicit_unverified status, so dashboard and
polling semantics of downstream readers stay unchanged; no TS changes.

Blast radius: append_message behavior is unchanged (same replay returns
the existing row); callers of the two drain loops see the same retry
semantics for transport faults; session-resolution behavior is unchanged
(docstrings only).

Regression coverage: five families (positive retry without fake receipt;
attempt-only transport; channel-scoped transcript-only completion with the
explicit channel pin asserted; payload conflict terminal state without
retry, both loops covered separately; transport-unavailable retry state),
plus replay-idempotence anchoring, normalization boundaries, and
constructor-recovery after a divergent replay.

Refs loopx-project#5462

Signed-off-by: AronSwan <10492180@users.noreply.github.com>
@AronSwan
AronSwan force-pushed the fix/5462-chat-store-contracts branch from f88729d to 9f799b8 Compare October 4, 2026 07:44
@AronSwan

AronSwan commented Oct 4, 2026

Copy link
Copy Markdown
Contributor Author

Revision pushed (9f799b8): conflict detection moved into the delivery loops as described above, the store's replay path restored to its intended idempotent semantics, the sender-shape validation kept, and the checked-in project registry I/O manifest regenerated with the official generator. The test_generated_message_id_skips_transcript_deduplication path is green again, and the three registry/census diffs from the first run no longer reproduce locally.

@AronSwan

AronSwan commented Oct 4, 2026

Copy link
Copy Markdown
Contributor Author

Second CI round triage: of the previously-failing five tests that were ours, four are now fixed by 9f799b8 (dedup semantics restored + registries regenerated). The remaining diff vs main's run is a single test — test_effect_runtime_integration.py::test_retry_safe_typed_write_recovers_after_unexpected_runtime_exit — which passes on our side on both a clean upstream tree and the patched tree (Windows, direct single-run and full-file). It is process/timing-sensitive by construction ("recovers after unexpected runtime exit"), so we read it as a shard/environment flake rather than a patch regression, but we'll keep an eye on it in the next round and will dig in if it reproduces deterministically.

@huangruiteng huangruiteng 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.

Reviewer: model_agent — gpt-6.1-sol (OpenAI); runtime_reported; reasoning_effort=xhigh

Exact reviewed head: 9f799b8

动机

需要把委托结论自动送回原会话的操作者,会遇到相同消息标识下新旧正文不一致、却仍显示发送成功的问题。

此前旧正文留在本地会话,新正文却发到外部并写下成功回执;现在旧正文保留,冲突明确终止,不再发送或反复重试。

实测普通注册表路径的冲突、相同正文重放、空附件兼容和私有会话完成符合预期;精确 Goal 实例路径仍在授权前置阶段失败,尚不能认定完整交付。

本轮核验限定消息回传、发送器与会话查询契约;不证明付费模型收益、真实外部账户发送或整个 Goal 已完成。

改动思路

本轮从原会话、原请求和不可变 transcript 出发。内容冲突与 transport 失败的生命周期不同:前者不能靠重新发送修复,后者应保留既有结果让配置修复后恢复。比较和发送器解析复用 shared delivery adapter,provider 验证仍由原 TS owner 判断;exact 实例的 lifetime/admission 与 legacy return writer 保持各自约束,不再把 mock 的成功等同真实绑定。

具体改动

依据维护者已接受的 review frame:https://github.com/loopx-project/loopx/issues/5462#issuecomment-5967721942,固定版本 issuecomment-5967721942。逐项映射 1:普通注册表路径通过,精确实例路径未满足真实集成资格;2:普通路径支持 attempt-only object、私有无发送器、外部无效发送器的 retry_pending,精确路径等待同一项依赖;3:保留 goal.<goal_id> 默认,list_sessions 用于显式发现,没有增加跨频道 fallback。以上标准来自改动前维护者决议,不用新增测试的当前输出自证规则。

关键代码讲解

_replayed_return_payload_conflict 比较六项语义载荷,把缺失/None 附件统一为空列表,时间戳不参与内容身份。两个 drain 分支复用比较函数,但各自保留既有 admission/settlement owner;同一个消息 ID 的旧正文不会被覆盖。_resolve_delivery_sender 先识别 send_with_attempt,再识别普通 callable,外部无效发送器写 return_transport_unavailable,不捏造 provider receipt。ChatSessionStore.latest_session 等查询只补充既有显式频道契约文档,append_message 的通用重放语义未改。

Diff 是 5 文件 +628/-9:主要行为在 roundtrip.py,store 是文档说明,两个 pytest 文件提供边界压力,IO census 是生成更新。未来重构检查认可复用比较/发送器 helper;精确和 legacy 的 settlement 约束不同,暂不抽成参数庞大的通用 writer。没有新 capability、配置开关或第二份消息数据库。

对主干的风险

[P2] 精确路径的集成资格仍缺失。 新增 test_exact_loop_payload_conflict_sets_terminal_state_without_retry 在 tests/test_manager_context_roundtrip.py:1199 mock 掉 _exact_return_context 和 _write_exact_return_state,因此通过不能证明原授权、Goal 实例 admission、持久 settlement 可用。真实运行 tests/test_collaboration_goal_instance.py 得到 46 failed / 17 passed;独立 source_session_v1 探针在 deliver 就收到 context recipient is not authorized or registered。基线同样拒绝,所以我没有把它归为本 PR 新引入的运行回归,也不要求本 PR 再造授权 owner;但这不能作为已修改精确路径的完整业务验收。最小修复:在已通过独立审核的精确 Goal 实例授权修复基线上重放本改动,增加真实 source_session_v1 的冲突/同正文/无效发送器测试,避免 mock 掉 admission、原授权和持久 settlement。

普通路径的 92 项测试通过;独立相同输入比较证实:基线冲突会外部发送新正文并保留旧正文、标 delivered;head 则 explicit_unverified / manager_return_payload_conflict,发送零次,重启第二次处理零次。同正文、空附件兼容、私有 transcript-only 没有误拒绝。无效发送器从笼统错误变为明确 typed error,结果仍保留可恢复。完整 canary 技术检查通过,未等候或读取 CI;canary scope 含不属于 PR 的未跟踪 uv.lock,没有提交它。首次两条 pytest 命令因误选不存在文件而未收集,已用真实文件清单修正,失败记录保留。

语义与 CI 对齐

新增两个错误扩展既有 DELIVERY_ERRORS,复用 explicit_unverified / retry_pending 状态;development advisory 发现此扩展,full-tree semantic/census canary 通过。普通成功与旧重放契约有同输入证据;精确路径仍未验证,必须在真实源实例 backend 上跑上述两个测试文件,并补充冲突和发送器恢复用例,才能把局部收益认定为完整交付。

我的整体评价

REQUEST_CHANGES。方向有正向价值:普通回传避免会话与外部内容分叉,冲突终止后不浪费后续轮次,用户也能读到明确失败;user_experience=improved。long_horizon=not_yet_proven:精确 Goal 实例的真实续接还没通过,mock 不能替代它。代码量有对应验收目的,未发现需要另建控制面框架的理由;当前阻塞是同一改动的真实入口与依赖资格。无需扩大到付费模型长程实验,先在可用 authority 基线上补齐这些有界正例、拒绝与恢复。

English verdict: REQUEST_CHANGES — the legacy/File-store delta is useful and verified: conflicting payloads stop without sending, same-payload retries remain idempotent, empty attachments normalize correctly, and invalid external senders expose a typed recoverable error. However, the touched exact-instance branch is only tested with admission/context and settlement mocked. The real source-session suite at this head has 46 failures before the new guard; the immutable base has the same setup rejection. This is an existing integration dependency, not a new authority regression. Rebase/replay on an independently qualified exact-authority fix and add unmocked source-session conflict, identical retry and invalid-sender recovery tests. All selected canary technical checks passed; CI was not queried. Do not claim full shared-delivery qualification yet.

@AronSwan

AronSwan commented Oct 4, 2026

Copy link
Copy Markdown
Contributor Author

Thank you for the thorough review — and for the pre-frozen review frame, which made the bar unambiguous.

To confirm our read of the blocking item: the exact-instance branch in this PR is only exercised with admission/context/settlement mocked, and the real source-session suite at this head fails before our guard (46 failures on the immutable base as well — we independently saw the same population in our CI triage: test_collaboration_goal_instance fails identically on pristine upstream main), so it is an existing integration dependency rather than something this diff introduced. We agree the right path is to rebase/replay onto an independently qualified exact-authority fix and add unmocked source-session cases (conflict, identical retry, invalid-sender recovery), and we won't claim full shared-delivery qualification until then.

Two questions so we plan correctly:

  1. Is there an in-flight PR/branch we should rebase onto for the exact-authority integration dependency, or should we wait for it to land on main first?
  2. For the unmocked source-session tests, do you want them added to this PR now (marked as depending on that fix), or in a follow-up after the authority fix lands?

We'll hold this branch until the dependency question is settled either way.

This branch has not been deployed

No deployments
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.

2 participants