Skip to content

fix(desktop): clarify delivery states and recover failed messages - #5058

Open
jackeyfaker77 wants to merge 38 commits into
apache:mainfrom
jackeyfaker77:codex/5010-message-delivery-recovery
Open

jackeyfaker77 wants to merge 38 commits into
apache:mainfrom
jackeyfaker77:codex/5010-message-delivery-recovery

Conversation

@jackeyfaker77

@jackeyfaker77 jackeyfaker77 commented Sep 8, 2026 •

Copy link
Copy Markdown
Contributor

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

  • Passed: 37 focused desktop tests, 448 shared UI tests, build, desktop/UI typechecks and Knip, source-scoped lint/format, renderer architecture comparison, E2E budget and diff checks.
  • The repository recovery E2E passed, including durable attachment recovery, draft protection, resend, deletion and Stop/next-send regression coverage.
  • Additional direct Electron acceptance: 4/4 passed, with no renderer page errors. Covered duplicate clicks, task switches during recovery, deletion during a reply, unknown-outcome identity reconciliation, renderer reload and full application restart. Used a deterministic fake model backend and controlled failures; the native file picker was stubbed.
  • Independent clean-build desktop comparison: baseline 2475 passed / 24 failed / 8 skipped; candidate 2482 passed / 23 failed / 8 skipped. All 23 shared failures have matching normalized errors (Windows shell/path fixtures, symlink permissions and SQLite cleanup). Six added tests passed; the remaining fail-to-pass change limits an existing POSIX permission assertion to non-Windows platforms. These failures are outside this issue's scope.
  • That comparison preceded the final renderer Stop fix; the Stop regression was separately reproduced against baseline and candidate, then verified with the official recovery E2E and complete Electron acceptance after fixing it.
  • Root lint/format encounter an unrelated nested Biome configuration under untracked tmp/; source-scoped checks passed. The complete repository test suite was not run.

Screenshots

Before — waiting to send

pending

After — waiting to send

pending

Failed-message recovery with attachment

restored-draft

Stop and subsequent send

05-stop-and-continue

Checklist

  • Tests cover the change and fail without it
  • Lint, format, typecheck and the affected suites pass locally

The second checkbox remains unchecked because the full desktop suite has the baseline failures described above.

Does this PR entail a change in behavior?

  • Yes — described under Summary above
  • No

Generative tooling

OpenAI Codex contributed substantively to implementation, regression tests, and review/validation assistance for this PR.

@github-actions github-actions Bot added the effort/L Under 1000 readable lines label Sep 8, 2026
@jackeyfaker77
jackeyfaker77 marked this pull request as ready for review September 8, 2026 20:49
@jackeyfaker77 jackeyfaker77 changed the title fix(desktop): clarify message delivery and restore failed drafts fix(desktop): clarify delivery states and recover failed messages Sep 8, 2026

@hqhq1025 hqhq1025 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

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,

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

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() };

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

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 hqhq1025 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

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 },

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Fixed in 70f5c05. Recovery now passes item.name to attachmentKindFromMimeType, preserving extension-based attachment classification.

@hqhq1025 hqhq1025 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

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);

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

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 5263fb78a and 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.

@jackeyfaker77
jackeyfaker77 force-pushed the codex/5010-message-delivery-recovery branch from 2a84181 to 872942d Compare September 22, 2026 13:54

@hqhq1025 hqhq1025 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

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);

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

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

Merge apache/maka main at 87fc9f6, preserving ordinary-send placement and local recovery controls. Read attachment and directory state synchronously during failed-draft recovery; cover same-batch races and owner-scoped cleanup.

Generated-by: OpenAI Codex

@hqhq1025 hqhq1025 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

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.

Comment thread apps/desktop/src/shared/session-local-contract.d.ts Outdated
Comment thread packages/ui/src/chat-turn.tsx Outdated
// 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 })

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

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

Keep recovered attachment bytes in Main behind scoped one-shot approvals, restore delivery metadata visibility, retire missing local rows, and settle not-admitted messages without losing recovery actions.

Generated-by: OpenAI Codex
Preserve upstream message ownership and failure presentation. Release abandoned recovery snapshots precisely and hold attachment capabilities across composer preparation and Main admission, with regression coverage for concurrent removal and cleanup.

Generated-by: OpenAI Codex

@hqhq1025 hqhq1025 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

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

Keep main staging ownership and turn layout while preserving paused-message replacement identity, attachment leases, and live recovery guards. Capture the submitting draft across asynchronous reference preparation and cover the integrated owner paths.

