fix(desktop): send quote-only and attachment-only messages - #4805
Conversation
Generated-by: OpenAI Codex
c1036d8 to
0311ad1
Compare
|
Thanks for tracing this across the full Desktop admission and durable replay path. The main direct-send path now has the right semantic model: non-empty inline text is not the only form of model-visible user input. I found three remaining gaps before this is fully converged:
From an Occam razor perspective, the problem statement is already minimal: message validity must be based on model-visible content, not only authored text. The production-file breadth is justified because the message crosses several trust boundaries. The implementation is not fully converged yet, though, because Composer, IPC, protocol/storage, replay, compaction, and side chat still carry different definitions of substantive content. A canonical |
Astro-Han
left a comment
There was a problem hiding this comment.
Reviewed current head 0311ad1cfe906fc7765632f1feb54ad011e234d9 (OPEN). Two P1s plus three P3s below — and neither P1 is a defect in the code itself; both are about this PR's relationship to the repo's current state.
P1 — the green checks are stale; the epoch gate fails on current main
The CI invocation (.github/workflows/ci.yml:100) runs node scripts/protocol-epoch-check.mjs --base 'HEAD^1' on GitHub's merge ref — whose base 3fdaddb4 is an old main. Re-running the same command on the current merge ref fails with exit 1: protocol files changed (protocol/turn.ts) with no compatible-change declaration, epoch still 112 — while current origin/main is already at 113 (protocol/index.ts:104, latest dd7d1d595). So this PR now needs ≥ 114, not 113. The moment CI re-runs or the PR rebases, the check goes red; the current green cannot be relied on for merge. Note: this is the #3313 hole (sibling branches bumping the same number merge without git conflict) firing in practice, not in theory.
P1 — same change as still-open PR #4815, in the same files; the two will collide
PR #4815 (OPEN, unmerged) admits "empty text with quote or attachment" at the admission decoder decodeMessageAdmissionContent; this PR does the same thing one layer down, in the shared decodeMessageContent via hasStructuredContent relaxing requireUtf8String. Both touch protocol/turn.ts and runtime-event.ts's runtimeEventHasModelVisibleContent — textual conflict and semantic duplication. Technical comparison: this PR's placement is better — the earlier review of #4815 flagged a P1 where admission was widened but read-back was not, so an admitted structured message would break the whole queue snapshot frame; because this PR changes the shared decoder, the read-back paths (queue snapshot in message.ts, steering events in session-continuity.ts) widen automatically and that P1 does not exist here. But the two must be merged into one or one explicitly closed — not merged independently.
P3s (non-blocking)
- Three "what counts as content" predicates now disagree on
directoryReferences:runtime-event.tscounts it,turn.ts'shasStructuredContentandagent-run-store.ts'shasModelContentdo not. Currently unreachable (the composer gate only looks at quotes/attachments), but three copies of one predicate have started drifting — either all three should recognize it (if theformatTextWithInlineRefsclaim holds) orruntime-event.tsshould not. - Empty-text messages lose the bubble container (
chat-turn.tsx): content (quotes/attachments render separately) is not lost, but the user-message visual container is. Worth the author confirming it is deliberate, with one line of comment either way. - While streaming with a staged quote, Stop becomes Send (
stopShowngains!hasPendingQuote && !hasPendingAttachment). A defensible consequence of "quotes are sendable", with Esc as the documented fallback — but the comment describing Stop's disappearance should be updated to include this new case.
Checked and found sound
The asymmetric quotes.quotes === undefined check in the permission guard is safe (normalizeOptionalQuotes returns {} for empty arrays, so the two forms are equivalent here). Hoisting the two throwing validators into locals is correct (avoids double validation). Widening is conditional on structured content — bare empty text is still rejected.
What I could not judge
Desktop not run (bubble/Stop-button points are from code reading; actual look needs eyes). Whether formatTextWithInlineRefs really folds directoryReferences into provider text was not verified beyond the comment. The #4805/#4815 sequencing call belongs to orchestration/maintainers — technical comparison only, no ruling made here.
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.
|
Thanks for the detailed diagnosis, implementation, and regression coverage. Superseded by #4815 for #4804. I recommend keeping #4815 as the single integration PR and closing this PR as superseded, rather than merging two overlapping fixes. There are two useful details here that I have asked to preserve in #4815: accepting retained attachments without inline text, and retaining message metadata/actions when omitting the empty text bubble. Please preserve the original contribution attribution for any code carried over. This does not mean #4815 is ready to merge yet; its remaining correctness fixes are tracked there. Thank you for the work on this issue. |
A structured-only message (apache#4804) omitted its empty text bubble but dropped the metadata row with it, taking the timestamp, copy action, and the edit entry away from exactly the messages that carry only structured content. The empty-text branch now renders the metadata directly, matching apache#4805's rendering shape (review finding on apache#4815). The quote-only rendering regression moves into the chat-turn answer-identity suite on the shared rendering fixture and now asserts both obligations — no empty bubble plus a surviving metadata/edit entry — instead of maintaining a second DOM lifecycle. Generated-by: GLM-5.3-Flash (ZCode)
…nd body An edit that keeps an existing attachment while dropping all inline text sends the retained refs separately from attachmentItems, and normalizeSessionSendCommand refused it with "Invalid send text" before those refs were normalized. Normalize the retained attachments first and count them in the content-presence check; ownership and size validation stay downstream (review finding on apache#4815). The shape follows apache#4805's guard. Generated-by: GLM-5.3-Flash (ZCode)
|
@Jarad-z heads-up: in the review on #4815 the maintainers recommended that PR as the single integration PR for #4804, with this one superseded. #4815 now incorporates the two details this PR handled — the metadata/edit entry surviving the omitted empty bubble, and retained attachments counting as content in the Desktop send guard — with attribution to your PR in the commit messages and the review reply. Closing it is your call since it is your branch; flagging so you have the full picture, and thanks — both details made the integrated version better. 中文:#4815 被建议作为 #4804 的唯一整合 PR,本 PR 的两处优点(空气泡场景保留元信息/编辑入口、保留附件计入发送 guard)已吸收进 #4815,commit 与回复中已归因。是否关闭由你决定,特此告知,谢谢你的并行工作。 |
Summary
Maka Desktop treated
MessageContent.textas the only substantive input at several boundaries even though quotes and attachments are model-visible content. A quote-only send could be blocked in the composer or rejected during admission; after reaching the runtime, the durable replay predicate classified it as empty and failed the turn before the provider request. Attachment-only sends hit the same text-only admission assumption.This change:
RuntimeEventreplay;Fixes #4804
Verification
npm run build:with-depsfromapps/desktopnode --test packages/ui/dist/__tests__/composer-send-toggle.test.js packages/ui/dist/__tests__/chat-turn-answer-identity.test.jsnode --test apps/desktop/dist/main/__tests__/permission-response-ipc-boundary.test.jsnode --test packages/core/dist/__tests__/runtime-event.test.js packages/storage/dist/__tests__/root-turn-admission-normalization.test.jsnode --test packages/runtime-host/dist/__tests__/protocol.test.jsnode --test packages/runtime/dist/__tests__/ai-sdk-backend.test.js packages/runtime/dist/__tests__/directory-reference-model-context.test.js packages/runtime/dist/__tests__/runtime-event-read-model.test.jsnpx playwright test e2e/send-message.spec.ts --grep "reference-sized paste" --config e2e/playwright.config.tsfromapps/desktopnpm run lintnpx biome checkon the 16 changed files;git diff --checkNot run: the full Electron E2E suite. The focused user journey and all affected lower-tier suites passed.
Full-tree
npm run format:checkandnpm run check:asf-headersare not claimed: the checkout contains unrelated untracked artifacts and Windows-line-ending baseline files outside this PR that fail those whole-tree commands. All changed files pass the targeted formatter check and retain their ASF headers.Root cause
runtimeEventHasModelVisibleContentreturned false for every text event whose text field was empty. The model-history formatter separately folds quotes, attachments, and directory references into provider-visible text, so the visibility predicate and the materializer disagreed. The durable current-turn replay therefore emittedempty_text_skipped, materialized no current user message, and failed closed as a non-replayable projection. The UI and admission boundaries repeated the same text-only predicate.AI use
Select exactly one:
Tool(s) and scope: OpenAI Codex — reproduced the Desktop failures, traced the persisted
RuntimeEventand replay diagnostics, implemented the UI/admission/replay fixes, added regression coverage, ran the verification above, and drafted the issue and PR descriptions. The commit carries aGenerated-by: OpenAI Codextrailer.Checklist
Does this PR entail a change in behavior?