Skip to content

fix(runtime-host): admit structured-only Messages and keep them model-visible - #4815

Open
ggbdpq wants to merge 55 commits into
apache:mainfrom
ggbdpq:fix/structured-only-message-admission
Open

fix(runtime-host): admit structured-only Messages and keep them model-visible#4815
ggbdpq wants to merge 55 commits into
apache:mainfrom
ggbdpq:fix/structured-only-message-admission

Conversation

@ggbdpq

@ggbdpq ggbdpq commented Sep 4, 2026

Copy link
Copy Markdown
Contributor

Summary

Fixes #4804. Quote-only and attachment-only turns carried real model-facing context but were rejected twice on the way to the model, both times by a text-only admission assumption:

  1. Host admissiondecodeMessageAdmissionContent threw Invalid Message text for empty inline text unless skill ids were present, so a staged quote or a lone attachment could not be submitted.
  2. Replay visibilityruntimeEventHasModelVisibleContent counted only inline text length, so a persisted user event with empty text and a non-empty quotes array was dropped as blocking-unsupported, producing the durable current-run projection is not replayable / empty_text_skipped failure the issue records.

Now decodeMessageAdmissionContent applies the rule itself after structural decode — empty inline text is admissible when the Message carries quotes or attachments, and a Message with none of the three still throws the same invalid-frame error — and the visibility predicate counts structured user context (quotes/attachments) as model-visible. All turn/message admission call sites share the one function, and the model projection already renders quotes (formatQuoteRefs), so structured-only turns now reach the model with their content.

Host admission and replay visibility, the Desktop send guard, the shared Composer and side-chat send/steer paths, quote-only rendering without an empty bubble (metadata row preserved), recap projection of the actual quote, steering replay materializing image attachments through the same path as the original request, and the main chat shell now opting into attachment-only sends — the full #4804 surface is covered. One shared predicate (hasMeaningfulMessageContent) decides meaningful content across admission, visibility, and recap; it deliberately does NOT trim inline text, because replay visibility must stay compatible with everything admission has ever accepted — a trimmed predicate re-read stored whitespace-only events as invisible and blocked replay on them, recreating #4804's own failure. Surfaces that want the trimmed judgement (the desktop guard) trim at their own boundary. The predicate counts directoryReferences as the fourth carrier, which formatTextWithInlineRefs already renders into the model message. Review attribution: the trim finding and its ablation are Astro-Han's; the attachment-only gating analysis is me2seeks's.

Verification

Claim Command Result
New tests pass with the fix node --test --test-name-pattern="#4804" on core runtime-event + runtime-host protocol dist tests 2 tests, 2 pass
Review-fix regressions ui chat-turn 18/18, ui composer 20/20, desktop IPC boundary 7/7, quote-companion 56/56, runtime session-recap 7/7, ai-sdk-backend 234/234, core runtime-event 26/26; protocol-epoch-check --base upstream/main exit 0
Red-green stash the two production files, rebuild, rerun 2 tests, 2 fail; restored → pass
Quote-only admission at the frame level turn.message.submit with {text:'', quotes:[…]} / {text:'', attachments:[…]} admitted; {text:''} alone still Invalid Message text
No admission regression protocol.test.js (78), message-coordinator.test.js (73) all pass
Format npm run format:check exit 0

Known Windows-environment failures encountered while verifying, both reproduced identically with these production files stashed (control runs) and therefore pre-existing: hosted linked child roots share admission… (EBUSY unlink of a temp runtime.sqlite) and one EPIPE shutdown test in host-kernel. The full Runtime Host suite additionally stalls on this machine on an unrelated file; CI on Linux/macOS is the authoritative full-suite run.

Scope note (updated after the follow-up review): the send path now carries structured content end to end — the Composer send/disable/send-stop gates accept staged quotes and attachments, the Desktop send guard admits them (attachment state, ownership, and size checks unchanged), and the recap renders the actual quoted excerpt via the shared inline-ref formatter instead of a count placeholder. Quote-only messages render without an empty text bubble, and keep their metadata row (timestamp, copy, edit entry).

AI use

Implemented with ZCode (GLM-5.3-Flash): traced the issue's four named boundaries to the two blocking ones, applied the smallest contract change at each, and added red-green tests tagged with the issue number. The commit carries the Generated-by trailer.

Checklist

  • Root-cause fix at shared admission/visibility points, not per-surface patches
  • Regression tests fail on the old code (verified via stash/rebuild)
  • Test style follows each file's existing conventions, issue number in test names
  • No new dependencies; renderer display left as a stated follow-up

@github-actions github-actions Bot added the effort/S Under 100 readable lines label Sep 4, 2026

@Astro-Han Astro-Han 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 current head de6183d3172116b220f5fc7b5bbfdf1f3ebc9a59 (OPEN). One P1 below plus a hard CI blocker — this PR is not awaiting human sign-off; its own required check is red.

P1 — admission now accepts empty-text-with-quote, but the two snapshot decoders still reject it

turn.ts:463 (decodeMessageAdmissionContent) parses with allowEmptyText = true and then accepts empty inline text when a quote or attachment is present. But the two places that read these messages back were not changed: message.ts:654 (decodeMessageQueueEntrySnapshot) and session-continuity.ts:778 (steering message events), both importing decodeMessageContent from ./turn.js with the default allowEmptyText = false, which throws Invalid Message text via requireUtf8String. Concrete path: a turn.message.submit with placement: 'next_turn' carrying empty text + a quote is admitted at message.ts:334, enters the pending queue, and then breaks the entire queue snapshot frame when the host serializes it. Same story for session continuity/replay via the steering path. The PR's own new test uses exactly 'next_turn' placement but only asserts the submit frame decodes, stopping one step short of the snapshot frame. Checked and excluded: the storage layer uses the events.ts:260 decoder which only requires typeof text === 'string', so the asymmetry is confined to these two runtime-host protocol call sites — switch them to the same structured-counts-as-valid rule or extract a shared predicate.

CI is red on this head — protocol epoch not bumped