@Astro-Han Astro-Han left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

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

@jackeyfaker77

Copy link
Copy Markdown
Contributor Author

Thanks @Astro-Han for the review. Following up on the open P2 and the gate status for exact head 86f9b2dadd55dd97665e57712ba90e25466c6fe8:

  • The not_admitted finding was addressed earlier. The original inline was written against 98a720c6. The fix landed in cd56e5dfd, which is included in the current head. The observer preserves the resolution kind; the candidate routes not_admitted directly to failNotAdmittedMessages and returns before any cancellation query. The production boot wiring reaches the local service/store, which persists failed, clears the old result, and retains the message content and attachment bytes. The queue retraction therefore does not discard the durable recovery copy.

  • The second check covered persistence as well as the callback. The existing candidate integration regression drives observation reconciliation through the real observer, candidate, local service and SQLite store. It asserts zero cancellation queries, then closes and reopens the database and checks the persisted failed state and retained attachment bytes. The targeted removed queue|not_admitted checks passed 13/13, including ownership boundaries, the no-Turn case and late completion fencing. This was a targeted verification of the open finding.

  • The exact-head gate is now green. The hosted test job has completed successfully, and GitHub currently reports mergeable: true / clean.

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 not_admitted P2 on the current head?

中文对照

感谢这次审查。针对当前固定版本 86f9b2dadd55dd97665e57712ba90e25466c6fe8,补充旧 P2 和 CI 状态:

  • not_admitted 问题此前已经修复。 原始 inline 针对的是 98a720c6,修复提交为 cd56e5dfd,已包含在当前版本中。observer 会保留实际证明类型;candidate 遇到 not_admitted 时直接调用 failNotAdmittedMessages,在查询取消记录之前返回。生产环境的 boot 接线会调用本地 service/store,将状态持久化为 failed、清除旧结果,同时保留消息内容和附件字节。因此,撤回队列展示不会丢弃可恢复的持久化原件。

  • 此次复核也检查了持久化结果。 现有 candidate 集成回归通过真实 observer、candidate、本地 service 和 SQLite store 驱动观察状态协调,断言取消查询次数为零,并在关闭、重新打开数据库后检查 failed 状态和附件字节仍然保留。相关定向检查 13/13 通过,包含归属边界、没有 Turn 的情况及迟到完成结果的保护。本次属于对该未结案问题的定向复核。

  • 当前固定版本的 CI 已通过。 上述 hosted test job 已完成且成功,GitHub 当前显示 mergeable: true / clean。

保守的 Delete 保护仍是此前说明过的 UX 取舍,三种语言的修正文案也保持有效。

请确认这些证据是否足以将当前版本上的旧 not_admitted P2 结案。

Keep edited local messages lossless across session switches, including the first restore into an inactive session. Preserve ordinary draft limits and replacement cleanup, and cover navigation, references, eviction, and actual sends.
@jackeyfaker77

Copy link
Copy Markdown
Contributor Author

Follow-up in 5ead6ce71: fixed a newly reproduced P2 in paused-message editing.

  • Problem: the send boundary accepts 128,000 characters, while the ordinary per-session draft cache retains only the last 120,000. Editing a longer paused message, switching sessions and returning could truncate the draft while keeping its replacement ID. Sending then replaced the complete durable original with the shortened text.

  • Scoped fix: drafts with an actual replacement link now retain their full body. Ownership is established before the first cache write, including recovery into an inactive session. Ordinary drafts keep their existing character limit, and all text entries still share the 32-entry eviction policy. Clearing, eviction and successful-send ownership cleanup remain covered. This follow-up changes two UI implementation files and two existing test files.

  • Regression evidence: all three new tests failed against the previous code with 120000 !== 128000; they pass with the fix. They cover active/inactive restoration and the real Composer send. Additional assertions cover reference offsets, repeated append/save, ordinary draft limits and eviction. A Composer + recovery-service + SQLite probe also confirms complete replacement content for both 120,000- and 128,000-character inputs.

  • Validation and review: 255/255 focused Desktop/UI tests, UI build, UI/Desktop typechecks, lint, format check under the existing configuration, ASF headers and git diff --check passed. An automated review-agent pass found no new P0–P3 in this four-file follow-up and independently reran 19/19 related tests. The hosted test check for exact head 5ead6ce71 is now green.

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.

中文对照

