Skip to content

fix(desktop,ui): restage a selected message's quotes and attachments on edit - #5274

Open
ggbdpq wants to merge 28 commits into
apache:mainfrom
ggbdpq:fix/desktop-revision-structured-context
Open

ggbdpq wants to merge 28 commits into
apache:mainfrom
ggbdpq:fix/desktop-revision-structured-context

Conversation

@ggbdpq

@ggbdpq ggbdpq commented Sep 13, 2026

Copy link
Copy Markdown
Contributor

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 plates make the carried context visible and explicitly removable, satisfying the issue's "removing is an explicit edit" expectation;
  • 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 — the Host's copier already rewrote them, so the replacement submit only claims files the new Session owns (no protocol change);
  • a no-op send (text and staged context both unchanged vs the draft) stays refused, cancel restores the pre-edit plates, and mid-edit staging of new context is refused so user additions cannot silently mix with the restored set.

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

Check Result
New ui tests (attachment- and quote-carrying messages keep the edit action) red on the old gate (2/2 fail via stash rebuild), 2/2 green after
Desktop revision-actions tests (source attachments/quotes stage and are recorded on the draft) 3/3 green; the replaced upstream test asserted the exact negation (rejection) and was green pre-change
@maka/ui full suite 446/450 — 3 failures (session-todo-panel, prompt-rail-observer-identity, transcript-history-notice) fail identically on pristine upstream/main via stash rebuild; composer-plus-menu fails only under full-suite concurrency and passes 11/11 in isolation
npm --workspace @maka/desktop run build:main (typecheck incl. renderer sources) clean
check-locale-hygiene passed
biome check on all 7 changed files clean
check:asf-headers passed locally

AI use

Select exactly one:

  • No generative tool made a substantive contribution
  • Generative tooling made a substantive contribution

Tool(s) and scope: GLM-5.3-Flash (ZCode) implemented the desktop/ui changes and tests under human direction and review.

Checklist

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

Does this PR entail a change in behavior?

  • Yes — a selected message carrying quotes or attachments is now editable; the replacement submit restages that context (attachments rewritten to target-owned refs after the revision commit).
  • No

…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)
@github-actions github-actions Bot added the effort/M Under 500 readable lines label Sep 13, 2026
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)
@ggbdpq

ggbdpq commented Sep 13, 2026

Copy link
Copy Markdown
Contributor Author

Marking draft while the renderer debt ratchet question is settled — see the gate comment below.

The renderer architecture strict-base check freezes app-shell-revision-actions.ts at 2155 non-trivia tokens; the feature needs ~160 more than the slimmest extraction I could land (88dc60e, 2318). Two honest paths forward:

  1. I extract the beginEditUserMessage orchestration into @maka/ui/revision-staged-context as well (fits the budget, but moves shell orchestration into the package), or
  2. a one-time sanctioned budget bump for app-shell-revision-actions.ts in renderer-architecture.json.

Happy to do either — flagging before burning another CI round.

@ggbdpq
ggbdpq marked this pull request as draft September 13, 2026 22:34
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)
@github-actions github-actions Bot added effort/L Under 1000 readable lines and removed effort/M Under 500 readable lines labels Sep 14, 2026
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)
@ggbdpq ggbdpq closed this Sep 15, 2026
@ggbdpq ggbdpq reopened this Sep 15, 2026
@github-actions

Copy link
Copy Markdown

No description provided.

@github-actions github-actions Bot added effort/XL Under 2500 readable lines and removed effort/L Under 1000 readable lines labels Sep 15, 2026
@ggbdpq

ggbdpq commented Sep 15, 2026

Copy link
Copy Markdown
Contributor Author

The ratchet conflict is resolved without touching the frozen budgets: the whole revision lifecycle (beginEdit / prepare / cancel / rollback + copy-attempt bookkeeping) moved into @maka/ui/revision-staged-context, and app-shell-revision-actions.ts is now a thin assembler injecting the bridge, the locale catalog, and the attempt tracker.

Measured against the frozen base budgets: app-shell-revision-actions.ts 2305 -> 506 tokens / 6 specifiers (base 2155/8); app-shell.tsx 13482 -> 13344 tokens / 104 specifiers (base 13386/104). The duplicate pre-send gate in the shell goes away with the move — prepareRevisionSend owns the unchanged and mixed-context refusals — and one stale guard referencing removed draft fields is gone with it.

Heads 0a5af05 / 0a5af05 have not started a CI run for a few hours (the pull_request workflow never fired on the pushes or the reopen — likely the external-contributor workflow approval); pull_request_target checks ran. Once a run is approved: local verification passed — desktop revision-actions 3/3, ui chat-turn 26/26, biome clean, ASF clean, and the refreshed ledger confirms both files under budget.