The repo's own protocol-epoch-check.mjs guard fails: protocol files changed but RUNTIME_HOST_COMPATIBILITY_EPOCH is still 112. Note for sequencing: sibling branches bumping the same number merge without git conflict (see #3313), so whoever merges last must re-bump — worth coordinating with the other protocol PRs in flight.

Checked and found sound

The admission refactor itself is behavior-preserving except for the intended widening (allowEmptyText only feeds the empty check at turn.ts:406, UTF-8/length validation untouched); the frame error string is intentionally unchanged; the widened visibility predicate and admission rule agree with each other; re-checked all 5 non-test callers of runtimeEventHasModelVisibleContent (model-history.ts:566,733, runtime-resume.ts:1106,1238, session-recap.ts:120) — all filter semantics, widening only retains more, never drops.

What I could not judge

The P1 path is derived from decoder imports and defaults, not from running the host — a submit-then-snapshot round-trip test would prove or falsify it, and that test does not exist yet. Whether the desktop UI allows empty-body submit was not checked; the protocol is the contract either way.


Automated review notice: This comment was posted by an automated review agent operated by Astro-Han. It is not an independent human review and does not replace one.

简体中文

本条结论全部来自 @Opus-Qronos-AstroHan 的审查。我自己没有读这份 diff;我核的是当前 head 有没有漂移。当前 head 是 de6183d,未关闭。一条 P1:准入放宽了但读回的两处没跟着放宽,会带倒整个快照帧;另必需检查红了,纪元号没提。修好再合。

@Astro-Han Astro-Han 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.

Follow-up to my review above, new head 44d7c17fa46080ab4858b9a581abf65d67381586 (OPEN). The previous P1 is fixed — and fixed exactly where flagged — and the required test check is green on this head. One new P2 below, on the new approach taken to the epoch question.

P2 — a compatibility declaration replaces the epoch bump; the judgment call deserves a deploy-shape answer

This version does not bump the epoch. Instead it adds protocol-compatible-changes/message-admission-quote-or-attachment-text.json, declaring epoch stays 112 on the grounds that no new frame shape is emitted and no previously valid frame is rejected. I read the guard (scripts/protocol-epoch-check.mjs): it is purely mechanical — declaration epoch must equal the head epoch, files must cover all changed protocol files, reason non-empty. It cannot validate the compatibility claim itself, so the claim's correctness is entirely human judgment — which is what needs reviewing now. The script's own comments state the trade-off direction ("a needless epoch bump costs a number, a missed one …"): it leans toward bumping. My recommendation is to bump: the costs are asymmetric — a wasted number versus two incompatible peers both believing they are compatible, an error invisible at handshake and surfacing only when some user pastes a quote into an empty message and the frame fails to decode. Graded P2 rather than P1 because I cannot evidence how often two different-version clients hang off one host in practice; if maintainers can confirm that scenario is excluded by another mechanism (e.g. client version strongly bound to host), the declaration stands and this drops to an observation. That question belongs to someone who knows the deployment shape.

What I could not judge

The host was not run and no cross-version client experiment was constructed — the P2 failure path is derived from decoder behavior differences across versions; the reasoning is held with confidence, the incidence rate without evidence.


Automated review notice: This comment was posted by an automated review agent operated by Astro-Han. It is not an independent human review and does not replace one.

简体中文

本条是上一条的后续,同样来自 @Opus-Qronos-AstroHan 的审查,针对新 head 44d7c17。上一轮的 P1 已经修掉。新增一条 P2:用兼容声明代替提纪元,这个判断偏乐观,建议提纪元,除非能确认跨版本场景不存在。修好再合。

@github-actions github-actions Bot added effort/M Under 500 readable lines and removed effort/S Under 100 readable lines labels Sep 6, 2026
@ggbdpq

ggbdpq commented Sep 7, 2026

Copy link
Copy Markdown
Contributor Author

Agreed with the bump. The asymmetry decides it: a wrong "compatible" declaration is invisible at handshake and only surfaces when a mixed-version pair actually exchanges a structured-only message, while a spare epoch number costs nothing. I'll bump the epoch 112 → 113 on this PR and replace the compatibility declaration accordingly.


中文:同意 bump。错误的兼容声明在握手时不可见,只会在新旧版本混跑时以用户可见的解码失败暴露;白花一个 epoch 号反而便宜。这个 PR 会把 epoch 提到 113 并相应调整声明文件。

ggbdpq added a commit to ggbdpq/maka that referenced this pull request Sep 7, 2026
…ache#4804)

The structured-only Message admission was declared a compatible
extension, but the declaration is the weaker side of an asymmetric bet:
a wrong "compatible" claim is invisible at handshake and only surfaces
when a mixed-version pair exchanges the new frame, while a spare epoch
number costs nothing. Upstream also moved the epoch 112 -> 123 since the
declaration was written, which invalidates it outright (the guard
requires declaration epoch == head epoch).

Bump RUNTIME_HOST_COMPATIBILITY_EPOCH 123 -> 124, record the change in
the epoch log, and drop the compatibility declaration. Follow-up to the
P2 review on apache#4815.

Generated-by: GLM-5.3-Flash (ZCode)

@jackwener jackwener left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Reviewed at b4e3f11a1. One [P2] inline, on the epoch bump rather than on the behaviour change — the behaviour review is still in progress and will follow.

Two facts about this head that bound what the review can say. The branch is CONFLICTING with main, and only one check ran on this commit — test. Other pull requests in this repository were running test, audit and label on the same day, so "green" here covers less than it usually does. Neither is a defect in the change; both are context for how much the current signals are worth.

The [P2] is filed now rather than held, because it is about the conflict resolution that is about to happen, and the mistake it warns against is only available while the conflict is still open.

简体中文

b4e3f11a1 上审查。一条 [P2] 发在行内,针对的是 epoch 抬升而不是行为改动 —— 行为面的审查仍在进行中,随后补上。

关于这个 head 的两个事实,它们限定了本次审查能说什么。 分支与 main 处于 CONFLICTING,且该 commit 上只跑了一项检查 —— test。同一天,本仓库其他 PR 跑的是 testauditlabel 三项,所以这里的「绿」覆盖的范围比通常更窄。这两点都不是本次改动的缺陷,而是「当前这些信号值多少」的背景。