补充提交 5ead6ce71:修复了暂停消息编辑中新复现的一个 P2。

  • 问题: 发送接口允许 128,000 字符,而普通会话草稿缓存只保留末尾 120,000 字符。编辑较长的暂停消息、切换会话再返回时,草稿可能被截短,但替换 ID 仍然保留。随后发送会用截短内容替换完整的持久化原件。

  • 修复范围: 实际带有替换关联的草稿现在完整保留正文。关联在首次缓存写入前建立,也覆盖恢复到非当前会话的情况。普通草稿仍使用原有字符上限,所有正文条目仍共用 32 项淘汰规则。清空、淘汰和发送成功后的关联清理仍有测试覆盖。这次只修改了两个 UI 实现文件和两个已有测试文件。

  • 回归证据: 三个新增测试在旧代码上均以 120000 !== 128000 失败,修复后全部通过,覆盖当前/非当前会话恢复和真实 Composer 发送。额外断言检查了引用位置、重复追加与保存、普通草稿上限和淘汰行为。Composer+恢复服务+SQLite 探针也确认,120,000 和 128,000 字符输入在替换后都保留了完整内容。

  • 验证与审查: 255/255 项 Desktop/UI 定向测试、UI 构建、UI/Desktop 类型检查、lint、按现有配置执行的格式检查、ASF 头检查和 git diff --check 均通过。自动化 review-agent 对这四个文件的补充改动未发现新的 P0–P3,并独立复跑了 19/19 项相关测试。固定提交 5ead6ce71 的上述 hosted test 检查现已通过。

本地采用定向验证,没有重复运行全部本地测试或打包 Electron。Composer 探针使用 linkedom 和 SQLite,并非 Electron 窗口。

@Astro-Han Astro-Han left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Reviewed 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:

  1. The ownership marker was established too late. setDraft now records replacesMessageId before 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").
  2. The bound is now exemption-aware. rememberComposerDraft takes preserveFullText, 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 Astro-Han left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

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 SessionLocalMessages and registered draftContextRestorer inside useComposerSubmission. Both now come from StagedLocalMessages, which ComposerSubmissionProvider mounts 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 to restoreQueuedDraftContext, which appends attachments, staged approvals, directories (keyed by host) and quotes. That covers everything main's version restored.
  • Provider position. ComposerSubmissionProvider moved inside ModuleHubSkillCatalogRevisionBoundary. 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 canStageComposerContext from the gate changes nothing, because canRestoreDraft already requires a Session id.
  • canStageContext and allowAttachmentOnlySend. These are now derived in conversation-readers.tsx from the same contextPickEnabled input that app-shell passed before.
  • Replacement id. replacesLocalMessageId is forwarded at chat-actions.ts:315,477 and through composer-submit.ts for ordinary, /swarm and /graph sends. 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.

@jackeyfaker77

jackeyfaker77 commented Oct 9, 2026 •

Copy link
Copy Markdown
Contributor Author

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.

  1. Keep the pause-and-refuse behavior described in this review. The original remains durably paused, with its text and attachment bytes preserved. After navigation, the late restore is refused and unused temporary recovery approvals are released. Returning to A, the user can choose Edit again or Continue sending. This keeps the recovery guard simpler, but changes refactor(desktop): move the Composer's reads, edits and delivery recovery below the shell (R2 M3) #5954's automatic background-draft restoration and requires another user action.

  2. Preserve refactor(desktop): move the Composer's reads, edits and delivery recovery below the shell (R2 M3) #5954's navigation behavior while retaining fix(desktop): clarify delivery states and recover failed messages #5058's safeguards. Restore the complete text, staged context and replacement identity into A if A's draft has not changed, leaving B's content and focus untouched. Subsequent edits or clearing A, Session deletion, or loss of the recovery owner must invalidate the pending restore. This needs reliable draft-change tracking—including typing and then deleting back to empty—and coordinated ownership of text, context, attachment approvals and the replacement link. Future draft-editing paths and lifecycle tests would need to preserve that contract.

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。

  1. 保留本次 review 所描述的“暂停原件、拒绝恢复”行为。 原消息以暂停状态持久化,原文和附件字节仍保留。切换会话后拒绝迟到的恢复,并释放未使用的临时恢复凭证。用户回到 A 后,可再次编辑或继续发送。这种方案的恢复判断较简单,但改变了 refactor(desktop): move the Composer's reads, edits and delivery recovery below the shell (R2 M3) #5954 自动恢复到后台草稿的体验,需要用户再操作一次。

  2. 兼容 refactor(desktop): move the Composer's reads, edits and delivery recovery below the shell (R2 M3) #5954 的导航行为,同时保留 fix(desktop): clarify delivery states and recover failed messages #5058 的安全保护。 只要 A 的草稿未发生变化,就将完整文本、暂存上下文和替换关联恢复到 A,保持 B 的内容与焦点不变。如果 A 后续被编辑或清空、会话被删除,或者负责恢复的组件已失效,则拒绝旧恢复。这需要可靠的草稿变更记录,覆盖“输入后又删空”等情况,并统一协调文本、上下文、附件凭证及替换关联的归属。后续新增草稿修改入口和生命周期测试时,也需要维护这一约定。