@ggbdpq
ggbdpq marked this pull request as ready for review September 15, 2026 13:02
@ggbdpq

ggbdpq commented Sep 15, 2026

Copy link
Copy Markdown
Contributor Author

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 @maka/ui/revision-staged-context; both frozen files now sit under their base budgets, ledger refreshed in-tree).

…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)
@ggbdpq

ggbdpq commented Sep 15, 2026

Copy link
Copy Markdown
Contributor Author

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:

dependencyPaths for app-shell-revision-actions.ts (base e06cf84ef): {"./locales/conversation-copy.js":1, "./locales/shell-copy.js":1, "./platform/desktop/session-message-settlement.js":1, "./session-copy-attempt.js":1, "./session-workspace-errors.js":1, "@maka/core/session":1}. The ratchet forbids new keys and count increases. The #5109 restage feature irreducibly requires ONE new runtime edge — importing the lifecycle factory from @maka/ui — which is exactly such a new key. The relocation to @maka/ui (this branch) removed every other violation (tokens 506 <= 2155, specifiers 6 <= 8), but the assembler's own @maka/ui edge cannot be eliminated: app-shell.tsx sits at @maka/ui count 1 of 1 with zero headroom, and every renderer file is frozen with a fixed dependency key set.

So the remaining decision is binary and maintainer-owned:

  1. a one-time sanctioned ledger change for app-shell-revision-actions.ts (dependencyPaths @maka/ui: 1 + nonTriviaTokens ~508), or
  2. a sanctioned new extraction module inside the renderer (which today the ledger forbids).

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

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

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 at packages/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. sendWithAttachments captures the payload before send() (apps/desktop/src/renderer/app-shell.tsx:1759), so the swap — which runs inside prepareRevisionSend — cannot change this send. The submit therefore carries session_file refs owned by the source session into the branch child, and retainedAttachmentsForSession throws "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, prepareRevisionSend reads the live plate (now empty). revisionSendGate only compares lengths (0 > 1 is false) and the text differs, so the gate returns 'pass', draft.draftSessionId !== draft.sourceSessionId short-circuits to true, 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-379 refuses an edit when the composer has staged quotes, but toasts copy.revisionDraftAttachmentConflict ("The composer already has pending attachments…"). Since hasPendingAttachments is bound to hasPendingContext (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: revisionAttachmentsUnsupported now 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's previousQuotes parameter is always [] at its only call site (:624), so the restore branch is unreachable — drop it or use it.
  • attachmentToPending (:68-77) duplicates retainedToPending (packages/ui/src/use-composer-attachments.ts:152-160, not exported). It only feeds attachmentKey, which ignores stagingKey, so the synthetic revision:${JSON.stringify(...)} key is dead weight.
  • The new "./revision-staged-context" subpath in packages/ui/package.json is 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.tsx gate now fails closed only on directoryReferences, and the reason chain (directory → transformed → running) is coherent.
  • No other consumer of the removed editMessageDisabledAttachments / editMessageDisabledQuotes keys exists in the repo (only chat-turn.tsx and conversation-copy.ts referenced them).
  • Edit → resubmit ordering and dedup: source quotes/attachments are carried in original order, and beginEditUserMessage refuses while anything is staged, so no duplication on resubmit; cancel clears both plates and restores previousComposerText.
  • The no-op-send refusal moved cleanly out of app-shell.tsx into prepareRevisionSend, with the duplicated pre-checks removed and a comment left behind at apps/desktop/src/renderer/app-shell.tsx:1606-1612.
  • revisionStagedContextUnchanged compares 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)
@ggbdpq

ggbdpq commented Sep 17, 2026

Copy link
Copy Markdown
Contributor Author

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 'before' boundary against conversation-copy.ts before touching anything: the premise behind restageRevisionAttachments ("the Host's copier already rewrote them") is simply false, and the function could only delete the plate and restore nothing. Since a revision copy excludes the revised turn, something outside this PR must produce target-owned refs for the revised message — I don't think that should happen inside a fix(desktop,ui) PR, so attachments now fail closed at beginEdit: a message carrying attachments refuses to edit with its own truthful copy ("messages with attachments cannot be edited yet: attachments cannot follow into the new version"), and restageRevisionAttachments is deleted, not patched. This is the state your first review already blessed ("attachments can continue to fail closed until their target-owned refs can be resolved"). Of your three protocol directions, re-ingesting into the branch child (or a copier change that retains the source turn's attachment refs into the target) looks like the right shape for a follow-up — I'd rather propose it there with a Host-side test than grow this PR into runtime-host.

