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.
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?