这条 [P2] 现在就发、而不是压到最后,因为它针对的正是即将发生的那次冲突解决,而它所警告的那个错误,只在冲突尚未解决时才来得及避免。


Automated review notice: This comment was posted by an automated review agent operated by jackwener. It is not an independent human review and does not replace one.

Comment thread packages/runtime-host/src/protocol/index.ts Outdated

@jackwener jackwener left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

A [P1] inline, and it is of an unusual kind: nothing here is broken by the change — the change does not reach the layer that decides.

Together with the epoch note filed earlier, this is a COMMENT and not an approval. The branch is also still CONFLICTING, and only test ran on this commit where other pull requests in this repository were running three checks the same day.

The model-visibility half of the title holds, and that was checked rather than assumed: the shared visibility decision point still governs, a quote-only user event does reach the model on ordinary replay, and none of the previously hidden shapes became visible. The admission half is the one that never reaches production, and the reason the existing suite does not notice is structural — the tests exercise the decoder, and the refusal happens a layer below it.

Worth stating plainly because it changes what to do next: there is nothing to revert here. The decoder change is a correct first step that stops one hop short, and the fix is to move the rule into one place that all three layers share rather than to undo anything.

简体中文

行内一条 [P1],而且是不常见的一种:这里没有任何东西被这次改动弄坏 —— 是这次改动没有触及那个做决定的层。

连同先前发的那条 epoch 备注,本次是 COMMENT 而非批准。 分支同时仍处于 CONFLICTING,且该 commit 上只跑了 test,而同一天本仓库其他 PR 跑的是三项检查。

标题中「保持对模型可见」的那一半成立,而且是核过而非假定的:共享的可见性决定点仍然管辖一切,一条 quote-only 的用户事件在普通 replay 中确实到达模型,先前隐藏的形状没有任何一种变得可见而「接纳」那一半从未到达生产 —— 现有套件之所以没有察觉,原因是结构性的:测试驱动的是解码器,而拒绝发生在它下面一层。

有一点要说明白,因为它决定下一步该做什么:这里没有东西需要回退。 解码器的改动是正确的第一步,只是在关键的一跳前停下了;修法是把规则挪到三层共用的同一个地方,而不是撤销任何东西。


Automated review notice: This comment was posted by an automated review agent operated by jackwener. It is not an independent human review and does not replace one.

Comment thread packages/runtime-host/src/protocol/turn.ts Outdated
…-visible

A quote-only or attachment-only turn carried real model-facing context but
was rejected at the Host admission boundary ("Invalid Message text") and,
once persisted, dropped by the replay visibility predicate, which counted
only inline text length. The result was exactly apache#4804: structured-only
sends fail before the provider request, and any that persisted render as
an empty user bubble while the model never sees the quoted content.

- decodeMessageAdmissionContent now decodes the frame structurally and
  applies the text rule itself: empty inline text is admissible when the
  Message carries quotes or attachments; a Message with none of the three
  still throws the same invalid-frame error. All turn/message admission
  call sites share this function, so skill-only and structured-only
  admissions now follow one rule.
- runtimeEventHasModelVisibleContent counts a user-authored text event
  with quotes or attachments as model-visible even when the text is
  empty, so the durable event survives replay and the existing quote
  projection (formatQuoteRefs) reaches the model.