#2 (nothing carries the staged context across the commit) — fixed for quotes, moot for attachments. After every rollback check has passed, prepareRevisionSend re-keys the restored quotes onto the branch child's draft key — restoring from the draft snapshot, not the copied transcript, since the transcript provably cannot contain the revised turn. The source-key plate empties at the same moment, and cancelRevisionDraft now clears both draft keys explicitly. The staged-context mutators take their owner explicitly (your related note): the context type is now { quotes, attachments, restoreQuotes(ownerKey), clearQuotes(ownerKey) } — the read-only attachment view stays for the conflict gates.

#3 (missing coverage) — added where the code lives. New packages/ui/src/__tests__/revision-staged-context.test.ts drives createRevisionActions through a fake env: the re-key test feeds prepareRevisionSend exactly the realistic branch-child transcript you described (source turn absent, an earlier turn present) and asserts the re-key reads the draft snapshot; the unchanged/conflict gates, the attachment refusal, and the both-keys cancel are pinned alongside. Matrix tests cover revisionSendGate, stageRevisionSourceContext, clearRevisionStagedContext, and revisionStagedContextUnchanged. On the desktop side the harness gains the fail-closed refusal (no draft committed, composer untouched); prepareRevisionSend/cancelRevisionDraft themselves are lifecycle-level and now covered in @maka/ui, which is where this PR moved them.

