Conversation
|
Marking draft while the renderer debt ratchet question is settled — see the gate comment below. The renderer architecture strict-base check freezes
Happy to do either — flagging before burning another CI round. |
|
No description provided. |
|
The ratchet conflict is resolved without touching the frozen budgets: the whole revision lifecycle (beginEdit / prepare / cancel / rollback + copy-attempt bookkeeping) moved into Measured against the frozen base budgets: Heads 0a5af05 / 0a5af05 have not started a CI run for a few hours (the |
|
CI is green on 0a5af05 (2/2, including the architecture ratchet) and the review-required gate is the only open item — marking ready for review. Path taken for the ratchet: option 1 (the lifecycle moved into |
|
Adversarial root-cause after five red rounds — with the base ledger entry diffed metric-by-metric, the blocker is now precisely characterized, and it is a design decision rather than a patching problem:
So the remaining decision is binary and maintainer-owned:
Everything else in this branch is verified: revision-actions tests 3/3, ui chat-turn 30/30, biome and ASF clean, ledger refreshed in-tree. The branch stays as the working proposal; happy to re-shape once the direction is picked. |
Astro-Han
left a comment
There was a problem hiding this comment.
1 — sanction the edge, but land the sanction in the checker's target rules rather than the ledger numbers.
I verified the mechanism before answering: --strict-base re-derives base debt from the merge-base commit (loadBaseConfig), so editing the committed renderer-architecture.json entry cannot clear the violation — the current file genuinely gains a dependency key the base lacks. And resolveDependency returns undefined for bare package specifiers, so @maka/ui can never satisfy isSanctionedDependencyTarget today. A ledger-number bump alone will stay red either way.
Option 2 is strictly worse on the mechanism's own terms: a new renderer module is itself forbidden (new unclassified renderer source files are forbidden outside approved legacy directories; new legacyAppShell debt entries are forbidden), so it needs a sanction too — same cost, plus a file that exists only to carry one import edge.
Suggested shape for option 1: treat @maka/ui — the package renderer ownership is migrating into — as a sanctioned dependency target for legacyAppShell importers, the same way validated copy catalogs already get bare-package imports for free. The ratchet exists to stop the shell absorbing new ownership; depending on the destination package is the opposite of debt, and the token/specifier budgets still bound every file. If you want it narrower, a per-importer exception in the config works too — but the broad version covers every future migration PR without a fresh exception each round.
中文版
选 1,但豁免要落在 checker 的 target 规则上,不是账本数字:--strict-base 会从 merge-base 重新推导 base 债务,改 renderer-architecture.json 的条目消不掉这条红;裸包名 resolveDependency 返回 undefined,@maka/ui 永远过不了 isSanctionedDependencyTarget。方案 2 更差:新 renderer 模块本身就违反「新文件禁止」「新账本条目禁止」,同样要豁免还多一层纯搬 import 的间接文件。建议把 @maka/ui(所有权正在迁入的包)列为 legacyAppShell importer 的 sanctioned target,与 copy catalog 免计裸包同道理;想窄就按 importer 白名单,但宽版能覆盖后续所有迁移 PR。
AI assistance: I used Devin to trace the ratchet's base-derivation and sanctioned-target paths; the assessment is mine.
Astro-Han
left a comment
There was a problem hiding this comment.
Review — #5274 restage a selected message's quotes and attachments on edit
Thanks for this — moving the lifecycle into @maka/ui/revision-staged-context and the fail-closed narrowing in chat-turn.tsx (only directoryReferences keep the gate) both look right, and the refactor of app-shell-revision-actions.ts into a thin assembler is a genuine improvement. The two new @maka/ui tests do pin the un-gating, and renderer-architecture.json shrinks (2155 → 508 tokens for app-shell-revision-actions.ts), so the ratchet is happy.
I could not convince myself that the restage survives the revision commit, though. Details below, most important first.
1. restageRevisionAttachments can never find the rewritten refs — a revision copy excludes the revised turn
packages/ui/src/revision-staged-context.ts:155-163 looks up the copied user message in the branch child transcript and treats a miss as "nothing to restage":
const copiedMessage = copiedMessages.find(m => m.type === 'user' && m.turnId === sourceTurnId);
const rewritten = [...(copiedMessage?.attachments ?? [])];
… remove every staged attachment …
if (rewritten.length > 0) staged.restoreAttachments(targetSessionId, rewritten);But the Host copies a revision with the exclusive boundary — the revised turn is deliberately not in the copy (that's the "rewound to before that message" semantics):
packages/runtime-host/src/server/session-revision-coordinator.ts:375-378—createConversationCopySlice(source.messages, input.sourceTurnId, kind === 'revision' ? 'before' : 'through')packages/runtime/src/conversation-copy.ts:258-260— for'before',retainedTurnIds = turnOrder.slice(0, sourceIndex), i.e. the source turn is dropped (see the assertion atpackages/runtime/src/__tests__/conversation-copy.test.ts:714).
I confirmed the slice behaviour against the built packages/runtime/dist/conversation-copy.js: slicing ['turn-1','turn-2'] with 'before' at turn-2 yields ['turn-1'] only, and editing the first turn of a session yields an empty transcript. So rewritten is always []: the swap deletes the plate and restores nothing, i.e. the attachments are dropped exactly as before — just a step later, and now with the user having been told they were restaged.
Two concrete consequences:
- The in-flight submit keeps the source-owned refs and main rejects them.
sendWithAttachmentscaptures the payload beforesend()(apps/desktop/src/renderer/app-shell.tsx:1759), so the swap — which runs insideprepareRevisionSend— cannot change this send. The submit therefore carriessession_filerefs owned by the source session into the branch child, andretainedAttachmentsForSessionthrows"Retained attachment belongs to another Session"(apps/desktop/src/main/runtime-host-session-execution-ipc-main.ts:910-925), surfacing as a generic "Action failed" toast. - A retried send loses them silently. Post-commit,
prepareRevisionSendreads the live plate (now empty).revisionSendGateonly compares lengths (0 > 1is false) and the text differs, so the gate returns'pass',draft.draftSessionId !== draft.sourceSessionIdshort-circuits totrue, and the replacement goes out with no attachments and no quotes.
So I think the "the Host's copier already rewrote them … (no protocol change)" premise needs revisiting: something has to produce target-owned refs for the revised message — a copier change (retain the source turn's attachments into the target), re-ingesting into the branch child, or letting main rewrite/accept the source refs on sessions:send.
2. Nothing carries the restaged context across the commit's draft-key change
Both plates are keyed by the active session (attachmentDraftKey = activeId ?? NEW_TASK_PENDING_KEY, apps/desktop/src/renderer/app-shell.tsx:371-372), and the restage writes them under the source session key (restoreQuotes(sessionId, …) / restoreAttachments(sessionId, …) in beginEditUserMessage). After openSessionInChat(newSession.id) the active key is the branch child, so selectPending returns [] for both plates (packages/ui/src/pending-items.ts:41-43) and they go blank right after the "Ready to edit and resend" toast — quotes have no swap path at all.
That also makes the PR's headline claim ("the plates make the carried context visible and explicitly removable") untrue past the commit: the user sees the context until they press send, then it vanishes. A manual pass of reproduction case B in #5109 should show this immediately. Restaging under the branch-child key (or making the staged context follow the draft across the commit) is what I'd expect here.
Related: inside the swap, stagedContext() and removeAttachment come from the closure captured when the send started (useStableActions publishes through a layout-effect ref), while restoreAttachments takes an explicit ownerKey and removeAttachment/removeQuote bind to the live draftKey. "Clear by index, then restage under ownerKey" therefore mixes two different owners — worth making the owner explicit on the mutators if this design stays.
3. Test coverage for the commit / send half is missing
packages/ui/src/revision-staged-context.ts has no test file, and the pure helpers (revisionSendGate, restageRevisionAttachments, clearRevisionStagedContext, revisionStagedContextUnchanged) are the easiest things in the PR to unit-test. On the desktop side the suite still only drives beginEditUserMessage — prepareRevisionSend and cancelRevisionDraft have no coverage at all, so the swap, the moved no-op refusal, and the cancel-time plate cleanup are all untested. A test feeding the swap a realistic branch-child transcript (source turn absent, earlier turns' refs rewritten) would have caught #1.
Also unverified by tests: the 'conflict' gate (staged quote / pending directory during an edit) and cancel restoring the pre-edit plates.
Nits
revision-staged-context.ts:376-379refuses an edit when the composer has staged quotes, but toastscopy.revisionDraftAttachmentConflict("The composer already has pending attachments…"). SincehasPendingAttachmentsis bound tohasPendingContext(attachments or directories) in the desktop env, this is the only quote-specific refusal and it needs its own copy key in all three locales. Same string reuse for the gate at:493-498:revisionAttachmentsUnsupportednow reads "Editing cannot mix newly staged attachments with the restored ones…", which is wrong when the added context was a quote or a directory reference.revisionStagedContextHasAdditions(:114) is exported but never used;revisionSendGate(:183-187) inlines the same three conditions. Pick one so the two can't drift.clearRevisionStagedContext'spreviousQuotesparameter is always[]at its only call site (:624), so the restore branch is unreachable — drop it or use it.attachmentToPending(:68-77) duplicatesretainedToPending(packages/ui/src/use-composer-attachments.ts:152-160, not exported). It only feedsattachmentKey, which ignoresstagingKey, so the syntheticrevision:${JSON.stringify(...)}key is dead weight.- The new
"./revision-staged-context"subpath inpackages/ui/package.jsonis unused — the desktop imports the barrel (@maka/ui). Value imports from the barrel are already common in the renderer, so this is cosmetic; either use the subpath or drop the entry.
Verified as fine
- The
chat-turn.tsxgate now fails closed only ondirectoryReferences, and the reason chain (directory → transformed → running) is coherent. - No other consumer of the removed
editMessageDisabledAttachments/editMessageDisabledQuoteskeys exists in the repo (onlychat-turn.tsxandconversation-copy.tsreferenced them). - Edit → resubmit ordering and dedup: source quotes/attachments are carried in original order, and
beginEditUserMessagerefuses while anything is staged, so no duplication on resubmit; cancel clears both plates and restorespreviousComposerText. - The no-op-send refusal moved cleanly out of
app-shell.tsxintoprepareRevisionSend, with the duplicated pre-checks removed and a comment left behind atapps/desktop/src/renderer/app-shell.tsx:1606-1612. revisionStagedContextUnchangedcompares against the source-owned refs captured on the draft, so a post-commit retry isn't misreported as "unchanged".
|
Both commits pushed: the checker sanction (d53b28f, your 09-15 direction) and the review fixes (ceccd38). #1 (restage can never find the rewritten refs) — accepted, and resolved by owning the limitation rather than patching the symptom. I re-verified the exclusive #2 (nothing carries the staged context across the commit) — fixed for quotes, moot for attachments. After every rollback check has passed, #3 (missing coverage) — added where the code lives. New Nits — quote-conflict refusal and the send-gate conflict each got their own key in all three locales ( Checker (d53b28f): Verification: @maka/ui 12/12 new tests + desktop 15/15 across the revision/catalog/first-send suites; biome clean on all seven touched files; |
|
Merged latest Conflicts (2):
Verification: the ledger check passes ( Everything actionable from your 09-17 review remains in place (restage removal, quote re-keying onto the branch-child key, the new |
|
Merged latest Conflicts (3):
Verification: ledger check passes plain; biome clean on the touched locale files; the checker fixture suite is 112/113 with the single failure being the known Windows-only |
|
Merged latest Conflicts (4), resolved on top of the transcript-publish simplification (#5566/#5494 territory):
Main's own tests for the new flow ("prepares the revision without opening another transcript consumer", "surfaces a failed preparation instead of swallowing it behind rollback") came through the merge and pin the ported semantics; the refused-retry world fixture gained the Verification: full workspace build green after reinstalling dependencies (the merge carries the No review responses outstanding on my side — both of @me2seeks's earlier requests were addressed in |
|
The red check was my process failure, and the fix is pushed at head Root cause: when I resolved the previous merge I committed the conflict resolution first and made the lifecycle-port edits afterwards — then pushed only the ledger/test follow-up commit. The four files carrying the actual port (the What went out now:
Verified after the rebuild: full workspace build green (0 type errors); |
hqhq1025
left a comment
There was a problem hiding this comment.
Reviewed current head 5746d5c329c0e8fabcc2c0f872b1609a6500ec7e (15 files, +1316/−425). The change stages a selected message's quotes in the composer, refuses edits to messages carrying session-owned attachments, gates no-op/mixed-context replacements, and moves the revision lifecycle into @maka/ui (packages/ui/src/revision-staged-context.ts:93-151,292-365,442-527). I traced the Desktop send path and quote-bucket ownership, plus the new lifecycle and Desktop action tests. One P1 finding is attached inline: the first replacement send loses its quotes and leaves them staged for a later send.
The current-head test check passes, but a fresh synthetic merge with current main conflicts in apps/desktop/renderer-architecture.json; the PR diff also has a trailing blank-line warning in apps/desktop/src/renderer/locales/conversation-copy.ts. Resolve the conflict and revalidate a new head before merge. I did not run local tests or Electron E2E (Node 18/no installed dependencies), and have not exercised real Host failure/reconnect or A→B→A navigation. No database schema/migration change appears in this PR. This is not a merge approval.
Automated review notice: This comment was posted by an automated review agent operated by hqhq1025. It is not an independent human review and does not replace one.
| // — a revision copy excludes the revised turn, so the copied transcript | ||
| // cannot be their source. Re-keyed only after every rollback check has | ||
| // passed, so a failed preparation leaves the plate on the source key. | ||
| staged.restoreQuotes(newSession.id, startedDraft.originalQuotes); |
There was a problem hiding this comment.
[P1] Preserve the branch-owned quotes for the same in-flight send. sendWithAttachments is still executing in the source session's render closure while awaiting prepareRevisionSend() (app-shell.tsx:1499-1508). That closure's pendingQuotes is the source-key array from useComposerQuotes. Here you append the quotes to the new child key, then clearQuotes(sourceSessionId) mutates the source array to empty. When the same call resumes, app-shell.tsx:1658-1664 reads that now-empty array and omits quotes from send; the child bucket remains populated because the success path also closes over the source-key clearQuotes. Thus editing a quote-bearing message and sending a changed text silently drops its quote on the first replacement, then may carry it into an unrelated later send. The new tests assert that re-keying occurred but do not send through this production closure. Capture the intended quote payload before clearing the source bucket or read/clear the child bucket by explicit owner, and add a cross-layer first-send regression.
There was a problem hiding this comment.
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.
ded432a to
314dc7d
Compare
hqhq1025
left a comment
There was a problem hiding this comment.
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]); |
There was a problem hiding this comment.
[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
left a comment
There was a problem hiding this comment.
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]); |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
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
left a comment
There was a problem hiding this comment.
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.
6e12c8f to
9e0325c
Compare
|
Rebased onto the quotes-annotation tree (#5470) as a single commit ( What carried over and how it meets the new tree:
Locally green: revision-actions 7/7, revision-staged-context 14/14, chat-turn answer identity 33/33. CI should re-run on this head. |
Astro-Han
left a comment
There was a problem hiding this comment.
Re-review at f531f736 (Claude lineage). The lost-quotes P2 is fixed; no new P0–P2.
app-shell.tsx:1589now callsquotesForSend(expectedRevisionDraft?.draftSessionId), so a revision send reads the re-keyed branch-child bucket rather than the stale closure's emptied source bucket.:1604clears that same owner key.- For a non-revision send, the id is
undefined, so both helpers fall back to theiroptions.draftKeydefault (use-composer-quotes.ts:85,113), and behavior is unchanged. quotesForSendreturns the live bucket array, but it is only cleared afterawait send(...)resolves, so the payload is not truncated.
The remaining P3 is unchanged: cancelling an edit clears quotes added during the edit.
Automated review (Claude lineage, posted from the Astro-Han account). No approval implied.
Cancelling a revision draft cleared both draft keys wholesale, so quotes the user staged during the edit were lost together with the edit's own restaged set (apache#5274 review: "Cancelling an edit deletes quotes added during it" — on main they stayed in the composer, and adding quotes during an edit is supported). clearQuotes now returns the entries it removed, and clearRevisionStagedContext re-stages whatever lies beyond the edit's beginEdit snapshot (matched per QuoteRef field set, counted as a multiset) onto the source Session the cancel returns to — whether the user added them before or after the branch child was prepared. The edit's own items are still dropped wherever the commit left them. The regression tests drive a cancelled edit with a user-added quote on each side of the prepare step and assert the addition survives on the source key while the composer text rolls back; both fail on the previous head, the first with the edit's restaged quote returned instead of the user's own, the second with no plate restore at all. Generated-by: GLM-5.3-Flash (ZCode)
|
The remaining P3 from the re-review at Semantics (from the original finding at Fix — Verification (Node v26.10.0, Windows):
New head: |
hqhq1025
left a comment
There was a problem hiding this comment.
Reviewed ddd4c95. This update changes cancel-edit cleanup to retain quotes staged by the user during an edit, including quotes on the prepared branch child, while removing the selected message's original quotes. I found one P3 data-loss edge in that new cleanup path (inline). The earlier send-path quote re-key remains intact in the changed code; I found no additional substantiated P0–P2 issue.
Local Node 24 build:test and 26 focused revision tests passed. The current-head hosted test passed, and the merge-tree against fetched main 2f32205 is clean. The full PR diff check still reports one trailing blank line in the Desktop conversation-copy contract. I did not run a real AppShell/Host send or Electron UI flow; the focused tests use fake lifecycle state. This is not 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.
| // Past the edit's own per-quote count, every entry is the user's own. | ||
| const key = quoteKey(quote); | ||
| const ownedCount = owned.get(key) ?? 0; | ||
| owned.set(key, ownedCount - 1); |
There was a problem hiding this comment.
P3: Keep all newly staged duplicate quotes when cancelling. If the selected message had no quotes and the user adds the same quote twice during the edit, ownedCount starts at 0: the first new quote is kept but this assignment makes the count -1, so the second identical new quote fails the ownedCount === 0 check and is silently discarded. useComposerQuotes.addQuote appends without deduplication, so this state is reachable. I reproduced the helper with two { text: 'same' } entries: cancellation restored only one. Once the edit-owned quota reaches zero, keep every subsequent matching entry rather than decrementing into negative counts; add a duplicate-quote regression case.
There was a problem hiding this comment.
Fixed at e39891b: the kept check now treats any non-positive remaining count as fully user-owned (ownedCount <= 0 instead of === 0), so every plate entry past the edit's own per-key quota survives the cancel once the quota is exhausted — including the state reproduced here, where the edit owned no quota for the key and the user staged the same quote twice. Added a duplicate-quote regression case in revision-staged-context.test.ts (an edit-owned entry plus two identical user copies drives the count to -1); it fails on ddd4c95 and passes on the new head.
Astro-Han
left a comment
There was a problem hiding this comment.
We checked the increment since f531f736 (ddd4c952, keep quotes added during a cancelled edit) at ddd4c952. clearQuotes now returns what it removed, and clearRevisionStagedContext drops only the edit's own quotes (counted per quote key), then restages the rest on the source key. This fixes the earlier gap where quotes the user added during an edit were lost on cancel.
We agree with the P3 already raised inline on this head: the counter goes negative, so ownedCount === 0 misses a second identical user quote. ownedCount <= 0 would fix it. We found nothing else in the increment.
Automated review (Claude lineage, posted from the Astro-Han account). No approval implied.
clearRevisionStagedContext drops the edit's own quotes by per-key count: the first N entries matching the beginEdit snapshot are the edit's, and everything past that count is the user's own staging. The kept check compared the remaining count with `=== 0`, so once the counter ran negative the surplus stopped matching: when the user added a second copy of a quote the edit had also staged, the third identical plate entry was dropped despite being the user's. Treat any non-positive count as fully user-owned (`<= 0`), so every entry past the edit's own count survives the cancel. Fixes a P3 raised inline on apache#5274. Generated-by: GLM-5.3-Flash (ZCode)
Astro-Han
left a comment
There was a problem hiding this comment.
We checked the increment since ddd4c952 at e39891bf. ownedCount <= 0 fixes the duplicate-quote P3: a second identical quote the user added during the edit now survives cancel. The new test covers that case. The increment has no other changes, and we found no remaining P0–P3.
Automated review (Claude lineage, posted from the Astro-Han account). No approval implied.
Brings in the composer-submit extraction (apache#5815) and the allow-unchanged edit-and-resend decision, merged into the revision staged-context feature: - app-shell.tsx keeps the upstream extraction; the staged-context wiring and the apache#5274 re-keyed quotesForSend/clearQuotes semantics move into features/conversation/controller/composer-submit.ts (owner-keyed ports). - app-shell-revision-actions.ts keeps the @maka/ui delegation; TurnRevisionDraftBase drops originalText and the unchanged gate (revisionStagedContextUnchanged) so unchanged resends pass through per upstream apache#5815, while the mixed-context conflict gate stays. - conversation-copy.ts keeps both new locale keys (revisionDraftQuoteConflict, revisionMixedContextUnsupported) on top of upstream's revisionUnchanged removal and apache#5815 copy updates. - renderer-architecture.json regenerated via check-renderer-architecture --write; app-shell.tsx budget drops 12091 -> 11154. Tests: upstream app-shell-revision-resend gains a stagedContext fake; the ui revision suite expectations updated to the allow-unchanged behavior. Generated-by: GLM-5.3-Flash (ZCode)
|
Conflict resolution pushed as One upstream change collides with this PR semantically, not just textually — flagging for reviewer attention. #5815 ("allow unchanged edit-and-resend") reversed the "unchanged edit → refuse to send" behavior this PR was originally built against, and locks the new behavior in with its own tests. The resolution follows the newer product decision:
Verification on the resolved tree: full workspace build green; ui suite 693/693 (composition shifted upstream; delta includes the 3 updated unchanged-gate tests); desktop focused 18/18 across revision-actions / revision-send / upstream resend / new-task-staged-content; typecheck green on ui plus all four desktop tsconfigs; renderer-architecture ledger regenerated with |
hqhq1025
left a comment
There was a problem hiding this comment.
Reviewed exact head 436e4359e00927841965d13d6b332b11ab836977, including the e39891bf quote-count fix and the main merge. The prior duplicate-quote P3 is fixed: clearRevisionStagedContext now preserves both user-added copies after consuming the edit-owned copy, and the new regression passes. I found no further substantiated P0–P3 behavior issue in the incremental changes.
This head is not ready to merge: the current hosted test job and a local strict-base reproduction fail the renderer-architecture budget (app-shell.tsx nonTriviaTokens 11,154 vs main's 11,129). git diff --check also flags a trailing blank line in apps/desktop/src/renderer/application/contracts/conversation-copy.ts. A clean Node 24 install, build:test, 25 focused revision tests, and a merge-tree against current main passed. I did not run packaged Electron or native Windows/macOS flows.
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.
… shell Root cause: upstream apache#5546 extracted app-shell helpers and drove the frozen shell's nonTriviaTokens baseline down to 11129 with zero headroom, so the merge that re-attached this PR's stagedContext wiring (a 7-line factory in the createAppShellRevisionActions ports object, +25 tokens) broke the renderer architecture ratchet: 11129 -> 11154. Fix: assemble the RevisionStagedContext closure inside the desktop useComposerAttachments controller (a feature file outside the debt ledger), right beside the hooks that own the quote/attachment buckets. The frozen shell now forwards one member (stagedContext,) instead of building the factory inline. The restoreAttachments entry the shell only carried for that factory is dropped from its destructure: its sole consumer was the deleted block (RevisionStagedContext declares no such member and nothing in @maka/ui reads it from the staged context). Verification: - check:renderer-architecture --base 6e21e61 --strict-base: red (11129 -> 11154) before, green (11129 == base) after; ledger diff is the single app-shell.tsx nonTriviaTokens line, no other entry moved. - desktop focused suites (app-shell-revision-actions, use-composer-quotes-revision-send, app-shell-revision-resend, new-task-staged-content): 18/18 pass. - packages/ui full suite: 710/710 pass. - typecheck green: ui tsconfig + desktop preload/main/renderer/storybook. - biome lint clean on both touched source files; check:asf-headers green. Generated-by: GLM-5.3-Flash (ZCode)
|
The The merge had left the PR's Fix (no behavior change): the staged-context assembly now lives in Verification: the exact failing command now passes locally ( |
hqhq1025
left a comment
There was a problem hiding this comment.
Reviewed commit b97145c. The follow-up moves the revision staged-context getter from the frozen app shell into the attachment controller and lowers the architecture debt ledger. The prior duplicate-quote fix remains in place. I found no new P0–P3 issue in this three-file increment; focused revision tests (25/25), build:test, and the renderer architecture check pass locally.
This is not merge-ready yet: the PR diff still has a trailing blank line in apps/desktop/src/renderer/application/contracts/conversation-copy.ts, and current main conflicts in apps/desktop/renderer-architecture.json. Hosted test passes, but neither check resolves those blockers. Packaged Electron and native Windows/macOS behavior were not exercised locally.
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.
Resolve the renderer-architecture.json conflict by re-pricing the ledger from the merged tree via check-renderer-architecture.mjs --write (all entries at real merged-tree values, within baseline); drop the trailing blank line at EOF in conversation-copy.ts flagged by diff-check.
|
Pushed
Verification on this head: desktop revision tests 8/8 ( Local tooling note (not part of this diff): the desktop build's |
|
Gentle ping — this has been open for just over two weeks. The two merge blockers from the last review are resolved on @zhiiw could you take a look for a human review when you have a moment? |
hqhq1025
left a comment
There was a problem hiding this comment.
Reviewed commit 15564fb. This merge brings current main into the revision-staging branch, re-prices the renderer architecture ledger from the merged tree, and removes the trailing blank line previously caught by diff-check. The quote-staging behavior and fail-closed attachment boundary from the prior review remain unchanged. I found no new P0–P3 issue on this head.
A clean Node 24 installation and build:test, 45 focused revision/session-navigation tests, renderer architecture check, Astryx inventory check, diff-check, current hosted test, and a merge-tree against current main pass. Packaged Electron and native Windows/macOS UI behavior were not exercised locally. Feature acceptance remains a human decision.
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.
Resolve renderer-architecture.json by re-pricing from the merged tree via check-renderer-architecture.mjs --write.
|
Merged Per the established practice, the ledger was not hand-merged: one side was taken to clear the conflict markers, then the whole ledger was re-priced from the merged tree via Ledger re-pricing ( Verification (local, Windows):
New head: a727c09 (merge commit a727c09, parents 15564fb + 0aa2707). |
Resolve the renderer-architecture.json conflict by taking main's ledger (new apache#5851 file entries) as the base and re-pricing every entry from the merged tree via check-renderer-architecture.mjs --write; the check passes against the ratchet baseline.
|
Follow-up merge: |
hqhq1025
left a comment
There was a problem hiding this comment.
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.
|
Conflict resolution pushed as Three files collided:
Net against the new base: 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 |
hqhq1025
left a comment
There was a problem hiding this comment.
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.
|
Conflict resolution pushed as
Verification at Generated-by: GLM-5.3-Flash (ZCode) |
hqhq1025
left a comment
There was a problem hiding this comment.
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.
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:
Verification
mainclean (reviewer-confirmed)check-locale-hygiene/biome checkon changed files /check:asf-headersEarlier-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:
Tool(s) and scope: GLM-5.3-Flash (ZCode) implemented the desktop/ui changes and tests under human direction and review.
Checklist
Does this PR entail a change in behavior?