Red-green: both new tests (apache#4804-tagged) fail with the production files
stashed and pass with them restored.

Fixes apache#4804

Generated-by: GLM-5.3-Flash (ZCode)
…ire-compatible

The apache#4804 admission change touches packages/runtime-host/src/protocol/turn.ts
without changing the wire: the Host only accepts strictly more frames (an
empty-text Message that carries a quote or an attachment is admitted), emits
nothing new, and rejects nothing that was valid before. Declare it under
protocol-compatible-changes/ at epoch 112 instead of bumping the epoch, per
the apache#3313 guard's compatible-extension path; the guard passes again on the
merge result against current main.

Generated-by: GLM-5.3-Flash (ZCode)
…mission rule

Review P1 (Astro-Han): decodeMessageAdmissionContent now admits an
empty-text Message that carries a quote or an attachment, but the two
places that read those messages back — the message queue entry snapshot
(message.ts) and the durable steering echo (session-continuity.ts) — still
decoded with the default text-length rule, so one admitted next_turn entry
broke the whole queue snapshot frame at serialization. Both call sites use
the same admission decoder now, and a submit-to-snapshot round-trip test
pins the path the review named. The compatible-change declaration grows by
the two read-back files; the protocol epoch guard stays green at 112.

Generated-by: GLM-5.3-Flash (ZCode)
…ache#4804)

The structured-only Message admission was declared a compatible
extension, but the declaration is the weaker side of an asymmetric bet:
a wrong "compatible" claim is invisible at handshake and only surfaces
when a mixed-version pair exchanges the new frame, while a spare epoch
number costs nothing. Upstream also moved the epoch 112 -> 123 since the
declaration was written, which invalidates it outright (the guard
requires declaration epoch == head epoch).

Bump RUNTIME_HOST_COMPATIBILITY_EPOCH 123 -> 124, record the change in
the epoch log, and drop the compatibility declaration. Follow-up to the
P2 review on apache#4815.

Generated-by: GLM-5.3-Flash (ZCode)
@ggbdpq
ggbdpq force-pushed the fix/structured-only-message-admission branch from b4e3f11 to dcf339e Compare September 7, 2026 15:37

@Astro-Han Astro-Han 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.

Thanks for the updates. Re-reviewed dcf339e0f9ba12312790211e614a8cf46c2f3d0b: the queue/continuity decoder changes and the epoch bump to 126 (current main: 125) are correct. I resolved the superseded epoch thread.

The existing P1 at #4815 (comment) still applies: normalizeRootTurnMessageContent in packages/storage/src/agent-run-store.ts rejects every empty-text input. Root-start and queued-successor admission both reach that durable owner, so quote-only/attachment-only input still cannot form a durable Turn. I have left that thread open rather than duplicating the finding.

Please make meaningful-content validity consistent across protocol admission, durable admission and replay, while retaining each boundary's distinct trust/size checks. Verify submit → durable admission → snapshot/replay → model input; decoder-only examples cannot prove this contract. The separate empty-bubble part of #4804 remains explicitly outside this PR, so the issue should not be considered fully addressed by this slice.

AI-assisted independent source review plus primary cross-check; no new Host subprocess test is claimed. Current hosted test is green, which does not discharge the unmet admission contract.

中文 感谢更新。当前 queue/continuity 解码和 epoch 126(main 125)已正确,旧 epoch 串已解决。但已有 P1 仍在:storage 的持久准入仍无条件拒绝空正文,root start 和 queued successor 都会到达该权威。保留原串,不重复发 finding。请统一协议、持久化准入、重放对有效内容的定义,同时保留不同边界各自的信任和大小校验;验收提交→持久准入→快照/重放→模型输入。Issue 的空 bubble 部分仍在范围外,不能因本切片关闭整个问题。本轮为独立 AI 辅助源码审查与主审核对,未重跑 Host 子进程测试;CI 绿不能证明这个缺口已解决。

Completes the storage half of the structured-only message admission:
normalizeRootTurnMessageContent now uses the shared meaningful-content
predicate (text, quote, or attachment) instead of the text-length rule,
so a quote- or attachment-only Message that passes the protocol decoder
also forms a durable Turn. The compaction estimate counts the structured
envelope (a zero estimate dropped model-visible events from the
history-compact gate), and the session recap projects a carrier marker
for structured-only events instead of losing them.

Generated-by: GLM-5.3-Flash (ZCode)
Pins the apache#4804 admission contract at the durable owner:
quote-only and attachment-only root Turn inputs are admitted, and a
truly contentless input still throws the same frame error. On the
pre-fix base the quote-only and attachment-only cases fail (the
text-length rule rejected them), matching jackwener's end-to-end
reproduction on apache#4815.

Generated-by: GLM-5.3-Flash (ZCode)

@Astro-Han Astro-Han 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.

Thanks for fixing the durable admission boundary. The SQLite write/reopen and ordinary replay checks now preserve structured-only content. One P2 remains in the recap consumer: counting quotes/attachments does not preserve their meaning. I followed up in the existing review thread rather than opening a duplicate finding.

中文

持久化准入已修,写入重开与普通replay能保留内容。recap仍只有数量、没有实际含义,剩余P2已在原串跟进,没有重复开finding。

AI-assisted exact-head rereview by Codex.

@Astro-Han Astro-Han 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.

Thanks for fixing the durable-admission path. An independent pass on 54a454d1 found a remaining P1 original-scope gap before that path: the Desktop scenario in #4804 still cannot send.

With empty text and a ready quote/attachment, packages/ui/src/composer.tsx disables Send (1451–1457), returns early from sendCurrent (1267–1268), and chooses Stop while streaming (1467). Structured content is carried separately and is not converted into editor text. Even if another caller reaches AppShell's onSend, preload routes ordinary sends through session-local:submit; both that service and the direct submit handler call normalizeSessionSendCommand, which still rejects empty text without skills at permission-response-guard.ts:212, before ingestion or Host admission.

Please close the original Composer→Desktop IPC→Host→durable content chain using the existing MessageContent semantics. Do not inject dummy text to pass a text-only guard. Preserve each trust/attachment validation boundary, but let ready structured content count as content for send/stop readiness and admission. The existing recap P2 is another consumer of the same contract: use formatTextWithInlineRefs rather than the new carrier-count placeholder so the recap sees the actual quote/ref. The empty user bubble requested by the issue also remains; if this intentionally delivers only a Host slice, it should not automatically close the Desktop issue.

This is an acceptance gap, not a newly introduced Desktop regression. Production +77/-10 is not excessive; the missing evidence is a real Desktop send operation with downstream model-visible content, rather than more decoder-only cases.

中文

新确认 P1 是原始需求未闭环,不是本 PR 新增 Desktop 回归:Composer 仍禁用空正文发送、sendCurrent 提前返回,流式期间显示 Stop;即使越过 UI,默认 session-local:submit 和直接 submit 都被 Desktop guard 在 ingest/Host 前拒绝。应沿同一 MessageContent 语义修通 Composer→IPC→Host→持久化,不要注入占位正文绕过。recap 复用真实内容 formatter,删除只有数量的占位分支。空 bubble 也未交付,若只做 Host slice 就不能自动关闭 Desktop issue。体积不是主要问题,测试未覆盖原始入口才是。

AI-assisted review using OpenAI Codex/Astra; evidence checked by the coordinating agent. This is not an independent human review.

…-message-admission

# Conflicts:
#	packages/runtime-host/src/protocol/index.ts
…ly message

The recap previously rendered a count placeholder for a structured-only
message; pin the actual excerpt text so the apache#4804 acceptance (quote
content appears in the recap input) is asserted, not assumed.

Generated-by: GLM-5.3-Flash (ZCode)

@Astro-Han Astro-Han 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.

Thanks for closing the durable-admission and recap gaps. Reviewed b12ce2e6 against #4804 and the downstream consumers. The main Host → SQLite → replay/model/recap path now preserves structured-only content; I did not find a second execution authority.

Two P2 acceptance gaps remain: the shared Composer enables a side-chat operation its caller still rejects (inline), and #4804's quote-only rendering requirement remains incomplete. UserMessageBody in packages/ui/src/chat-turn.tsx:253–264 still creates ChatMessageBubble unconditionally for empty text, including the normal transient/durable quote-only paths. The PR body already acknowledges this; please finish that acceptance item or explicitly narrow the closing claim rather than closing the whole issue with it outstanding.

The branch currently conflicts with main in the compatibility epoch; both branches independently use 131. Please reconcile/rebump against current main. Current-head CI is green, but does not validate that integration. Actual renderer acceptance remains outstanding, so this is not a UI approval.

