Repository navigation
fix(desktop): clarify delivery states and recover failed messages - #5058
Conversation
jackeyfaker77
changed the title
hqhq1025
left a comment
hqhq1025
left a comment
There was a problem hiding this comment.
Technical NO-GO on exact head 24219c259697474a9092948d605b229bee45173f: two current-head P2 regressions remain in failed-message recovery and Stop retraction persistence. No other P0-P3 findings were identified in the reviewed scope.\n\nValidation completed: build:test; UI 420/420; Desktop 2367/2367; focused local-message tests 37/37; renderer architecture 101/101; E2E budget 38/38; recovery Electron E2E 4/4 under Xvfb; Desktop typecheck; Biome; repository format check; ASF headers; git diff --check. Hosted test and label checks are green, and the head cleanly merges with current main 9cb5cc93c03f8d07550e739164d5e33168892563.\n\n> Automated review notice: This comment was posted by an automated review agent operated by hqhq1025. It is not an independent human review and does not replace one.
| const prefix = skillTokens.length ? `${skillTokens.join(' ')} ` : ''; | ||
| return { | ||
| messageId, | ||
| text: prefix + command.content.text, |
There was a problem hiding this comment.
[P2] Preserve one-shot orchestration when restoring a failed draft\n\nreadFailedMessage() reconstructs the editable content but drops command.turnOrchestration. For a normal /swarm audit repository send, the production composer removes /swarm before enqueueing, stores text = "audit repository", and records { mode: "swarm", source: "slash_command" } separately. If that intent definitively fails, Edit therefore restores only the task tail; pressing Send follows the current/default mode and silently changes the requested execution. A probe through the compiled DesktopSessionLocalStore and this service reproduced the lost field on this head. Please preserve or reconstruct the one-shot /swarm and /graph intent and add a regression through the actual slash-command send/edit/resend path.
There was a problem hiding this comment.
Fixed in 70f5c05. Failed-draft recovery now reconstructs the original /swarm or /graph command before skill tokens and rebases inline references accordingly. Added regression coverage through the actual composer send/edit/resend path to verify that the original orchestration mode is preserved.
| }, [sessionId, services]); | ||
|
|
||
| useEffect(() => { | ||
| if (published.current.sessionId !== sessionId) published.current = { sessionId, ids: new Set() }; |
There was a problem hiding this comment.
[P2] Keep Stop retractions durable across navigation and restart\n\nThis suppression set is reset whenever the session changes and is recreated on renderer restart. A Host message_admission: retracted event only removes the renderer-owned transient row; the accepted outbox record remains in SQLite until a canonical transcript observes it. Returning to the session or restarting while the Host is offline makes listMessages() return that record again, so this branch calls publish and resurrects the cancelled follow-up as “Delivered for a later reply”. The cancellation proof query cannot repair it while disconnected (and intentionally leaves presentation intact on failure). Persist/retire the retraction in the local store, or retain an equivalent durable tombstone, and cover Stop followed by session switch/restart with the Host unavailable.
There was a problem hiding this comment.
Fixed in 70f5c05, with cross-Host-restart handling added in b9dd787. Confirmed cancellations now retire the local SQLite outbox records instead of relying only on renderer suppression. Durable Host cancellation proofs can also reconcile records from an earlier Host epoch while preserving authority and session boundaries.
Added coverage for offline recovery, a real Host restart with missed local cleanup, and late submit acknowledgements.
hqhq1025
left a comment
hqhq1025
left a comment
There was a problem hiding this comment.
One additional current-head P3 found while reconciling the attachment recovery path. The two P2 findings in review 5147194929 remain the readiness blockers.\n\n> Automated review notice: This comment was posted by an automated review agent operated by hqhq1025. It is not an independent human review and does not replace one.
| const file = new File([new Uint8Array(item.content).buffer], item.name, { type: item.mimeType }); | ||
| return { | ||
| stagingKey: crypto.randomUUID(), displayName: item.name, mimeType: item.mimeType, | ||
| kind: attachmentKindFromMimeType(item.mimeType), size: file.size, source: { type: 'file', file }, |
There was a problem hiding this comment.
[P3] Preserve filename-based attachment classification on recovery\n\nThe normal staging paths call attachmentKindFromMimeType(mimeType, file.name), but recovered staged bytes omit item.name. That changes reachable attachment kinds after Edit: for example, text/plain + handler.ts restores as other instead of code, and application/octet-stream + report.docx restores as other instead of doc, so the composer and resent message use the generic attachment presentation. The new E2E uses recovery.txt and only checks the name, so it cannot catch this. Pass item.name here and add an extension-driven recovery case.
There was a problem hiding this comment.
Fixed in 70f5c05. Recovery now passes item.name to attachmentKindFromMimeType, preserving extension-based attachment classification.
github-actions
Bot
added
effort/XL
hqhq1025
left a comment
hqhq1025
left a comment
There was a problem hiding this comment.
Technical NO-GO on exact head 2a84181144e1f34d449c92161bd3d73e1a170a9b: the prior orchestration-recovery and attachment-classification findings are fixed, and the previous cancellation presentation issue no longer republishes accepted queue rows. One P2 remains in the durable Stop cleanup across a Host restart.\n\nValidation completed: build:test; UI 432/432; Desktop 2453/2453; focused changed-path tests 151/151; renderer architecture 112/112; recovery Electron E2E 6/6 under Xvfb; Desktop and UI typecheck; Biome lint/format; ASF headers; E2E budget; git diff --check. Hosted test is green. The head is currently conflicting with main 4cd71eaed26dbf296f1142db3296b27136d7d131 in three files, so it also needs a freshness update before merge.\n\n> Automated review notice: This comment was posted by an automated review agent operated by hqhq1025. It is not an independent human review and does not replace one.
| }; | ||
| const retireRetractedMessages = (sessionId: string, messageIds: readonly string[]) => { | ||
| if (target.access === 'owner' && isTargetActive()) { | ||
| deps.retireRetractedMessages?.(scope, client.hostEpoch, sessionId, messageIds); |
There was a problem hiding this comment.
[P2] Retire a stopped queue entry across Host epochs\n\nAfter a Host restart, an accepted local outbox row still carries the epoch that originally admitted it, but this Stop callback always supplies the current client.hostEpoch. DesktopSessionLocalStore.retireRetractedMessages() then rejects the deletion when those epochs differ. A probe through the compiled store/service and the registered production sessions:stop IPC retracted 256 old-epoch queue entries successfully under a new Host epoch; all 256 remained accepted after reopening the database, and the next send failed with Local message storage is full. The observer-side durable query is asynchronous and cannot make the successful Stop transaction reliable; the new E2E also invokes queryCancelledMessages() manually rather than exercising this normal Stop path. Please retire the exact message identities returned by a successful retraction without comparing them to the current Host epoch (while keeping the active-target fence), and add a regression for an old-epoch accepted row stopped after Host restart.
There was a problem hiding this comment.
Fixed the remaining cross-epoch Stop cleanup issue in 5846f4b27.
The normal sessions:stop path now durably retires exactly the message IDs confirmed as retracted, without comparing their original dispatch epoch with the current Host epoch. The owner/active-target fence and authority/session boundaries remain intact.
Added regression coverage through the registered production IPC with a full local outbox containing old-epoch records. It verifies cleanup before Stop returns, persistence after reopening SQLite, preservation of unrelated records, and a successful subsequent send. This does not manually invoke queryCancelledMessages() or depend on observer events. Rejected Stop and inactive-target cases are also covered.
The subsequent pushes additionally:
- Synced with upstream
5263fb78aand resolved the conflicts. Recovery orchestration now stays within the conversation feature boundary. - Hardened draft recovery against newer drafts and pending Session references, and prevented local rows from reappearing after Host queue handoff.
- Corrected started-Turn placement, disabled recovery actions, and visible feedback with narrow-pane wrapping.
- Updated E2E synchronization for actual Host readiness and persisted reply content, without increasing timeouts, adding retries, or skipping tests.
Current head: 9238cf06a. The complete CI run is green, including strict architecture checks, affected workspace tests, desktop E2E, browser/WorkHub smoke, Storybook smoke, and transcript geometry checks. GitHub also reports no merge conflicts.
2a84181 to
872942d
Compare
hqhq1025
left a comment
hqhq1025
left a comment
There was a problem hiding this comment.
Technical NO-GO on exact head 9238cf06abae35d2803480a14bc56806c66ca12b: two P2 findings remain. The inline finding covers a reachable failed-draft recovery race that can overwrite a newer attachment-only draft and merge its context with the failed message. Separately, the PR description does not state whether generative tooling contributed or name the tool, although the first five material commits carry Generated-by: OpenAI Codex; CONTRIBUTING.md:36 requires that declaration in each pull request. Please update the PR description.\n\nThe prior old-Host-epoch Stop cleanup defect is fixed: successful queue retraction and Turn interruption now retire exact dispatched message identities without requiring the current Host epoch, while authority/session boundaries and never-dispatched records remain protected. The production candidate/IPC/store path and offline restart regression pass.\n\nValidation completed on this head: clean install; build:test; Desktop 2657/2657; UI 620/620; focused recovery/observer/IPC tests 201/201; recovery Electron E2E 6/6 under Xvfb; full typecheck; lint; format; ASF headers; renderer architecture 112/112; E2E budget 38; git diff --check. Hosted test is green. The PR is 14 commits ahead and 62 behind current main 8d5a3cac3504c36cb1849b060153b58188e2c7b2, with merge conflicts in seven Desktop files, so it also needs rebasing and re-review before merge. Native Windows/macOS recovery was not exercised.\n\n> Automated review notice: This comment was posted by an automated review agent operated by hqhq1025. It is not an independent human review and does not replace one.
| // Read them at invocation time, including the second pre-restore check. | ||
| const composer = deps.composerRef.current; | ||
| return Boolean(deps.sessionId && deps.enabled && composer && !composer.getText() && | ||
| !deps.hasPendingContext && deps.pendingQuotes.length === 0); |
There was a problem hiding this comment.
[P2] Make this pending-context check read live state. hasPendingContext is a boolean captured by the last render. If a file selection finishes after the first guard but before readFailedMessage() resolves, setPendingState() can be queued without a render; this second guard still reads false, and restoreDraft() overwrites the composer text and appends the failed message context to the newly selected file. On this exact head, a hook-level probe using the production useComposerAttachments and recovery helper staged new.txt and resolved the failed draft in one React batch; canRestoreDraft() returned true, the text became the old failed draft, and pending attachments became [new.txt, old.txt]. The new race tests cover same-tick Session references via a live ref and typed text, but not attachment staging. Please expose a live pending-context getter/ref and add this regression.
hqhq1025
left a comment
hqhq1025
left a comment
There was a problem hiding this comment.
Technical NO-GO on exact head 98a720c668784a5b7fbc08c14dd7bc6436acbdd6: two P1 and two P2 findings remain. The previous same-batch attachment/directory recovery race and the generative-tooling disclosure gap are fixed, and the old cross-Host Stop cleanup regression remains fixed.
Validation completed on this head: clean install; build:test; Desktop 2776/2776; UI 629/629; focused recovery/observer/IPC tests 211/211; recovery Electron E2E 6/6 under Xvfb; full typecheck; lint; format; ASF headers; renderer architecture 112/112; E2E budget; git diff --check; and a clean merge tree against current main 714b0b687fedf931e885cf4e681dc3b101471289. Storybook built successfully, but the required hosted test job and an isolated local smoke both fail the same prompt-sent-before-turn-lands assertion. The PR is 15 commits ahead and 1 behind main.
I did not test native Windows/macOS recovery or a packaged production build.
Automated review notice: This comment was posted by an automated review agent operated by hqhq1025. It is not an independent human review and does not replace one.
| // Projection disappearance alone is not durable cancellation proof. | ||
| // Another client may have stopped the Turn: confirm with the Host first. | ||
| if (target.access !== 'owner' || !isTargetActive()) return; | ||
| void client.queryMessages({ sessionId, messageIds }) |
There was a problem hiding this comment.
[P2] Do not discard a positive not_admitted execution result
The observer maps both cancelled and not_admitted to a retraction, but this callback then re-queries queryMessages(), which deliberately returns only cancellation tombstones. A not_admitted result therefore becomes an empty ID list and the local accepted row is never retired or settled. A real store/service probe through the observer path produced one execution query and one cancellation query, then still had durableState: accepted with its staged bytes intact. Since accepted rows are skipped by the delivery scheduler, this record can survive restart and later reappear without converging. Preserve the resolution kind in this callback and settle not_admitted explicitly (or retire it under an equivalent durable contract), with a regression that checks the SQLite row after observation reconciliation.
hqhq1025
left a comment
hqhq1025
left a comment
There was a problem hiding this comment.
Reviewed exact head b8f653be2188b7c66217b998943e8509182c0da4 (54 files, +4,154/-261), focusing on the changes since the previous review and the four reported failure paths. I found no substantiated new P0–P3 issue in that scope. This is a COMMENTED review, not merge approval.
The recovery bridge now returns scoped, one-shot Main-owned attachment approvals rather than raw bytes to the renderer (session-local-attachment-recovery.ts, session-local-draft.ts). The message CSS now keeps actionable delivery feedback visible when it is a sibling of metadata. The local-message presenter retires IDs missing from a durable snapshot, including cancellation without a Turn. Positive Host not_admitted proof now marks the exact dispatched local record failed while retaining its staged content, rather than querying only cancellation tombstones. The final commit moves failed-message alignment geometry from Electron E2E to the existing Storybook play and keeps the cross-process recovery journey.
The PR diff is whitespace-clean and the merge tree against current main is clean. The current-head test check was still in progress at publication, so I cannot call the gates green or the PR ready to merge. I did not run local tests or packaged Desktop/native Windows/macOS recovery on this checkout; those remain validation gaps.
Automated review notice: This comment was posted by an automated review agent operated by hqhq1025. It is not an independent human review and does not replace one.
|
Thanks for the follow-up review, @hqhq1025. The CI run for the same reviewed head, For completeness, here is a consolidated summary of the changes addressing the earlier findings:
The follow-up also completes recovery attachment cleanup: abandoned recovery results and removed attachments release their unused approvals, while attachments already held by an in-flight send are protected from explicit cleanup. The existing live draft/context guards remain in place. For the final E2E failure, the fixture now matches ordinary composer sending: The retained tradeoff is bounded, time-limited Main-owned recovery snapshots. Recovered images remain metadata-only in the composer; in-flight holds do not bypass expiry, revocation, or scope checks. A The linked Linux CI run is green. This does not establish complete native Windows/macOS or packaged Desktop recovery validation. 中文对照感谢你的后续审查，@hqhq1025。你本次审查的同一个提交 为方便集中查看，下面汇总针对此前审查意见的修改：
后续还补齐了恢复附件的清理：恢复结果被放弃、附件被移除时，会准确释放不再使用的审批；已经被在途发送持有的附件，则不会因为显式清理而失效。此前防止覆盖新草稿的实时文本和上下文检查继续保留。 最后一次 E2E 失败修复的是测试准备数据与普通发送语义不一致：现在使用 保留的取舍： 继续使用有容量上限和有效期的 Main 内恢复快照。恢复的图片在输入框中只展示元数据，不提供基于历史附件字节的缩略图；在途发送保护不会绕过有效期、撤销或作用域检查。对于 链接中的 Linux CI 已通过；这不代表原生 Windows/macOS 或打包后的 Desktop 恢复流程已经完成全面验证。 |
github-actions
Bot
added
effort/XXL
|
Addressed the review findings and synced with upstream.
|
hqhq1025
left a comment
hqhq1025
left a comment
There was a problem hiding this comment.
Reviewed exact head 96741deb3549935c414ff53b77878ba49fbd0f6f. No substantiated P0–P3 finding in the checked change.
This update merges current upstream queue behavior into the failed-message recovery path and refreshes the UI surface inventory. I checked the merge resolution in the composer, queue, attachment and draft ownership paths, plus the four findings from the earlier 98a720c6 review. Attachment recovery returns Main-owned approval IDs rather than bytes (apps/desktop/src/shared/session-local-contract.d.ts:55-63); the delivery label now keeps the metadata visible (apps/desktop/src/renderer/styles/chat-message.css:319); disappearing local rows are retired (apps/desktop/src/renderer/features/conversation/controller/session-local-messages.tsx:88-94); and a positive not_admitted proof retains a failed, editable copy (apps/desktop/src/main/runtime-host-desktop-candidate.ts:683-686, apps/desktop/src/main/session-local-store.ts:374-395). The new keyed-draft/reference test covers session switching, offsets and clearing (packages/ui/src/__tests__/composer-draft-references.test.tsx:28-68).
The exact-head hosted test check passed. The diff check and merge-tree against current main 0fd75408 are clean. I did not rerun the local suites, packaged Electron, or native Windows/macOS recovery and file-picker behavior; those remain validation gaps, not passing claims. This is a COMMENTED review, not a merge approval.
Automated review notice: This comment was posted by an automated review agent operated by hqhq1025. It is not an independent human review and does not replace one.
Astro-Han
left a comment
Astro-Han
left a comment
There was a problem hiding this comment.
Independent second review of head 96741deb (a different model lineage from the parallel review). No P0–P2 found.
Earlier findings, all fixed on this head:
- Attachment ownership: attachment bytes stay in main, and the renderer only holds opaque, single-use, session-bound approval ids.
- Delivery states: the delivery label is visible again, and a stale row whose durable record is gone is removed.
not_admittedmessages: these stay failed and editable.- Carried-over fixes that still hold:
/swarmand/graphare preserved on restore, Stop works after a Host restart, attachment types are derived from the extension, and recovery reads the composer's live state.
The delivery state machine, duplicate-send protection, session isolation, IPC checks, locale coverage (en, zh-CN and zh-TW complete) and the interaction with the upstream queue lifecycle all look correct. There is no schema change.
P3 (non-blocking; neither is a regression, since on base Edit lost staged attachments immediately):
- Editing an unsent message can lose it after a restart or 30 minutes (
apps/desktop/src/main/session-local-service.ts:243-259,cancelUnsentToDraft).- Edit deletes the persisted row and its attachment bytes immediately. From then on the bytes live only in memory with a 30-minute TTL, and the session draft text is also memory-only.
- Scenario: while offline, a user clicks Edit on a waiting message to fix a typo, then doesn't resend within 30 minutes or the app restarts. The text and attachments are lost. After a TTL expiry alone, the attachment still shows in the composer but sending fails with
source_expired. - Consider keeping the durable row until the draft is actually resent or discarded.
- A refused restore after Edit discards the message silently (
apps/desktop/src/renderer/features/conversation/controller/session-local-messages.tsx:154-167withuse-session-message-queue.ts:91).- The row is deleted before the draft restore. If the composer isn't mounted (the user navigated away during the round trip), the restore returns
false, and the code releases the attachments and returns normally with no error shown. - This is low likelihood, but the loss is silent.
- The row is deleted before the draft restore. If the composer isn't mounted (the user navigated away during the round trip), the restore returns
Tests run: the 14 touched Desktop test files (315/315) and the 3 UI test files (16/16) pass after npm ci and build:test.
Not verified: Electron E2E, Storybook smoke, the full suites, typecheck/lint, and packaged or native builds. Those rely on the green hosted CI.
Automated review (Claude lineage) by the Qronos review line on behalf of @Astro-Han; please verify before acting.
me2seeks
left a comment
me2seeks
left a comment
There was a problem hiding this comment.
Automated review notice: This comment was posted by an automated review agent operated by me2seeks make. It is not an independent human review and does not replace one.
Summary
Clarifies message delivery states and adds recovery for failed messages: the transient projection carries deliveryTone/deliveryDiagnostic (preserved across updates only when an update doesn't explicitly overwrite them — the conditional spread is the right fence), stagedAttachments, and inlineReferences, and a new SessionLocalAttachmentRecovery holds main-only byte snapshots of a failed message's attachments so a retry doesn't require re-uploading. The recovery design has the correct security property, documented in code: recovery tokens CANNOT be redeemed by preview or Host-direct ingest — a renderer can only recover its own session's bytes (owner key = sender/partition/host epoch/sessionId), TTL-pruned via ATTACHMENT_APPROVAL_TTL_MS, with lease counting and explicit release tracking. The 58-file change keeps delivery authority in main (session-local-service/store) and presentation in the projection, with e2e recovery specs, queue UI state tests, and attachment recovery tests. test lane passes.
Findings
- [P3] Only the
testlane appears in the rollup on this head — for a +4,536-line change touching message delivery and attachment recovery, the full gate (typecheck, package, windows_recovery) should run green before merge. - [P3]
#prune()only runs onissue— expired entries linger between issues (bounded by map size, and read paths would re-prune on next issue), so a renderer that stops retrying leaves the snapshot for up to one TTL window. Acceptable; worth confirming the map can't grow unboundedly across many failed sends (per-issue prune bounds it per writer, not globally).
Verdict
merge-ready — correct delivery-state fencing with a properly scoped, non-redeemable recovery token; request full CI.
|
Thanks @Astro-Han for the reviews.Updated to
Merged newer upstream Validation: UI 713/713, targeted Desktop tests 146/146, and Storybook browser checks 7/7 passed. Desktop production build, typechecks, lint, formatting, and architecture checks passed. Follow-up review found no remaining blocking issue. Hosted CI is also green. Validation limits: Full Desktop tests and Electron E2E were not rerun locally. The paused-queue Storybook checks cover presentation, layout, and focus—not successful recovery clicks. Recovery behavior is covered by service and React regression tests. Scope: Unsaved edits remain in-memory; after restart, recovery restores the retained original. 中文对照已更新至
已合并更新后的上游 验证结果： UI 测试 713/713、Desktop 定向测试 146/146、Storybook 浏览器检查 7/7 通过。Desktop 生产构建、类型检查、lint、格式及架构检查通过。后续复审未发现剩余阻塞问题，远端 CI 也已通过。 验证边界： 本地未重跑完整 Desktop 测试和 Electron E2E。暂停队列的 Storybook 检查覆盖展示、布局和焦点，不代表已验证点击恢复成功；恢复行为由服务与 React 回归测试覆盖。 范围说明： 未发送的修改稿仍保存在内存中；重启后恢复的是保留的原件。 |
|
Thanks @me2seeks for the review. I checked both points against the reviewed commit
Cleanup is lazy rather than timer-driven: during complete inactivity, expired snapshots can remain resident beyond the TTL until another operation or service cleanup occurs. Expired approvals are rejected on use, and retained snapshots remain bounded by those limits. These protections were already present in |
hqhq1025
left a comment
hqhq1025
left a comment
There was a problem hiding this comment.
Reviewed commit 0d7f2b7cb180e28721448207d762608a97023968 against current main. The new change pauses a never-dispatched local message during editing, retains its durable bytes, and atomically replaces it only after the edited send is admitted. The merge also integrates the current queue presentation and its local delivery actions.
I found no substantiated P0–P3 issue in the paused-message state transitions, replacement transaction, attachment recovery, or queue presentation paths checked. The prior-head findings are not treated as current-head evidence.
Verification: Node 24 build:test; 112 focused Desktop/UI tests; git diff --check; and a clean merge-tree against main 03237142. The hosted test run for this exact head is green. I did not run packaged Electron, native Windows/macOS, or a full end-to-end restore/restart journey. GitHub still reports BLOCKED; this comment is not an approval or merge recommendation.
Automated review notice: This comment was posted by an automated review agent operated by hqhq1025. It is not an independent human review and does not replace one.
Astro-Han
left a comment
Astro-Han
left a comment
There was a problem hiding this comment.
Review at 0d7f2b7c. Our two earlier notes from 96741deb are fixed:
- An edited unsent message now keeps its paused original with the attachment bytes across restart and the 30-minute window.
- A refused restore now leaves a visible "Sending paused" row with Continue / Edit / Delete.
The Main-process delivery logic holds up. The pause is re-checked after every await in the delivery worker, and we found no double-send across Retry, Edit, Pause, Cancel, a session switch or a Host restart. The replace-on-send is a single transaction, and the rollback to the paused original is tested. There is no runtime-host protocol or schema change, and the en/zh-CN/zh-TW strings are complete. Focused Desktop tests (320/320) and UI tests (30/30) pass locally.
P2: deleting the paused original makes the edited draft unsendable.
"Delete unsent message" is offered on a paused row (session-local-messages.tsx delivery actions, remove). remove does not clear the composer's link to that original, and a failed send keeps the link on purpose (composer.tsx send failure path). session-local-store.ts then rejects every send whose original is no longer paused, with "The original message is no longer paused for editing".
Scenario: while offline, the user clicks Edit on a waiting message with an attachment, then deletes the paused original to avoid a duplicate. From then on, every send of the edit fails. The only way out is to clear the text and every attachment and quote, and by then the recovered attachment bytes are gone, because the original was deleted.
Possible fixes:
- Block Delete while an edit holds the original (Continue sending is already blocked in that state).
- Or clear the composer's link on delete.
Main should not simply ignore a missing original, because that reopens duplicates after a lost acknowledgement. There is no test for delete-then-send.
P3s:
- Edited messages move behind later ones. An edit moves the message behind later queued ones: the replacement gets a new timestamp, the outbox is ordered by timestamp, and paused rows stop holding session order. If A and B are queued offline and A is edited, the Host receives B first.
- Refused restore leaves a waiting message paused. Edit pauses the message before the second check. If that check fails (for example the user keeps typing) or the session changes, the message stays paused until Continue is clicked.
- Older builds would send paused messages.
pausedis a new stored state value, and an older build's delivery filter would send a deliberately paused original. This only matters on downgrade.
Automated review (Claude lineage, posted from the Astro-Han account). No approval implied.
hqhq1025
left a comment
hqhq1025
left a comment
There was a problem hiding this comment.
Correction to my review #5348971164 at this same head: I withdraw its "no substantiated P0–P3" conclusion. The P2 reported in review #5349054273 is supported by the code: session-local-messages.tsx offers Delete for a paused original without clearing the composer replacement link; composer.tsx retains that link after a failed send; session-local-store.ts rejects a replacement when the original is no longer paused. The edited draft is therefore stuck after Delete. I did not run a full UI reproduction. The existing P2 review has the detailed finding; this is a correction to my earlier assessment, not an additional issue.\n\n> Automated review notice: This comment was posted by an automated review agent operated by hqhq1025. It is not an independent human review and does not replace one.
|
Thanks @Astro-Han and @hqhq1025. I addressed the delete/edit issue and the downgrade risk in two separate commits:
For the remaining P3 observations:
Validation: 88 related tests passed for the delete protection; 101 passed after the downgrade fix. Compatibility probes using the actual store/service implementations from These are targeted service/React and compatibility checks, not packaged Electron or full application downgrade E2E validation. The hosted CI checks for 中文对照感谢 @Astro-Han 和 @hqhq1025。我通过两个独立提交处理了删除编辑稿的问题和降级风险：
对于剩余两个 P3：
验证方面：删除保护的相关测试 88 项通过，降级修复后 101 项通过。使用 这些验证属于定向服务、React 和兼容性检查，不代表已完成打包版 Electron 或完整应用降级的端到端验证。 |
Astro-Han
left a comment
Astro-Han
left a comment
There was a problem hiding this comment.
Re-reviewed exact head 4beb7a85eb58389f908f22cf083dea2c3f401cef against my earlier review on 0d7f2b7c.
The earlier P2 is fixed. You can no longer delete a paused original while the composer still holds content that might be linked to it:
session-local-messages.tsx:122-127refuses Delete on a paused row unlesscanRestoreDraft()holds and there are no pending references. This is the same guard Continue sending uses.use-composer-draft.ts:184-187defines when the draft can no longer link to the original.- The strict missing-original check in
session-local-store.tsis unchanged, which is right because it prevents duplicates after a lost ack.
The new tests in message-queue-ui-state.test.ts:176-243 cover:
- Delete blocked, then send, for a text+attachment edit and for an attachment-only edit.
- Delete after the draft is cleared.
With the guard disabled in the compiled output, those tests fail. With it restored, 24/24 pass.
The earlier downgrade P3 is also fixed. Paused rows are stored as failed in the SQLite state column, with paused kept in the record, so older builds skip them and the new code reads them back as paused. The migration runs once and is safe to repeat. Removing the mapping makes the new test fail. The two other P3s (queue position after an edit, and a refused restore staying paused) are intentional per your explanation, and I agree.
New P3 (inline, UX only): the Delete guard also blocks when the composer is disabled, or when the draft has unrelated text after a restart. In those cases the "clear the draft… send the draft instead" message is misleading. It errs on the safe side and no data is lost.
Checks run:
check:asf-headers: passes.check:app-shell-hooks: passes.git diff --check: clean.- Targeted Desktop tests: 184/184 and 59/59.
- Composer UI tests: 30/30.
Not run or verified:
- Electron end-to-end tests.
- Multi-window behaviour. The guard only sees the draft in the same window.
- A real downgrade to a shipped older build.
Automated review notice: This comment was posted by an automated review agent (Claude) operating on behalf of @Astro-Han. It is not an independent human review and does not replace one.
| const remove = async () => { | ||
| // An edited draft still needs this durable original for atomic replacement. | ||
| // Read live context at activation, just as resuming the original does. | ||
| if (message.state === 'paused' && (!latest.current.canRestoreDraft() || latest.current.hasPendingSessionReferences?.())) { |
There was a problem hiding this comment.
P3 (UX): this also blocks Delete when the composer is disabled (e.g. while editing a sent turn), and when the draft holds text unrelated to this message (e.g. after a restart drops the replacement link). In those cases the "clear the draft… send the draft instead" hint is misleading, because sending creates a new message rather than replacing this one. It's safe as-is. Could the guard check for an actual replacement link to this message, or could the copy be adjusted?
hqhq1025
left a comment
hqhq1025
left a comment
There was a problem hiding this comment.
Reviewed exact head 4beb7a85eb58389f908f22cf083dea2c3f401cef. I found no remaining P0-P2 in the two-commit follow-up.
The earlier delete/data-loss issue is fixed: session-local-messages.tsx:122-127 now checks the live composer state before deleting a paused original, so text+attachment and attachment-only edits keep the durable source needed for atomic replacement. The replacement ownership is cleared only when both text and pending context are gone (use-composer-draft.ts:107-113,180-185).
The downgrade auto-dispatch issue is also fixed. session-local-store.ts:69-77,123-125,275-286 stores semantic paused as legacy-visible failed in the SQLite column while retaining paused in the payload, and migrates the earlier raw encoding without deleting content or attachment rows.
One P3 UX issue remains: the Delete guard only knows whether the composer is empty/enabled, not whether it actually owns a replacement for this paused message. Unrelated draft content or a disabled composer therefore blocks Delete too, while the message says sending that draft will preserve the edit. This is conservative and does not lose data, but the action/copy is misleading outside the actual replacement case.
The new regressions discriminate the prior failures: removing the Delete guard made all three focused delete tests fail, and restoring the raw paused column write made the legacy-reader test fail. After restoring the exact head, build:test, Desktop typecheck, 93 focused Desktop/UI tests, lint, format, ASF headers, renderer architecture (112/112), E2E budget, and git diff --check passed. The hosted test check is green.
This head is not merge-ready against current main 53f566b41f8b3c540b2e8717e0c324f46e002368: git merge-tree reports content conflicts in apps/desktop/renderer-architecture.json and apps/desktop/src/renderer/app-shell.tsx. The resolved head needs re-review. I did not run packaged Electron, native Windows/macOS, multi-window behavior, or an actual older Desktop binary against the migrated database.
Automated review notice: This comment was posted by an automated review agent operated by hqhq1025. It is not an independent human review and does not replace one.
|
Thanks @Astro-Han and @hqhq1025 for re-reviewing
For the two suggested approaches:
Validation: 122 targeted tests passed after conflict resolution. After the copy change and corresponding assertion update, all 24 message-queue UI-state tests passed, retaining the blocked-delete, attachment preservation, and successful replacement checks. Desktop Main compilation, scoped lint/format checks, and These checks do not establish packaged Electron, multi-window, or actual older Desktop binary compatibility; those were not revalidated in this follow-up. Please re-review the updated head when convenient. 中文对照感谢 @Astro-Han 和 @hqh1025 对
对于提出的两种方案：
验证方面：解决冲突后，122 项定向测试通过。更新文案及对应断言后，消息队列 UI 状态测试全部 24 项通过，保留了删除被阻止、附件保留及成功替换的检查。Desktop Main 编译、涉及文件的 lint/格式检查和 这些检查不代表已验证打包版 Electron、多窗口或实际旧版 Desktop 二进制兼容性；此次后续修复未重新验证这些场景。方便时请复核更新后的提交。 |
hqhq1025
left a comment
hqhq1025
left a comment
There was a problem hiding this comment.
Reviewed exact head e9107b87feaa3e65220f058740437e4af31fafeb. I found no remaining P0-P3 issue in the current-head changes.
The merge resolution keeps current main's feature-owned submit factory while preserving this PR's replacement identity and attachment lease across ordinary, /swarm, and /graph sends (apps/desktop/src/renderer/features/conversation/controller/composer-submit.ts:157-165,268-286,320-338,346-380; apps/desktop/src/renderer/app-shell.tsx:1376-1413). The previous P3 copy issue is fixed: deletion remains conservative, but the en/zh-CN/zh-TW text now accurately states that the composer must be available and free of drafts, attachments, or references (apps/desktop/src/renderer/locales/session-local-copy.ts:28,51,71; apps/desktop/src/main/__tests__/message-queue-ui-state.test.ts:214-216). The earlier paused-original deletion and downgrade protections remain intact.
The merge regression is discriminating: temporarily removing ordinary-send replacesLocalMessageId forwarding made submit preserves replacement and attachment ownership: edited message fail before send; restoring the exact head returned the suite to green.
Validation on Node 24.18.1: clean npm ci; build:test; Desktop 2986/2986; UI 713/713; focused recovery/submit tests 98/98; Desktop typecheck; lint; format; ASF headers; Windows test inventory; renderer architecture 112/112; E2E budget (37 tests); and git diff --check. The exact-head hosted test check is green. The PR is 26 commits ahead and 0 behind current main 53f566b41f8b3c540b2e8717e0c324f46e002368, and git merge-tree is clean. GitHub still reports BLOCKED, so branch protection or other repository requirements remain outside this code review.
I did not run packaged Electron, native Windows/macOS, multi-window behavior, or an actual older Desktop binary against the migrated database.
Automated review notice: This comment was posted by an automated review agent operated by hqhq1025. It is not an independent human review and does not replace one.
Astro-Han
left a comment
Astro-Han
left a comment
There was a problem hiding this comment.
Reviewed 86f9b2dadd55dd97665e57712ba90e25466c6fe8 (75 files, +5657/−432, 30 commits). This card was scoped to shape plus gate, with prior findings and the long-standing conflict checked, and big items reported back rather than deep-dived — so that is what this is.
No P0–P3 from my own pass, and one open finding I could not reproduce.
Shape. Only 11 of the 75 files are production; the rest are tests and e2e fixtures, which is where the +5657 mostly lives. The production surface is tightly scoped to the stated subject: session-local-attachment-recovery.ts (+153, new), session-local-service.ts (+183/−37), session-local-store.ts (+81/−9), runtime-host-session-observer.ts (+29/−3), runtime-host-desktop-candidate.ts (+27), small IPC/boot edits, one e2e spec (+314) and two ledger lines.
The conflict is genuinely resolved. main is an ancestor of this head — the history carries "Merge main and preserve recovery through composer staging owner" — and mergeable is true, so the long shelving is behind it.
The previous round's clean bill still stands at a comparable point. The newest prior review (e9107b87, which is an ancestor of this head) reported no remaining P0–P3 and described the merge resolution as faithful: current main's feature-owned submit factory kept, with this PR's replacement identity and attachment lease preserved across ordinary, /swarm and /graph sends. It also recorded the earlier copy P3 as fixed, with the delete text now accurate in all three locales.
The open P2 — I read the code and could not reproduce its premise. The inline on this head says the observer maps both cancelled and not_admitted to a retraction and that "this callback then re-queries", discarding a positive result. At the callback in runtime-host-desktop-candidate.ts the two proofs are separated and not_admitted returns before any query:
// Execution resolution is already positive Host proof. Preserve its
// kind: not_admitted has no cancellation tombstone to query, and its
// unsent content must remain recoverable instead of being discarded.
if (proof === 'not_admitted') {
deps.failNotAdmittedMessages?.(scope, sessionId, messageIds);
return;
}
if (proof === 'cancelled') { retireCancelledMessages(sessionId, messageIds); return; }
// Projection disappearance alone is not durable cancellation proof …So at this layer not_admitted does not retire anything; it routes to failNotAdmittedMessages with a comment stating exactly why the content must stay recoverable. The observer does emit a retraction for both kinds — its own test asserts that — so the finding may be aimed one layer up. Either way, as written against this callback it does not hold, and since the dispatch asked me to report big items rather than settle them, I am flagging it rather than declaring it invalid: this is the one place in this PR that deserves a second opinion.
My line has an earlier P3 at session-local-messages.tsx:125 on this head as well — that the Delete guard also blocks Delete while the composer is disabled or holds an unrelated draft. It is a UX trade-off in a conservative guard, unchanged, and I am not re-grading it here.
Gate on this head: test is not finished (in_progress when I looked), so I make no CI claim; mergeable is true.
What I did not judge
- The internal correctness of the recovery path across the 11 production files; that is the deep pass this PR would need, and the dispatch deliberately reserved it.
- No Electron run; no reproduction of the recovery scenarios beyond the tests present.
I did not approve, request changes, or merge.
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 @Astro-Han for the review. Following up on the open P2 and the gate status for exact head
The conservative Delete guard remains the previously documented UX choice; the corrected three-locale hint remains in place. Could you confirm whether this evidence resolves the old 中文对照感谢这次审查。针对当前固定版本
保守的 Delete 保护仍是此前说明过的 UX 取舍，三种语言的修正文案也保持有效。 请确认这些证据是否足以将当前版本上的旧 |
|
Follow-up in
Local validation was targeted; the full local suite and packaged Electron were not rerun. The Composer probe uses linkedom and SQLite, not an Electron window. 中文对照补充提交
本地采用定向验证，没有重复运行全部本地测试或打包 Electron。Composer 探针使用 linkedom 和 SQLite，并非 Electron 窗口。 |
Astro-Han
left a comment
Astro-Han
left a comment
There was a problem hiding this comment.
Reviewed the increment 86f9b2da..5ead6ce71b8ee94a517202ba26a5bc2b87180f84 only, as scoped (4 files, +106/−12): the earlier head has been cleared by two lineages including a second opinion on its three P2s.
No P0–P3 findings, and the gate is green on this head (test: completed/success).
What the increment fixes. A replacement draft could be truncated on its first cache write, and the reason that matters is not cosmetic: sending a replacement can delete the durable original, so a shortened body would lose content the user still needed. Two things were wrong and both are addressed:
- The ownership marker was established too late.
setDraftnow recordsreplacesMessageIdbefore the first cache write, with the comment "establish ownership before the first cache write, including restores to an inactive Session whose full text is never held in the live input". Previously the marker was set after the write, so that first write was treated as an ordinary draft and hit the character bound. This is the ordering bug, and the new test pins exactly it ("the first cache write must retain the full edit"). - The bound is now exemption-aware.
rememberComposerDrafttakespreserveFullText, and the truncation is skipped when it is set — the call site passes it only for keys that are marked replacements. The comment states why: "Replacement drafts must stay lossless: sending them can delete the durable original. They still share the entry limit and whole-draft eviction below." Ordinary drafts keep the 120k character bound, and the cap's documentation was updated to say "ordinary draft" so the asymmetry is visible.
The test covers the paths that matter at 128,000 characters, for an active and an inactive draft: the first write retains the full body, an inactive draft's own value is untouched, switching sessions does not truncate the replacement, references survive, replacementMessageId is preserved, and a later append plus save still yields the full body followed by the addition.
The trade-off, stated rather than hidden. A replacement draft is now unbounded in characters; it remains bounded by the entry count and whole-draft eviction. That is deliberate — losslessness over the size bound — and is documented in the code, so I am recording it rather than objecting to it.
Gate on this head: test completed successfully; mergeable is true; no Grok involvement.
What I could not judge
- I did not run the new tests; the above is read from the diff and the test's assertions.
- No desktop run, so the interactive restore path is verified at the hook level rather than exercised.
I did not approve, request changes, or merge.
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.
Astro-Han
left a comment
Astro-Han
left a comment
There was a problem hiding this comment.
Automated review notice: This comment was posted by an automated review agent (Claude) operating on behalf of @Astro-Han. It is not an independent human review and does not replace one.
Incremental review of 5ead6ce7..867299d9. The increment is one merge of main 3597abe84, with no new author commits. No P0–P3 findings. CI test is green on this head, and the PR is mergeable with 0 commits behind main.
What changed. Every Main-process, shared-contract and packages/ui file in this PR has the same patch as at 5ead6ce7. The increment only moves the recovery wiring into the Composer owners that main introduced in #5954, #5935 and #5936.
The conflict resolution keeps main's behaviour:
- Recovery mount. Main mounted
SessionLocalMessagesand registereddraftContextRestorerinsideuseComposerSubmission. Both now come fromStagedLocalMessages, whichComposerSubmissionProvidermounts once and unconditionally (composer-submission-provider.tsx:73-76,staged-local-messages.tsx:41-47). The queue-edit restorer is therefore always installed, as on main. restoreContext. It now resolves torestoreQueuedDraftContext, which appends attachments, staged approvals, directories (keyed by host) and quotes. That covers everything main's version restored.- Provider position.
ComposerSubmissionProvidermoved insideModuleHubSkillCatalogRevisionBoundary. Every reader of its contexts still renders inside it, and the boundary is a render prop with no key, so the provider does not remount when the skill catalog revision changes. - Recovery gate. Dropping
canStageComposerContextfrom the gate changes nothing, becausecanRestoreDraftalready requires a Session id. canStageContextandallowAttachmentOnlySend. These are now derived inconversation-readers.tsxfrom the samecontextPickEnabledinput that app-shell passed before.- Replacement id.
replacesLocalMessageIdis forwarded atchat-actions.ts:315,477and throughcomposer-submit.tsfor ordinary,/swarmand/graphsends. The new test "one recovery owner restores attachment approvals and sends the linked replacement" covers Edit, then the blocked Delete, then the linked submit, end to end.
For awareness: a main test was rewritten. #5954 added the test "a cancelled local message hands its text and staged context back to the Session it left". The merge replaced it with "navigation during local editing keeps the original paused and releases unused recovery approvals". This follows the PR's existing pause-and-refuse-stale-restore design, which was accepted earlier as intentional. It is not a new regression, but please confirm with #5954's owner.
Prior findings. Nothing above P3 was open at 5ead6ce7. All earlier P1 and P2 findings stay fixed, since those files are unchanged. The Delete-guard copy P3 and the downgrade P3 stay fixed. The queue-position and refused-restore behaviours are unchanged and remain intentional.
Runtime-host protocol. Not touched, so no epoch change is needed.
Gate: test passes. GitHub reports MERGEABLE / BLOCKED. I did not run tests locally, and did not do an Electron or multi-window run. I did not approve, request changes, or merge.
|
Thanks @Astro-Han for flagging the rewritten main regression in your review. @chihumyum and maintainers, since #5954 is already merged, I would like to confirm the intended navigation behavior before treating either option as the agreed resolution. The scenario is a local, never-dispatched message: the user starts Edit in Session A, then switches to B before the recovery request completes.
Both options must retain #5058's durable paused original and atomic replacement on successful send. Refusing a restore must release only unclaimed temporary approvals, not the stored source. The paused-original Delete protection and downgrade safeguards should remain intact. My preference is to keep option 1 if this behavior difference is acceptable to maintainers, to limit additional state coordination in this PR. If preserving #5954's automatic restoration is a requirement, option 2 would be the compatibility work to do, ideally through the existing draft owner and one conditional restore operation. Could you confirm whether option 1 is acceptable, or whether #5954's navigation behavior must be retained via option 2? The test change records a real behavior choice; the earlier discussion within #5058 established intent, but I would like explicit confirmation of that choice against merged main. The regression tests should then enforce the agreed behavior and its content-preservation guarantees. 中文对照感谢 @Astro-Han 在 review 中指出 main 的回归测试被改写。 @chihumyum 及维护者，考虑到 #5954 已经合并，希望先确认导航时应保留哪种行为，再把其中一种视为已经达成一致的处理方案。 场景是尚未发送到 Host 的本地消息：用户在会话 A 点击编辑，在恢复请求完成前切换到 B。
两种方案都必须保留 #5058 的持久化暂停原件，以及发送成功时的原子替换。 拒绝恢复时只释放未接管的临时凭证，不删除保存的原始内容。暂停原件的 Delete 保护和降级兼容保护也应继续保留。 如果维护者接受这项行为差异，我倾向于保留方案一，限制本 PR 新增的状态协调复杂度。如果 #5954 的自动恢复体验必须保留，那么方案二就是需要完成的兼容工作，优先考虑复用现有草稿管理能力，并集中为一个条件恢复操作。 请确认：是否接受方案一，还是必须采用方案二保留 #5954 的导航行为？ 这次测试变化对应一项真实的行为取舍；此前在 #5058 内的讨论明确了设计意图，但希望再就它与已合并 main 的差异获得明确确认。之后的回归测试应固定双方认可的行为及内容保全保证。 |
Astro-Han
left a comment
Astro-Han
left a comment
There was a problem hiding this comment.
Automated review notice: This comment was posted by an automated review agent. It is not an independent human review and does not replace one.
At 087baf6f2c5fc69043bfe657c5a6ded7a498d477, the transcript-retry test regression is fixed. The Composer fixture receives the configured conversation services instead of shadowing them with defaults, and the test observes error feedback from both toast providers. All 11 retry tests pass locally, closing the preceding head's P2 CI regression. At the preceding head afbee248, all 11 failed with "Transcript observation not configured"; the same suite passed against the merged main commit 66da38ef.
The shutdown merge in apps/desktop/src/main/session-local-service.ts:578 preserves both catalog cancellation and recovery-approval cleanup. Its regression test confirms that closing twice aborts active catalog reads, prevents queued reads and late notifications/database writes, and rejects both old approvals and already-prepared leases. The paused original and its attachment bytes survive reopening, and recovery issues fresh approvals.
All 178 related tests pass locally, including transcript retry, local storage/recovery, attachment approval, presentation, Composer ownership and draft restoration. The dependency and Desktop test builds pass; the changed source files pass Biome.
Previously fixed recovery, paused-original deletion and downgrade issues are unchanged by this increment. The existing paused-message design is retained. There are no new P0–P3 findings in the merge and test update.
The Runtime Host protocol is unchanged and retains main's epoch 212. No epoch increment is required by these changes.
The hosted CI run for this head is still in progress: CI run. The previous head's failed run is not evidence that this head fails, and local tests do not establish that hosted CI is green. Electron, Storybook and the full local test suite were not rerun.
Astro-Han
left a comment
Astro-Han
left a comment
There was a problem hiding this comment.
Automated review notice: This comment was posted by an automated review agent. It is not an independent human review and does not replace one.
At 36a6afed15ee7021c61806303b712008a50f7e1e, the merge preserves both recovered-message context and main's revision-quote ownership. The captured submission reads and clears an explicitly named revision child's live quote bucket; ordinary sends still use their captured snapshot and remove only those captured entries. The attachment-retention interface and queued-draft restoration remain available alongside the staged-context getter.
I verified this with a mounted production quote hook: after moving quotes to the revision child and editing their annotation, the current capture returns the edited quotes and clears only the child. Substituting the preceding head's capture function reproduces a stale annotation and leaves the child's consumed quotes staged. New quotes in the original draft and quotes in another draft survive. Ordinary snapshot clearing also preserves subsequently staged quotes.
The preceding transcript-retry fixture fix remains intact, and all 11 retry tests pass. The local-message service, durable store and recovery-approval implementation are unchanged from 087baf6f; the merge retains their existing shutdown and paused-original protections. The related local tests cover recovery ownership, refusal/navigation cleanup, atomic replacement identity, sending after revision preparation, and attachment leases across asynchronous sends.
After a clean dependency installation and full test build, all 278 tests in 15 related files pass without failures or skips. Desktop type checks, strict renderer-architecture checks, the 37-test E2E budget check, locale hygiene, the 334-file surface inventory and checks on the seven integration files also pass. The E2E budget check counts tests; it does not execute them.
There are no new P0–P3 findings in this merge update. It changes no Runtime Host protocol files and retains main's epoch 217. The merge with main 6ffa2030 is conflict-free, and the CI run for this exact head completed successfully. GitHub still reports the PR as blocked despite a clean merge.
This update does not repeat a full review of the earlier feature implementation. I did not rerun Electron, Storybook, visual comparisons or the full local repository suite.
Astro-Han
left a comment
Astro-Han
left a comment
There was a problem hiding this comment.
Automated review notice: This comment was posted by an automated review agent. It is not an independent human review and does not replace one.
Reviewed range: the PR's own diff, merge-base 6ffa203 to head 36a6afe (82 files, +6024/-501). I did not review main's changes.
Verdict: I found no P0-P2 defects. There are three P3 notes below.
What I checked
- Delivery state machine (
session-local-service.ts)- The worker's
stillOwned()now treatsfailedandpausedas not owned. - Every await in
#deliver(createSession, eachingestAttachment, submit) is followed bystillOwned(). The writes that follow are synchronous:attachmentsPrepared, thenoriginHostEpochwithsending. - So
cancelUnsentToDraft(which requires nooriginHostEpoch) cannot pause a message after dispatch intent is written, and a paused message cannot be dispatched. - The delivery picker skips
paused, andresumeMessageclears the retry and probe keys. failNotAdmittedMessagesandretireCancelledMessagesonly touch dispatched identities, and they clear the retry, checking and probe keys.- I found no path that shows a message as delivered without one of these: an
acceptedrow, asendingrow, or asavedrow being actively delivered. That last rule was already on main.
- The worker's
- Atomic replacement (
session-local-store.tsenqueue)- I ran the head store standalone under Node 22's
node:sqlite(--experimental-transform-types). Results:- A paused row is stored as column
failedwith payloadpausedand reads back aspaused. - A replacement from another session is rejected.
- A same-session replacement deletes the original and its
outbox_attachmentsrows in the same transaction (via cascade). - An identical retry is idempotent.
- A later real failure reads back as
failed.
- A paused row is stored as column
- Main refuses to replace a resumed (non-paused) original. Combined with the UI rule that Continue sending needs an empty composer, I found no path where both the original and the edited draft are sent automatically.
- I ran the head store standalone under Node 22's
- Recovery attachment approvals (
session-local-attachment-recovery.ts)- Owner keys include sender, partition, host, target epoch and session.
- Leases are taken only after every item validates, and are re-checked at commit around the synchronous SQLite enqueue.
- A second prepared send that tries to reuse the same approval fails.
- Release during an in-flight send is deferred, not lost.
- Purge and close clear the entries, and
disposetolerates missing ones. - On the renderer side,
syncRecoveryApprovalsandretainAttachmentskeep tokens for every draft bucket plus in-flight sends, and release them on unmount.
- Drafts and quotes
captureComposerStagingsnapshots quotes and clears only the submitted snapshot.- Attachment clearing now matches on
stagingKey, so it removes only the exact submitted items. - Foreign-draft clearing after a send now requires the stored draft to equal the sent text.
- IPC and preload: the preload revalidates draft projections and strips
replacesLocalMessageIdfrom the Host-admission path.replacesLocalMessageIdstays client-only and is never forwarded to the Host. - Protocol and i18n: no
packages/runtime-hostprotocol or epoch changes; the epoch stays at 217. New copy exists in en, zh-CN and zh-TW, and the catalog is type-checked throughUiCatalog. - Tests: unit tests and e2e specs cover the main claims, including pause fencing of in-flight uploads, atomic replacement, approval leases and TTL, legacy-reader inertness, and not_admitted fencing.
Findings
-
P3 (no runtime impact): a migration and docs for an encoding that never shipped.
apps/desktop/src/main/session-local-store.ts:129rewrites rows whose column state ispaused.docs/desktop-message-queue.md:60-65describes a database "created by the earlier implementation".pausednever existed on main (git log -S paused origin/mainon the store and contract files is empty), so only builds of earlier iterations of this PR could have written it.- The UPDATE is a harmless no-op, but the doc paragraph and comment describe a compatibility case users cannot have. Consider dropping both.
-
P3 (design note): the replacement link is lost on restart but the edited text is not.
- The replacement identity lives only in renderer memory (
packages/ui/src/use-composer-draft.ts:82,replacementsByKey). The edited text is persisted (use-composer-draft.ts:131), and recovery attachment chips are also in-memory. - After a renderer reload or app restart, the edited draft comes back as plain text without its attachments. Sending it creates a new message, and the paused original stays paused.
- This is not an automatic duplicate: Main only replaces a paused original, and resume needs an empty composer. The copy also tells users to "Edit it again to recover it after a restart" (
session-local-copy.ts:25). - Still, a user who sends the restored text and later presses Continue sending will deliver both versions. Consider persisting the replacement id with the draft, or clearing the persisted text for replacement drafts.
- The replacement identity lives only in renderer memory (
-
P3 (unverified UX suspicion): recovery approvals can expire while their chips are still shown.
- Approvals expire after
ATTACHMENT_APPROVAL_TTL_MS(30 minutes; pruning atsession-local-attachment-recovery.ts:53). They are bound totargetEpoch(ownerKey, line 42), and leases do not stop expiry. - So a restored draft left in the composer for 30 minutes, or across a target re-activation, fails on send with
attachment_blocked/source_expiredwhile its chips still look valid. - The paused or failed original keeps its bytes, so nothing is lost. Recovery means clearing the draft and editing again, which is not obvious.
- I did not check how the composer shows this error, or whether a normal reconnect changes
targetEpoch.
- Approvals expire after
Not verified
- I did not build the app or run the unit or e2e suites locally; CI is green on this head.
- I did not exercise the renderer flows (double-click, session switch during an edit) in a real Electron window. My conclusions there come from reading the
pending/busyguard and the generation checks insession-local-messages.tsx. - I did not look further into the Host-side
not_admittedrace (proof followed by a late resubmit), beyond confirming the Host computes it under the session admission gate (message-coordinator.ts:569-606).
Astro-Han
left a comment
Astro-Han
left a comment
There was a problem hiding this comment.
Automated review notice: This comment was posted by an automated review agent. It is not an independent human review and does not replace one.
At 4c9a6914ad60932f06d545b521275284d1e15450, I found no new P0–P2 defects and one P3 feedback issue: a valid recovered attachment can make an unrelated expired file-picker approval receive the wrong recovery instructions. The inline comment identifies the condition and reproduction.
The raw-paused migration and its documentation/test have been removed. Main's store/contract history contains no such encoding. The current column-failed/payload-paused representation, reopening behavior and explicit-resume protection remain intact.
Replacement drafts now clear their persisted body instead of restoring text without its runtime-only replacement identity. With a general persistence adapter, both active and inactive edits disappear after a hook remount; substituting the previous hook restores the edited body without the link. Live navigation still preserves edits, ordinary drafts remain persistent, and a matching successful-send acknowledgement persists newer text left in an inactive draft. There is an important limit to the earlier restart concern: Desktop's actual persistence adapter only writes new-task keys. With that adapter, neither the preceding nor current hook persists an existing Session's replacement edit. I did not reproduce a shipped Desktop duplicate-send path from that premise.
Recovery approvals still expire after 30 minutes; this update adds guidance rather than renewal. Real Main IPC refuses expired sources without accepting the edited message or consuming its paused original. After reopening the file-backed store, editing the original issues fresh approvals with the original bytes, and successful replacement removes the original atomically.
The new advice works for a recovery-only expiry, but its selection is too broad for mixed batches. I reproduced this through the real chat actions and Main IPC in all three locales and all three send placements: the recovery approval is fresh and valid, the newly picked file's approval has expired, yet the toast tells the user to clear context and edit the original again. That cannot restore the newly picked file. The old feedback instead advises picking the files again. The send is refused and the draft/bytes remain intact, so this is a P3 guidance issue, not data loss.
After a fresh dependency installation and clean full test build, all 295 tests in 17 related files pass without skips. Desktop/UI type checks, checks on the eight changed TypeScript files, strict renderer architecture, locale hygiene and the 334-file surface inventory also pass. This is an incremental review of the nine-file update, not a repeat of the entire earlier feature review; I did not rerun Electron, Storybook, native restart journeys or the full local repository suite.
No Runtime Host protocol changes are introduced; epoch remains 217. This head merges cleanly with main 6ffa2030, and its CI run succeeded. GitHub still reports the PR as blocked.
Astro-Han
left a comment
Astro-Han
left a comment
There was a problem hiding this comment.
Automated review notice: This comment was posted by an automated review agent. It is not an independent human review and does not replace one.
At 6e30ec6498e980fb3ea7110d04a3daac6f9b9615, the preceding P3 mixed-source expiry guidance is fixed. skill-invocation-feedback.ts now distinguishes all-recovery batches from mixed batches instead of treating the presence of any recovery token as proof that it expired. The new mixed-source copy in English, Simplified Chinese and Traditional Chinese explains both recovery paths without claiming to know which source failed.
I reran the original nine real chat-action → Desktop-adapter → Main-IPC paths with a valid recovery approval and an expired file-picker approval. All now give mixed-source guidance. Substituting the preceding feedback module reproduces the wrong recovery-only instructions on those same nine paths.
I also checked recovery-only expiry and simultaneous expiry within mixed batches, for 27 real refusal paths in total. They retain the draft inputs, paused original, original bytes and any still-valid approval, and leave no optimistic duplicate. Refreshing only the expired source or sources then succeeds in each expiry group, with both original and newly picked file contents preserved. Direct feedback controls also confirm that pure recovery batches keep recovery guidance, pure picked-file batches keep file-selection guidance, order does not affect classification, and other error codes retain their existing messages. The new 27 repository regressions additionally exercise successful retries on every locale/send-placement combination.
There are no new P0–P3 findings in this four-file update. It does not change approval lifetimes, durable admission, replacement persistence or the underlying message recovery implementation.
After a fresh dependency installation and clean full test build, all 322 tests in 17 related files pass without skips. Desktop/UI type checks, checks on the three changed TypeScript files, strict renderer architecture, locale hygiene and the 334-file surface inventory pass. I did not rerun Electron, Storybook, native restart journeys or the full local repository suite.
The Runtime Host protocol remains unchanged at epoch 217. This head merges cleanly with main 6ffa2030, and its CI run succeeded. GitHub still reports the PR as blocked.
|
Thanks @Astro-Han for pointing out the navigation behavior introduced by #5954. Following up on the earlier two-option discussion, I've adopted option 2 in
Local validation used component/IPC/SQLite tests. Packaged Electron, real-browser focus/selection, multi-window behavior and an actual older Desktop binary were not tested in this round. The review-agent result is automated review, not human approval. 中文对照感谢指出 #5954 引入的导航行为。接着此前的两方案讨论，这次已在提交
本地验证使用组件、IPC 和 SQLite 测试。本轮未实测打包 Electron、真实浏览器焦点与选区、多窗口行为或实际旧版 Desktop。review-agent 的结果属于自动化审查，不代表人工批准。 |
Astro-Han
left a comment
Astro-Han
left a comment
There was a problem hiding this comment.
Automated review notice: This comment was posted by an automated review agent. It is not an independent human review and does not replace one.
I found no new P0–P3 issue in the delayed local-draft restoration introduced at c4dba49303011135ba1313473996e6732e5f2b86.
Leaving Session A while Edit is pending now restores the complete draft to A without replacing Session B's text or context. The restored text, attachments, directories, quotes and replacement identity remain together, including attachment-only messages and bodies exceeding the ordinary draft limit.
The request stays invalid after A is modified and then cleared, after context is added and removed, or after its Session is temporarily archived, deleted or blocked for deletion. A newer replacement draft also survives. Changes to B or unrelated Session metadata do not incorrectly invalidate A. A refused restore releases its returned temporary attachment approvals while preserving the paused original.
I connected the mounted Composer and recovery components to the real Main IPC handlers and SQLite store. Background restoration followed by sending atomically replaced the original and preserved its attachment bytes. An abandoned restoration revoked its unused approval, retained the original bytes, and allowed a fresh Edit to issue a new approval and send successfully. Disabling mutation invalidation reproduced stale-restoration failures on the same component scenarios.
A fresh dependency installation and clean test build succeeded. All 343 tests in 19 related files and 18 additional mounted-component scenarios passed without skips. Desktop/UI and story type checks, changed-file lint, renderer architecture, locale hygiene and surface inventory passed. A fresh Storybook build and all three message-queue stories completed their browser assertions. The existing mixed-source attachment-expiry and retry checks also passed.
This head merges cleanly with main a393c506; the intervening main change only adjusts a test deadline. The protocol remains unchanged at epoch 217, and the current CI run succeeded. GitHub still reports the PR as blocked. I did not run native Electron/restart journeys or the entire local test suite.
me2seeks
left a comment
me2seeks
left a comment
There was a problem hiding this comment.
Approved at me2seeks' explicit direction after reviewing exact head c4dba49303011135ba1313473996e6732e5f2b86.
Automated review disclosure: Codex inspected the implementation and performed validation; this submission records the user's explicit approval decision and does not claim an independent human code-reading pass.
Failed-message recovery retains the failed source, never-dispatched editing pauses and atomically replaces its original only on successful local admission, and uncertain delivery keeps stable identity for reconciliation. The latest background-A restoration checks all draft/context/session owners before restoring and preserves B's content/focus. Reviewed old findings have corresponding fixes: orchestration prefix restoration, durable Stop retraction across Host epochs, filename-based attachment classification, live pending-context checks, explicit not_admitted settlement, and accurate conservative-delete copy.
Validation: isolated dependency and Desktop main builds, all four Desktop typechecks, 494 tests in 22 affected Desktop test files and all 761 shared UI tests passed. Current exact-head hosted test is successful. Packaged Electron/manual multi-window acceptance was not exercised in this pass. Replacement edit changes are intentionally runtime-only; the durable paused original survives restart.
Current main has a mechanical import conflict in runtime-host-desktop-candidate.test.ts following #5394. The user authorized helping rebase while retaining original-author coauthorship, then validating the new head/CI before merge. This approval is of the reviewed implementation, not a claim that the current branch is already mergeable.
Preserve the reviewed PR apache#5058 change set while rebasing it onto current main. Retain failed-message recovery, paused-original atomic replacement, durable Stop reconciliation, and conditional restoration into unchanged background drafts. Original implementation and follow-up commits: jackeyfaker77. Rebase and import-conflict integration: me2seeks with OpenAI Codex assistance. Co-authored-by: WuHaoYang <120299996+jackeyfaker77@users.noreply.github.com> Generated-by: OpenAI Codex
c4dba49 to
02db887
Compare
me2seeks
left a comment
me2seeks
left a comment
There was a problem hiding this comment.
Re-reviewed rebased head 02db887c0f893ad096887a9c3072fb35de71ba64 at me2seeks' explicit direction to help rebase and merge.
Automated review disclosure: Codex performed the implementation review, mechanical integration and validation; this records the user's approval decision.
The 27 non-merge original commits were consolidated into their exact reviewed change set (the pre-rebase trees match), then replayed on current main 08a092ec0fb64a788320a97ee0efdd7e309a0459. The sole textual conflict was the candidate test imports; the integration retains ManagedArtifactPreview, DesktopSessionLocalStore and DesktopSessionLocalService. No reviewed feature was intentionally dropped. The commit credits original contributor jackeyfaker77 through Co-authored-by and carries Generated-by: OpenAI Codex.
New-head validation: normal clean build:test, full workspace typecheck, 517 tests across 24 affected/overlapping Desktop files, all 771 shared UI tests, all 305 fullscreen/runner/copy tests, 167 architecture-checker tests plus strict current-main architecture comparison, protocol guard (no additional protocol change; epoch 219) and diff whitespace checks passed. The original recovery, paused replacement, background-draft restore and Stop/not_admitted regressions remain green alongside the newly merged preview/fullscreen behavior. No new blocking defect found. Hosted CI is still running; merge will wait for its exact-head required checks. Physical-terminal or packaged-Electron manual acceptance was not rerun.
4 participants
Summary
Failed local messages now offer Edit and resend and Delete failed message. Editing restores the original text and attachment data into an empty composer, protects drafts created during the read, and keeps the failed copy until explicitly deleted. Unknown delivery outcomes retain the original message identity for reconciliation.
Delivery labels distinguish waiting, sending, failure, checking, live queue placement and processing. Errors and recovery actions stay with their message, and local rows share the canonical conversation column. Presentation refreshes no longer recreate queued messages retracted by Stop.
Fixes #5010
Verification
tmp/; source-scoped checks passed. The complete repository test suite was not run.Screenshots
Before — waiting to send
After — waiting to send
Failed-message recovery with attachment
Stop and subsequent send
Checklist
The second checkbox remains unchecked because the full desktop suite has the baseline failures described above.
Does this PR entail a change in behavior?
Generative tooling
OpenAI Codex contributed substantively to implementation, regression tests, and review/validation assistance for this PR.