两种方案都必须保留 #5058 的持久化暂停原件,以及发送成功时的原子替换。 拒绝恢复时只释放未接管的临时凭证,不删除保存的原始内容。暂停原件的 Delete 保护和降级兼容保护也应继续保留。

如果维护者接受这项行为差异,我倾向于保留方案一,限制本 PR 新增的状态协调复杂度。如果 #5954 的自动恢复体验必须保留,那么方案二就是需要完成的兼容工作,优先考虑复用现有草稿管理能力,并集中为一个条件恢复操作。

请确认:是否接受方案一,还是必须采用方案二保留 #5954 的导航行为? 这次测试变化对应一项真实的行为取舍;此前在 #5058 内的讨论明确了设计意图,但希望再就它与已合并 main 的差异获得明确确认。之后的回归测试应固定双方认可的行为及内容保全保证。

@Astro-Han Astro-Han left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

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 Astro-Han left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

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 Astro-Han left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

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 treats failed and paused as not owned.
    • Every await in #deliver (createSession, each ingestAttachment, submit) is followed by stillOwned(). The writes that follow are synchronous: attachmentsPrepared, then originHostEpoch with sending.
    • So cancelUnsentToDraft (which requires no originHostEpoch) cannot pause a message after dispatch intent is written, and a paused message cannot be dispatched.
    • The delivery picker skips paused, and resumeMessage clears the retry and probe keys.
    • failNotAdmittedMessages and retireCancelledMessages only 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 accepted row, a sending row, or a saved row being actively delivered. That last rule was already on main.
  • Atomic replacement (session-local-store.ts enqueue)
    • I ran the head store standalone under Node 22's node:sqlite (--experimental-transform-types). Results:
      • A paused row is stored as column failed with payload paused and reads back as paused.
      • A replacement from another session is rejected.
      • A same-session replacement deletes the original and its outbox_attachments rows in the same transaction (via cascade).
      • An identical retry is idempotent.
      • A later real failure reads back as failed.
    • 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.
  • 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 dispose tolerates missing ones.
    • On the renderer side, syncRecoveryApprovals and retainAttachments keep tokens for every draft bucket plus in-flight sends, and release them on unmount.
  • Drafts and quotes
    • captureComposerStaging snapshots 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 replacesLocalMessageId from the Host-admission path. replacesLocalMessageId stays client-only and is never forwarded to the Host.
  • Protocol and i18n: no packages/runtime-host protocol or epoch changes; the epoch stays at 217. New copy exists in en, zh-CN and zh-TW, and the catalog is type-checked through UiCatalog.
  • 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

  1. P3 (no runtime impact): a migration and docs for an encoding that never shipped.

    • apps/desktop/src/main/session-local-store.ts:129 rewrites rows whose column state is paused.
    • docs/desktop-message-queue.md:60-65 describes a database "created by the earlier implementation".
    • paused never existed on main (git log -S paused origin/main on 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.
  2. 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.
  3. P3 (unverified UX suspicion): recovery approvals can expire while their chips are still shown.

    • Approvals expire after ATTACHMENT_APPROVAL_TTL_MS (30 minutes; pruning at session-local-attachment-recovery.ts:53). They are bound to targetEpoch (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_expired while 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.

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/busy guard and the generation checks in session-local-messages.tsx.
  • I did not look further into the Host-side not_admitted race (proof followed by a late resubmit), beyond confirming the Host computes it under the session admission gate (message-coordinator.ts:569-606).

Comment thread apps/desktop/src/main/session-local-store.ts Outdated
Comment thread packages/ui/src/use-composer-draft.ts
Comment thread apps/desktop/src/main/session-local-attachment-recovery.ts

@Astro-Han Astro-Han left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

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 Astro-Han left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

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.

@jackeyfaker77

Copy link
Copy Markdown
Contributor Author

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 c4dba4930: preserve #5954's background-draft restoration while retaining #5058's safety guarantees.

  • Navigation compatibility: when Edit starts on a local, never-dispatched message in Session A and the user switches to B before recovery completes, the complete text, inline references, staged attachments, directories, quotes and replacesLocalMessageId are restored into A if its draft is unchanged. B's content and enabled state do not affect A's eligibility, and restoring into inactive A does not request focus. The original refactor(desktop): move the Composer's reads, edits and delivery recovery below the shell (R2 M3) #5954 navigation regression now expects background restoration again.

  • Conditional recovery: navigation no longer invalidates the operation by itself. Synchronous, per-draft mutation tracking rejects stale recovery after edits or clearing, including typing and deleting back to empty, adding/removing context, and selecting/removing pending Session references. Session deletion (including the in-progress deletion window), archive, Host/Profile ownership changes and owner unmounts also invalidate recovery. All owners are checked before text, context and replacement identity are handed back synchronously.

  • Safety and scope: the durable original remains paused, with its content and attachment bytes retained. Successful sending atomically replaces it. A refused restore releases only the unused temporary recovery approvals. The paused-original Delete guard and downgrade encoding remain intact. This changes only the local unsent Edit recovery path; Host queue and failed-message recovery keep their existing behavior. There are no Main implementation, database or IPC protocol changes in this follow-up.

  • Verification: 245 affected Desktop cases and 28 UI cases were verified. One existing large-file case hit local ENOSPC and passed when rerun with a workspace temporary directory. Builds, Desktop typechecks, lint, format, ASF headers, locale hygiene, strict renderer architecture, surface inventory, E2E budget and git diff --check passed. Reinstating the old navigation refusal made all 3 background-restoration cases fail; removing mutation invalidation made all 6 selected change-then-clear cases fail. An independent automated review-agent pass found no new P0–P3 in this follow-up. The hosted test check for this exact head is green.

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 引入的导航行为。接着此前的两方案讨论,这次已在提交 c4dba4930 中采用方案二:兼容 #5954 的后台草稿恢复,同时保留 #5058 的安全保证。

  • 导航兼容: 用户在 Session A 编辑尚未发送到 Host 的本地消息,并在恢复完成前切换到 B 时,只要 A 的草稿未发生变化,就将完整文本、行内引用、暂存附件、目录、摘录和 replacesLocalMessageId 恢复到 A。B 的内容及输入框是否可用不影响 A 的恢复资格,恢复到非当前会话 A 时也不会请求焦点。原先 refactor(desktop): move the Composer's reads, edits and delivery recovery below the shell (R2 M3) #5954 的导航回归用例现在重新断言后台恢复行为。

  • 条件恢复: 单纯导航不再使恢复失效。按草稿同步记录修改,拒绝编辑或清空后的旧恢复,包括“输入后又删空”、添加后移除上下文,以及选择后移除待处理的 Session 引用。Session 删除(包括删除进行中的窗口)、归档、Host/Profile 归属变化和负责恢复的组件卸载,也会使恢复失效。所有归属检查通过后,才同步交接文本、上下文和替换关联。

  • 安全保护与范围: 持久化原件仍保持 paused,原文和附件字节继续保留,发送成功时才原子替换原件。拒绝恢复只释放未被接管的临时恢复凭据。暂停原件的 Delete 保护和降级编码保持不变。这次只调整本地未发送消息的 Edit 恢复路径,Host 队列和失败消息的恢复继续使用原有行为,没有修改 Main 实现、数据库或 IPC 协议。

  • 验证: 已验证 245 项受影响的 Desktop 用例和 28 项 UI 用例。其中一个已有的大文件用例遇到本地临时目录空间不足(ENOSPC),改用工作区临时目录重跑后通过。构建、Desktop 类型检查、lint、格式、ASF 头、locale hygiene、严格 Renderer 架构检查、surface inventory、E2E 数量预算和 git diff --check 均通过。临时恢复旧导航拦截后,3 项后台恢复用例全部失败;移除修改失效通知后,6 项选定的“修改后清空”用例全部失败。独立自动化 review-agent 对本次补充改动未发现新增 P0–P3。该固定提交的上述 hosted test 检查已通过。

本地验证使用组件、IPC 和 SQLite 测试。本轮未实测打包 Electron、真实浏览器焦点与选区、多窗口行为或实际旧版 Desktop。review-agent 的结果属于自动化审查,不代表人工批准。

@Astro-Han Astro-Han left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

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.

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

effort/XXL Over 2500 readable lines

Projects

None yet

Development

Successfully merging this pull request may close these issues.

feat(desktop): clarify message delivery states and failure recovery

4 participants