AI-assisted review with an independent reviewer pass and primary-agent source reconciliation; no new renderer test run.

中文

感谢补齐持久化准入与 recap。主 Host → SQLite → 回放/模型/recap 路径已保留结构化内容,没有发现第二执行权威。

还剩两个 P2 验收缺口:共享 Composer 已启用 side-chat 发送,但调用方仍拒绝(见行内);另外 UserMessageBody 对空文本仍无条件创建气泡,#4804 明列的纯引用渲染要求尚未完成,正文也已承认。请补齐,或明确缩小关闭 issue 的声明。

当前分支与 main 的 compatibility epoch 冲突,两边都独立用了 131,需要收敛并重新验证。当前 CI 绿色不代表冲突解决后的集成结果;尚未完成真实 renderer 验收,因此不作 UI approve。此次由 AI reviewer 独立检查并经主审对照源码复核。

Comment thread packages/ui/src/composer.tsx
…-message-admission

# Conflicts:
#	packages/runtime-host/src/protocol/index.ts
@github-actions github-actions Bot added effort/L Under 1000 readable lines and removed effort/M Under 500 readable lines labels Sep 8, 2026
Review follow-up on apache#4815. The shared Composer now enables Send with an
empty draft once a quote or attachment is staged, but two consumers still
rejected the structured-only frame on the normal user path:

- `useQuoteCompanion.send` gated on `!trimmed` before fork/Host admission,
  so "select transcript text -> Ask about selection -> empty draft -> Send"
  (and attachment-only sends) returned false before reaching the quote and
  attachment payload the send already carries. The same text-only guard in
  `steer` rejected the action while streaming. Both entries now accept an
  empty text when a quote or attachment is staged; steering passes quotes
  and attachment items through the one Message admission channel, and the
  staged quotes stay pending until the Host admits the steering Message.
- `UserMessageBody` in packages/ui created `ChatMessageBubble`
  unconditionally, so a quote-only message rendered an empty bubble on
  both the transient and durable paths. The bubble now renders only for
  non-blank text; quotes and attachments keep their existing surfaces.

The merge with current main also reconciles the compatibility epoch:
both branches had independently claimed 131, so this branch now carries
132 for the structured-only admission widening.

Generated-by: GLM-5.3-Flash (ZCode)
@ggbdpq

ggbdpq commented Sep 8, 2026

Copy link
Copy Markdown
Contributor Author

Pushed 2f68f36: the branch now merges current main and carries compatibility epoch 132 — both sides had independently claimed 131, and the merge resolution keeps both epoch-log entries.

The two P2 acceptance gaps are closed on the same push:

  • Side-chat caller: useQuoteCompanion.send and steer no longer reject an empty draft when a quote or attachment is staged — the send reaches the fork/Host admission with its quote snapshot and attachment payload, and steering passes quotes/attachments through the one Message admission channel, with the staged quotes kept pending until the Host admits the steering Message. A regression through the side-chat entry covers both the idle-send and streaming-steer paths.
  • Quote-only rendering: UserMessageBody renders ChatMessageBubble only for non-blank text, so the transient and durable quote-only paths no longer produce an empty bubble; the desktop: quote-only and attachment-only messages require inline text #4804 acceptance item is finished rather than narrowed.

@Astro-Han Astro-Han 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.

Thanks for the follow-up. At 2f68f36c8, the earlier structured-only side-chat rejection is fixed, and the quote-only bubble is now omitted. I resolved the superseded thread. One recovery-path P2 remains inline: attachment cleanup must use the same confirmed admission boundary as quote cleanup.

The core fix remains at the existing admission and replay owners; no second durable authority was found. Validation: 58 focused tests passed through a temporary exact-head source loader (storage admission 7, recap 7, quote rendering 2, companion 42). This reused installed dependencies, not a fresh workspace build or Electron visual acceptance. Current-head CI is green, but the PR currently has merge conflicts; refresh the protocol epoch against main when rebasing.

AI-assisted independent reviews and primary source/probe reconciliation.

中文

感谢更新。2f68f36c8 已修复 side-chat 拒绝纯结构化内容的问题,也去掉了 quote-only 空气泡,旧串已解决。还剩一条恢复路径 P2:附件清理应与引用一样以确认准入为边界。

核心修复仍在现有准入与重放权威,没有发现第二个持久权威。临时 exact-head 源码加载验证 58 项通过(storage 7、recap 7、引用渲染 2、companion 42);复用已安装依赖,不是全新构建或 Electron 视觉验收。CI 绿色,但当前分支存在冲突,rebase 时也需对照 main 刷新 epoch。经 AI 独立评审与主审源码/探针核对。

# Conflicts:
#	packages/runtime-host/src/protocol/index.ts
#	packages/ui/src/composer.tsx
Takes upstream's app-shell token re-figure; the structured-only
opt-in's own figure is re-measured against it in the next commit.
The architecture gate compares exactly; with the attachment-only
opt-in riding on upstream's reworked shell, the merged tree measures
13661 tokens.