Nits — quote-conflict refusal and the send-gate conflict each got their own key in all three locales (revisionDraftQuoteConflict, revisionMixedContextUnsupported), and revisionAttachmentsUnsupported now says what it actually means; revisionStagedContextHasAdditions removed (the gate's inline form won); clearRevisionStagedContext's unreachable previousQuotes parameter removed; the unused ./revision-staged-context subpath export dropped. One nit skipped deliberately: attachmentToPending vs retainedToPending stays as-is for now because the attachment comparison it feeds is dead-in-practice under fail-closed and the whole dimension should land or die together with the Host follow-up.

Checker (d53b28f): @maka/ui is a sanctioned dependency target for legacyAppShell/legacyAppShellClosure importers, with fixture tests in the git-fixture suite (a shell file migrating onto @maka/ui passes strict-base; a root-debt entry gaining the same edge stays priced — both verified red against the unfixed checker). Ledger regenerated; every touched budget went down (app-shell.tsx 13288 → 13242, revision-actions 2155 → 508, quotes hook 358 vs 360).

Verification: @maka/ui 12/12 new tests + desktop 15/15 across the revision/catalog/first-send suites; biome clean on all seven touched files; check:renderer-architecture plain passes with the regenerated ledger. CI is the oracle for --strict-base, which segfaults locally.

…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
@ggbdpq

ggbdpq commented Sep 20, 2026

Copy link
Copy Markdown
Contributor Author

Merged latest main (d329239) — no functional changes, conflict resolution only. Head is now 070e7f4ec.

Conflicts (2):

  • packages/ui/src/conversation-copy.ts: main added the turnStatus* / gitBranch* / switchWarningDismiss / processExpandAll copy inside the same interface block and giant locale lines; resolved by keeping main's additions and re-applying this PR's removal of editMessageDisabledAttachments / editMessageDisabledQuotes across the interface and all three locales.
  • apps/desktop/renderer-architecture.json: kept this PR's post-extraction reductions together with main's independent repricings (e.g. app-shell-project-actions 2284 → 2157); app-shell.tsx was repriced to the merged tree's exact actuals (13089 → 13081 non-trivia tokens — main's follow-ups shifted the file since the last regeneration; still below main's 13127 baseline), importSpecifiers stays 88.

Verification: the ledger check passes (check:renderer-architecture plain). The checker fixture suite is 112/113 — the single failure (attests the canonical main source in the final Vite entry graph) is a pre-existing Windows-only test bug: it reproduces byte-identically on a pristine upstream/main worktree. The fixture mocks facadeModuleId as /fixture/src/renderer/index.html, but path.resolve('/fixture/src/renderer', 'index.html') drive-prefixes the canonical path on Windows, so the equality never holds off-Linux; CI (Linux) is unaffected — happy to file a follow-up issue. The @maka/ui suite is 547/547 (three initial failures were stale 09-08 dist orphans of tests since deleted by #5366 — cleared and re-run clean); biome is clean on all 13 files this PR touches; @maka/desktop typecheck is green.

Everything actionable from your 09-17 review remains in place (restage removal, quote re-keying onto the branch-child key, the new @maka/ui coverage, and the checker sanction in your 09-15 direction).

…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
@ggbdpq

ggbdpq commented Sep 20, 2026

Copy link
Copy Markdown
Contributor Author

Merged latest main (205a06e) — conflict resolution only, head is now b867dacd6. (This also folds in the earlier 070e7f4ec line that had landed on the branch in the meantime.)

Conflicts (3):

  • packages/ui/src/conversation-copy.ts: main added the turnStatus* copy and removed processExpandAll / processRestore (post-d3292393c); resolved by taking main's current lines and re-applying this PR's removal of editMessageDisabledAttachments / editMessageDisabledQuotes across the interface and all three locales.
  • apps/desktop/src/renderer/locales/conversation-copy.ts: main removed regenerateStartedTitle / regenerateStartedDescription; resolved by keeping that removal together with this PR's revision* additions.
  • apps/desktop/renderer-architecture.json: regenerated against the merged tree (check:renderer-architecture plain passes).

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 attests the canonical main source case (byte-identical on a pristine upstream checkout, detailed in my earlier comment).

…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.
@ggbdpq

ggbdpq commented Sep 22, 2026

Copy link
Copy Markdown
Contributor Author

Merged latest main (8bde344) — head is now 204e884b4.

Conflicts (4), resolved on top of the transcript-publish simplification (#5566/#5494 territory):

  • app-shell-revision-actions.ts + its test: this PR's version of the file is the thin assembler (the lifecycle lives in @maka/ui), so main's simplification was ported into the extracted lifecycle rather than unioned textually — readSettledMessages/setMessages and the preparation abort machinery are gone from the env surface, refreshSessions() now runs right after openSessionInChat, and the failure toast fires before rollback (rollback navigates away, so checking after it is always stale). The assembler and app-shell.tsx shed the two dropped deps.
  • conversation-copy.ts: main's facts-only footer simplification of the turnStatus* family, with this PR's editMessageDisabledAttachments/Quotes removal re-applied.
  • renderer-architecture.json: repriced to the merged tree's exact actuals — app-shell.tsx 12957 → 12682, app-shell-revision-actions.ts 537 → 482; every touched budget stays at or below the main baseline.

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 stagedContext stub the extracted lifecycle reads.

Verification: full workspace build green after reinstalling dependencies (the merge carries the @astryxdesign/core 0.6.2 patch); check:renderer-architecture plain passes on the repriced ledger; arch fixture suite 112/113 (the single failure is the pre-existing Windows-only attests the canonical main source bug, reproduced on pristine upstream/main — Linux CI unaffected); @maka/ui 629/629 after clearing stale dist orphans; desktop app-shell-revision-actions 7/7, app-shell-first-send-cleanup 20/20, model-catalog-choices 4/4; desktop typecheck (preload/main/renderer) green; biome clean on all six touched sources; check:asf-headers pass.

No review responses outstanding on my side — both of @me2seeks's earlier requests were addressed in 87e605295 (#5466) and cb6d10882/c07c577a7 (#5265); this PR had no new findings, only the merge.

…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)
@ggbdpq

ggbdpq commented Sep 23, 2026 •

Copy link
Copy Markdown
Contributor Author

The red check was my process failure, and the fix is pushed at head ae023e13c.

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 @maka/ui lifecycle, the assembler's narrowed deps, the app-shell.tsx call site, the ui test) stayed uncommitted in my working tree, so CI checked a tree that still had readSettledMessages/setMessages/the abort machinery — 537 tokens in app-shell-revision-actions.ts including the dropped import, exactly what the strict-base cross-check flagged as debt growth against its own repriced ledger.

What went out now:

  • a06e0430d — the port itself: RevisionActionsEnv loses readSettledMessages/setMessages and the preparation AbortController; refreshSessions() runs right after openSessionInChat; a preparation failure toasts before rollback (rollback navigates away, so checking after is stale); the assembler and app-shell.tsx shed the dropped deps.
  • merge of latest main (6cb8c58) — one file conflicted (the ledger), resolved and repriced to the merged tree's actuals (chrome-actions 408 → 122 after main's own extraction, app-shell.tsx → 12680); every touched budget stays at or below baseline.

Verified after the rebuild: full workspace build green (0 type errors); check:renderer-architecture plain passes; @maka/ui 630/630; desktop app-shell-revision-actions 7/7, app-shell-first-send-cleanup 20/20; desktop typecheck (preload/main/renderer) green; biome and check:asf-headers clean.

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

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.

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

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/XL Under 2500 readable lines

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants