perf(desktop): publish session state at the granularity of change

[Astro-Han]

Summary

Investigation of #5441 found that background session work drove renderer cost far beyond the components actually displaying it. Five commits apply the same invariant at each layer — publish a new reference iff the value changed:

• Session catalog (a462b122f): sessions:changed already carries the row id, but the shell answered every hint with a full sessions.list() and committed fresh row objects. Now folds same-id hints into sessions.get, reconciles committed rows by id, and the rail/palette/inbox/settings-intent subscribe where they consume instead of through a shell-carried sessions array.
• Side chat observation (5361e2a88): a mounted-but-hidden quote companion kept its Runtime Host observer alive and handled every fork delta. The subscription now follows panelVisible && selected; hiding unobserves, showing re-seeds and reconciles against the durable transcript.
• Transcript timeline (aab1b6746): turn-level reconcile rebuilt every timeline item and fold entry per delta, so each token re-rendered the whole transcript. Timeline items reconcile by timelineItemKey + valuesEqual, fold entries reconcile across renders, and TurnTimelineEntry/ProcessingBlock are memoized.
• Onboarding snapshot (d92ce6d3a): every sessions:changed event triggered a full getSnapshot IPC (4+ parallel queries in main) plus a setSnapshot that re-rendered AppShellContent at event rate. Emits are deduped on a render projection that excludes the live sessions field (catalog is its authority), and pulls are serialized with a single follow-up while in flight.
• Palette subscription (5e8c93ded): visibleSessions was subscribed at shell level though only palette session rows consume it. Moved into useAppShellCommands; while the palette is closed the selector is a constant-empty function so catalog churn cannot emit.

Measured

Fixture app, two background sessions steering (~124 sessions:changed/s) while viewing an idle session:

• DOM mutations: ~3,796/s → 383/s, of which ~360/s are the rail rows actually displaying the changing data
• Shell-level "other" bucket: ~648/s → 23/s (all real status-dot updates)
• Hidden side-chat fork streaming: 0 mutations / script / layout / recalc
• Terminal flood in a hidden workbar panel: ~3,300/s → ~114/s (the residual is a fixed xterm fit-check interval, already cheap)

Test plan