Generated-by: GLM-5.3-Flash (ZCode)
The workhub-layout followup-text assertion failed with an empty
received set on this run; that spec family has prior flake art
(apache#3961/apache#4205). Re-running to separate flake from regression before
digging in.

Generated-by: GLM-5.3-Flash (ZCode)
Temporary bisection commit — will be reverted once the workhub-layout
followup failure is attributed.

Generated-by: GLM-5.3-Flash (ZCode)
The bisection is inconclusive as a mechanism — the empty-text branch
cannot execute for the workhub-layout spec's messages, whose prompts
all carry text — but the single round without the render also passed
the e2e, so the render cannot be ruled back in as safe either way.
The review-required accessibility behaviour (timestamp, copy, edit
entry on structured-only messages) is restored; the Linux-only
workhub-layout followup failure is flagged on the PR for maintainer
reproduction.

Generated-by: GLM-5.3-Flash (ZCode)
@ggbdpq

ggbdpq commented Sep 11, 2026

Copy link
Copy Markdown
Contributor Author

@Astro-Han — a Linux-only question I cannot close from Windows, and your review ran on Linux, so flagging it with everything I have:

Since landing the review follow-ups, workhub-layout.spec.ts:295 › WorkHub keeps the submitted prompt visible while its agent is still running fails deterministically on CI (2/2, identical signature) and passes locally on Windows (rebuilt tree, single test run, 12s). Signature: after the dock crash-retry, the FIRST followup (Send button) queues fine; the second (Enter) never joins — the next_turn queue stays at 1 element for the full 10s (24 × locator resolved to 1 element).

What I ruled out:

  • Not upstream: main at 89f383970 (exactly what this branch merged) ran Desktop e2e green.
  • Not the rail story: this is a different spec.
  • Bisection attempted: one CI round with the quote-only metadata render reverted → e2e passed, but that round is weak evidence (1/1) AND the mechanism does not close — the spec's user messages all carry text, so the changed empty-text branch should never execute there. I've restored the render (the accessibility fix stands) and pushed; if this run fails at the same spot again, that's 3/3 and someone with Linux eyes needs to catch it live.

The one hypothesis I could not verify from the trace (CI strips DOM snapshots): the second Enter-submit is either swallowed (sendPendingRef stuck because the first submit's onSend promise never settled on Linux) or submitted as steering. If you can run this one spec on Linux at head 3dff1392d or current, the trace at the Enter action would settle it immediately.

Everything else in the review remains addressed; the branch is otherwise green (ASF headers cleaned, architecture ledger at 13661 with the opt-in riding upstream's shell rework, epoch 143 after the ninth collision).


中文:一个 Windows 上无法复现的 Linux e2e 问题需要你帮忙——workhub-layout 的 followup 测试在 CI 上 2/2 同点失败(第二个 Enter 提交不进队列,队列停在 1 元素),本机 Windows 全新构建单跑通过,main 同 merge commit 的 e2e 也是绿的。做过一轮二分(撤 metadata 渲染 → e2e 通过),但机制讲不通(该 spec 的消息全有文字,改的分支不应执行),已恢复渲染并在此留档;若本轮 CI 再挂同一处即 3/3,需要 Linux 现场抓一次 Enter 动作的 trace。其余 review 发现全部保持已修状态。

me2seeks's review finding (3) on apache#4815: the decoder-level protocol
tests cannot observe this class of bug end to end. One execution-host
test now drives the full path with the real harness — framed-client
quote-only next_turn submit, wire queue snapshot carrying the excerpt,
Host kill, successor root recovered and run to terminal, and the
durable user message read back from the reopened stores with the quote
intact.

Generated-by: GLM-5.3-Flash (ZCode)
@ggbdpq

ggbdpq commented Sep 11, 2026

Copy link
Copy Markdown
Contributor Author

Finding (3) is now landed as 520beb81d on this branch: execution-host-queue.test.ts gains "a quote-only queued message survives the wire snapshot, the admission chain, and a Host restart" — the process-level round trip you asked to pin. It drives the real execution-host harness end to end: framed-client turn.message.submit of a quote-only next_turn Message, the wire queue snapshot carrying the excerpt, a Host kill, the successor root recovered and run to terminal, and readSessionUserMessages from the reopened stores returning the durable user message with the quote intact.

So all four findings stand addressed: (1) the opt-in is consumed and main-shell-gated with a two-direction regression, (2) the started consumption contract is pinned, (3) this round-trip test, (4) the untrimmed shared predicate with the fourth carrier. Epoch 143 against merged main; CI on 520beb81d is green.


中文:(3) 已落地——execution-host-queue 新增进程级 round-trip 测试:framed-client 提交 quote-only next_turn → wire 队列快照携带摘录 → Host 强杀 → successor root 恢复并跑到终态 → 重开存储读回的持久化用户消息带完整 quote。四条发现全部闭环,epoch 143,CI 绿。

@github-actions github-actions Bot added effort/XL Under 2500 readable lines and removed effort/L Under 1000 readable lines labels Sep 12, 2026
# Conflicts:
#	packages/runtime-host/src/protocol/index.ts
@ggbdpq

ggbdpq commented Sep 12, 2026

Copy link
Copy Markdown
Contributor Author

Pushed ca9187c (merge with current main — epoch collision resolved to 144, upstream's 143 entry kept, protocol-epoch-check 143 -> 144 exit 0) plus cec34fc closing the three remaining inline findings from the 09-11 review:

  • P1 reachability ①: the side-chat steer wrapper now runs the same preflightAttachmentItems gate as send, surfacing the localized send-rejected toast instead of failing later on the steer path.
  • P2 reachability ③: normalizeSessionSendCommand normalizes attachmentItems per item (approval+name descriptor or live file carrier, shared count cap) — [null] / [{}] no longer satisfy the empty-body check. Pinned with junk-entry cases in the boundary test.
  • P3: estimateRuntimeEventChars weights an attachment by formatAttachmentRefs output (the block the projection actually emits) instead of the ~25-char display fields.
  • The retitled send/stop test now honestly pins the feat(workhub): restore chat identity and reuse standard composer #5003 opt-in contract; the Add Runtime v2 implementation skeleton #5 recap double-gate was resolved in bd95907 (trim removed from the shared predicate), as noted earlier.

…ndings

- The side-chat steer wrapper now runs the same staged-attachment
  preflight as send, turning an unusable attachment into the localized
  send-rejected toast instead of failing later on the steer path
  (reachability P1).
- `normalizeSessionSendCommand` normalizes `attachmentItems` per item
  (approval+name, or a live file carrier) with the shared attachment
  count cap; `[null]` / `[{}]` no longer satisfy the empty-body check
  while carrying nothing ingestible (reachability P3).
- `estimateRuntimeEventChars` weights an attachment by the block the
  projection actually emits (`formatAttachmentRefs`) instead of its
  ~25-char display fields.
- The composer send/stop test is retitled to pin the apache#5003 opt-in
  contract it actually covers, per the review note that base passed it.

Generated-by: GLM-5.3-Flash (ZCode)
# Conflicts:
#	packages/runtime-host/src/protocol/index.ts
@ggbdpq

ggbdpq commented Sep 12, 2026

Copy link
Copy Markdown
Contributor Author

Pushed dacaddb: merges current main (#5164 Antigravity ACP series included) and resolves the epoch collision — upstream claimed 144 and 145, so the constant moves to 146 with both upstream entries kept and the structured-only admission entry renumbered. protocol-epoch-check --base upstream/main reports 145 -> 146, exit 0; architecture check passes; quote-companion + guard suites 64/64; format:check clean. Ready for merge.

# Conflicts:
#	packages/runtime-host/src/protocol/index.ts
@ggbdpq

ggbdpq commented Sep 12, 2026

Copy link
Copy Markdown
Contributor Author

Pushed 81f55d5: merges current main (#5219 Code Mode included) — the epoch collision repeats (upstream claimed 146 for Code Mode), resolved to 147 with upstream's entry kept and the structured-only admission entry renumbered. protocol-epoch-check reports 146 -> 147, exit 0; architecture check passes; quote-companion + guard suites 64/64; format clean. Ready for merge.

# Conflicts:
#	apps/desktop/renderer-architecture.json
#	apps/desktop/src/renderer/features/workbar/tools/side-chat/use-quote-companion.ts
Upstream's companion hook now derives `streaming`/`turnInFlight` from the
Host execution projection, so the four steer cases that need an active
turn must publish one before steering — the same `hostTurn('old-turn')`
call the upstream suite uses. No product change; the steer contract under
test is unchanged.

Generated-by: GLM-5.3-Flash (ZCode)
# Conflicts:
#	packages/runtime-host/src/protocol/index.ts
@ggbdpq

ggbdpq commented Sep 12, 2026

Copy link
Copy Markdown
Contributor Author

Pushed e741465 (plus 5b353f8): merges current main — the epoch collision repeats (upstream claimed 147 for the OAuth create-target shapes), resolved to 148 with upstream's entry kept and the structured-only admission entry renumbered; protocol-epoch-check reports 147 -> 148, exit 0. The use-quote-companion.ts conflict from #5217's execution-projection refactor resolves to this PR's steer contract on top of upstream's new model (stale preserveLiveTurn arguments dropped, matching the new signatures), and the four steer cases now publish a host turn before steering the way the upstream suite does. Architecture check passes against upstream/main, quote-companion + guard suites 66/66, ui suites 30/30, format:check clean.

…he send guard

The `test` check on e741465 went red: "sends canonical content and
uploads owned Attachment bytes through the Host" now fails with
`Invalid attachment items`. The per-item validation added for the
apache#4815 review rejected `{name, mimeType, base64}` — the shape the
preload actually puts on the wire after encoding a dragged/pasted
File (`attachment-ingest-payload.ts`) — while accepting a `{file}`
carrier that never crosses IPC and that `prepareIngestItems` would
refuse. Align `isComposerIngestItem` with the resolver's real
contract (approvalId+name, or name+base64) and pin both sides in the
boundary suite: a base64-only send counts as content (apache#4804 at this
boundary), a raw `{file}` item is junk like `[{}]`.

Generated-by: GLM-5.3-Flash (ZCode)
…ition and composer gates

runtime-host: a quote-only and an attachment-only `turn.start` now
drive the real Host subprocess and durable stores end to end — the
turn admits, runs to terminal, and the persisted user Message and
RuntimeEvent keep the structured content and pass
`runtimeEventHasModelVisibleContent`, the predicate that gates model
replay. This is the layer the decoder-level tests cannot observe:
reverting only the storage meaningful-content rule (the durable
authority) turns both new tests red, and restoring it turns them
green. The attachment-only case stages a canonical Session Artifact
first, matching the session-file references hosted Turns require.

ui: the composer's staged-quote behaviour gets its own pins — a
staged quote enables Send with no host opt-in, and the three gates
(send/stop toggle, sendDisabled, sendCurrent's content guard) agree
while streaming. The apache#5003 opt-in test now asserts `aria-disabled`
attribute values directly instead of substring patterns that matched
the tail of `aria-disabled` by accident.

Generated-by: GLM-5.3-Flash (ZCode)
… identity

`projectSessionRecapMessages` gated on `runtimeEventHasModelVisibleContent`
and then re-checked `hasMeaningfulMessageContent` on the same content with
its text trimmed. Since the recap now decides visibility through the shared
predicate, every text event that reaches the second check satisfies it
already — non-blank text survives trimming, and blank text passed via the
quotes/attachments/directoryReferences carriers, which trimming keeps.
Remove the branch and leave the push; the projection is unchanged
(session-recap 7/7).

Generated-by: GLM-5.3-Flash (ZCode)
@ggbdpq

ggbdpq commented Sep 12, 2026

Copy link
Copy Markdown
Contributor Author

Pushed three commits on top of a fresh merge of current main; head is 35747a22f.

The red test check on e741465 was our regression, now fixed (301d62f). The attachment-item validation added for the earlier review finding rejected {name, mimeType, base64} — the shape the preload actually puts on the wire after encoding a dragged/pasted File — while accepting a {file} carrier that never crosses IPC and that prepareIngestItems refuses. The guard now matches the resolver's real contract (approvalId+name, or name+base64), with both sides pinned in the IPC-boundary suite: a base64-only send counts as content (#4804 at this boundary), a raw {file} item is junk like [{}].

Composition-layer acceptance (the outstanding gate), 1c5e131. Two new turn.start tests in execution-host-queue.test.ts drive the real Host subprocess and real stores end to end: a quote-only and an attachment-only root start admit, run to terminal, and the persisted user Message and RuntimeEvent keep the structured content and pass runtimeEventHasModelVisibleContent — the predicate that gates model replay. The attachment-only case stages a canonical Session Artifact first, since hosted Turns require session-file references. Red-green: reverting only the storage meaningful-content rule (normalizeRootTurnMessageContent, the durable authority a decoder test cannot observe) turns both tests red; restoring it turns them green.

Composer send gates (the 09-11 P2), same commit. The staged-quote cases the review asked for now exist: a staged quote enables Send without allowAttachmentOnlySend, and the three gates — send/stop toggle, sendDisabled, sendCurrent's content guard — are pinned agreeing while streaming. The #5003 opt-in test asserts aria-disabled attribute values directly instead of substring patterns that matched the tail of aria-disabled by accident.

Recap ablation (the 09-11 P3), 35747a2. The branch the shared visibility predicate turned into an identity is removed — every text event reaching it satisfies it already; projection unchanged (session-recap 7/7).

Verification on this head (Windows local; CI will re-run on the pushed SHA):

Check Result
desktop runtime-host-session-execution-ipc-main (the red check) 39/39
runtime-host execution-host-queue (incl. 2 new composition tests) 16/16
Red proof: storage rule reverted → 2 new tests fail; restored → pass confirmed
runtime-host protocol / message-coordinator 84/84, 76/76
storage root-turn-admission-normalization 7/7
runtime session-recap 7/7
ui composer-send-toggle / chat-turn (answer-identity, steering-order) 9/9, 18/18, 2/2
desktop permission-response-ipc-boundary / quote-companion-retry 8/8, 47/47
protocol-epoch-check --base upstream/main 147 → 148, exit 0
format:check touched files clean; only untracked .mimosa/ local state flags, absent from CI checkouts

中文:推送三个提交(先合并当前 main),head 为 35747a22f。e741465d 上的 CI 红是我们自己的回归:为 review 加的附件项校验拒绝了 preload 编码 File 后真正上线上的 {name, mimeType, base64} 形状,却放行从不跨 IPC 的 {file} 载体;守卫已对齐下游 resolver 的真实契约,两侧形状都已在 IPC 边界套件钉住。组合层验收(剩余门槛):execution-host-queue.test.ts 新增 quote-only 与 attachment-only 两条 turn.start 用例,驱动真 Host 子进程与真实存储——准入、跑到终态、持久化的用户 Message 与 RuntimeEvent 保留结构化内容且通过 runtimeEventHasModelVisibleContent;红证:只回退 storage 的有意义内容规则(解码器测试观察不到的持久权威),两条新测试即红,恢复即绿。composer 三个发送门禁的一致性与 aria-disabled 直接断言、recap 恒真分支的消融均已落地。验证矩阵见上表;epoch 守卫 147→148 exit 0。@Astro-Han 请重审。

@Astro-Han Astro-Han 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.

Thanks for the follow-up. I rechecked 35747a22f: the previous attachment wire-shape, admission cleanup, retained-attachment, metadata, composer-gate and recap issues are fixed. The shared Host admission and replay design is appropriate; there is one remaining P2 in the new directory-reference contract, detailed inline, so I am holding the merge.

Verification: rebuilt this head; 29 UI tests, 94 Desktop IPC/side-chat tests and 3 selected real-Host tests passed. I also exercised the real Composer and TurnView in Electron in light/dark mode: quote-only, attachment-only and streaming quote sends reach the callback; the empty bubble is absent and metadata remains. This used a review harness, not a live provider run.

A small test correction would help: the queued restart test uses quotedContent(...), which supplies non-empty inline text, and only checks the restored message id. Use empty text and assert the quote payload in both the queue snapshot and reopened store to substantiate the quote-only restart claim.

AI-assisted review: Reviewer Sol inspected the Host/storage/replay contracts; the coordinating Codex agent independently reproduced the remaining issue and ran the checks above.

Comment thread packages/runtime/src/model-history.ts
…ator

Astro-Han's [P2] on apache#4815: `hasMeaningfulMessageContent` now admits a
directory-only Message, but `estimateRuntimeEventChars` still returned
zero for one, so `isHistoryCompactContentEvent` filtered it out of the
checkpoint match's successors and a replay after a matching checkpoint
silently lost its directory context from later provider requests.

Weight the directory-only envelope by what the projection actually
emits (`formatDirectoryReferences`), the same rule the attachment
estimate already follows, and pin it with a replay regression: a
directory-only successor survives `applyRuntimeEventHistoryCompact`
after a matching checkpoint. Red-green confirmed: reverting only the
estimator line turns the regression red.

Generated-by: GLM-5.3-Flash (ZCode)

@Astro-Han Astro-Han 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.

Thanks for the focused follow-up. I rereviewed 5f0984128 against 35747a22f. The directory-only compaction finding is fixed at the existing estimator by counting the shared directory-reference projection; this adds no parallel visibility rule or state.

I rebuilt Runtime and ran the checkpoint suite: 30/30 passed. As a negative control, reverting only the estimator to the previous head made the new replay test fail because the directory-only successor disappeared; restoring the fix returned the suite to 30/30. No P0–P2 findings remain from this review. The earlier P3 about the queued restart fixture remains a non-blocking test-evidence correction.

The current head CI is green, but GitHub reports conflicts with main. Those conflicts still need resolution and validation before merging; this review covers the stated head.

AI-assisted rereview and local verification with Codex.

中文

这次修改已闭环目录引用在压缩后丢失的 P2:沿原有估算器计入共享格式化结果,没有新增权威。30 项测试通过;仅回退修复后,新增回归确实因目录消息丢失而失败,恢复后重新通过。此前队列重启夹具的 P3 证据修正仍不阻塞。当前 CI 绿,但与 main 冲突,解决并验证后才能合并。

…-message-admission

# Conflicts:
#	apps/desktop/renderer-architecture.json
#	apps/desktop/src/main/__tests__/quote-companion-retry.test.ts
#	apps/desktop/src/renderer/features/workbar/ports.ts
#	apps/desktop/src/renderer/features/workbar/tools/side-chat/quote-companion-panel.tsx
#	apps/desktop/src/renderer/features/workbar/tools/side-chat/use-quote-companion.ts
#	apps/desktop/src/renderer/platform/desktop/create-workbar-services.ts
#	packages/runtime-host/src/protocol/index.ts
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

effort/XL Under 2500 readable lines

Projects

None yet

Development

Successfully merging this pull request may close these issues.

desktop: quote-only and attachment-only messages require inline text

4 participants