Skip to content

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

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

ggbdpq wants to merge 18 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).

Scope after review (09-28): quotes are restaged; attachments fail closed. The review established that a revision copy excludes the revised turn, so nothing within this PR's reach can produce target-owned attachment refs for the revised message — restaging the source-owned refs re-creates the cross-session rejection, and the late plate swap could only delete them. Producing target-owned refs on the Host side remains the open attachment half of #5109 and should land there.

What this PR does now:

  • the edit stages the selected message's quotes into the quote plate, recorded on the revision draft; after the commit they are re-keyed onto the branch child, so the replacement submit carries them — visible, explicitly removable, with the no-op send still refused and cancel restoring the pre-edit plate;
  • a message carrying attachments refuses to edit with its own truthful copy: the invalid cross-session ref submission and the cancellation residue are prevented by construction, and the plate handlers of the withdrawn attachment path are removed;
  • the chat-turn edit gate drops quotes from its disabled list; directory references keep the gate, and their two dead copy keys are removed together with the branches that rendered them.

Verification

Check Result
Current head CI (test job) green; static merge-tree against latest main clean (reviewer-confirmed)
Focused revision suites on the review head 56/56 (reviewer-confirmed before the final dead-code removal)
New ui tests (quote-carrying messages keep the edit action) red on the old gate, green after
Desktop revision-actions tests (source quotes stage and are recorded on the draft) green; the replaced upstream test asserted the exact negation and was green pre-change
check-locale-hygiene / biome check on changed files / check:asf-headers clean

Earlier-round rows (ui full-suite counts, stash-rebuild baselines) are kept in the review thread; the attachment-half rows are withdrawn with the feature.

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 is now editable and the replacement submit restages them; messages carrying attachments keep refusing to edit, now with a truthful reason and no leftover plate handlers.

@github-actions github-actions Bot added the effort/M Under 500 readable lines label Sep 13, 2026
@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
@github-actions github-actions Bot added effort/L Under 1000 readable lines and removed effort/M Under 500 readable lines labels Sep 14, 2026
@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).

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

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

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

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

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

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

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

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

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 at 314dc7d3a on the suggested lines: sendWithAttachments now snapshots the staged payload into revisionQuotes before awaiting prepareRevisionSend, prefers that snapshot when sending, and on success clears the branch child's bucket by explicit owner (clearQuotes(expectedRevisionDraft?.draftSessionId)) instead of the stale closure's default key — the child bucket no longer survives the send, and the first replacement carries the quote.

On the cross-layer first-send regression: the production closure lives in the Desktop assembler (a React render closure), and the repo's Desktop tests are main-process only — there is no renderer harness that can drive sendWithAttachments with an async interleaving. The data invariant the fix relies on is pinned at the ui layer instead (keeps the pre-gate quote snapshot equal to the child bucket the send reads): whatever the gate does while the send awaits, the re-keyed child bucket equals the pre-gate snapshot. A renderer harness for the closure itself would be its own piece of infrastructure.

@ggbdpq
ggbdpq force-pushed the fix/desktop-revision-structured-context branch from ded432a to 314dc7d Compare September 26, 2026 13:28

@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 30025d52331c84907bc95ca462a6d3ef93f0b56d (14 files, +1352/−427). The new increment closes the previous first-send P1: app-shell.tsx snapshots the revision quotes before prepareRevisionSend() re-keys the source bucket, then clears the branch-child bucket explicitly after a successful send. I traced the edit, prepare, send, retry, cancel, quote-selection, and session-snapshot paths. One P2 finding remains inline: the unchanged gate compares only part of QuoteRef, so a legitimate provenance-only quote edit is rejected as “Nothing changed.” I do not recommend merging until that canonical-content comparison is fixed and covered.

Validation on the exact head: clean npm ci; build:test; full typecheck; UI 677/677; Desktop 2753/2753; focused revision/send tests; renderer architecture 114/114; lint and format. Hosted test is green. A conflict-free synthetic merge tree 8dba8b32433273a2e70ed6c319181859f400f3b4 against current main 86c61d420601bc03f1a9c8bd144bb7cbe861b5bc passed build:test, 53 focused tests, and renderer architecture 114/114; the P2 reproduces there as well. git diff --check still reports the existing extra blank line at EOF in apps/desktop/src/renderer/locales/conversation-copy.ts:1068.

I did not run packaged Electron/native Windows or macOS flows, real Runtime Host reconnect, or a manual A→B→A navigation pass. No schema or migration change is present.

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.



function quoteKey(quote: QuoteRef): string {
return JSON.stringify([quote.text, quote.label ?? null, quote.sourceTurnId ?? null]);

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] Compare the complete quote contract before declaring a revision unchanged

quoteKey drops sourceSessionId, sourceSessionName, sourceCapturedAt, and sourceTruncated, although those fields are part of the persisted, user-visible session-snapshot quote. A reachable context-only edit is therefore refused: start editing a message with a session quote, remove that token, then select the same source Session again after a fresh snapshot. With unchanged snapshot text/label but a new capture time, the production helper reports canonicalChanged: true, revisionStagedContextUnchanged: true, and revisionSendGate: "unchanged"; two distinct Sessions with the same name/body collide too. The composer keeps quote removal and quote/session-reference selection enabled during revision (app-shell.tsx:2384-2386,2526-2531), and #5109 explicitly requires changing/replacing structured context to count as an edit. Please compare a normalized complete QuoteRef (including snapshot provenance) and add a regression for a recaptured session quote.