• npm run typecheck (desktop, all four tsconfigs)
• node --test dist/main/__tests__/*.test.js — 2755/2755
• npm run test:dist -w @maka/ui — 533/533, incl. new item/fold reconcile-identity tests
• Playwright-Electron measurement script across idle / terminal flood / background steering / hidden fork / 1-vs-3 concurrent sessions

Generated with Devin

[jackwener]

[P3] This projection is complete today — I checked it field by field against OnboardingSnapshot, and it covers all seven minus the documented sessions exclusion. The finding is that nothing keeps it complete.

Add an eighth field to OnboardingSnapshot and this function keeps comparing six. The snapshot then stops publishing when only that field changes, and the symptom is a value that never updates in the UI — which reads as a data-plumbing bug, not as a dedup that silently stopped covering something. That is an expensive thing to debug precisely because the dedup is correct-looking code far from the symptom.

It matters more here than it would elsewhere, because this PR's thesis is "publish a new reference iff the value changed" applied at five layers. Every such dedup is only as correct as the completeness of its key, and this is the one place where completeness is maintained by hand rather than by the compiler.

A witness type costs a line and fails the build instead of failing silently:

const ONBOARDING_PROJECTION_FIELDS = {
  state: true, milestones: true, connections: true,
  defaultSlug: true, chatModelChoices: true, sessionSendOutcomes: true,
} satisfies Record<Exclude<keyof OnboardingSnapshot, 'sessions'>, true>;

Then adding a field to the snapshot is a type error here until someone decides whether it belongs in the key — which is exactly the decision that should not be made by forgetting.

The sessions exclusion itself I have no concern about: sessionsRef.current is updated before the setSnapshot call and getSessions() reads the ref, so consumers still see fresh rows without a re-render. The exclusion and the getter-shaped API are the same design decision, and they agree.

[jackwener]

Review of exact head 5e8c93dedb0f10e4d58afb4b1abcbfc5725bfced (43 files, +1120/−323). No P0–P2 from my line. One [P3]. Not approving — the branch is CONFLICTING and only label has run on this SHA; test has not.

Where I went after it

A PR whose invariant is publish a new reference iff the value changed has one characteristic failure: it gets fast by publishing less, and loses an update that mattered. So I probed the three seams where that would show up, and all three hold:

• The onboarding dedup excludes sessions — and that is safe here, for a reason in the code rather than in the description. sessionsRef.current is assigned before setSnapshot, and the hook exposes getSessions() reading that ref rather than a sessions value. So suppressing the re-render cannot staleth the rows: consumers pull them imperatively. The exclusion and the getter-shaped API are one decision, and they agree.
• The palette's closed-state selector re-seeds on open. paletteOpen ? selectPaletteSessions : selectClosedPaletteSessions changes the selector's identity, and useExternalStoreSelector memoizes getSnapshot on [store, select, arg, isEqual] — so React re-reads immediately rather than waiting for the next store emit. "Cheap while closed" is the kind of optimisation that usually costs an empty first frame; this one does not.
• That cache is not defeated by an unstable argument. arg is hiddenSessionIds, which comes from a useState set, so its identity survives re-renders. Had it been rebuilt each render, the memo would recompute every time and quietly undo the win.

[P3] — inline

onboardingSnapshotProjectionEqual is complete today (I checked all seven fields against the type) but hand-maintained, so a future eighth field would silently drop out of the dedup key and stop publishing. A satisfies Record<Exclude<keyof OnboardingSnapshot,'sessions'>, true> witness turns that into a build error.

Not covered — and on this PR the first line is the important one

• I reproduced none of the measurements. The 3,796→383 mutations/s, the 648→23 shell bucket, the zeroed hidden-fork streaming, the terminal flood figures — all of it is your Playwright-Electron run, not mine. I did not run that harness, or any build, suite, browser, or Electron session. For a perf PR, that is the central limit of this review: I checked that the change cannot lose an update, not that it is faster.
• No CI corroboration: test has not run on this SHA.
• The branch is CONFLICTING (merge-base d3292393c, 3 commits behind main as measured at 09:09Z), so the resolution will move code this review is bound to.
• Of the five layers, I probed the catalog/onboarding/palette seams. The transcript-timeline reconcile and the side-chat observation lifecycle I read but did not exercise — memoization and subscription-teardown bugs are exactly the kind that reading does not catch, and other seats are reviewing this PR independently.

---

Automated review notice: Posted by an automated review agent (Claude Opus 5) through the shared jackwener account (seat: kabi-opus). Several seats are reviewing this head independently and publishing separately; none is a human review.

简体中文

审查 exact head 5e8c93ded(43 文件 +1120/−323)。我这条线无 P0–P2,一条 [P3];不批准 —— 分支 CONFLICTING,该 SHA 上只有 label,test 未跑。

我往哪里查:一个以"值变了才发布新引用"为不变量的 PR,有一种特征性失败 —— 靠少发布变快,结果丢掉了本该发的更新。所以我探了三处最可能暴露它的接缝,三处都站得住:
① onboarding 的去重把 sessions 排除在外,而这是安全的 —— 理由在代码里,不在描述里:sessionsRef.current 在 setSnapshot 之前赋值,且 hook 暴露的是读该 ref 的 getSessions() 而不是 sessions 值 ⇒ 压掉重渲染不会让行数据变陈,消费者是主动拉取的。排除与 getter 形态的 API 是同一个决定,两者一致。
② 面板关闭态的 selector 在打开时会重新取数:paletteOpen ? A : B 改变的是 selector 的身份,而 useExternalStoreSelector 以 [store, select, arg, isEqual] 记忆 getSnapshot ⇒ React 立即重读,而不是等下一次 store 事件。"关着的时候便宜"这类优化通常的代价是打开后的第一帧是空的,这里没有。
③ 该缓存不会被不稳定的参数打掉:arg 是 hiddenSessionIds,来自 useState 的集合,身份跨渲染稳定;若它每次渲染重建,memo 就会次次重算、悄悄抵消收益。

[P3](内联):onboardingSnapshotProjectionEqual 今天是完整的(我逐个字段对过类型),但靠手维护 —— 将来新增第八个字段会悄悄掉出去重键并停止发布。用 satisfies Record<Exclude<keyof OnboardingSnapshot,'sessions'>, true> 见证类型,可把它变成编译错误。

未覆盖 —— 这一段的第一条才是重点:

• 那些测量我一条都没有复现:3,796→383 次/秒、shell 桶 648→23、隐藏 fork 流式归零、终端洪泛那组数字,全部是你的 Playwright-Electron 跑出来的,不是我的。我没跑那套测量,也没跑任何构建/套件/浏览器/Electron。对一个 perf PR,这就是本评审的核心限制:我查的是"这个改动不会丢更新",不是"它更快"。
• 无 CI 佐证(该 SHA 上 test 未跑);分支 CONFLICTING(落后 main 3 个提交,09:09Z 测),解冲突会移动本评审所绑的代码。
• 五层里我探的是 catalog / onboarding / palette 三处接缝;transcript 时间线的 reconcile 与侧边对话的观察生命周期我只读了、没有实际驱动 —— memo 与订阅拆除类的缺陷恰恰是"读代码抓不住"的那类,其他席位在独立审这一单。

[jackwener]

Seat: kabi-grok-reviewer. Independent review of 5e8c93dedb0f10e4d58afb4b1abcbfc5725bfced.

Reference frame: this head vs merge-base d3292393c575d2019ca406a5048099abf3399e0f. Live origin/main is aa86f9549, 3 commits ahead. I did not review as-if-rebased onto current main.

Not a draft. DIRTY / CONFLICTING. Only hosted label; no test. COMMENT only — no approve, no rebase, no merge.

Verdict

No P0–P3 from this seat. The identity-reuse / unobserve mechanics look fail-closed on the paths I walked. Perf numbers are 未验证 — I did not run the Playwright-Electron fixture or any mutation counter.

What I walked

• Catalog: sessions:changed folds same-id into sessions.get (use-app-shell-session-list.ts). commitSessions / commitPatch keep the prior row object when summaryValuesEqual or when prior.revision > next.revision (do not regress a newer patch).
• Transcript: valuesEqual is fail-closed for non-plain objects (transcript-projection.ts); reconcileTimelineItems keys by timelineItemKey. Tests exist in transcript-projection.test.ts / timeline-fold.test.ts. I did not execute them (no dist rebuild).
• Hidden side-chat: use-quote-companion.ts unsubscribes when !active and re-subscribes + re-seeds when shown. Missed live tokens while hidden is the intended trade; show path is the lost-subscription recovery.
• Onboarding: onboardingSnapshotProjectionEqual excludes sessions (catalog is live authority). getSessions() still returns the last seed in a ref — boot-time only.
• Palette: selectClosedPaletteSessions is a constant empty array while closed; opening switches to selectPaletteSessions. I did not click the palette.

未验证

• Author's ~3796/s → 383/s DOM mutations, shell "other" bucket, hidden fork 0, terminal flood 3300→114. I have no measurement of my own.
• Did not rebuild @maka/* dist; did not run desktop 2755 or ui 533.
• No Playwright, no browser, no Windows.
• Did not prove Host revision never goes backwards (stale-summary keep depends on it).
• Author-claimed typecheck/test counts are not evidence.

Issue #5441 is the investigation claim, not a measurement I reproduced.

---

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

[jackwener]

Review seat: kabi-sol. Independently reviewed 5e8c93dedb0f10e4d58afb4b1abcbfc5725bfced against merge-base d3292393c, without reading existing PR comments. One P1; I do not recommend merging this head.

A background Session update is now interpreted as removal of the currently selected Session: the singleton row result reaches a retirement check that expects the complete catalog. I reproduced the resulting selection/transcript clearing with the production bootstrap hook and workspace action factory, and the same probe using the base hook preserves the selected conversation.

I independently measured one narrow benefit: with 100 background-B catalog updates, a React selector of unchanged A renders 101 times with the base catalog controller versus once with this controller (initial render included). The PR's whole-app DOM/CPU, hidden-fork and terminal-flood figures remain unverified by me.

Fresh complete Desktop dependency chain and main build passed; 20 focused existing tests passed. The defect probe uses real hooks with the repository's fake DOM and controlled bridge data; I did not run full Electron/browser, real Host streaming, Windows/Linux or all suites. The PR remains conflicting and has no test check. This is not a rebased/current-main integration approval.

---

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

[jackwener]

[P1] Do not treat a changed row as the complete Session catalog

This branch returns only [changedSession], but the continuation below still calls retiredSessionIds(sessions), whose implementation checks whether the active/requested IDs are absent from a complete list. If A is open while background B appends a message, [B] therefore retires A. AppShell's retirement callback clears A's selected/requested identity, displayed transcript and session UI state even though A still exists.

A probe mounting the actual bootstrap hook and executing the actual workspace action factory starts with catalog [A,B], active A and one displayed message. A B message-appended event produces retired=[A], no active/requested Session and zero displayed messages. The same probe with the merge-base hook preserves A; the head's id-less full-refresh branch also preserves it. Please keep complete-membership retirement separate from row patches: only act on the changed ID's absence, or inspect the full committed catalog after applying the patch. A partial result must not retire unrelated Sessions.

Seat: kabi-sol. This is a normal-path regression; I am not claiming durable Host data deletion.

---

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

[jackwener]

Seat: kabi-grok (same GitHub owner as the coordinating review agent, different model). Review of exact head 5e8c93dedb0f10e4d58afb4b1abcbfc5725bfced.

I did not read existing pull-request comments. I am not approving. This head is not a draft, but it is CONFLICTING/DIRTY with main (behind 3, ahead 5). Required test has not run; only label is green. Conflict resolution will change code; this review dies if the head moves. I will not rebase or merge it.

Frame of reference

A. Versus merge-base d3292393c: does “publish a new reference iff the value changed” hold at each layer, and is it safe?

B. Versus live main: the three commits this SHA is missing are #5468 (edit/resend), #5412 (tool-call presentation), #5527 (project groups scoped by runtime host). #5412/#5527 are the likely conflict surfaces (chat-turn.tsx / transcript, session rail).

A — versus merge-base

The problem is real: a sessions:changed flood should not rebuild the whole shell. The invariant is the right layer — identity lives at the publisher (catalog / transcript projection / onboarding poller / palette selector), not as a guess in memo downstream.

What I could check in code:

• Catalog commitPatch / commitSessions keep row identity when summaryValuesEqual or when the committed revision is newer. replaceState skips only on object identity; selectors that read sessions therefore depend on the array/row identities this reconcile produces.
• Transcript valuesEqual is fail-closed for non-plain objects (a false “unchanged” would freeze a WeakMap presentation). Tests exist for item/fold identity.
• Hidden quote companion unsubscribes and re-seeds via subscribeEvents + readSettledMessages. There is a test: releases the fork observation while the panel is hidden and re-seeds on return.
• Onboarding emit equality excludes sessions (catalog is the live authority) and serializes pulls with one follow-up. That is a correctness choice, not just a perf trick.
• Palette: while closed, the selector is a constant empty array, so catalog churn cannot rebuild commandOptions.

I did not find a P0–P2 versus this merge-base.

Performance claims — unverified

The PR quotes fixture numbers (DOM ~3796/s → 383/s, shell “other” ~648/s → 23/s, hidden fork 0, terminal flood ~3300/s → ~114/s). I did not run the Playwright-Electron script or any profiler. Those numbers are author-reported, not evidence in this review. The mechanism is consistent with that shape of win; I am not affirming the magnitudes.

[P3] this pull request — commitSessions still publishes when rows did not move

When every row is reused, sessions keeps the previous array, but revision still increments and replaceState gets a new snapshot object, so every catalog subscriber is notified. Row memos survive; anything that selected the snapshot or revision does not. The invariant is applied to rows, not to the catalog snapshot.

[P3] this pull request — two structural-equality functions

valuesEqual (ui, fail-closed) and summaryValuesEqual (catalog, walks any object keys). Summaries are supposed to be JSON-shaped. If a non-plain field ever appears on DesktopSessionSummary, the catalog walk is the weaker one.

B — merge action, not a severity upgrade

Keep this SHA’s identity reconcile when merging #5412/#5527. Do not drop the discriminated row reuse to resolve a conflict.

Verification bounds

• Walked: catalog reconcile, transcript/fold identity, companion hide/re-seed, onboarding projection, palette closed selector.
• Did not run desktop test:dist, @maka/ui tests, or the measurement script.
• Did not read other reviews.
• Did not inspect the three main-only commits beyond their titles.

---

Automated review notice: This comment was posted by an automated review agent operated by jackwener. It is not an independent human review and does not replace one. Seat: kabi-grok.

简体中文

席位 kabi-grok。只绑 5e8c93ded。CONFLICTING，test 没跑，不 approve。

相对 merge-base：不变量层放对了，未发现 P0–P2。作者给的 DOM 3796→383 等数字我没测，标 未验证。

P3：commitSessions 行没变仍 bump revision 并通知；catalog 的相等函数不如 ui 的 fail-closed。落后 main 的 #5412/#5527 解冲突时保住 identity reconcile。

[jackwener]

[P3] Row identity is reused, but revision still increments and replaceState always gets a new object ({...current, ...}), so every catalog subscriber is notified even when sameRows is true.

The stated invariant is “publish a new reference iff the value changed.” That holds for rows, not for the catalog snapshot. Early-return when sameRows (keep current) if the revision bump is not itself a signal a reader needs.

[zhiiw]

Independent review (blind — I have not read existing comments on this PR). Conclusions bind to 5e8c93dedb0f10e4d58afb4b1abcbfc5725bfced (CONFLICTING vs main; only the label check ran — a code review, not a merge judgment).

Verified locally (real Windows 11, Node 24.18.1): full build chain + the ten touched test files — 179/179 (desktop: quote-companion-retry, session-navigation-controller, session-setting-intent, session-settings-controller, use-onboarding-snapshot, first-send-cleanup, import-tasks-settings; ui: timeline-fold, transcript-projection, session-setting-intent).

The invariant — publish a new reference iff the value changed — is implemented correctly at every layer I read:

• Catalog reconcile (session-catalog-state.ts): rows keep identity when values are equal; a committed row at a newer revision is never regressed by an older snapshot (isStaleSummary); the patch path re-sorts and preserves array identity when nothing moved. The hot-path batching (drainSessionPatches) folds same-id events into one sessions:get per drain, and a failed row read falls back to a full refresh instead of evicting the row — the failure mode that matters.
• Companion observation follows panelVisible && selected; hiding releases the fork's observer, showing re-seeds through the same recovery path a lost subscription takes. A send while hidden does not wait on observation — correct, since send needs the settled promise, not the live stream.
• Timeline reconcile: items match by timelineItemKey + valuesEqual (survives mid-timeline inserts), fold entries reconcile by boundary id + children identity, and the memo boundaries sit below both. The reconcilers are pure and pinned by the new identity tests.
• Onboarding snapshot: the projection equality excludes the live sessions field (the catalog is its authority — the comment says why), and pulls serialize with a single follow-up while in flight.
• Palette: the subscription moved to the consumption point; a closed palette selects through a constant-empty function, so catalog churn cannot emit there. The palette equality deliberately narrows to id/name/flag — the rail reads status through its own subscription.

Zero-residue on the −323: the deletions are reorganization (shell-carried sessions arrays, the shell-level palette subscription, the old poller), and the touched test files keep or extend their assertions — I found no deleted protection.

Performance numbers: not verified by me. The measurement script is not committed and the scenarios need a live Electron window; this machine has an active human user, so I do not run visible-window automation on it. What I can confirm is that the mechanism the numbers attribute the win to (reference-identity preservation) is implemented as described and pinned by tests; the magnitude (3796/s → 383/s DOM mutations etc.) should be treated as the author's measurement, not mine.

Not verified: e2e, the renderer's perceived first paint, and anything the conflict resolution will touch next — app-shell.tsx and the catalog store are moving on main too.

COMMENT per this round's convention (no approval on a conflicting, CI-less head). No P0–P3 findings within scope.

---

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

[jackwener]

Re-review at exact head 96a155c0220903c1bd0e0bc4c8b9e7a8bff17498 (12 commits past 5e8c93ded; base now aa86f9549, branch MERGEABLE). My [P3] is closed. test is red, and it is this PR's account — mechanically.

The [P3] fix is better than what I proposed

96a155c02 adds the satisfies Record<Exclude<keyof OnboardingSnapshot, 'sessions'>, true> witness, and then derives the comparison from it (Object.keys(COMPARED_KEYS).every(...)) instead of leaving a hand-written conjunction beside it. I only asked for the witness; a witness and a separate list of && clauses could still drift apart, and this removes that possibility by construction. The comment now states the invariant too.

[P1] — test fails, and it is the third instance of one pattern today

Check renderer architecture rejects the run because renderer-architecture.json still describes the previous structure:

• the legacy AppShell closure and the renderer root closure both gained src/renderer/session-change-effects.ts (the new file this PR adds);
• app-shell-command-actions.ts changed dependencyPaths, hookCalls (useExternalStoreSelector added), importDeclarations, importSpecifiers and nonTriviaTokens;
• app-shell-effects.ts gained ./session-change-effects.js in its dependency paths.

Every one of those is a true consequence of the refactor, so the ledger is what needs regenerating — the gate is not miscounting. This is the same failure I attributed on #5470 (debt counters) and #5494 (hooks inventory) earlier today: the change is right, the generated record simply has not been re-run. Three PRs in one day suggests it is worth making the regeneration part of the normal loop rather than something discovered from CI each time.

Scope of this pass

I re-checked my own finding and the CI state only. The [P1] from another seat — session retirement judged against a single-row patch, which cleared the session the user was viewing — is theirs to re-verify, and 296baa60b fix(desktop): sweep session retirement against the committed catalog looks aimed at it. I have not verified that, and a commit title is not evidence. Nor have I re-run anything here: no build, no suites, no Electron, and none of the measured numbers.

---

Automated review notice: Posted by an automated review agent (Claude Opus 5) through the shared jackwener account (seat: kabi-opus). Not an independent human review.

简体中文

在 exact head 96a155c02 上复审(距 5e8c93ded 12 个提交;base 现为 aa86f9549,分支 MERGEABLE)。我那条 [P3] 已关闭;test 红,是本单的账,但属机械性。

[P3] 的修法比我提的更好:96a155c02 加了 satisfies Record<Exclude<keyof OnboardingSnapshot,'sessions'>, true> 见证类型,并让比较从它派生(Object.keys(COMPARED_KEYS).every(...)),而不是在旁边留一串手写的 &&。**我只要了见证;而"见证 + 另一份手写清单"仍可能彼此漂移,这个写法从构造上消除了那种可能。**注释也把不变量写下来了。

[P1] test 失败,且这是今天同一模式的第三例:Check renderer architecture 拒绝本次运行,因为 renderer-architecture.json 仍描述旧结构 —— legacy AppShell 闭包与 renderer root 闭包都新增了本 PR 的 session-change-effects.ts;app-shell-command-actions.ts 的 dependencyPaths/hookCalls(新增 useExternalStoreSelector)/importDeclarations/importSpecifiers/nonTriviaTokens 均已变化;app-shell-effects.ts 的依赖路径新增了 ./session-change-effects.js。
**这些全都是本次重构的真实后果 ⇒ 需要重新生成的是台账,不是 gate 数错了。**这与我今天在 #5470(债务计数)与 #5494(hooks 清单)归因的是同一种失败:改动是对的,只是生成的记录没有重跑。一天之内三个 PR 都撞上,说明这个重新生成值得纳入常规流程,而不是每次从 CI 发现。

本次范围:我只复核了自己的 finding 与 CI 状态。另一席那条 P1由他自己复核;296baa60b 看起来正是冲它去的,但我没有验证,而提交标题不是证据。本轮我也没有重跑任何东西:无构建、无套件、无 Electron,那些测量数字也一个没复现。

[jackwener]

Follow-up on exact head 96a155c0220903c1bd0e0bc4c8b9e7a8bff17498. Review 5260181243 stays bound to 5e8c93ded. I only rechecked the two P3s from that review. I did not re-walk the rest of the diff and I am not diagnosing test.

Both P3s are closed on this head (a1b6bac3a):

1. commitSessions returns early when sameRows && current.revision > 0. commitPatch returns early when sameRows. A no-op list/patch no longer bumps revision or notifies. The revision-0 exception (first authoritative observation, including empty) is the right fence.
2. summaryValuesEqual is gone. Row compare uses @maka/ui's valuesEqual (the fail-closed walk from transcript-projection.ts, re-exported from the package).

I am not changing the original P3 ratings; they applied to 5e8c93ded. Base is now aa86f9549 (includes the three commits the previous review listed as behind). MERGEABLE. Still not approving.

Perf numbers from the PR body remain unverified here — I did not run the measurement script on this head either.

---

Automated review notice: This comment was posted by an automated review agent operated by jackwener. It is not an independent human review and does not replace one. Seat: kabi-grok.

简体中文

只核 96a155c02 上我那两条 P3，都闭环了：无变化不再 bump revision；相等函数换成 ui 的 fail-closed valuesEqual。不 diagnose test。不 approve。性能数字仍未验证。

[jackwener]

Seat: kabi-sol. Scoped re-review at 96a155c0220903c1bd0e0bc4c8b9e7a8bff17498.

My previous P1 is fixed. Retirement now reads the complete committed catalog after refresh, rather than interpreting a singleton row result as complete membership.

I mounted the production bootstrap subscription hook together with the actual session-list hook/catalog controller and workspace actions under the repository's React fake DOM, with controlled bridge inputs:

• Background B message update preserves selected/requested A and its displayed transcript.
• Deleting B preserves A.
• A failed B row read, followed by the production full-list fallback, preserves A.
• Deleting A through a row read correctly clears A; a complete-list refresh that removes A also clears it.
• Replacing only the bootstrap-hook source with 5e8c93ded reproduces the original failure: B's update clears A even though the catalog still contains both. The current source passes the same scenario.

Fresh install, dependency patches, full Desktop dependency build and Desktop main build passed. All four new retirement tests passed. I reran the narrow catalog-selector measurement: 100 B changes while selecting unchanged A still produce 101 renders with the original merge-base controller versus 1 with this head, including initial render. This is not a whole-app performance benchmark; the author's DOM/CPU, hidden-side-chat and terminal-throughput claims remain unverified by me.

Scope is limited to my previous P1. I did not run full Electron/browser integration, actual Host streaming, Windows/Linux or the full suite, and did not re-review other changes. Hosted test is currently failing; closing this finding is not approval of the PR.

---

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

[zhiiw]

Incremental review of the new head. Conclusions bind to 96a155c0220903c1bd0e0bc4c8b9e7a8bff17498.

Rebase character (verified with git range-diff, not assumed): the five original layer commits survived the rebase — 2/4/5 are identical; 1 and 3 differ only where main moved the same files (#5527's project scopes in the navigation fixtures, #5412's chat-turn refactor, which absorbed this PR's onSwitchToBypassAndRetry stabilization because the call sites it protected are gone). The layer logic is unchanged.

The four new commits:

• 296baa60 (sweep session retirement against the committed catalog) — verified the fix, not just the description: retiredSessionIds is contractually the complete catalog, and the single-row fetch path violated that. The sweep now reads sessionsRef, and the timing claim holds: commitPatch updates the mirror before resolving the drain waiters, so a resolved refresh promise means the mirror is current. The extracted leaf module (session-change-effects.ts) plus the new regression test pin the exact scenario. This was reported by another seat; my read of the fix is independent agreement.
• 979bf638 (sink the stale-session selector into the rail provider) — the shell keeps only its three genuine whole-tree reads; the hooks-gate ledger entry is updated accordingly.
• a1b6bac3 (publish the catalog only when content changed) — no-op commits now publish nothing, with the revision-0 exception preserving the first authoritative observation. Subscribers read current state at subscribe time, so the exception is sufficient.
• 96a155c0 (exhaustive onboarding dedup key) — the satisfies witness turns a silently-drifting key list into a compile error. Correct and the right shape.

Verified locally (Windows, Node 24.18.1): 119/119 on the touched suites, including the new session-change-retirement test.

The red test lane is the gate working, not a code bug: session-change-effects.ts (new in 296baa60) is absent from renderer-architecture.json — the ledger was not regenerated. Regenerating it closes the lane.

Performance numbers remain the author's measurements, not verified by me — same reason as my first-round review: no committed measurement script, and this machine runs no visible-window automation while its user is active. The mechanism those numbers attribute the win to remains correctly implemented on this head.

COMMENT per the round's convention. No P0–P3 findings in this increment.

---

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

[jackwener]

Seat: kabi-opus-review-orchestrator. Re-review at exact head 02d12fca217f92ea8e09d9ca7eee1c0a6fe5334e. COMMENT only, no approve.

Topology first, because the recorded base is misleading here

The PR's recorded base is 7baf8b053, which is not an ancestor of this head — diffing against it reports 83 files. The merge-base is 8c3e0e8ff, and against that the real change set is 72 files. Behind main by 3. Please use the merge-base for any review of this branch.

What actually changed since 9e642d7ae

Only the generated surface inventory (docs/astryx-surface-file-inventory.md / .paths, +5/−1). The 48-file structural refactor is byte-identical to the previously reviewed version. The inventory gate that was red is now satisfied.

P1 — the branch does not typecheck

apps/desktop/stories/command-search.stories.tsx:179 renders <CommandPalette commands={props.commands} />, but this PR widened that component's contract (features/overlays/ui/command-palette.tsx:59-67) to require five props:

export function CommandPalette(props: {
  readonly commands: Command[];
  readonly sessionCatalog: SessionCatalogController;
  readonly hiddenSessionIds: ReadonlySet<string>;
  readonly activeSessionId: string | undefined;
  readonly onSelectSession: (id: string) => void;
}) {

CI reports it as TS2739 … missing the following properties: sessionCatalog, hiddenSessionIds, activeSessionId, onSelectSession, and the source confirms it independently of the CI log: the call site passes one of the five.

This is the direct consequence of moving session selection into the palette — the production call sites were updated, the storybook story was not. tsc -p tsconfig.storybook.json --noEmit fails, so Typecheck fails, so the whole test job fails. Nothing else in the suite gets to run.

Graded P1 rather than P3 for what this SHA does: the branch does not typecheck, which blocks the gate for everyone, not just storybook. The fix itself is mechanical — give the story the four props, most simply a small fake catalog controller alongside the existing storyOverlayServices.

Note on the failure pattern, for the author

This is the first red on this PR that is not a lagging generated artifact. The previous ones (architecture closure ledger, hooks inventory, debt counters, surface inventory) were all cases where the change was right and only a generated file trailed it. This one is a real contract break that a regenerate will never fix.

未验证

• I did not run the typecheck locally; the claim rests on the CI error plus reading both the call site and the component signature at this head.
• I did not re-review the 48-file refactor itself in this pass, and I did not carry over the seats' earlier closures — their conclusions were bound to earlier heads and this refactor lands on the surfaces those findings were on.
• The author's local A/B performance numbers remain unreproduced by anyone else.

简体中文

先说拓扑:PR 记录的 base 7baf8b053 不是本 head 的祖先(据其 diff 会得到 83 个文件);merge-base 是 8c3e0e8ff,真实改动面是 72 个文件,落后 main 3 个提交。审这条分支请用 merge-base。

相对 9e642d7ae 只多了生成的表面清单(+5/−1),48 文件的结构重构逐字未变;原先红的清单门已满足。

P1 —— 分支过不了类型检查。 stories/command-search.stories.tsx:179 仍然只传 commands,而本单把 CommandPalette 的契约扩到了五个必填属性(command-palette.tsx:59-67)。CI 报 TS2739,源码也独立印证。这是「把会话选择挪进面板」的直接后果:生产调用点都改了,storybook 的 story 没改。tsconfig.storybook.json 过不去 ⇒ Typecheck 失败 ⇒ 整个 test job 失败,后面什么都跑不到。

定 P1 而非 P3,看的是这个 SHA 实际造成什么:分支不过类型检查,挡住的是所有人,不只是 storybook。修法是机械的:把四个属性补给 story,最简单是在现有 storyOverlayServices 旁边加一个小的假 catalog controller。

给作者的一句提醒:这是本单第一个不是生成产物落后的红。之前那几个(架构闭合台账、hooks 清单、债务计数、表面清单)都是改动本身对、只有生成文件没跟上;这一个重新生成永远修不好。

未验证:我没在本地跑类型检查,依据是 CI 报错 + 我在本 head 读到的调用点与组件签名;本轮没有重审那 48 文件的重构本身,也没有平移各席之前的关闭结论(他们绑的是更早的 head,而这次重构正落在那些 finding 所在的面);作者本机的 A/B 性能数字仍无人复现。

---

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

[jackwener]

Seat: kabi-grok-reviewer. Sealed at exact head 2dd87b6a3cf7f4c9449cc0408a71f5abd9dbb7ad. 1×P2, no P0/P1/P3. COMMENT only — no approve, no merge. I did not read existing PR comments before sealing.

Reference frame: vs merge-base 8c3e0e8ff0d75791f03ba9261c564fdb29eba34d (73 files, +2239/−912). The PR's baseRefOid 3ff84c589 is not an ancestor of this head. Live main is 3ff84c589; this branch is 3 behind. MERGEABLE. Hosted test is FAILURE (Knip) — not green.

Design

I am not NO-GO on the 73-file move. The performance invariant (publish iff changed) did not require extracting application/contracts/session-catalog — but this head is also a #4109 call-site move, and check-app-shell-hooks.mjs exists to record exactly that. I will not pretend the extraction is implied by the title.

The gate script this PR edits

scripts/check-app-shell-hooks.mjs keeps an exact inventory: both growth and shrink fail. The only delta against the merge-base is useEffect: 7 → 6. That is tightening to match reality, not loosening. The gate still resists silent drift. Held.

"Root stays a shim" — true of one file, not of three

session-catalog-state.ts at the renderer root is a real export * shim and is still imported — by use-app-shell-session-list.ts, use-app-shell-session-workspace.ts, and app-shell-command-actions.ts. For that file the claim matches the code.

Three other root shims are not imported by anything:

• apps/desktop/src/renderer/observable-state.ts
• apps/desktop/src/renderer/session-event-health.ts
• apps/desktop/src/renderer/stale-sessions.ts

Each is an export * to its contracts copy. session-catalog-state.ts was kept because callers remain; these three simply were not deleted. That is the incomplete part of the move.

Unused exports: selectSessionCount and selectCatalogRevision in the contracts catalog state, and sessionMatchesRail in the session-navigation index.

P2 — leftover husks fail CI

A regression of this contracts extraction. Hosted test is FAILURE on Knip: 3 unused files plus 3 unused exports. The refactor presents a finished move; the gate the repository actually runs says it is not finished.

Minimal fix: delete the three unused shims and the unused exports, or give them callers. Do not resolve this by weakening the knip config — that would trade a true signal for a green check.

What I did not do

I did not re-probe the earlier retirement P1. session-change-effects.ts:48-50,80 now sweeps sessionsRef.current after the patch, which is the intended fix; that is not a finding from this seat, and it is not a closure from this seat either.

Performance numbers: unverified. I did not measure.

No dist rebuild, no desktop or ui suites, no Playwright, no browser. I did not walk all 15 deleted feature→legacy edges one by one.

简体中文

1×P2,无 P0/P1/P3。 绑 2dd87b6a3,相对 merge-base 8c3e0e8ff(73 文件 +2239/−912);PR 记录的 base 3ff84c589 不是本 head 的祖先,落后 main 3;test 红(Knip),不是绿。

设计:我不对这次 73 文件的搬迁给 NO-GO。性能不变量(变了才发布)并不需要抽出 application/contracts/session-catalog,但这个 head 同时是 #4109 的调用点搬迁,而 check-app-shell-hooks.mjs 正是为记录这件事存在的。我不会假装抽取是标题里隐含的。

被改的门禁脚本:check-app-shell-hooks.mjs 是精确清单(涨和缩都会失败),相对 merge-base 只有 useEffect: 7 → 6 一处差异 —— 这是收紧对齐现实,不是放宽。门禁仍然挡得住悄悄漂移。成立。

「根模块保留为 shim」这个说法对一个文件成立,对另外三个不成立:session-catalog-state.ts 确实是 export * shim 且仍被引用(use-app-shell-session-list.ts、use-app-shell-session-workspace.ts、app-shell-command-actions.ts)。而 observable-state.ts、session-event-health.ts、stale-sessions.ts 这三个没有任何人引用 —— 前者是因为调用方还在所以保留,这三个只是没删。这就是搬迁没做完的部分。

P2 —— 残壳导致 CI 红:Knip 报 3 个未引用文件 + 3 个未使用导出。重构自称搬完了,仓库实际跑的门禁说没有。最小修法:删掉这三个 shim 与那些未使用导出,或者给它们调用方。不要靠放宽 knip 配置来变绿 —— 那是拿一个真实信号换一个绿勾。

我没做的:没有重新探测早先那条退休 P1(session-change-effects.ts:48-50,80 现在会在 patch 之后扫 sessionsRef.current,看起来是预期的修法 —— 但这既不是本席的 finding,也不是本席的关闭结论)。性能数字未验证,我没测。 没重建 dist、没跑 desktop/ui 套件、没有 Playwright、没有浏览器;15 条被删的 feature→legacy 边没有逐条走。

---

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

[jackwener]

Seat: kabi-grok (same GitHub owner as the coordinating review agent, different model). Review of exact head 2dd87b6a3cf7f4c9449cc0408a71f5abd9dbb7ad. Earlier comments on 5e8c93ded / 96a155c02 do not apply.

I did not read existing pull-request comments. I am not approving. Required test is completed/failure. I am not calling CI green.

Frame of reference

The PR's recorded base 3ff84c589 (#5541) is not an ancestor of this SHA. Merge-base with live main is 8c3e0e8ff. Behind 3: #5535, #5542 (composer Git-branch revert), #5541 (steering timeline). MERGEABLE.

The move

Catalog state, selectors, change effects and the patch drain move under application/contracts/session-catalog (drain on the platform adapter). Root files become export * shims. Features import the contracts paths. That is the right layer given the ratchet that forbids new feature→legacy edges.

Gate script

scripts/check-app-shell-hooks.mjs is still an exact inventory (fail on growth and on an unrecorded drop; no --write). This SHA only changes useEffect: 7 → 6. That is the intended record of moving one effect out of the shell body, not a loosening of the rule.

This gate never owned the 15 catalog edges. Those live in renderer-architecture.json, which deletes navigation→use-external-store-selector / observable-state rather than raising a budget.

[P3] this pull request — tests pin that the move exists, not that catalog identity still holds

Existing tests were retargeted at the contracts import. session-change-retirement.test.ts pins a sweep. I found no assertion that after the move commitSessions/commitPatch still early-return on sameRows, or that row compare is still @maka/ui valuesEqual. The hook/architecture gates pin the new graph. They do not pin "publish iff changed".

Verification bounds

Walked: merge-base, hook-gate diff, architecture-json edge deletions, shim vs feature imports, test file list. Did not run desktop tests or Knip. Did not read other reviews.

简体中文

参照系:PR 记录的 base 3ff84c589(#5541)不是本 SHA 的祖先;与 live main 的 merge-base 是 8c3e0e8ff,落后 3(#5535、#5542、#5541),MERGEABLE。test 为 completed/failure,我没写成绿。没读已有评论,不批准。

这次搬迁:catalog 的状态、选择器、变更副作用与 patch drain 搬到 application/contracts/session-catalog(drain 落在平台适配层),根文件变成 export * shim,feature 改走 contracts 路径。考虑到那条禁止新增 feature→legacy 边的棘轮,层次是说得通的。

门禁脚本:check-app-shell-hooks.mjs 仍是精确库存(涨会红、未记录的减少也会红,没有 --write)。本 SHA 只把 useEffect: 7 改成 6,这是「把一个 effect 搬出 shell 主体」该留的记录,不是把规则调松。另外这个门禁从来就不管那 15 条 catalog 边 —— 那些在 renderer-architecture.json 里,而本 SHA 是删掉 navigation→use-external-store-selector / observable-state 这些边,不是抬高预算。

[P3] 测试钉住了「搬迁发生了」,没钉住「catalog 的恒等性仍成立」:已有测试只是改指向 contracts;session-change-retirement.test.ts 钉了一次 sweep。我没找到任何断言说明搬迁之后 commitSessions/commitPatch 仍会在 sameRows 时提前返回,或行比较仍走 @maka/ui 的 valuesEqual。hook/architecture 门禁钉的是新的依赖图,不是「变了才发布」。

验证边界:走过 merge-base、hook 门禁 diff、architecture-json 的删边、shim 与 feature 的引用关系、测试文件清单。没跑 desktop 测试,没跑 Knip,没读其他评审。

---

Automated review notice: This comment was posted by an automated review agent operated by jackwener. It is not an independent human review and does not replace one. Seat: kabi-grok.

[jackwener]

Seat: kabi-sol. Independent structural re-review at exact head 2dd87b6a3cf7f4c9449cc0408a71f5abd9dbb7ad. Reference: verified merge-base 8c3e0e8ff0d75791f03ba9261c564fdb29eba34d, 73 changed files — fetched main 3ff84c589 is not this head's ancestor. COMMENT only; no approve, no merge.

1×P2 — edit-and-resend can abandon its newly created revision before the renderer catalog catches up. I do not recommend merging this head.

The prior unrelated-session-retirement P1 remains fixed.

Absence during admission is mistaken for retirement

apps/desktop/src/renderer/application/contracts/session-catalog/catalog-row-watch.tsx:59-67

CatalogRowWatch now runs its callback when the watched IDs change, even if the catalog has not changed. The revision owner switches from A to the new B inside prepareRevisionSend immediately after reviseBeforeTurn returns (app-shell-revision-actions.ts:316-320); the explicit catalog refresh comes only after transcript settlement. Creation admits B into the preload cache, not synchronously into the renderer catalog, and a created-event row fetch can still be in flight.

So with the renderer catalog holding [A], the new [A,B] watch returns [A, undefined]. AppShell's actual callback reads that as deletion: it clears the drafts for both B and A, calls abandonTurnRevisionCopyAttempt, and resets the revision draft. The pending send then fails its ownership check and prepareRevisionSend returns false.

This is an ordinary asynchronous creation/catalog timing window, not an adversarial case.

Reproduction and two controls

A mounted React probe using production createAppShellRevisionActions, CatalogRowWatch, the current production catalog controller, and the retirement callback extracted verbatim from AppShell. The bridge creation response is controlled; transcript settlement is deliberately held and then released.

run	result
this head	draftOwner null, cleared [B,A], one abandonment, B's edited draft missing, prepareRevisionSend → false
control: restore the old catalog-change-only effect timing	draftOwner B, no clears, no abandonment, edited text retained → true
control: admit B to the renderer catalog before returning the creation result	same successful outcome under the new watcher

Three controlled repeated runs failed consistently under the fixed ordering. I have not measured real-world occurrence frequency — it is a race, not a guaranteed order.

Consequence, stated at the width I can support

The Composer implementation clears its remembered draft and writes an empty value through configured persistence; clearing the active key blanks the input. The send handler simply returns on false, and the Composer adds no failure notice — so the user sees the edit disappear and nothing sent, silently.

These consequences are source-traced. The probe substitutes the ComposerHandle and does not demonstrate browser undo or recovery after reopening. So this should be read as "clears the edited draft and cancels the send", not as permanent data loss, and it is not a claim of Host transcript deletion.

Minimum correction

Do not treat an unobserved, newly created owner as a removed row. Either admit the returned revision summary to the renderer catalog before publishing the new draft owner, or fence retirement until an authoritative observation covering that owner. Retain genuine deletion/archive cleanup.

Restoring the old timing is a control, not a sufficient fix: an unrelated catalog update can still land inside this window.

Design and scope

The performance problem is real — the prior shell consumed broad catalog/revision changes on behalf of consumers that need narrower values, and consumer-local subscriptions with a platform-owned patch drain are coherent with that. I inspected the migrated catalog/selector contracts, consumers, row drain, retirement path, onboarding dedup, and the hook-gate change. The gate edit lowers the allowed useEffect count by one, matching extraction of the revision cleanup effect; it does not disable the gate. That extraction is nevertheless what changes lifecycle timing, as shown above.

Functionality failed, so this is not a completed complexity or style endorsement of all 73 files. Moving a subscription out of the shell cannot by itself establish behaviour preservation.

Measured performance — partial, and narrower than the claim

Repeating the narrow production React selector measurement: 100 B updates while selecting an unchanged A produce 101 renders at base, 1 at head (initial included). The baseline catalog module used by that probe is byte-identical to this round's actual merge-base module (SHA-256 6f5dd81276da17896def432ea8ceb8570ea0ac94ed4a8aeada8ca72e49be051b).

This is reference-publication evidence only. The author's whole-window DOM mutations/event −86%, CPU, hidden-side-chat and terminal-throughput claims remain unverified by this seat.

Validation and limits

Fresh npm install and dependency patches; complete Desktop dependency chain and full Desktop build passed. Twenty-six focused retirement/revision/navigation/settings/onboarding tests passed — none of them composes revision creation with the new watcher, which is why this regression survives a green targeted run.

Not done: real browser/Electron for this round, real Host/provider stream, full repository suite, Windows/Linux, whole-window A/B benchmark, full runtime-transport integration. No production changes, rebase, merge or approval. Live head unchanged at final check; hosted test remains failing. No peer findings or PR review bodies read for this round.

简体中文

1×P2 —— 编辑重发会在 renderer catalog 追上之前放弃它刚创建的修订版本。当前 head 不建议合入。 早先那条无关会话退休的 P1 仍保持修复。

「录入过程中的缺席」被当成了「已删除」。 CatalogRowWatch 现在只要被监视的 ID 变了就跑回调,哪怕 catalog 没变。而修订所有者在 reviseBeforeTurn 返回后立刻由 A 切到新的 B,显式 catalog 刷新要等到 transcript 落定之后;创建只把 B 录入preload 缓存,不是同步录入 renderer catalog。于是 renderer catalog 还是 [A] 时,[A,B] 的监视返回 [A, undefined],AppShell 的真实回调把它读成删除:同时清掉 B 和 A 的草稿、调用 abandonTurnRevisionCopyAttempt、重置修订草稿,随后发送的归属检查失败,prepareRevisionSend 返回 false。这是普通的异步创建/目录时序窗口,不是攻击场景。

复现 + 两组对照:挂载式 React 探针,用生产的 createAppShellRevisionActions、CatalogRowWatch、当前生产 catalog 控制器,以及从 AppShell 逐字提取的退休回调。本 head:草稿槽 B 和 A 都被清、发出放弃、返回 false;对照一(恢复旧的「仅目录变化才触发」时机):保留编辑文本、返回 true;对照二(创建结果返回前先把 B 录入 renderer catalog):在新 watcher 下同样成功。固定该顺序后三次复跑稳定失败。真实发生率未测 —— 这是竞态,不是必然顺序。

后果只说到我能支撑的宽度:Composer 会清掉记住的草稿并向持久层写空值,清掉活动键也会清空输入框;发送处理在 false 时直接返回,而且 Composer 不给任何失败提示 —— 用户看到的是编辑内容消失、什么也没发出去,静默。这些是源码追踪得出的;探针替换了 ComposerHandle,没有验证浏览器 undo 或重开后的恢复。所以对外表述应为**「清掉编辑草稿并取消发送」,不是「永久数据丢失」,也不是 Host 侧记录被删**。

最小修法:不要把「尚未观察到的新建所有者」当成「被删除的行」。要么在发布新草稿所有者之前先把返回的修订摘要录入 renderer catalog,要么把退休判定围栏到一次覆盖该所有者的权威观察之后。真实的删除/归档清理要保留。恢复旧时机只是对照,不是充分修法 —— 无关的目录更新仍可能撞进这个窗口。

设计面:性能问题是真的,消费端本地订阅 + 平台侧 patch drain 与该问题吻合。门禁改动是把允许的 useEffect 数减一,对应搬出那个修订清理 effect,并没有停用门禁 —— 但正是这次搬出改变了生命周期时序。功能既然失败,这就不构成对全部 73 文件在复杂度/风格上的完整背书。

实测性能(部分,且比主张窄):窄 selector 场景下,对未变的 A 做 100 次 B 更新,base 101 次渲染,head 1 次(含首次)。该探针用的基线模块与本轮真实 merge-base 模块逐字节相同(SHA-256 6f5dd812…051b)。这只是「变了才发布」的参考证据;作者主张的整窗 DOM mutations/event −86%、CPU、隐藏侧聊、终端吞吐,本席均未验证。

边界:全新安装与依赖补丁、完整 Desktop 依赖链与全量构建通过;26 项定向测试通过 —— 但没有任何一项把「创建修订」和「新 watcher」组合起来,这正是这条回归能在定向测试全绿的情况下存活的原因。未做:本轮真实浏览器/Electron、真实 Host/provider 流、全库测试、Windows/Linux、整窗 A/B、完整运行时传输集成。未改生产代码、未 rebase、未合并、未批准。最终检查时 live head 未变,hosted test 仍失败。本轮未读任何同行 finding 或 PR 评审正文。

---

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

[zhiiw]

Re-review of the restructured head. Conclusions bind to 2dd87b6a3cf7f4c9449cc0408a71f5abd9dbb7ad (test is red on Knip — see inline; that is the gate working).

Independent perf measurement (this was missing; here it is). I measured the store layer directly — same synthetic stream against the pre-PR commit path and this head's createSessionCatalogController, 10,000 commits of 50 rows each:

stream	old semantics	this head
unchanged content (fresh identities)	10,000 publications	1
every commit changes	10,000	10,000 (no false suppression)
1% of commits change	—	100

Reconcile cost on the unchanged path: ~34µs per 50-row commit. This verifies the mechanism the DOM-mutation numbers are attributed to — publish iff changed, no over-suppression. It does not measure the renderer pipeline; the DOM-mutation figures remain the author's.

The structural claim — subscriptions at consumption points, contract module as the boundary — is right, and the code matches it. The palette selects its own rows (constant-empty selector while closed), the rail provider owns the stale-session selection, settings surfaces subscribe through CatalogSessions/CatalogRowWatch where hooks are metered, and the patch drain lives in the platform layer (session-catalog-sync.ts). The moved modules are content-identical to what I reviewed in the previous round (diffed, not assumed).

The one break is the restructure's own leftovers (inline): three export * shims nothing imports (Knip's "unused files") plus three dead exports. The PR says the root modules became re-export shims — but a shim with zero importers is not a compatibility surface, it is dead code, and the repo's own gate says so. Delete them (and the unused exports), or name the consumer they exist for.

Verified locally (Windows, Node 24.18.1): 99/99 on the touched suites (session-change-retirement, session-navigation-controller, use-onboarding-snapshot, quote-companion-retry, session-event-health, stale-sessions) plus the probe above; build chain clean.

Not verified: the renderer-level DOM-mutation numbers (no committed measurement script; no visible-window automation on this machine while its user is active); e2e.

COMMENT per the round's convention; the Knip cleanup is the only blocker I found.

---

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

[zhiiw]

[P1] This shim (and session-event-health.ts, stale-sessions.ts beside it) re-exports the moved modules, but nothing imports from these paths — Knip flags all three as unused files, plus selectSessionCount / selectCatalogRevision in the contracts module and the sessionMatchesRail re-export in features/session-navigation/index.ts as unused exports. That is the red test lane. A re-export shim with zero importers is dead code, not a compatibility surface — delete the three shims and the three exports (or name the consumer they exist for). Everything else in the move is clean; this is the restructure's own leftover.

[jackwener]

Seat: kabi-grok-reviewer. Targeted re-check of my own leftover-shim P2 at exact head 3a9cb5f5fe86591d197b204367b1ea582cffb116. Draft=false, MERGEABLE. Hosted test was IN_PROGRESS at lock — not green. COMMENT only, no approve. I did not judge another reviewer's finding.

My P2 is closed

Shims: the five renderer-root files are gone — observable-state.ts, session-event-health.ts, stale-sessions.ts, use-external-store-selector.ts, and session-catalog-state.ts. The callers I named last round now import the contracts path (use-app-shell-session-list.ts:34, app-shell-command-actions.ts:35, use-app-shell-session-workspace.ts).

Dead exports: selectSessionCount and selectCatalogRevision are gone from the contracts catalog state. sessionMatchesRail is no longer re-exported from features/session-navigation/index.ts; the real function still lives in session-rail-visibility.ts and is used.

The two ways this could have been faked, checked

I said last round not to resolve this by weakening the knip config. It was not weakened — knip.json is not in 2dd87b6a3..3a9cb5f5f at all (14 files). This is a true delete.

renderer-architecture.json drops 51 lines, which deserves the same scrutiny, and it holds up: that file is the regenerate-and-compare inventory, and the delta removes nodes for the deleted shims and rewrites dependencyPaths from ./session-catalog-state.js / ./use-external-store-selector.js to the contracts paths. Counts on the remaining files go down with the import retarget — use-app-shell-session-list importDeclarations 6→5, for instance. No added allowlist, no new unresolvedDependencies. Edge deletion, not a raised budget.

未验证

I did not run Knip or the architecture check locally; hosted test was not terminal at lock. This re-check covers only my own finding.

简体中文

我那条 P2 已关闭。 五个 renderer 根 shim 全部删除(含上轮我判定「仍被引用、说法成立」的 session-catalog-state.ts),我点名的那几个调用方现已改走 contracts 路径。未使用导出也清了:selectSessionCount / selectCatalogRevision 从 contracts 目录状态里移除;sessionMatchesRail 不再从 features/session-navigation/index.ts 重导出,真正的函数仍在 session-rail-visibility.ts 且有人用。

我上轮说过不要靠放宽 knip 配置变绿 —— 没有放宽:2dd87b6a3..3a9cb5f5f 这 14 个文件里根本没有 knip.json,是真删。

renderer-architecture.json 减了 51 行,同样值得盯,结论是应然:它是「重新生成并比对」的清单,这次的增量是删掉被删文件的节点,并把 dependencyPaths 从 ./session-catalog-state.js / ./use-external-store-selector.js 改写到 contracts 路径;剩余文件的计数随引用改写而下降(例如 use-app-shell-session-list 的 importDeclarations 6→5)。没有新增豁免名单,也没有新的 unresolvedDependencies。是删边,不是抬预算。

未验证:本地没跑 Knip 与架构检查;锁定时 hosted test 未终态。本次复核只覆盖我自己那条 finding。

---

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

[jackwener]

Seat: kabi-opus-review-orchestrator. Review at exact head 9e2224c89ab7d82eeb52ece12b2ed7c38c22905f. Not a draft; test FAILURE. COMMENT only, no approve.

P1 — Desktop e2e fails across most of the suite on this branch, and the Knip failure was hiding it

head	test job failed at	Desktop e2e
2dd87b6a3	Knip	did not run — the step was skipped, and the results upload found no files
3a9cb5f5f	Desktop e2e	23 failed / 14 passed
9e2224c89 (this head)	Desktop e2e	24 failed / 13 passed

For comparison, the test job is green on main at 3ff84c589 and at d9c670423.

The failures are broad and shallow-looking — element(s) not found on assistant replies, sent messages and session rows, across session-local-recovery, session-workbar, streaming-remount, workhub-layout, skill-draft-lifecycle, side-chat-followups, slash-command-compact and more. That shape is one shared cause, not two dozen independent regressions.

Why this went unnoticed until now: the test job runs Knip before Desktop e2e. While Knip was red, the e2e step never executed, so the branch looked like it had one tidy dead-code problem. Fixing Knip did not introduce this — it revealed it. The first head on which e2e actually ran already failed 23 specs.

What I can and cannot attribute. I cannot pin the onset to a specific commit, precisely because e2e never ran on the head before 3a9cb5f5f. It may have been introduced anywhere in this branch's 73-file move. What is established is that the branch fails what main passes, and that the latest commit did not fix it — it went from 23 to 24.

I did not diagnose the shared cause. I checked the most obvious candidate and ruled it out: every import of the deleted root modules now resolves to a real file inside application/contracts/session-catalog, so this is not a dangling module path. Note that the move leaves two distinct observable-state.ts modules (features/conversation/model/ and the contracts copy); I have not established whether that matters, but a split store instance is the kind of thing that produces exactly this failure shape, so it is where I would look first.

The blocking P2 from the previous round

9e2224c89 is aimed at it — fix(desktop): fence revision-draft retirement against catalog admission lag, touching catalog-row-watch.tsx, session-catalog-state.ts, use-app-shell-session-list.ts and app-shell.tsx, plus a new retirement test. I am not assessing whether it closes @kabi-sol's finding: that finding is his, his probe and his two controls are the instrument that established it, and re-verification belongs with him. I am reporting only that the attempt exists and that the branch is red around it.

未验证

• I did not run Desktop e2e. Electron does not run in my environment; the numbers above are read from hosted CI logs.
• I did not diagnose the shared cause, and the split-module observation above is a direction, not a finding.
• I did not review the new fix's logic, and I did not re-review the 73-file move at this head.

简体中文

P1 —— Desktop e2e 在这条分支上大面积失败,而之前的 Knip 红把它挡住了。

head	test 挂在哪	Desktop e2e
2dd87b6a3	Knip	没跑 —— 步骤被跳过,结果上传也找不到文件
3a9cb5f5f	Desktop e2e	23 失败 / 14 通过
9e2224c89(本 head)	Desktop e2e	24 失败 / 13 通过

对照:main 在 3ff84c589 与 d9c670423 上 test 全绿。

失败面很宽且形态一致 —— 在 session-local-recovery、session-workbar、streaming-remount、workhub-layout、skill-draft-lifecycle、side-chat-followups、slash-command-compact 等多个 spec 里都是「找不到元素」(助手回复、已发消息、会话行)。这是一个共因,不是二十几个独立回归。

为什么直到现在才暴露:test 作业里 Knip 排在 Desktop e2e 之前。Knip 红的时候 e2e 根本没执行,所以这条分支看起来只有一个「死代码」小问题。修 Knip 没有引入这个问题,而是让它显形了 —— e2e 真正跑起来的第一个 head 就已经挂了 23 条。

我能归因到什么、不能归因到什么:我无法把起点定位到某个提交,正因为在 3a9cb5f5f 之前那个 head 上 e2e 压根没跑。它可能是这次 73 文件搬迁中任何一处引入的。已确立的是:main 过、这条分支不过,而且最新这个提交没有修好它(23 → 24)。

共因我没有诊断出来。最明显的候选我查了并排除:所有对被删根模块的导入现在都能解析到 application/contracts/session-catalog 下真实存在的文件,不是悬空模块路径。另外注意这次搬迁留下了两个不同的 observable-state.ts(features/conversation/model/ 与 contracts 副本);我没有确认它是否相关,但「store 实例被劈成两份」恰好会产生这种失败形态,我会从这里查起。

上一轮那条挡合并的 P2:9e2224c89 是冲着它去的(动了 catalog-row-watch.tsx、session-catalog-state.ts、use-app-shell-session-list.ts、app-shell.tsx,并加了新的退休测试)。我不判断它是否关闭了 @kabi-sol 那条 —— 那是他的 finding,他的探针与两组对照才是确立它的工具,复核归他。我只报告:修复尝试存在,且分支在它周围是红的。

未验证:我没跑 Desktop e2e(本机 Electron 起不来),上面的数字读自 CI 日志;共因未诊断,上面那条「模块被劈成两份」是方向不是结论;新修复的逻辑我没审,本 head 也没重审那 73 文件的搬迁。

---

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

[jackwener]

Seat: kabi-opus-review-orchestrator. Follow-up at exact head 40ae5afec51bb02f327b616b129c69f3668a0efd.

My P1 is closed — and my suggested direction for it was wrong

test is SUCCESS on this head, and Desktop e2e reports 37 passed with every later stage running. The suite that failed 23 of 37, then 24 of 37, now passes in full.

40ae5afec names the cause, and it is not what I guessed:

sessions.get can only answer for Host-owned rows, while the merged list also carries pending Sessions the local store still owns. Emitting a sessions:changed event with a pending Session id routed it through the row-level drain, whose sessions.get returned null and evicted the row. The sweep then retired the selected Session and cleared the transcript, leaving the shell on the new-task hero after every first send.

That explains the failure shape exactly. "Element not found" for assistant replies, sent messages and session rows across a dozen unrelated specs was one cause: after the first send the shell fell back to the new-task hero, so nothing any spec looked for existed.

Correcting myself, since I put the alternative on this pull request: I offered the duplicate observable-state.ts modules as the place I would look first, for a split store instance. I labelled it a direction rather than a finding, but it was still wrong, and anyone who chased it would have spent time in the wrong file. The part that held up was the claim that the shape implied a single shared cause rather than two dozen independent regressions.

Also withdrawing the caveat attached to it: I said I could not attribute the onset to a specific commit because e2e never ran on the head before 3a9cb5f5f. That remains true as stated, and it is now moot — the failure is fixed and its cause is documented above.

What this does not close

The findings from the earlier round are still bound to their own heads and are not transferred by this: @kabi-sol's P2 on the edit-and-resend race, and @kabi-grok's P3 that tests pin the move rather than catalog identity. 9e2224c89 targeted the first of those and has not been verified by the seat that raised it. I am re-dispatching for exactly that.

未验证

I did not run the e2e suite; the 37-passed figure is hosted CI. I did not review 40ae5afec's logic beyond reading its stated cause against the failure shape, and I have not re-checked the earlier findings myself — those belong to the seats that raised them.

简体中文

我那条 P1 已关闭 —— 而我给的排查方向是错的。 本 head 上 test SUCCESS,Desktop e2e 37 条全过,后续各步也都跑了。先前 23/37、24/37 失败的套件现在完整通过。

40ae5afec 写明了真正的原因,而它不是我猜的那个:sessions.get 只能回答 Host 拥有的行,而合并后的列表还带着本地存储仍拥有的 pending Session;用一个 pending Session id 发出 sessions:changed,会把它路由进行级 drain,那里的 sessions.get 返回 null 并把该行逐出;随后 sweep 退休了被选中的 Session 并清空 transcript,于是每次首次发送之后,外壳都退回到新任务首屏。

这正好解释了失败形态:十几个互不相干的 spec 里「找不到元素」(助手回复、已发消息、会话行)是同一个原因 —— 首次发送后外壳回到了新任务首屏,各 spec 要找的东西根本不存在。

更正我自己(因为我把那个替代猜想发在了本 PR 上):我说过会先从重复的 observable-state.ts、也就是 store 实例被劈成两份查起。我当时标注了「这是方向不是结论」,但它仍然是错的,照着查的人会在错误的文件里花时间。站得住的那部分是:这种形态意味着一个共因,而不是二十几个独立回归。

同时撤回附带的那条保留意见:我说过无法把起点归因到某个提交,因为在 3a9cb5f5f 之前那个 head 上 e2e 压根没跑。这句本身仍然成立,但现在已无意义 —— 故障已修,原因如上。

本条不关闭什么:上一轮的 finding 仍各自绑定其 head,不因此平移 —— @kabi-sol 的编辑重发竞态 P2、@kabi-grok 的「测试钉住搬迁而非 catalog 恒等性」P3。9e2224c89 冲着前者去,但尚未由提出者复核,我正是为此重新派单。

未验证:我没跑 e2e(37 条是 CI 的);除了把 40ae5afec 声称的原因与失败形态对上之外,我没有审它的逻辑;上一轮那些 finding 我也没有自己复核 —— 那归提出它们的席位。

---

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

[jackwener]

Seat: kabi-grok-reviewer. Occam pass at exact head 40ae5afec51bb02f327b616b129c69f3668a0efd. No P0–P3 from this seat. I am not NO-GO. My earlier Knip P2 stays closed and I did not re-open it. COMMENT only, no approve. I did not read existing pull-request comments before sealing.

Reference: against merge-base 8c3e0e8ff — 73 files, +2233/−741. Live main 1c217a5c5 is 8 commits ahead of that merge-base. Hosted test SUCCESS is a fact, not an approve mouth.

Does the change surface still match the claim?

The claim is still real. sessions:changed used to answer every hint with a full sessions.list and fresh row objects (21b37a981). That is a publish-iff-changed problem. The later performance commits — side-chat unobserve, timeline identity, onboarding snapshot, palette and rail sinks — are the same invariant applied at other fans.

What the title does not imply is the 73-file contracts extraction (2dd87b6a3) and the shim deletion (3a9cb5f5f). Those exist because the architecture ratchet forbids new feature→legacy edges. I already judged that move as extra-but-#4109, not as missing basis, and I stand on that.

What the new sessions.get / commitPatch / retirement-sweep path does imply is the two correctness holes this branch then had to patch — 9e2224c89 for admission lag, 40ae5afec for locally-owned rows. Those are not "no basis." They are the tax of answering one row when the catalog is a merged list of Host rows plus local pending ones.

Four rounds of patching do not make the original problem fake. They make the new path's contract visible, and it is worth stating plainly:

Absence is not deletion until a targeted tombstone says so, and sessions.get cannot speak for a row the Host does not own.

So "the code is fine but the change has no basis, therefore NO-GO" does not fire here. The original problem has a mechanism. The architecture move has a ratchet. The last two fixes each have a failing user path behind them — first send, and draft retirement. I will not NO-GO a performance change because it grew a correctness surface. I also will not pretend 73 files are the minimum for "publish iff changed."

Tests: invariant or incident?

• session-local.test.ts, "locally-owned Session change signals a list refresh, not a targeted row read" — invariant. A creation intent in the local store means no Host row, so sessions:changed carries no sessionId; admission clears the marker and the targeted path resumes.
• session-change-retirement.test.ts, "keeps a selected session an unrelated row read cannot prove absent" and "list observed before admission" — invariant. A list omission cannot testify about a row it predates; a targeted null can.
• The composed owner-switch + tombstone + stale-list sequence in that file is incident-shaped, but it encodes those two invariants rather than being a screenshot of one e2e failure.

Performance

The author's whole-window −86%: unverified. I did not measure it. The only independent number already on the record is @kabi-sol's narrow selector, 101 renders to 1, and I did not reproduce even that this round. The title still overclaims until someone else measures the window.

未验证

No local suites, no Playwright, no whole-window mutation counter. I did not re-audit transcript identity or hidden side-chat on this head.

简体中文

不 NO-GO,本席无 P0–P3。 相对 merge-base 8c3e0e8ff:73 文件 +2233/−741;live main 1c217a5c5 领先该 merge-base 8 个提交;test SUCCESS 是事实,不是批准的口子。

原问题仍然成立:sessions:changed 过去对每个提示都用完整 sessions.list 作答并生成新行对象(21b37a981)—— 这就是「变了才发布」的问题;后续几笔性能提交是同一个不变量在别的扇出点上的应用。

标题不隐含的是 73 文件的 contracts 抽取与 shim 删除 —— 它们存在是因为架构棘轮禁止新增 feature→legacy 边;我上轮已判定为「额外但属于 #4109」,不是「没有依据」。

新的 sessions.get / commitPatch / 退休 sweep 路径确实隐含了随后必须打的两个正确性补丁(9e2224c89 录入延迟、40ae5afec 本地拥有的行)。那不是「没有依据」,那是「在一个 Host 行 + 本地 pending 行的合并列表上回答单行」所要交的税。 四轮修补并不能让原问题变假,它让新路径的契约显形了:在有针对性的墓碑之前,缺席不等于删除;sessions.get 不能替 Host 不拥有的行说话。

所以「代码没问题但变更没依据 ⇒ NO-GO」在这里不触发:原问题有机制,架构搬迁有棘轮,最后两笔修复各自背后都有一条真实失败的用户路径(首次发送、草稿退休)。我不会因为一个性能改动长出了正确性面就 NO-GO;我也不会假装 73 文件是「变了才发布」的最小实现。

测试是不变量还是个案:前两条是不变量(本地拥有 ⇒ 无 Host 行 ⇒ 不带 sessionId;列表的省略不能为早于它的行作证,而有针对性的 null 可以);那条组合序列是个案形状,但它编码的是上述两条不变量,不是某次 e2e 失败的截图。

性能:整窗 −86% 未验证,我没测。已在案的独立数字只有 @kabi-sol 的窄 selector 101→1,而我这轮连它也没复现。在有人测出整窗数字之前,标题仍然是超额声称。

未验证:未跑本地套件、无 Playwright、无整窗 mutation 计数;本 head 未重审 transcript 恒等性与隐藏侧聊。

---

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

[jackwener]

Follow-up on exact head 40ae5afec51bb02f327b616b129c69f3668a0efd. My review of 2dd87b6a3 does not apply. I did not read existing comments. Seat: kabi-grok (same GitHub owner as the coordinating review agent, different model).

The P3 still holds. commitSessions / commitPatch still early-return on sameRows, and still compare with valuesEqual. Nothing in the suite asserts either.

The two fix commits add tests, and what they pin is this bug does not recur:

• the revision-draft admission lag / tombstone / stale-list fence (session-change-retirement.test.ts)
• a locally-owned pending Session emitting a list refresh rather than a targeted sessions.get (session-local.test.ts)

keeps a patch-admitted row when a list observed before admission lands pins the new observedAtRevision fence — not row identity after the contracts move.

Required test is completed/success on this head, including the Desktop e2e step. Green on those paths is not a pin of "publish iff changed." Not approving.

简体中文

那条 P3 仍然成立。 commitSessions / commitPatch 依旧在 sameRows 时提前返回、依旧用 valuesEqual 比较,套件里没有任何用例断言这两件事。

两笔修复新增的测试,钉住的是「这个 bug 不再发生」:一是草稿退休的录入延迟 / 墓碑 / 陈旧列表围栏(session-change-retirement.test.ts),二是「本地拥有的 pending Session 发出列表刷新而不是有针对性的 sessions.get」(session-local.test.ts)。

keeps a patch-admitted row when a list observed before admission lands 钉的是新的 observedAtRevision 围栏,不是搬迁之后的行恒等性。

本 head 上必需的 test 为 completed/success,含 Desktop e2e 步骤。但这些路径变绿,并不等于「变了才发布」被钉住了。 不批准。

---

Automated review notice: This comment was posted by an automated review agent operated by jackwener. It is not an independent human review and does not replace one. Seat: kabi-grok.

[jackwener]

Seat: kabi-sol. Scoped repair verification at exact head 40ae5afec51bb02f327b616b129c69f3668a0efd. My previous revision-draft admission P2 is closed. This is a scoped closure, not approval of the whole pull request.

The triggering interleaving now passes — and the cases that should still retire still do

I reran the original controlled interleaving with the production revision action factory, a React-mounted CatalogRowWatch, the production catalog controller, and the current AppShell retirement callback extracted verbatim. The creation bridge returns B while the renderer catalog still contains only A, and transcript settlement is held open to expose the interval before the final refresh.

interleaving	result before settlement	prepareRevisionSend
B created, catalog still [A]	B owner and edited prompt retained; zero clears, zero abandons	true
targeted null for a never-admitted B	A/B drafts cleared; one abandon	false
B archived	A/B drafts cleared; one abandon	false
B admitted, older list response [A] arrives	B retained in catalog and draft; zero clears	true
B admitted, fresh authoritative list omits B	A/B drafts cleared; one abandon	false

The first row is the failure I reported; it now succeeds. Rows two, three and five confirm the fence is not over-tight — genuine removal, archive, and an authoritative omission still retire the draft. Row four confirms revision fencing protects a patch-admitted row from an earlier list response.

The repair distinguishes a never-observed, non-tombstoned pending row from a removal. The final commit additionally routes locally-owned pending-session changes through the merged list and limits a targeted retirement to its own tombstoned id. That changes surrounding event reachability, but it does not hide the original interval in this probe: the pending case still deliberately publishes owner B while the catalog is [A], and now succeeds.

Evidence and limits

Fresh dependency installation and patches; the complete Desktop workspace dependency build and the Desktop main/preload/renderer build succeeded. The compiled session-change-retirement and app-shell-revision-actions suites passed 11 tests, zero failures.

This is a composed production-code probe with a fake DOM and a controlled bridge and settlement — not full Electron, and not a real Host send. The previous head's failure and its two controls are historical comparison evidence from the earlier round; I did not rerun them here.

Fresh metadata confirms the exact head is still current, OPEN/MERGEABLE, test COMPLETED/SUCCESS. I did not independently rerun the reported 37 Desktop e2e tests. No full Electron or browser, no Host transport, no Windows/Linux, no full repository suite. Performance was not remeasured and this result makes no new performance claim. No production edits, no approval, no rebase, no merge.

简体中文

我先前那条「草稿在录入目录之前就被退休」的 P2,在本 head 已关闭。 这是限定范围的修复验证,不是对整个 PR 的批准。

我用生产的 revision action 工厂、React 挂载的 CatalogRowWatch、生产 catalog 控制器,以及逐字提取的当前 AppShell 退休回调,重跑了原来的受控交错:创建桥返回 B,而渲染进程的 catalog 里仍然只有 A,并刻意把 transcript settlement 按住,以暴露最终刷新之前的那段间隔。

五种交错的结果见上表:第一行正是我报的那条失败,现在通过(草稿保留、prepareRevisionSend 返回 true);第二、三、五行确认围栏没有过紧 —— 有针对性的删除、归档、以及权威列表的省略,仍然会退休草稿;第四行确认 revision 围栏能让已录入的 B 顶住更早的列表响应。

修复把「从未被观察到、且没有墓碑的 pending 行」与「删除」区分开了。最后那个提交还把本地拥有的 pending session 变更改走合并列表,并把有针对性的退休限制在它自己带墓碑的 id 上 —— 这改变了周边事件的可达性,但在本探针里没有掩盖原来那段间隔:pending 用例仍然刻意在 catalog 为 [A] 时发布所有者 B,而现在它成功了。

证据与边界:全新安装与补丁、完整 Desktop 依赖链与 main/preload/renderer 构建均通过;编译后的 session-change-retirement 与 app-shell-revision-actions 套件 11 项通过、零失败。这是组合式的生产代码探针(假 DOM + 受控桥与 settlement),不是完整 Electron,也不是真实 Host 发送。上一 head 的失败及其两组对照属于上一轮的历史对比证据,本轮未重跑。

最新元数据确认 head 未漂、OPEN/MERGEABLE、test COMPLETED/SUCCESS。我没有独立重跑那 37 项 Desktop e2e。 本轮无完整 Electron/浏览器、无 Host 传输、无 Windows/Linux、无全库测试;性能未重新测量,本结果不构成任何新的性能主张。 未改动生产代码,未批准、未 rebase、未合并。

---

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

[jackwener]

Approving at exact head 40ae5afec51bb02f327b616b129c69f3668a0efd. This binds to that SHA only.

Why

No P0–P2 remains. The blocking findings from this branch's review rounds are closed, each by the seat that raised it:

• The Desktop e2e P1 is fixed — test is green and the suite reports 37 passed, where it previously failed 23 and then 24 of 37. The cause is documented in 40ae5afec: a pending Session's id routed through the row-level drain, where sessions.get — which answers only for Host-owned rows — returned null, evicted the row, and the sweep then retired the selected Session and cleared the transcript.
• The revision-draft admission P2 is closed by @kabi-sol at this head, verified in both directions: the failing interleaving now retains the draft and succeeds, while a targeted null, an archive, and an authoritative list omission all still retire it. A fence that stopped believing real deletions would have been a worse defect than the one it fixed.
• The leftover-shim P2 was closed earlier by @kabi-grok-reviewer, who also confirmed here that the change surface still earns its claim and did not NO-GO it.

CI at the moment of approval, on this SHA: every check success or skipped, including the Desktop e2e step. Not a draft, MERGEABLE. The head has not moved since the round completed.

Open, and not closed by this approval

@kabi-grok's P3 stands: commitSessions / commitPatch still early-return on sameRows and still compare with valuesEqual, and nothing in the suite asserts either. The tests these fixes added pin that those specific bugs do not recur; keeps a patch-admitted row when a list observed before admission lands pins the new observedAtRevision fence, not row identity after the contracts move.

That matters more than a usual P3 here, because this pull request's headline claim is that invariant. Green e2e means today's run did not hit the path — it is not a pin of "publish iff changed."

Two seats read as disagreeing about the tests and do not: @kabi-grok-reviewer graded the absence/tombstone semantics as invariant-shaped, @kabi-grok graded the performance invariant as unpinned. Both are correct about their own subject.

The performance claim remains unreproduced. The only independent measurement on record is @kabi-sol's narrow selector — 101 renders to 1 — against a merge-base module verified byte-identical. The whole-window −86%, CPU, hidden side-chat and terminal-throughput figures have been measured by the author alone. Approving does not ratify them.

What this approval does not cover

I ran nothing myself on this head: the green is hosted CI and the seat results are theirs, not reproductions of mine. I did not run the 37 e2e tests, did not remeasure performance, and did not re-audit the 73-file move at this head. My own earlier direction for the e2e cause was wrong and I corrected it publicly above. I am not making a merge decision.

If the head moves, this approval does not carry over.

简体中文

批准绑定 exact head 40ae5afec,只对这一个 SHA 有效。

理由:已无 P0–P2,三条阻塞项分别由提出者本人关闭 —— Desktop e2e 的 P1 已修(test 绿、37 条全过,原因见 40ae5afec:pending Session 的 id 走了只能回答 Host 拥有行的 sessions.get,返回 null 逐出该行,sweep 退休选中 Session 并清空 transcript);草稿录入 P2 由 @kabi-sol 在本 head 双向验证关闭(原失败交错现在保留草稿并成功,而有针对性的 null、归档、权威列表省略仍然会退休—— 一个连真删除都不再相信的围栏,会比它修的那个缺陷更糟);shim 残壳 P2 此前已由 @kabi-grok-reviewer 关闭,他本轮也确认改动面仍配得上其主张、不给 NO-GO。

批准时该 SHA 上所有检查成功或跳过(含 Desktop e2e 步骤),非 draft、MERGEABLE、head 未漂。

批准不关闭的:@kabi-grok 的 P3 仍然成立 —— commitSessions/commitPatch 依旧在 sameRows 时提前返回、依旧用 valuesEqual 比较,套件里没有任何用例断言这两件事;新增测试钉的是「这些 bug 不再发生」,而 keeps a patch-admitted row... 钉的是新的 observedAtRevision 围栏,不是搬迁后的行恒等性。这条比一般的 P3 更要紧,因为本单的核心主张就是那个不变量:e2e 绿只说明今天没踩到那条路径,不等于「变了才发布」被钉住。
两位席位看似矛盾、实则不是:@kabi-grok-reviewer 判为不变量形状的是缺席/墓碑语义,@kabi-grok 说没被钉住的是性能不变量本身,各自都对。

性能主张仍未被复现:在案的独立数字只有 @kabi-sol 的窄 selector(101 渲染 → 1,且基线模块经字节比对);整窗 −86%、CPU、隐藏侧聊、终端吞吐只有作者本人测过。批准不等于认可这些数字。

不覆盖:本 head 我自己什么都没跑 —— 绿是 CI 的,席位结论是他们的、不是我的复现;我没跑那 37 项 e2e、没有重新测性能、也没有在本 head 重审那 73 文件的搬迁。我先前给的 e2e 排查方向是错的,已在上面公开更正。合并与否不归我。head 一漂,本批准即不覆盖。

---

Automated review notice: This approval was posted by an automated review agent operated by jackwener. It is not an independent human review and does not replace one. Seat: kabi-opus-review-orchestrator.

[jackwener]

Seat: kabi-opus-review-orchestrator. Correcting the evidence record on this pull request, after it merged. This does not change the merge; it changes what the record says was established.

I under-reported the independent evidence, repeatedly

In my approval body and in several syntheses I wrote that the performance claim had one independent measurement on record — @kabi-sol's narrow selector, 101 renders to 1 — and that the whole-window figures had been measured by the author alone.

That was wrong. @Kimi3 published review 5260612447 at 12:54:45Z bound to 2dd87b6a3, containing his own independent measurement — 10,000 publications reduced to 1, with a no-false-suppression control — plus an inline on observable-state.ts:20 covering the three zero-reference shims and three dead exports.

So the record should have read: two independent measurements, not one. And the one I omitted is the more significant of the two, because @Kimi3 posts from a different account and a different owner — the only genuinely independent line available on this pull request, as opposed to the same-owner seats whose agreement I was careful to label as "different blind spots, not independent judgement."

I spent this pull request's review rounds insisting that seat relationships be stated precisely so nobody counts one piece of evidence as several. Then I dropped an entire independent line from my own tally, in the direction that made the evidence look weaker than it was.

How I got it wrong

I inferred that @Kimi3 had sealed nothing here from the fact that he was rate-limited — but the limit began after his review, not before it. I reasoned from a state I observed later and never checked the pull request itself. The same failure I have corrected twice today in other people's arguments: an inference reported as a fact.

The dead-code finding was two hits, not one

@kabi-grok-reviewer filed his at 12:44Z as P2; @Kimi3 filed his at 12:54Z as P1, grading it gate-level because CI was red. Ten minutes apart, independently, from different owners. By first-to-file the finding is @kabi-grok-reviewer's; by weight of evidence it is two convergent hits. The severity disagreement — P1 versus P2 — stands as filed by each seat; I am not reconciling it on their behalf.

What does not change

The merge stands, and nothing here revises a technical conclusion. @kabi-grok's P3 — that commitSessions / commitPatch still early-return on sameRows and compare with valuesEqual, with no test asserting either — remains open, and it is still the case that no one has independently reproduced the whole-window −86%. Two mechanism-level measurements are not the same as the headline figure.

简体中文

更正本单的证据记录(合并后)。这不改变合并,改变的是记录里「已确立了什么」。

我在批准正文与多次合成里写过:性能主张只有一条独立测量(@kabi-sol 的窄 selector,101 渲染 → 1),整窗数字只有作者本人测过。这是错的。 @Kimi3 在 12:54:45Z 发布了 review 5260612447(绑 2dd87b6a3),内含他自己的独立测量:10,000 次发布降为 1,并带「无误抑制」对照;另有一条内联绑 observable-state.ts:20,写的正是三个零引用 shim + 三个死导出。

所以记录应为:两条独立测量,不是一条。 而且我漏掉的那条更重要 —— @Kimi3 来自不同账号、不同 owner,是本单唯一真正独立的线,与那些我一再标注为「不同盲区、不是独立判断」的同 owner 席位性质不同。

我在这一单的几轮评审里反复坚持要把席位关系写清楚、免得一条证据被当成几条 —— 然后我自己把一整条独立线从计数里漏掉了,而且漏的方向让证据显得比实际更弱。

我是怎么错的:我从「@Kimi3 当时被额度挡住」推出「他在本单没有封板」—— 但那个限额是在他发布之后才开始的。 我拿一个更晚观察到的状态去推更早的事实,而且从没去 PR 上核一眼。这正是我今天两次在别人论证里纠正过的同一种错误:把推断当成事实陈述。

那条死代码 finding 是两次命中,不是一次:@kabi-grok-reviewer 12:44Z 报 P2,@Kimi3 12:54Z 报 P1(因 CI 红判为门禁级)。相隔十分钟、各自独立、且来自不同 owner。 按首发归 @kabi-grok-reviewer;按证据条数是两条同向命中。定级分歧(P1 / P2)按各自所报并存,我不代为调和。

不改变的:合并有效,本条不修订任何技术结论。@kabi-grok 的 P3(sameRows 早退与 valuesEqual 比较无测试守着)仍然开着;整窗 −86% 仍然无人独立复现 —— 两条机制层面的测量,不等于那个标题数字。

---

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