Conversation
…on edit Edit & resend refused a selected message that itself carried quotes or attachments: the replacement submit only carried the human-facing text, silently dropping the context the original answer was grounded in (apache#5109, reproduction cases A and B). The desktop edit now stages the selected message's quotes into the quote plate and its attachments into the attachment plate as retained refs, recording both on the revision draft. After the revision commit the staged attachment refs are swapped for the copied message's target-owned refs read back from the branch child transcript, so the replacement submit only claims files the new Session owns. A no-op send (text and staged context both unchanged) stays refused, and cancel restores the pre-edit plates. The chat-turn edit gate drops quotes and attachments from its disabled list; directory references keep the gate since they have no client-side restage path, and their two dead copy keys are removed. Part of apache#5109 Generated-by: GLM-5.3-Flash (ZCode)
The revision-actions restage work grows app-shell-revision-actions and app-shell token counts; the ledger follows the sources. Part of apache#5109 Generated-by: GLM-5.3-Flash (ZCode)
The renderer architecture ratchet freezes app-shell-revision-actions and app-shell at their base token counts. The staged-context gate, plate restage/restore helpers, and the draft shape move into the @maka/ui revision-staged-context module; the shell keeps thin orchestrators. Part of apache#5109 Generated-by: GLM-5.3-Flash (ZCode)
|
Marking draft while the renderer debt ratchet question is settled — see the gate comment below. The renderer architecture strict-base check freezes
Happy to do either — flagging before burning another CI round. |
Structural gate/stage signatures and a one-line restage call shave further renderer tokens (2305 against the frozen 2155 budget); the ledger follows. Remaining gap is tracked for the beginEdit extraction. Part of apache#5109 Generated-by: GLM-5.3-Flash (ZCode)
The renderer architecture ratchet freezes app-shell-revision-actions and app-shell at their base token counts, and the apache#5109 restage feature could not fit under them. The whole revision lifecycle — beginEdit, prepare, cancel, rollback, and the copy-attempt bookkeeping — now lives in @maka/ui/revision-staged-context; the desktop file is a thin assembler injecting the bridge, the locale catalog, and the attempt tracker. The duplicate pre-send gate in app-shell goes away with it: prepare owns the unchanged and mixed-context refusals, and one stale guard referencing removed draft fields goes with the duplication. Ledger: app-shell-revision-actions 2305->506/9->6, app-shell 13482->13344 /105->104, against a frozen 2155/8 and 13386/104. Part of apache#5109 Generated-by: GLM-5.3-Flash (ZCode)
Empty commit: the push event for c4d7065 did not start a workflow run. Part of apache#5109 Generated-by: GLM-5.3-Flash (ZCode)
|
No description provided. |
|
The ratchet conflict is resolved without touching the frozen budgets: the whole revision lifecycle (beginEdit / prepare / cancel / rollback + copy-attempt bookkeeping) moved into Measured against the frozen base budgets: Heads 0a5af05 / 0a5af05 have not started a CI run for a few hours (the |
|
CI is green on 0a5af05 (2/2, including the architecture ratchet) and the review-required gate is the only open item — marking ready for review. Path taken for the ratchet: option 1 (the lifecycle moved into |
…n-structured-context # Conflicts: # apps/desktop/renderer-architecture.json # packages/ui/src/conversation-copy.ts
Two import declarations from the same package registered a second dependency-path entry for @maka/ui, tripping the ratchet's new-debt check. One declaration, all four names. Part of apache#5109 Generated-by: GLM-5.3-Flash (ZCode)
|
Adversarial root-cause after five red rounds — with the base ledger entry diffed metric-by-metric, the blocker is now precisely characterized, and it is a design decision rather than a patching problem:
So the remaining decision is binary and maintainer-owned:
Everything else in this branch is verified: revision-actions tests 3/3, ui chat-turn 30/30, biome and ASF clean, ledger refreshed in-tree. The branch stays as the working proposal; happy to re-shape once the direction is picked. |
Astro-Han
left a comment
There was a problem hiding this comment.
1 — sanction the edge, but land the sanction in the checker's target rules rather than the ledger numbers.
I verified the mechanism before answering: --strict-base re-derives base debt from the merge-base commit (loadBaseConfig), so editing the committed renderer-architecture.json entry cannot clear the violation — the current file genuinely gains a dependency key the base lacks. And resolveDependency returns undefined for bare package specifiers, so @maka/ui can never satisfy isSanctionedDependencyTarget today. A ledger-number bump alone will stay red either way.
Option 2 is strictly worse on the mechanism's own terms: a new renderer module is itself forbidden (new unclassified renderer source files are forbidden outside approved legacy directories; new legacyAppShell debt entries are forbidden), so it needs a sanction too — same cost, plus a file that exists only to carry one import edge.
Suggested shape for option 1: treat @maka/ui — the package renderer ownership is migrating into — as a sanctioned dependency target for legacyAppShell importers, the same way validated copy catalogs already get bare-package imports for free. The ratchet exists to stop the shell absorbing new ownership; depending on the destination package is the opposite of debt, and the token/specifier budgets still bound every file. If you want it narrower, a per-importer exception in the config works too — but the broad version covers every future migration PR without a fresh exception each round.
中文版
选 1,但豁免要落在 checker 的 target 规则上,不是账本数字:--strict-base 会从 merge-base 重新推导 base 债务,改 renderer-architecture.json 的条目消不掉这条红;裸包名 resolveDependency 返回 undefined,@maka/ui 永远过不了 isSanctionedDependencyTarget。方案 2 更差:新 renderer 模块本身就违反「新文件禁止」「新账本条目禁止」,同样要豁免还多一层纯搬 import 的间接文件。建议把 @maka/ui(所有权正在迁入的包)列为 legacyAppShell importer 的 sanctioned target,与 copy catalog 免计裸包同道理;想窄就按 importer 白名单,但宽版能覆盖后续所有迁移 PR。
AI assistance: I used Devin to trace the ratchet's base-derivation and sanctioned-target paths; the assessment is mine.
…n-structured-context # Conflicts: # apps/desktop/renderer-architecture.json
Astro-Han
left a comment
There was a problem hiding this comment.
Review — #5274 restage a selected message's quotes and attachments on edit
Thanks for this — moving the lifecycle into @maka/ui/revision-staged-context and the fail-closed narrowing in chat-turn.tsx (only directoryReferences keep the gate) both look right, and the refactor of app-shell-revision-actions.ts into a thin assembler is a genuine improvement. The two new @maka/ui tests do pin the un-gating, and renderer-architecture.json shrinks (2155 → 508 tokens for app-shell-revision-actions.ts), so the ratchet is happy.
I could not convince myself that the restage survives the revision commit, though. Details below, most important first.
1. restageRevisionAttachments can never find the rewritten refs — a revision copy excludes the revised turn
packages/ui/src/revision-staged-context.ts:155-163 looks up the copied user message in the branch child transcript and treats a miss as "nothing to restage":
const copiedMessage = copiedMessages.find(m => m.type === 'user' && m.turnId === sourceTurnId);
const rewritten = [...(copiedMessage?.attachments ?? [])];
… remove every staged attachment …
if (rewritten.length > 0) staged.restoreAttachments(targetSessionId, rewritten);But the Host copies a revision with the exclusive boundary — the revised turn is deliberately not in the copy (that's the "rewound to before that message" semantics):
packages/runtime-host/src/server/session-revision-coordinator.ts:375-378—createConversationCopySlice(source.messages, input.sourceTurnId, kind === 'revision' ? 'before' : 'through')packages/runtime/src/conversation-copy.ts:258-260— for'before',retainedTurnIds = turnOrder.slice(0, sourceIndex), i.e. the source turn is dropped (see the assertion atpackages/runtime/src/__tests__/conversation-copy.test.ts:714).
I confirmed the slice behaviour against the built packages/runtime/dist/conversation-copy.js: slicing ['turn-1','turn-2'] with 'before' at turn-2 yields ['turn-1'] only, and editing the first turn of a session yields an empty transcript. So rewritten is always []: the swap deletes the plate and restores nothing, i.e. the attachments are dropped exactly as before — just a step later, and now with the user having been told they were restaged.
Two concrete consequences:
- The in-flight submit keeps the source-owned refs and main rejects them.
sendWithAttachmentscaptures the payload beforesend()(apps/desktop/src/renderer/app-shell.tsx:1759), so the swap — which runs insideprepareRevisionSend— cannot change this send. The submit therefore carriessession_filerefs owned by the source session into the branch child, andretainedAttachmentsForSessionthrows"Retained attachment belongs to another Session"(apps/desktop/src/main/runtime-host-session-execution-ipc-main.ts:910-925), surfacing as a generic "Action failed" toast. - A retried send loses them silently. Post-commit,
prepareRevisionSendreads the live plate (now empty).revisionSendGateonly compares lengths (0 > 1is false) and the text differs, so the gate returns'pass',draft.draftSessionId !== draft.sourceSessionIdshort-circuits totrue, and the replacement goes out with no attachments and no quotes.
So I think the "the Host's copier already rewrote them … (no protocol change)" premise needs revisiting: something has to produce target-owned refs for the revised message — a copier change (retain the source turn's attachments into the target), re-ingesting into the branch child, or letting main rewrite/accept the source refs on sessions:send.
2. Nothing carries the restaged context across the commit's draft-key change
Both plates are keyed by the active session (attachmentDraftKey = activeId ?? NEW_TASK_PENDING_KEY, apps/desktop/src/renderer/app-shell.tsx:371-372), and the restage writes them under the source session key (restoreQuotes(sessionId, …) / restoreAttachments(sessionId, …) in beginEditUserMessage). After openSessionInChat(newSession.id) the active key is the branch child, so selectPending returns [] for both plates (packages/ui/src/pending-items.ts:41-43) and they go blank right after the "Ready to edit and resend" toast — quotes have no swap path at all.
That also makes the PR's headline claim ("the plates make the carried context visible and explicitly removable") untrue past the commit: the user sees the context until they press send, then it vanishes. A manual pass of reproduction case B in #5109 should show this immediately. Restaging under the branch-child key (or making the staged context follow the draft across the commit) is what I'd expect here.
Related: inside the swap, stagedContext() and removeAttachment come from the closure captured when the send started (useStableActions publishes through a layout-effect ref), while restoreAttachments takes an explicit ownerKey and removeAttachment/removeQuote bind to the live draftKey. "Clear by index, then restage under ownerKey" therefore mixes two different owners — worth making the owner explicit on the mutators if this design stays.
3. Test coverage for the commit / send half is missing
packages/ui/src/revision-staged-context.ts has no test file, and the pure helpers (revisionSendGate, restageRevisionAttachments, clearRevisionStagedContext, revisionStagedContextUnchanged) are the easiest things in the PR to unit-test. On the desktop side the suite still only drives beginEditUserMessage — prepareRevisionSend and cancelRevisionDraft have no coverage at all, so the swap, the moved no-op refusal, and the cancel-time plate cleanup are all untested. A test feeding the swap a realistic branch-child transcript (source turn absent, earlier turns' refs rewritten) would have caught #1.
Also unverified by tests: the 'conflict' gate (staged quote / pending directory during an edit) and cancel restoring the pre-edit plates.
Nits
revision-staged-context.ts:376-379refuses an edit when the composer has staged quotes, but toastscopy.revisionDraftAttachmentConflict("The composer already has pending attachments…"). SincehasPendingAttachmentsis bound tohasPendingContext(attachments or directories) in the desktop env, this is the only quote-specific refusal and it needs its own copy key in all three locales. Same string reuse for the gate at:493-498:revisionAttachmentsUnsupportednow reads "Editing cannot mix newly staged attachments with the restored ones…", which is wrong when the added context was a quote or a directory reference.revisionStagedContextHasAdditions(:114) is exported but never used;revisionSendGate(:183-187) inlines the same three conditions. Pick one so the two can't drift.clearRevisionStagedContext'spreviousQuotesparameter is always[]at its only call site (:624), so the restore branch is unreachable — drop it or use it.attachmentToPending(:68-77) duplicatesretainedToPending(packages/ui/src/use-composer-attachments.ts:152-160, not exported). It only feedsattachmentKey, which ignoresstagingKey, so the syntheticrevision:${JSON.stringify(...)}key is dead weight.- The new
"./revision-staged-context"subpath inpackages/ui/package.jsonis unused — the desktop imports the barrel (@maka/ui). Value imports from the barrel are already common in the renderer, so this is cosmetic; either use the subpath or drop the entry.
Verified as fine
- The
chat-turn.tsxgate now fails closed only ondirectoryReferences, and the reason chain (directory → transformed → running) is coherent. - No other consumer of the removed
editMessageDisabledAttachments/editMessageDisabledQuoteskeys exists in the repo (onlychat-turn.tsxandconversation-copy.tsreferenced them). - Edit → resubmit ordering and dedup: source quotes/attachments are carried in original order, and
beginEditUserMessagerefuses while anything is staged, so no duplication on resubmit; cancel clears both plates and restorespreviousComposerText. - The no-op-send refusal moved cleanly out of
app-shell.tsxintoprepareRevisionSend, with the duplicated pre-checks removed and a comment left behind atapps/desktop/src/renderer/app-shell.tsx:1606-1612. revisionStagedContextUnchangedcompares against the source-owned refs captured on the draft, so a post-commit retry isn't misreported as "unchanged".
The renderer ownership migration target cannot clear the strict-base ratchet today: resolveDependency returns undefined for bare package specifiers, so an @maka/ui edge a shell file legitimately gains while shedding ownership reads as new dependency debt (review direction on this PR). Treat the migration destination as a sanctioned dependency target for legacyAppShell and legacyAppShellClosure importers, the same way validated copy catalogs take bare-package imports for free; root debt entries stay fully priced because they must become thin mounts. Regenerate the ledger under the new rule: dependency keys keep recording the raw edges, and import declaration/specifier counts now price only unsanctioned imports, so every shell file's budget moves down. Generated-by: GLM-5.3-Flash (ZCode)
The review on this PR showed the attachment restage could never work: a revision copy excludes the revised turn (exclusive 'before' boundary), so restageRevisionAttachments always found no copied message, deleted the staged refs, and restored nothing — while the user had been told the context was restaged. Worse, the in-flight replacement submit kept the source-owned refs and main rejected them as belonging to another Session. Own the limitation instead of promising what the protocol cannot deliver: - beginEdit refuses a message that carries attachments, with its own truthful copy (attachments fail closed until the Host can produce target-owned refs for the revised message, as first reviewed here); the broken restageRevisionAttachments is deleted, not patched. - Quotes are pure data and do follow: after every rollback check has passed, prepareRevisionSend re-keys the restored quotes from the draft snapshot onto the branch child's draft key (the plates read the active session's key, and the replacement send reads them live); cancel clears both draft keys explicitly. The staged-context mutators now take their owner explicitly. - The quote-conflict refusal and the send-gate conflict stop reusing the attachment copy; each gate gets its own key in all three locales. - revisionStagedContextHasAdditions (exported, never used) is gone, the unused ./revision-staged-context subpath export is gone, and clearRevisionStagedContext's unreachable restore parameter is gone. - The lifecycle and helpers gain the tests the review asked for: the re-key drives a realistic branch-child transcript whose revised turn is absent, and the gate matrix, staging, and cleanup are pinned. Ledger regenerated: the frozen files only shed tokens (13288 -> 13242 for app-shell.tsx), and the quotes hook lands at 358 against a 360 budget. Generated-by: GLM-5.3-Flash (ZCode)
|
Both commits pushed: the checker sanction (d53b28f, your 09-15 direction) and the review fixes (ceccd38). #1 (restage can never find the rewritten refs) — accepted, and resolved by owning the limitation rather than patching the symptom. I re-verified the exclusive #2 (nothing carries the staged context across the commit) — fixed for quotes, moot for attachments. After every rollback check has passed, #3 (missing coverage) — added where the code lives. New Nits — quote-conflict refusal and the send-gate conflict each got their own key in all three locales ( Checker (d53b28f): Verification: @maka/ui 12/12 new tests + desktop 15/15 across the revision/catalog/first-send suites; biome clean on all seven touched files; |
…n-structured-context # Conflicts: # apps/desktop/renderer-architecture.json # packages/ui/src/conversation-copy.ts
…ater The upstream chat-view rebuild turned the shell's setMessages into a MessageListUpdater that accepts a reactive next form, so the lifecycle env's settled-readonly-array contract no longer typechecks at the assembly site. Adapt at the assembler seam, the same place the concrete draft type is already bound: declare the dep as the shell's updater and hand the lifecycle a settled copy. The typecheck also exposed that the deps Omit listed the two workspace-error helper imports, which are not env keys, instead of the synthesized reportSessionWorkspaceUnavailable they feed — replace them with the real key. Generated-by: GLM-5.3-Flash (ZCode)
Two unrelated stories (appshell dock-affordance scroll, workhub filter list) failed their in-story wait predicates in one slow run (10-70s per story); neither surface touches this PR's delta, and the full catalog diff against upstream is exactly the two removed unused edit-gate keys with no remaining consumers.
…n-structured-context # Conflicts: # apps/desktop/renderer-architecture.json # apps/desktop/src/renderer/use-app-shell-composer-quotes.ts
|
Merged latest Conflicts (2):
Verification: the ledger check passes ( Everything actionable from your 09-17 review remains in place (restage removal, quote re-keying onto the branch-child key, the new |
…n-structured-context # Conflicts: # apps/desktop/renderer-architecture.json # apps/desktop/src/renderer/locales/conversation-copy.ts # packages/ui/src/conversation-copy.ts
…context' into fix/desktop-revision-structured-context # Conflicts: # apps/desktop/renderer-architecture.json # packages/ui/src/conversation-copy.ts
|
Merged latest Conflicts (3):
Verification: ledger check passes plain; biome clean on the touched locale files; the checker fixture suite is 112/113 with the single failure being the known Windows-only |
…tracted revision lifecycle Merge follow-up: the ledger is repriced to the merged tree's exact actuals (app-shell.tsx 12957 -> 12682, revision-actions 537 -> 482; every touched budget stays at or below the main baseline), and the two main-side tests that drive the refused-retry flow get a staged-context stub now that the lifecycle reads it from its env.
|
Merged latest Conflicts (4), resolved on top of the transcript-publish simplification (#5566/#5494 territory):
Main's own tests for the new flow ("prepares the revision without opening another transcript consumer", "surfaces a failed preparation instead of swallowing it behind rollback") came through the merge and pin the ported semantics; the refused-retry world fixture gained the Verification: full workspace build green after reinstalling dependencies (the merge carries the No review responses outstanding on my side — both of @me2seeks's earlier requests were addressed in |
…extracted revision lifecycle The merge intended to carry main's simplification of the revision preparation flow, but these four edits were left uncommitted while the ledger repricing went out alone - CI rightly flagged the tree as still carrying the dropped surface (readSettledMessages import, setMessages dep, preparation abort machinery, and the stale app-shell ledger numbers that priced them). What lands now, matching main's semantics inside the @maka/ui lifecycle: - readSettledMessages/setMessages and the preparation AbortController are gone from RevisionActionsEnv and the assembler; the shell call site stops passing the dep. - refreshSessions() runs right after openSessionInChat, before the final rollback checks, instead of reading a second transcript consumer. - a preparation failure toasts before rollback runs, because rollback navigates back to the source Session and a check after it is stale. Generated-by: GLM-5.3-Flash (ZCode)
|
The red check was my process failure, and the fix is pushed at head Root cause: when I resolved the previous merge I committed the conflict resolution first and made the lifecycle-port edits afterwards — then pushed only the ledger/test follow-up commit. The four files carrying the actual port (the What went out now:
Verified after the rebuild: full workspace build green (0 type errors); |
…n-structured-context # Conflicts: # apps/desktop/renderer-architecture.json
…d models.dev snapshot The apache#5678 snapshot refresh added minimal and xhigh to openai/gpt-5.1-thinking's declared efforts, but this test still pinned the pre-refresh list, so the runtime suite fails on any tree where the runtime workspace is exercised (main currently carries the same latent break; affected-workspace CI had simply not run this suite since the refresh). Carry the one-line expectation update so this PR's required check is green again; the same change belongs on main. Generated-by: GLM-5.3-Flash (ZCode)
…n-structured-context # Conflicts: # apps/desktop/renderer-architecture.json
hqhq1025
left a comment
There was a problem hiding this comment.
Reviewed current head 5746d5c329c0e8fabcc2c0f872b1609a6500ec7e (15 files, +1316/−425). The change stages a selected message's quotes in the composer, refuses edits to messages carrying session-owned attachments, gates no-op/mixed-context replacements, and moves the revision lifecycle into @maka/ui (packages/ui/src/revision-staged-context.ts:93-151,292-365,442-527). I traced the Desktop send path and quote-bucket ownership, plus the new lifecycle and Desktop action tests. One P1 finding is attached inline: the first replacement send loses its quotes and leaves them staged for a later send.
The current-head test check passes, but a fresh synthetic merge with current main conflicts in apps/desktop/renderer-architecture.json; the PR diff also has a trailing blank-line warning in apps/desktop/src/renderer/locales/conversation-copy.ts. Resolve the conflict and revalidate a new head before merge. I did not run local tests or Electron E2E (Node 18/no installed dependencies), and have not exercised real Host failure/reconnect or A→B→A navigation. No database schema/migration change appears in this PR. This is 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.
| // — a revision copy excludes the revised turn, so the copied transcript | ||
| // cannot be their source. Re-keyed only after every rollback check has | ||
| // passed, so a failed preparation leaves the plate on the source key. | ||
| staged.restoreQuotes(newSession.id, startedDraft.originalQuotes); |
There was a problem hiding this comment.
[P1] Preserve the branch-owned quotes for the same in-flight send. sendWithAttachments is still executing in the source session's render closure while awaiting prepareRevisionSend() (app-shell.tsx:1499-1508). That closure's pendingQuotes is the source-key array from useComposerQuotes. Here you append the quotes to the new child key, then clearQuotes(sourceSessionId) mutates the source array to empty. When the same call resumes, app-shell.tsx:1658-1664 reads that now-empty array and omits quotes from send; the child bucket remains populated because the success path also closes over the source-key clearQuotes. Thus editing a quote-bearing message and sending a changed text silently drops its quote on the first replacement, then may carry it into an unrelated later send. The new tests assert that re-keying occurred but do not send through this production closure. Capture the intended quote payload before clearing the source bucket or read/clear the child bucket by explicit owner, and add a cross-layer first-send regression.
Summary
Edit & resend refused a selected message that itself carried quotes or attachments: the replacement submit only carried the human-facing text, silently dropping the context the original answer was grounded in (#5109, reproduction cases A and B — case C, the TUI rewind side, landed in #5265).
The desktop edit now stages the selected message's quotes into the quote plate and its attachments into the attachment plate as retained refs, recording both on the revision draft:
The chat-turn edit gate drops quotes and attachments from its disabled list; directory references keep the gate, since they have no client-side restage path, and their two dead copy keys are removed together with the branches that rendered them.
Part of #5109 — with this, both clients restore the selected turn's quotes and attachments; directory references remain fail-closed on both.
Verification
@maka/uifull suitesession-todo-panel,prompt-rail-observer-identity,transcript-history-notice) fail identically on pristineupstream/mainvia stash rebuild;composer-plus-menufails only under full-suite concurrency and passes 11/11 in isolationnpm --workspace @maka/desktop run build:main(typecheck incl. renderer sources)check-locale-hygienebiome checkon all 7 changed filescheck:asf-headersAI use
Select exactly one:
Tool(s) and scope: GLM-5.3-Flash (ZCode) implemented the desktop/ui changes and tests under human direction and review.
Checklist
Does this PR entail a change in behavior?