@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 bec3eb3a21d9dcfe867bccc973bdca39793245dc (14 files, +1352/-427). This update merges current main; the previous first-send quote snapshot and branch-owner cleanup remain intact. I independently traced the edit, prepare, send, retry, cancel, and Session-reference paths on this head. One P2 finding remains inline: the no-op gate still compares only the visible subset of a QuoteRef, so a valid provenance-only replacement is rejected as unchanged. I do not recommend merging until the comparison and regression coverage are corrected.

Current-head validation: clean npm ci; build:test; full typecheck; UI 684/684; Desktop 2776/2776; focused revision/send 40/40; renderer architecture 114/114; lint, format, and ASF headers. The hosted test check is green. Current main (538c37cb655ffeafc1829fc77359a6b7cfaa077a) is already the second parent, so the PR is 33 ahead / 0 behind and the merge tree is conflict-free. git diff --check still reports the existing extra blank line at EOF in apps/desktop/src/renderer/locales/conversation-copy.ts:1068.

I did not run packaged Electron or native Windows/macOS flows, a real Runtime Host reconnect, or manual A->B->A navigation. No schema or migration change is present.

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.



function quoteKey(quote: QuoteRef): string {
return JSON.stringify([quote.text, quote.label ?? null, quote.sourceTurnId ?? null]);

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: Compare the complete QuoteRef here. The no-op gate currently ignores sourceSessionId, sourceSessionName, sourceCapturedAt, and sourceTruncated. On this exact head, replacing the restored Session quote with either a freshly captured snapshot (same text/label, newer capture metadata) or an identically named same-content snapshot from another Session makes revisionStagedContextUnchanged() return true and revisionSendGate() return unchanged, so the UI refuses the legitimate context-only edit as “Nothing changed.” Include the canonical provenance fields in the key (or compare the complete normalized ref) and add a regression that removes and reselects a Session snapshot without changing the prompt text.

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 at 6e12c8f73: the no-op gate's quote key now covers every QuoteRef field — text, label, source turn, source session id and name, capture timestamp, and the truncation flag — so a re-captured snapshot at the same prompt text passes the gate as a real edit, while the stale capture of the same snapshot is still refused as unchanged.

The regression covers both directions: the re-captured snapshot (same text/turn, sourceCapturedAt 100 → 200) passes, and re-staging the stale capture against the newer source remains unchanged.

On the cross-layer note: the same caveat as the review thread above applies — the assembler closure has no renderer harness in this repo, so the gate behavior is pinned at the ui layer where both the source staging and the comparison live.

ggbdpq added a commit to ggbdpq/maka that referenced this pull request Sep 27, 2026
The no-op gate's quote key covered text, label, and source turn but
ignored the cross-Session provenance fields (sourceSessionId,
sourceSessionName, sourceCapturedAt, sourceTruncated). Removing a
restored Session snapshot and reselecting a fresher capture of the same
excerpt — same text and turn, newer capture metadata — therefore hit
the unchanged gate and the UI refused a legitimate context-only edit.
The key now covers every QuoteRef field.

Carries the apache#5274 review finding; regression covers the re-captured
snapshot at unchanged text and the stale-capture no-op.

Generated-by: GLM-5.3-Flash (ZCode)

@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 6e12c8f738f1d19d67e7c6764da3d7969279d72e (14 files, +1412/-427). The new two-file increment closes the previous P2: packages/ui/src/revision-staged-context.ts:66-79 now includes every QuoteRef field in the no-op comparison, and packages/ui/src/__tests__/revision-staged-context.test.ts:291-336 covers a same-text snapshot recaptured with newer provenance while retaining the stale-snapshot no-op. I traced the edit, prepare, send, retry, cancel, quote-selection, and Session-reference paths again and found no remaining P0-P3 issue on this head.

The regression is behaviorally demonstrated: a fully populated production-shaped QuoteRef probe passes replacements that change each of the seven fields individually on this head, while the same sourceCapturedAt-only replacement is rejected as unchanged on parent head bec3eb3a. Exact-head validation passed clean npm ci, build:test, full typecheck, UI 685/685, Desktop 2781/2781, focused revision/send tests 21/21, renderer architecture 114/114, lint, format, ASF headers, and locale hygiene.

This head is not merge-ready yet. Current main is ab021efda1bbb0e359937106ea1e556cac3b438b; the PR is 34 commits ahead and 15 behind, and both GitHub and a local merge-tree report conflicts in apps/desktop/renderer-architecture.json, apps/desktop/src/renderer/app-shell-revision-actions.ts, and apps/desktop/src/renderer/app-shell.tsx. The full PR git diff --check also still reports an extra blank line at EOF in apps/desktop/src/renderer/locales/conversation-copy.ts:1068, and the current head has no hosted checks. Please resolve those conflicts and rerun the gates on the resulting head before merge.

I did not run packaged Electron or native Windows/macOS flows, a real Runtime Host reconnect, or manual A->B->A navigation. No schema or migration change is present.

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.

@ggbdpq
ggbdpq force-pushed the fix/desktop-revision-structured-context branch from 6e12c8f to 9e0325c Compare September 27, 2026 09:47
@ggbdpq

ggbdpq commented Sep 27, 2026

Copy link
Copy Markdown
Contributor Author

Rebased onto the quotes-annotation tree (#5470) as a single commit (9e0325ce4) — the series crossed the #5470/#5742 rewrites of the same files, so replaying it commit-by-commit produced only orphaned intermediate states.

What carried over and how it meets the new tree:

  • The no-op gate still compares the full QuoteRef provenance (round-two P2 fix at 6e12c8f73), which now keys over the annotated quote fields too.
  • The send's quote snapshot still precedes the revision await gate; outside the revision window it falls back to the new lazy quotesForSend() (feat(ui): annotate a quoted excerpt where the gesture happens #5470), which reads the live bucket the re-key drains mid-revision.
  • editMessageDisabledAttachments/Quotes copy keys are dropped in favor of staging, now in the moved application/contracts locale module.
  • The renderer architecture ledger is regenerated from the merged sources; check-renderer-architecture passes.

Locally green: revision-actions 7/7, revision-staged-context 14/14, chat-turn answer identity 33/33. CI should re-run on this head.

@ggbdpq

ggbdpq commented Sep 30, 2026

Copy link
Copy Markdown
Contributor Author

Follow-up merge: main moved again (#5851) right after the previous resolution, so this re-merges d6876d708. The renderer-architecture.json conflict recurred (both sides touch the ledger); resolved by taking main's ledger as the base — which records #5851's moved files under features/conversation/ — and re-pricing every entry from the merged tree via check-renderer-architecture.mjs --write. app-shell.tsx now prices at 11,012 nonTriviaTokens / 76 importSpecifiers (real merged-tree values, within the ratchet; #5851's extraction lowered it further). Architecture check, diff --check, biome, and merge-tree against current main pass; revision suites were unaffected by the ledger-only change and remain green from the previous head's run.

@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 bf7776454dcf989e58c8ed8a4624fc5f13b0b1d8. The two commits since the previous review merge current main; their manual conflict resolutions only update apps/desktop/renderer-architecture.json for the current renderer inventory (featurePrivateModules at line 297 and AppShell token counts at lines 779-780). The effective PR still stages selected-message quotes, preserves user-added duplicate quotes on cancellation (packages/ui/src/revision-staged-context.ts:130-152), and refuses attachment-bearing edits rather than dropping attachments (packages/ui/src/revision-staged-context.ts:306-311). I found no new substantiated P0-P3 issue in this increment.

Node 24 clean install and workspace build:test passed. The renderer architecture check passed (123 tests), as did 25 focused revision tests. The current-head hosted test check is successful; a fresh-main merge-tree and diff-check are clean. I did not run a packaged Electron flow, native cross-platform UI, or the full test suite. These checks do not replace that validation.

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.

@ggbdpq

ggbdpq commented Sep 30, 2026

Copy link
Copy Markdown
Contributor Author

Conflict resolution pushed as 94bbff134: the branch now integrates current main (d7dffca98, including the #5869 Conversation-publication refactor) — third structural rebase of this PR, resolved at the merge-commit level.

Three files collided:

  • app-shell.tsx: took upstream's side (the messages/transientMessages/commitTranscript destructures moved into the conversation controller; no remaining consumers here). This PR's two stagedContext references survive untouched.
  • app-shell-revision-actions.ts: kept this PR's 121-line thin adapter (checkout-ours) and ported upstream's real semantic changes in that range into the @maka/ui env — RefBox → ReadonlyRef on the three refs (no ui-side writes exist) and the messages array → a readMessages() accessor — threaded through ...deps, so the desktop file needs nothing further.
  • renderer-architecture.json: upstream's version becomes the new base; ledger regenerated for the PR's file set.

Net against the new base: app-shell.tsx 9767 vs base 9769 tokens (the two stagedContext references offset by dead destructures both sides already dropped) — ratchet intact; app-shell-revision-actions.ts 482 (extraction dividend re-realized). strict-base against d7dffca98 passes (exit 0, base-checker cross-check included).

Tests: ui suite 714/714 (upstream added 4), desktop focused 18/18, ui + all four desktop tsconfigs typecheck green; the two fixture assemblies moved from messages: to readMessages: () => mirroring upstream's own test rename — zero test-expectation changes. Key-symbol checklist (ownerKey pair, ownedCount <= 0, locale keys, snapshot semantics) all verified present. Biome and ASF headers clean.

@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 94bbff134c83037f66cf90b42720036d0ebd99ff. This PR stages quotes from a selected message during edit-and-resend, re-keys the current quote plate to the revision child before submission, and refuses edits of attachment-bearing messages until target-owned attachment refs can be produced. The latest commit merges main and resolves the renderer architecture ledger and relocated message-reader wiring; I found no new substantiated P0–P3 issue in that increment.

I checked the revision prepare/send/cancel paths and merge resolutions, the current-head hosted test result, and a clean merge-tree against current main. Locally, Node 24 build:test, 59 focused revision/UI tests, 123 renderer architecture tests plus its strict-base check, and git diff --check passed. I did not run a packaged Electron or native Windows/macOS interaction. The attachment half of #5109 remains intentionally unsupported; this review does not assert merge readiness or replace the repository's required checks.

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.

Fourth structural catch-up for apache#5274: carries apache#5868 (Composer staging
moved into a persistent ComposerStagingProvider owner) and apache#5884 (task
archive timestamps).

Conflict resolution ports the revision staged-context semantics onto the
new owner structure: ComposerStagingSubmission gains optional owner-key
reads/clears (snapshot for captured submissions, live plate for the
revision re-key) and ComposerStagingCommands gains stagedContext(); the
frozen shell forwards the one composerStaging handle and the revision
assembler derives the gate probe and plate reads from it.
@ggbdpq

ggbdpq commented Sep 30, 2026

Copy link
Copy Markdown
Contributor Author

Conflict resolution pushed as ae47a6534: the branch now integrates current main (c838e1fa7, #5868 Composer staging → persistent owner + #5884). Fourth structural catch-up — this one rebuilt the PR's home turf, so the resolution is a semantic port, not a mechanical merge:

  • Two semantics coexist by design. refactor(desktop): move Composer staging into a persistent owner #5868's captureStaging() is capture-owns-a-snapshot (follow-up/swarm/graph paths keep that behavior untouched); the revision re-key path passes an explicit owner key and reads/writes the live plate through the same commands surface (quotesForSend(k) / clearQuotes(k) / new stagedContext() command). The frozen shell forwards exactly one composerStaging handle; the revision assembler derives the gate probe and plate reads from it — the shell ledger went 9614 → 9603 against the new base.
  • One non-negotiated removal, documented: refactor(desktop): move Composer staging into a persistent owner #5868 mechanically kept hasPendingContext/hasStagedQuotes send-side checks in the revision block, which would false-reject every edit of a quoted message (beginEdit pre-stages a quote, so hasStagedQuotes is permanently true). That is not a product decision refactor(desktop): move Composer staging into a persistent owner #5868 made — the PR's ui-gate (revisionSendGate + revisionMixedContextUnsupported toast) carries the behavior instead; the fix(desktop): allow unchanged edit-and-resend #5815 unchanged-resend test and upstream's own new-owner test are both green alongside ours.
  • All semantic assets verified alive by grep (ownerKey read/clear pair, ownedCount <= 0, multiset-diff clear, full-QuoteRef quoteKey, both locale keys, RevisionStagedContext, restoreQuotes, settlement copy).

Verification at ae47a6534: build chain OK; ui 714/714; desktop focused 57/57; typecheck --workspaces (all four desktop tsconfigs) exit 0; ledger regenerated on the merged tree and --strict-base --base c838e1fa7 exit 0 (re-checked independently post-push prep); epoch guard "No protocol changes" (this PR touches no protocol files); four staged checks done manually with --no-verify (husky EINVAL, known environment issue).

Generated-by: GLM-5.3-Flash (ZCode)

@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 ae47a65340b7389277d08f653b30d7484ed3fa32, the merge of current main into #5274. The integration adapts selected-message quote staging to the persistent Composer staging owner introduced by #5868. app-shell-revision-actions.ts:84-85 derives the pending-context gate and live staged-context read from that owner. The provider snapshots ordinary submissions but supports explicit live owner-key reads/clears for a revision (composer-staging-provider.tsx:49-71); after revision-staged-context.ts:506-507 moves the current quote plate to the child, composer-submit.ts:369,384 reads and clears that child key. I found no new substantiated P0–P3 issue in this merge resolution. The attachment-bearing edit remains intentionally refused rather than silently losing session-owned refs.

Locally on Node 24, workspace build:test, Desktop typecheck, 38 focused revision/staging tests, and the renderer architecture check (123 tests) passed. The current-head hosted test is successful, and a fresh-main merge-tree and git diff --check are clean. I did not run a packaged Electron UI flow, native Windows/macOS interaction, or the full local test suite. This is not a merge approval; feature acceptance remains with maintainers.

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.

…ntext

# Conflicts:
#	apps/desktop/renderer-architecture.json
@ggbdpq

ggbdpq commented Oct 1, 2026

Copy link
Copy Markdown
Contributor Author

Conflict resolution pushed as d8cf8bdcb: the branch now integrates current main (1e80e3b88, #5896 archived-task cleanup).

Lightest catch-up of the five: #5896's session-navigation surface does not intersect this PR's composer staging/quotes semantics at all — the only content conflict was the ledger's token count for the frozen shell, regenerated from the merged tree per practice (app-shell.tsx: 9603 → 9565 against the new base, downward only). The four-command staging surface (captureSubmission snapshot/live-plate split, stagedContext(), owner-keyed reads/clears) and all semantic assets verified alive by grep; both landmark tests stay green (#5815 unchanged-resend + this PR's in-flight re-keyed plate).

Verification at d8cf8bdcb: build chain exit 0; ui 714/714; desktop focused 129/129; all four desktop tsconfigs typecheck clean; ledger --write + --strict-base --base 1e80e3b88 pass; epoch guard "No protocol changes"; four staged checks done manually with --no-verify (husky EINVAL, known environment issue).

Generated-by: GLM-5.3-Flash (ZCode)

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

Incremental re-review of exact head d8cf8bdcbc757ac7f1a37a4c07d3a13afe2a04cf. The previously reviewed head was ae47a653.

What changed: ae47a653..d8cf8bdc is a single merge of upstream main 1e80e3b88 (#5896). There was no rebase. The PR's effective diff against its base is identical before and after the merge: 23 files, +1702/-435. The only exception is the renderer-architecture.json ratchet entry for app-shell.tsx, where nonTriviaTokens moved from 9614→9603 to 9576→9565. Main's own -38 is preserved, and the PR's -11 delta is unchanged, so the conflict resolution is arithmetically consistent.

Scope checked:

  • Overlapping files: main's app-shell.tsx change (the archivedTasksBridge now uses projectScopes/commands; localProjects was removed) does not touch the revision/quote-staging paths.
  • Merge cleanliness: the head already contains the current origin/main tip.
  • Protocol: no protocol files are touched.
  • CI: the hosted test check is green at this head.
  • Prior reviews: the review at ae47a653 had no outstanding findings to re-verify.

Findings: none. No P0–P3 regressions were found in this update.

Not exercised: local build/tests and the renderer architecture check were not re-run. The review host's disk was full, so this round relies on comparing git objects and on the hosted CI result. No packaged Electron UI flow was run.

Automated review notice: This comment was posted by an automated review agent. It is not an independent human review and does not replace one.

…ntext

# Conflicts:
#	apps/desktop/renderer-architecture.json
@ggbdpq

ggbdpq commented Oct 2, 2026

Copy link
Copy Markdown
Contributor Author

Conflict resolution pushed as 4e93a8e28: the branch now integrates current main (c7fa6bb6a, 8 commits — #5904/#5910/#5916/#5918/#5913/#5893/#5892/#5906).

First catch-up with zero semantic adjudication: the only conflict was the renderer-architecture.json ledger, regenerated from the merged tree per practice. app-shell.tsx: 9565 → 9069 against the new base, downward only — the drop is upstream's own #5892/#5893 relocating root diagnostics and context compaction out of the shell; this PR's −11 delta is preserved. The relocation also required two mechanical ledger migrations (stale legacyPaths entry removed, new feature-private path for the compaction module added). --strict-base --base c7fa6bb6a passes; auto-merged files were opened and verified (app-shell.tsx diff vs upstream is exactly the one-line composerStaging passthrough; the deleted editMessageDisabled* copy keys stay deleted).

Compatibility check on the closest newcomer, #5904 (send-slot resume): resumeShown requires !hasSendableContent, and staged quotes make the slot send-able — so resume is a slot switch, never a rejection path for staged edits. The relocated use-shell-resume.ts references none of this PR's staging symbols, and upstream's new createStagedFollowUp path actually consumes this PR's captureStaging — the cross-wiring holds on both sides. No packages/protocol/ files touched, so no epoch bump.

Verification at 4e93a8e28: build chain exit 0 (core→storage→runtime→mcp→computer-use→runtime-host→ui); ui 718/718 (714 baseline + 4 new from #5904); desktop focused 136/136; all four desktop tsconfigs + ui typecheck clean; ledger --write idempotent + strict-base pass; epoch guard "No protocol changes (epoch 202)"; ASF headers (4259 covered) + git diff --check clean. Full desktop suite, Storybook smoke and e2e are left to hosted CI; the auto-merged preload/bridge-contract surface (#5904's sessions:queryResumeLatest IPC) is typechecked locally only. Environment note: this round ran on a fresh macOS clone with --ignore-scripts, so hooks were not installed — the applicable staged checks were run manually.

Generated-by: GLM-5.3-Flash (ZCode)

@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 4e93a8e282a29a6c554db809c37a7f6efad91c46 (23 files, +1702/−435, 14 commits). This card asked for a re-review against the findings already on this PR, so that is where I concentrated.

No P0–P3 findings; two of my earlier P2s are fixed, an earlier P3 is fixed, and the remaining P2 is now a documented boundary rather than an open defect.

My P2 about stale quotes — fixed, and explicitly so. The branch child is now built from the plate's current quotes rather than the snapshot the edit started from: staged.restoreQuotes(newSession.id, staged.quotes) followed by staged.clearQuotes(startedDraft.sourceSessionId), with a comment that states the reason the finding gave — entries the user removed or re-annotated during the edit must not reappear — and cites this review. The ordering is also deliberate: the re-key happens only after every rollback check has passed, so a failed preparation leaves the plate on the source key.

My P2 about missing copy — resolved. The footer block (regenerateAgain, requestRegenerate, …) that the earlier revision referenced without it existing on main is now defined, and I checked that it is present in all three locales (zh-CN, zh-TW, en). One observation rather than a finding: I could not find a consumer for those keys at this head, so they may currently be unused catalogue entries.

The P3 about cancelling newly staged duplicates — fixed. The kept-check now treats any non-positive remaining count as fully user-owned (ownedCount <= 0), which is what makes every plate entry past the edit's own per-key quota survive the cancel.

The remaining P2 — attachments — is now a stated boundary rather than a gap. Attachment-bearing messages still take the early return instead of creating a revision draft. What changed is that the refusal is now explicit and justified in place: the comment records that Host-side attachment ownership does not follow a revision copy — the copied transcript stops before the selected turn, so no target-owned refs exist to restage — and the user gets revisionUnavailableTitle / revisionAttachmentsUnsupported copy rather than a silent drop, with both review references attached. The capability of editing a message with attachments therefore remains unimplemented by design; I am recording that as the product boundary it now claims to be, not as an open defect.

Gate on this head: test is green; mergeable is true and the state is blocked.

What I could not judge

  • Scope, stated honestly: I verified the four prior findings and the code around them, not all 23 files line by line.
  • No Electron run, so the edit-and-resend flow is read from the controller, the staged-context helper and the tests 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.

Reviewed 4e93a8e282a29a6c554db809c37a7f6efad91c46 (23 files, +1702/−435, 14 commits). This card asked for a re-review against the findings already on this PR, so that is where I concentrated.

No P0–P3 findings; two of my earlier P2s are fixed, an earlier P3 is fixed, and the remaining P2 is now a documented boundary rather than an open defect.

My P2 about stale quotes — fixed, and explicitly so. The branch child is now built from the plate's current quotes rather than the snapshot the edit started from: staged.restoreQuotes(newSession.id, staged.quotes) followed by staged.clearQuotes(startedDraft.sourceSessionId), with a comment that states the reason the finding gave — entries the user removed or re-annotated during the edit must not reappear — and cites this review. The ordering is also deliberate: the re-key happens only after every rollback check has passed, so a failed preparation leaves the plate on the source key.

My P2 about missing copy — resolved. The footer block (regenerateAgain, requestRegenerate, …) that the earlier revision referenced without it existing on main is now defined, and I checked that it is present in all three locales (zh-CN, zh-TW, en). One observation rather than a finding: I could not find a consumer for those keys at this head, so they may currently be unused catalogue entries.

The P3 about cancelling newly staged duplicates — fixed. The kept-check now treats any non-positive remaining count as fully user-owned (ownedCount <= 0), which is what makes every plate entry past the edit's own per-key quota survive the cancel.

The remaining P2 — attachments — is now a stated boundary rather than a gap. Attachment-bearing messages still take the early return instead of creating a revision draft. What changed is that the refusal is now explicit and justified in place: the comment records that Host-side attachment ownership does not follow a revision copy — the copied transcript stops before the selected turn, so no target-owned refs exist to restage — and the user gets revisionUnavailableTitle / revisionAttachmentsUnsupported copy rather than a silent drop, with both review references attached. The capability of editing a message with attachments therefore remains unimplemented by design; I am recording that as the product boundary it now claims to be, not as an open defect.

Gate on this head: test is green; mergeable is true and the state is blocked.

What I could not judge

  • Scope, stated honestly: I verified the four prior findings and the code around them, not all 23 files line by line.
  • No Electron run, so the edit-and-resend flow is read from the controller, the staged-context helper and the tests 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.

@ggbdpq

ggbdpq commented Oct 3, 2026

Copy link
Copy Markdown
Contributor Author

Conflict resolution pushed as 868ed9512: the branch now integrates current main (255ae23ae — 6 commits: #5847/#5911/#5931/#5927/#5934/#5935).

The one semantic adjudication this round: #5935 moved the revision lifecycle assembly below the shell, rebuilding it inline (405 lines) without staged-context support. Resolution keeps upstream's assembly position and ownership (ComposerSubmissionProvider) while this PR's @maka/ui staged-context implementation stays the feature owner — the desktop-side revision-actions.ts is now a thin adapter delegating to @maka/ui's createRevisionActions, deriving hasPendingAttachments and stagedContext from the persistent staging handle. The export surface matches upstream's consumers one-for-one, and upstream's architecture guards (shell must not import createAppShell*Actions; Conversation's public entry must not export createRevisionActions) hold as-is. Upstream's own 25 neighborhood tests pass against the adapter: with empty plates the adapter's paths are per-step equivalent to the inline version (quote gate / mixed-context gate / re-key / dual-key cleanup are all no-ops on empty input).

#5934 (checker): the renderer-architecture checker gained R2 root-symbol-use and retained-root-hook checks; the ledger's numeric schema is unchanged. The ledger was regenerated with the merged tree's new checker — every count moves down (app-shell.tsx: 43→42 declarations, 63→54 specifiers, nonTriviaTokens 8221, equal to upstream's own count for its tip), and --strict-base --base 255ae23ae passes with the new checker's own base cross-check.

#5927 re-derivation: the compatibility conclusions from the previous merge still hold, with a new owner. composer-send-policy.ts is untouched upstream (resumeShown requires !hasSendableContent; staged quotes make the slot send-able — a slot switch, never a rejection path for staged edits); the relocated use-shell-resume.ts / resume-availability.ts still reference none of this PR's symbols; captureStaging wiring survives in the new assembly (captureStaging: staging.captureSubmission). What changed: the resume offer now lives in the composer send slot instead of the turn-footer button — during revision editing hasSendableContent stays truthy, so Resume and revision Send are mutually exclusive and no new conflict path appears. (The ui suite is 717 now: upstream itself removed one resume-notice test in #5927.)

Verification at 868ed9512: build chain exit 0; ui 717/717; desktop focused 150/150 (this PR's 4 suites + composer-submission-owner / resume-availability / stop-action / interrupted-resume / quote-companion ×4 / revision neighbors); all four desktop tsconfigs + ui typecheck clean; ledger --write idempotent + strict-base pass; epoch guard "No protocol changes (epoch 202)"; ASF headers (4275 files) + git diff --check clean; biome whole-repo clean. Full desktop suite, e2e, Storybook smoke and the new pixel-diff tool are left to hosted CI. Hooks ran normally this round (no --no-verify).

Generated-by: GLM-5.3-Flash (ZCode)

@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 868ed9512b3ad5b8f51970d2a98330ea95d8c9db — the increment plus CI, as scoped.

The increment is only the base move: the PR's own work is unchanged. git range-diff marks this PR's commits as = against the range I reviewed, with the differences being commits that arrived from main itself. My previous conclusions therefore carry over unchanged — including that the attachment-bearing edit case is now an explicit, justified refusal rather than a gap, and that the two earlier P2s are fixed.

Two current-state facts worth flagging, neither a code finding:

  • This revision is mergeable=false — it conflicts with its new base, so it needs a rebase or conflict resolution before it can land. The conflict is in the base relationship, not in the change itself.
  • There are no check runs on this head, so I make no CI claim. The previous head's gate was green; this one has no gate result at all.

Scope: because the rebase changed nothing in the PR, I did not re-audit its logic — re-verifying an unchanged tree would add nothing. What I verified is the rebase's fidelity and the two facts above.

What I could not judge

  • Whether the conflict is mechanical or reaches the quote/attachment logic; that needs the rebase to be done, at which point the resolution should be reviewed.
  • No desktop run.

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.

@ggbdpq

ggbdpq commented Oct 3, 2026

Copy link
Copy Markdown
Contributor Author

Follow-up conflict resolution pushed as 95bd2c4e3: main moved again 2.5 minutes after the previous push (#5936 landed at 08:33Z, before CI could start on 868ed9512 — a dirty PR triggers nothing), so this merge supersedes it and integrates current main (229e1b466 — #5936 retire Desktop bridge (R2 M5) + #5937 AppShell command/project actions moved to their owners). The PR's effective diff vs main is unchanged: 23 files, +1692/−382.

Ledger: both #5936 and #5937 rewrite renderer-architecture.json — the bridge retirement empties the bridgePaths entries (app-shell.tsx 5→0, effects 1361→886→856 after #5937, command-actions 6→0) and #5937 removes app-shell-project-actions.ts from the legacy region entirely. Resolution: upstream's ledger taken as the new baseline, regenerated by the checker from the merged tree (no hand-edited numbers). This PR's delta remains exactly 8 pure decreases, zero increases (app-shell.tsx imports 42→41, specifiers 60→51, …), and --strict-base --base 229e1b466 passes.

Semantic safety (M12 check): #5936 ∩ this PR = {ledger, composer-staging-owner.test.ts — the latter auto-merged and passes 15/15 on the merged tree, including the case asserting AppShell no longer reaches the retired bridge}; #5937 ∩ this PR = {ledger only} — it touches none of this PR's source files or tests. All staged-context assets verified alive on the merged tree (RevisionStagedContext/quoteKey/restoreQuotes/beginEdit gates/revisionSendGate/the composerStaging chain), and the thin adapter's shell-copy dependencies (getShellCopy, localizedShellErrorMessage) are still exported at 229e1b466.

Verification at 95bd2c4e3: build chain exit 0; ui 717/717; desktop focused 152/152; all four desktop tsconfigs + ui typecheck clean; ledger --write idempotent + strict-base pass; epoch guard "No protocol changes (epoch 202)"; ASF headers (4284 files) + git diff --check clean; biome whole-repo clean. Full desktop suite, e2e and Storybook checks are left to hosted CI.

Generated-by: GLM-5.3-Flash (ZCode)

@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 95bd2c4e3144890ef1cdcbc4c4369d619a82c491 — the rebase's fidelity and what the conflict resolution changed, using the reviewed-head-versus-merge-base comparison.

No P0–P3 findings.

The rebase is faithful, and the conflict resolution changed exactly four lines — all of them generated ledger counts. Comparing "reviewed head vs its merge-base" (868ed951 against 255ae23a) with "this head vs its merge-base" (95bd2c4e against 229e1b46) gives 2074 lines on both sides, with four lines unique to each side — i.e. four replaced, none added, none lost. The four are:

+        "importDeclarations": 41,     -        "importDeclarations": 42,
+        "importSpecifiers": 51,        -        "importSpecifiers": 60,

Those are renderer-architecture.json's per-file counts, re-baselined because two further PRs landed in main beneath this one between the revisions. So the resolution reintroduced no hand-written code change: the only difference between what I reviewed and what is now proposed is the generated ledger adapting to the new base. Nothing was lost, nothing wrongly retained, and no move was duplicated.

The earlier conflict is resolved. The revision I reviewed was mergeable=false; this head is mergeable true.

Gate on this head: test is completed/success — where the previous head had no check runs at all, so this is the first CI verdict available for it.

What I could not judge

  • I did not re-audit the logic, since the comparison shows the only delta is ledger counts.
  • No desktop run, so the quote/attachment behaviour is unchanged from my previous pass by construction rather than re-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.

@ggbdpq

ggbdpq commented Oct 3, 2026

Copy link
Copy Markdown
Contributor Author

Conflict resolution pushed as 54babb219: the branch now integrates current main (f7633c3d7 — #5826, 57 files).

Ninth catch-up, zero semantic adjudication. #5826 (ACP executor modes + scoped catalog lifecycle) touches the submission area but is purely additive: executor-submission.ts gains a session activator and error validation, the composer area gains sendBlocked/executorPicker.disabled gating in parallel to (not across) the composerStaging chain, and the canary suite extends assertions rather than rewriting them. All semantic assets verified alive on the merged tree: quoteKey with comment, RevisionStagedContext, restoreQuotes, composerStaging wiring, the three beginEdit gates, swap, and the thin revision-actions.ts adapter delegating to @maka/ui.

The only textual conflict was renderer-architecture.json: regenerated with the merged-tree checker against the upstream baseline — this PR's delta stays purely decreasing (8 line items), no hand-edited counts. Net diff vs the new base remains 23 files, +1692/−382, identical to the previous round.

Verification: epoch guard --base f7633c3d7 → "No protocol changes (epoch 204)", exit 0; renderer-architecture --strict-base cross-checked under the base checker, exit 0; ui 731/731; desktop focused 154/154 with the composer-staging-owner canary 15/15; four tsconfigs + ui typecheck 0 errors; ASF 4292 clean; biome format . 2305 files clean; git diff --check clean. Honest boundary: full desktop suite not re-run after focused cleanup (dist orphans of renamed sources, unrelated); e2e/Storybook smoke and Linux lanes left to CI.

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

Incremental re-review of exact head 54babb21. The previous reviewed head was 95bd2c4e, which was clean.

The increment is one merge of current main (f7633c3d). It is a fast-forward and the PR's own work is unchanged. The PR's own diff (head vs its merge-base f7633c3d) is still 23 files, +1692/-382. Compared with the previous head's own diff (95bd2c4e vs 229e1b46), it differs in a single unchanged context line of apps/desktop/renderer-architecture.json: app-shell.tsx's nonTriviaTokens is now 8009 instead of 8017. That value is main's own, not a PR edit. The PR still only lowers counts in the ledger.

The only file touched by both main's increment and this PR is renderer-architecture.json, so the merge carries no semantic interaction with the quote-staging code.

No P0-P3 findings.

CI: run 37140156938 passed, which includes the renderer architecture gate. The PR is mergeable.

@ggbdpq

ggbdpq commented Oct 3, 2026

Copy link
Copy Markdown
Contributor Author

Conflict resolution pushed as 3a3c2e31: integrates current main (7c90bac2d — #5951/#5952/#5871/#5880/#5954). Effective diff vs main unchanged: 23 files, +1692/−382.

The notable fact: #5954 (R2 M3) consumes this PR's staging handle rather than bypassing it. The new delivery-recovery owner registers staging.restoreContext as queue.draftContextRestorer, and its recovery path routes context.quotes → staging.restoreQuotes — a #5274 method now called by upstream code. The thin-adapter architecture from the earlier merge survives intact: use-composer-submission.ts keeps the staging injection (input type, revision deps, and both captureStaging: staging.captureSubmission call sites for onSend and stagedFollowUp), and the desktop-side revision-actions.ts adapter still derives hasPendingAttachments/stagedContext from the handle. Both real conflicts were mechanical: the staging contract gains upstream's RestoredDraftContent import alongside ours (restoreContext() and stagedContext() coexist on the interface), and the ledger was regenerated from upstream's new baseline (R2 M3/M5 removed the legacy entries — detail-panel, markdown, session-ui-reads, live-turn, turn-action-registry) with this PR's delta remaining 8 pure decreases (app-shell.tsx 35→34 declarations, 52→43 specifiers).

Verification at 3a3c2e31: build chain exit 0; ui 731/731; desktop focused 198/198 across 20 suites (this PR's 4 + resume/stop-action/interrupted-resume + quote-companion ×4 + revision neighbors + #5954's new owners: turn-actions, session-ui-reads); composer-submission-owner 29/29; checker self-tests 167/167; all four desktop tsconfigs + ui typecheck clean; ledger --write idempotent + --strict-base pass; epoch guard "No protocol changes (epoch 204)"; ASF headers (4294 files) + git diff --check clean; biome whole-repo clean. Full desktop suite is left to hosted CI. Process note: the first commit accidentally carried the baseline ledger (regenerated file not re-staged); caught in post-commit verification and amended — HEAD re-verified with both plain and strict-base checks.

Generated-by: GLM-5.3-Flash (ZCode)

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

Incremental re-review of exact head 3a3c2e31. The previous reviewed head was 54babb21, which was clean.

The increment is one merge of current main (7c90bac2). The PR's own work is unchanged. All five PR commits are identical in git range-diff (daf8229f..b97145c7, all =). The PR's own diff (head vs merge-base 7c90bac2) is still 23 files, +1692/-382. Compared with the previous own diff (54babb21 vs f7633c3d), the only differences are context lines and the absolute counts in apps/desktop/renderer-architecture.json.

Merge resolution checked. Main's #5954 (moving the Composer's reads, edits and delivery recovery below the shell) overlaps this PR in four files, and the merge had two conflicts:

  • composer-staging-contract.ts: both imports are kept (RevisionStagedContext from this PR, RestoredDraftContent from main). ComposerStagingCommands now carries both main's restoreContext and this PR's stagedContext, and the provider and the command proxy implement both.
  • renderer-architecture.json: app-shell.tsx is now importDeclarations 34 and importSpecifiers 43, which is main's 35/52 minus this PR's own -1/-9 (the same delta as before, 42->41 and 60->51). The other ledger entries keep the PR's -1 deltas on top of main's new values.
  • use-composer-submission.ts: the PR's staging hand-off to the revision lifecycle applies cleanly beside main's new turnActionRegistry, useShellResume and draftContextRestorer wiring.

Main's new restoreContext routes a withdrawn send's quotes through the existing restoreQuotes (use-composer-quotes.ts:97). That is the same primitive this PR already relies on, so the merge introduces no new path into the PR's per-key quote accounting.

No P0-P3 findings.

CI: run 37161137422 (test) passed on 3a3c2e31, including typecheck and the renderer architecture gate. The PR is mergeable; merge is BLOCKED only because review is required.

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.

4 participants