Conversation
The handoff progress projection spread the previous attention view, so stale fields like reason "retry_required" surfaced during an active update — exactly the misleading staging + retry_required combination reported in the diagnostics of issue 5476. Split HostHandoffView into a discriminated union (attention carries reason, progress carries phase) and build progress views explicitly instead of spreading. Refs apache#5476 apache#5488 Generated-by: Devin
Desktop startup awaited the Local Runtime Host connect/handoff chain before building the window or registering IPC, so first paint was gated on connect, generation checks, and on a version mismatch the full managed update. The manager already registers its reconnecting IPC router on construction, so construct it synchronously, create the main window and local caches immediately, and run start() in the background. A failed first connect now degrades the Local target to unavailable, the same semantics post-start fatals already used, instead of closing the router and quitting; retryLocalStart re-drives it in place with a fresh epoch so the profile entry and local session cache survive, and profile enablement routes the recovery dialog retry to it. The boot no longer opens a launch progress window; the handoff surface still opens on demand until the in-window surface replaces it. Refs apache#5488 Generated-by: Devin
…ing snapshot The pre-mount prefetch (retry + 2.5s timeout + prop plumbing + the workHub bypass) existed only to skip a transient loading frame. Mount immediately instead: the .maka-preload skeleton already covers the load gap, useOnboardingSnapshot pulls after mount and re-pulls on sessions:changed / connections:event, and a failed pull now falls back to the empty-chat surface instead of suppressing it forever. Generated-by: Devin
… window Replaces the standalone startup/progress window with an in-window attention surface. Host reconciliation now runs silently in the background; progress stays invisible and only views that need a user decision (attention state) are pushed to the renderer, which renders them as a required dialog with the existing copy-diagnostics action. - New runtime-host-handoff-surface publishes the current handoff view over IPC and routes renderer decisions back to the handoff submit. - New preload bridge (current/subscribe/decide) plus a renderer overlay mounted above AppShell; localized copy follows the existing UiCatalog pattern. - Deletes startup-progress-window.ts, startup-presentation.ts, their test, the onShow hook, the duplicated onUpdateProgress option (the onProgress callback already carries the same phases), and the renderer-architecture allowlist entry. - Exports the formatted handoff presentation type so the main-process surface and renderer share one contract. Generated-by: Devin
…lf-layer Managed updates no longer interoperate with Host operators older than dev.9, so the lease machinery that supervised a legacy child's directory lock goes away: - withLegacyFileUpdateLockLease and its .supervised marker producer are deleted; withProcessLifetimeFileUpdateLock keeps recovering stale .supervised + .lock directory pairs left behind by already-shipped builds and still refuses to steal a live legacy directory lock. - withRuntimeHostManagedServiceLegacyOperatorLeases and the update command's lock-protocol probe, inheritedFds stdio plumbing, and RuntimeHostOperatorInvocation are deleted; retire now runs the current operator directly. The process-lifetime-lock-v1 capability stays in the operator echo so older updaters still detect current operators. Trade-off to call out in review: upgrading FROM a pre-dev.9 Host is no longer a supported path; the retire loses the crash-safety umbrella the inherited leases provided for that case. Generated-by: Devin
Exercise the in-window attention surface the same way the bridge drives it in production: replacement consent clicks through to decide(), a retry-exhausted view offers only cancel, and a progress view mounts nothing. Generated-by: Devin
Text defaults to inline display, which pulled the ghost button onto the last line of the detail paragraph. Generated-by: Devin
…a is unverified A degraded Local start resolves instead of rejecting, so the continuation still reaches registerWorkBoardIpc — where require_current throws without a Host-verified schema. The throw aborted the rest of the chain, skipping guest-session restore, interrupted-setup recovery, and remote profile startup. Registration is now isolated and retried when a Local target reaches ready, so a recovered Host completes the board setup too. Generated-by: Devin
…erlay The snapshot fetch and the push subscription were in flight together; a publish landing between them was overwritten by the older current() response. Subscribe first and let the push win. Generated-by: Devin
…ntract The surface re-declared the wire shape it sends; the bridge contract is already the single declaration both sides import. Generated-by: Devin
useUiLocale() throws without LocaleProvider, which lives inside LegacyAppShell — mounting the overlay at Theme level crashed the renderer root on first paint and the window never reported ready. Mounting it in the provider subtree also scopes the dialog to the main surface instead of the floating WorkHub window. Catches added for the snapshot fetch and the clipboard write, which reject when the document is unfocused. Generated-by: Devin
…pdates Every update claimed the visible slot, so a concurrent handoff's progress view could displace a pending attention decision — indefinitely for manualRecheck views that never republish. Only attention views own the slot now, recency-ordered, and the presentation locale resolves per publish instead of once at boot. Unit coverage for the arbitration and the decide fencing, plus a source guard pinning the overlay mount inside the locale providers. Generated-by: Devin
…al start A permanently failed Local Host left the app alive but stranded: the recovery offer skipped the Local profile outright, so retryLocalStart() was unreachable even though the whole retry machinery behind it was already wired. Local failures now enter the same default-Host recovery loop, with a two-button prompt (Retry / Keep Offline) since "Use Local" is meaningless when Local itself is the one that failed. Generated-by: Devin
…agement zone A new flat renderer file is forbidden growth under the renderer architecture check and --strict-base rejects it outright, so the overlay moves into the existing Runtime Host feature zone: a `handoff` port on RuntimeHostManagementServices carries current/subscribe/decide, the platform adapter owns the only window.maka access, and the component consumes services through context — which also removes the provider-free window.maka?.runtimeHostHandoff probe that silently no-oped whenever the bridge was not up yet. Clipboard goes through the port like every other copy action in the feature, and a copied toast restores the feedback the retired startup window had. The stories wrap the real services provider and adapter around a fake bridge channel instead of stubbing the component's data source, and the architecture ledger is regenerated. Generated-by: Devin
getSessions/getConnections/getDefaultSlug and the refs feeding them were seeding plumbing for the removed onboarding prefetch; nothing calls them any more. Generated-by: Devin
…reconciliation startEnabledProfiles only drives remote targets, but it sat behind the Local Host's whole start lifecycle — a handoff parked on a user decision held every enabled remote profile hostage for the session. Generated-by: Devin
The retired startup window called focus() on every attention view; the in-window surface must keep that pull or a required decision waits silently behind a minimized window. One raise per revision keeps repeat updates of the on-screen decision from stealing focus. Generated-by: Devin
With first paint ahead of Host readiness, mount-time refreshes now race the Host coming up. A getDefault() rejection meant "still connecting", but every background read reported it as a failure: five startup toasts, a stuck memory pill, stale shell settings, and an onboarding snapshot error that cascaded into a bogus connection-refresh error. Gate background refresh reporting on default-Host resolvability and lean on the existing ready-transition re-fire for recovery. Mutations and post-ready failures still surface errors. The onboarding poller defers the same transient rejection via a message check — importing the probe would add a forbidden dependency edge in the architecture ledger. Verified end-to-end on a real Electron boot: first paint ~170ms, renderer mounted ~750ms, Host ready ~1.1s, zero startup error toasts, and the onboarding provider list hydrates on the ready transition. Generated with [Devin](https://devin.ai) Co-Authored-By: Devin <158243242+devin-ai-integration[bot]@users.noreply.github.com>
The window stayed hidden until the renderer's first React commit, so the designed .maka-preload loading surface — meant to be the loading UI — was never visible and perceived startup was bounded by React mount (~750-940ms). Electron's ready-to-show fires as soon as the skeleton has painted; routing it through the existing reveal gate shows the window ~500ms earlier while keeping inactive/hidden modes, deferred focus, and the notifyRendererReady/fallback backstops intact. Perceived startup now matches the UI-first goal: the window appears with the loading surface while Runtime Host reconciliation continues in the background. Generated-by: Devin
runtime-host-boot evaluates ~1100 compiled files before its first statement, so window creation could not start until ~400ms after the app was ready. early-window.ts holds the light slice the window actually needs — storage root, settings, locale, diagnostics, the window controller, the quit coordinator — and fires createWindow as soon as it exists; main.ts imports it first so the remaining Runtime Host graph loads while the renderer is already navigating. Window creation moves from ~400ms to ~210ms on this machine. The login-shell PATH probe now starts at module top and is awaited only where a child process is spawned (Local Host start, remote profiles, MCP) — it no longer sits serially ahead of the window (~100ms on a real launch; e2e fixtures skip it as before). Independent small reads (client instance id, Runtime Host startup) run in parallel. window:notifyRendererReady is not a Host-scoped channel: it is registered on ipcMain by early-window so the renderer's first commit cannot outrun scoped-router registration. The quit coordinator and the window controller keep their exact lifecycle semantics — before-quit aborts in-flight creation, close hooks and diagnostics reach the Runtime Host through boot-context late bindings. Generated-by: Devin
The previous commit deferred the Runtime Host module graph until the window existed, but the window still stayed hidden until ready-to-show: the user-visible surface arrived at ~500ms while the OS window itself could have been on screen ~160ms earlier with its theme-matched backgroundColor reading as a launch surface. - `show: revealMode === 'active'` — active runs display the native window at construction; the persisted appearance still picks the right backgroundColor so the first visible frame is theme-correct. Hidden/inactive e2e modes keep show:false and the reveal gate. - Saved-bounds/mkdir/appearance reads run in parallel ahead of the constructor; serialized they cost ~200ms of prelude. - A maximized session restores via maximize() directly in active mode (the window is already shown, so the reveal-gate deferral no longer applies); hidden/inactive runs still defer to markReady. - The firstWindowConstructed boundary moves from construction to the native 'show' event: the heavy module graph may evaluate once the window is on screen without starving the display path, and the launch-settle fallback keeps hidden/failed runs unblocked. Generated-by: Devin
github-actions
Bot
added
effort/XXL
The fake app-frame shimmer (a card with two gradient bars) read as a broken half-rendered UI rather than a launch screen. Codex's cold start shows the better pattern: a theme-matched window with a centered mark. - `.maka-preload` becomes the traced wordmark (the same MAKA_WORDMARK_PATH the hero and dock icon use) at --maka-brand on the theme background, with a slow opacity breath and a reduced-motion opt-out. - `body` gets the hardcoded theme background (#ffffff / #1c1d21) so the native window's `backgroundColor` hands off to the first frame with no colour step. - The renderer entry contract allowlists the three presentational tags the inline mark needs (svg/g/path); navigation and execution vectors stay closed via the single-module script check and the on*= scan. Generated-by: Devin
… frame The launch overlay moves out of #root into a fixed fullscreen layer, so React mounts underneath it instead of replacing it. AppShell drops it once the bootstrap snapshot resolves and no session view or transcript read is still in flight, which removes the bare-vibrancy window, the partial shell, and the skeleton beats from the startup sequence. main.tsx arms an 8s failsafe so a wedged read can never strand the logo. The window returns to show:false — ready-to-show now reveals the first painted frame, which is the launch surface itself, and restoring maximized state defers to markReady in every reveal mode. Generated-by: Devin
jackwener
left a comment
jackwener
left a comment
There was a problem hiding this comment.
Review of exact head bdf6845f98f99c8d990e681c4550adebd0575f81 (64 files, +1920/−1631). One [P2]. Not approving — draft, CONFLICTING/DIRTY, zero CI checks, and 27 commits behind main (merge-base f02ac9433). I reviewed against the merge-base, not against current main.
The [P2], in one line
The declared breaking change ships without detection: the probe that used to tell a pre-dev.9 operator apart kept its shape but lost capabilityRequest, so an old operator now passes it and gets retired without the advisory lease the old protocol depends on — with no error telling the user their Host is unsupported. Details and a few-line guard are inline; the capability constant the guard needs is still exported by runtime-host.
Things I checked that turned out fine
These are the removals most likely to be half-done, so I chased each rather than trusting the summary:
recoverSupervisedLegacyLocksurvives and is still called (process-lifetime-file-update-lock.ts:52), whilecreateSupervisionMarkeris gone. That asymmetry is exactly right: a machine that ran an older build may still have a stale.supervisedmarker on disk, so recovery must outlive creation. The description's claim that stale-marker recovery is unchanged holds.- No residue for
withLegacyFileUpdateLockLease,withRuntimeHostManagedServiceLegacyOperatorLeases,prefetchOnboardingSnapshot,initialOnboardingSnapshot,StartupProgressWindow,showDesktopStartupProgress— zero references outsidedist. process-lifetime-lock-v1still exists on purpose. The two remaining references are the Host-side declaration and a CLI test. Only the consumer was removed, which is coherent: the Host keeps advertising the capability, the updater stops asking. (That is also what makes the inline guard cheap.)- The startup chain still catches.
main.tsends its.then()withreturn import('./runtime-host-boot.js'), so the heavy boot stays inside the promise chain and its rejection reaches the existing.catch. The "fire-and-forget" in the description is about the manager launch inside boot, not about the window path — the window path is still awaited and still guarded.
Not covered
- No CI exists on this PR, so nothing below is corroborated by the repo's gates.
- I did not run the app, any suite, Storybook, or the smoke. No number in the Verification section is verified by me; the two screenshots are unverified.
- I reviewed the breaking change from this tree only. What a real pre-dev.9 operator does when retired without the lease, I could not determine — its binary is not here.
- I did not review the renderer/overlay work,
early-window.ts's 321 new lines, the handoff surface, or the discriminated-union view change beyond reading the summary. Other seats are reviewing this PR independently and may cover those. - 27 commits behind main means a semantic conflict with anything merged since
f02ac9433is outside what I checked — on the sibling PR this exact gap is where a real finding lived.
Automated review notice: Posted by an automated review agent (Claude Opus 5) through the shared
jackweneraccount (seat: kabi-opus). Several seats are reviewing this PR independently and publishing separately; each speaks only for itself, and none is a human review.
简体中文
审查 exact head bdf6845f9(64 文件 +1920/−1631)。一条 [P2];不批准 —— draft、CONFLICTING/DIRTY、零 CI check、落后 main 27 个 commit(merge-base f02ac9433)。我是对着 merge-base 审的,不是当前 main。
[P2]:声明的破坏性变更没有配套检测。原来那个能分辨"pre-dev.9 operator"的探测外壳还在,但丢掉了 capabilityRequest ⇒ 旧 operator 现在能通过这个探测,随后 retire 不带它所依赖的 advisory lease 就执行,而且不会有任何"你的 Runtime Host 早于 dev.9,不受支持"的报错 —— PR 描述里写给人看的那句话,在代码里没有对应物。守卫所需的能力常量仍由 runtime-host 导出,所以补上只需几行(内联给了)。
我无法确定的部分也写明了:真实的 pre-dev.9 operator 在没有 lease 的情况下被 retire 会怎样 —— 它的二进制不在这棵树里。这份不确定正是该加守卫的理由,而不是反对的理由:当初留这层互操作,就是因为"无保护"被认为不安全,而本 PR 没有证明它不再不安全。
我查过、结果没问题的几处(这些是最容易"删一半"的地方,所以逐个追了):①**recoverSupervisedLegacyLock 保留且仍被调用**,而 createSupervisionMarker 已删 —— 这个不对称恰恰是对的(旧机器上可能还留着 .supervised,恢复必须比创建活得久),描述的说法成立;②六个被删符号零残留;③process-lifetime-lock-v1 保留是对的(Host 侧声明 + 一个 CLI 测试,只删了消费方);④启动链仍然被 catch 住:main.ts 的 .then() 以 return import('./runtime-host-boot.js') 结尾,重模块仍在 promise 链内,拒绝会走到既有 .catch。
未覆盖:本 PR 无任何 CI;我没有跑应用、没跑任何套件、没跑 Storybook/smoke ⇒ Verification 一节的数字与两张截图我一个都没验证;渲染层/overlay、early-window.ts 的 321 行新代码、handoff surface、判别联合视图我都没审(其他席在独立审);落后 main 27 commit ⇒ 与此后合入内容的语义冲突不在我检查范围内 —— 姊妹单上真实的 finding 正是长在这条缝里。
jackwener
left a comment
jackwener
left a comment
There was a problem hiding this comment.
Seat: kabi-grok-reviewer. Independent review of bdf6845f98f99c8d990e681c4550adebd0575f81.
Reference frame: this head vs merge-base f02ac94336aa596146f80da3db19f35c92d307a4 (live origin/main 0117d76c5, 28 commits ahead). I did not review as-if-rebased onto current main. GitHub baseRefOid 5b9db1ce8 is not that merge-base.
Draft, DIRTY / CONFLICTING, no checks. COMMENT only — no approve, no rebase, no merge.
Verdict
1×P2, no P0/P1/P3. First paint is actually off the Host critical path on this head. Background reconciliation has no status surface, which is the half of #5488 this PR dropped.
Design
#5488 is still OPEN (author issue = claim). Desired outcome there: first paint without Host and “Host reconciliation … reports progress through a non-modal status surface instead of a blocking modal.” This PR implements the first and explicitly makes progress views silent.
P2 — progress is silent; Host-dependent IPC still times out at 15 s
本次回归 vs #5488. Class ①: every launch that still has a minutes-long handoff/update (#5488’s own numbers: ~124 MB / ~6800 files).
createDesktopHostHandoffSurfaceonly publishesattentionviews. Progress updates refresh the map but never ownactiveId, so the renderer getsnull.RuntimeHostHandoffOverlayreturns null unlessview.state === 'attention'.runtimeHostManager.start()isvoidafter window construction (runtime-host-boot.ts:1316). Good for first paint.- Host-dependent IPC still waits
DEFAULT_REPLACEMENT_WAIT_TIMEOUT_MS = 15_000thenHandlerWaitExpiredError. A send can sit in the local outbox; anything that goes through the reconnecting gate fails after 15 s while the Host is still staging, with no in-window progress explaining why.
The old modal was the hang. The new shell looking ready while Host is still replacing is a different hang: the user cannot tell working from stuck.
Minimal fix: a non-modal status on progress views (what #5488 asked for), or at least keep the 15 s wait from firing a user-visible error while a progress view is open.
What held (this head vs merge-base)
main.tswaits onlyearlyWindow.firstWindowConstructed(nativeshow) before importingruntime-host-boot.js. Host graph is not on the first-paint path.- Attention views still reach the main window as a required Dialog; a silent progress update cannot displace a pending decision (map reinsert + last-attention wins).
onFatalErrorno longer kills the app; default-host recovery can offer retry.- Pre-dev.9
withLegacyFileUpdateLockLeaseis gone;recoverSupervisedLegacyLockstill runs inside the current process-lifetime lock. I did not prove leftover pre-dev.9 Hosts still update — breaking change as declared. - Onboarding snapshot prefetch is gone;
useOnboardingSnapshothydrates async. Launch surface failsafe 8 s.
Not verified
- Did not rebuild
@maka/*dist (review is from this worktree’s source; I did not run desktop/cli/runtime-host suites). - No browser / no real first-paint timing.
- No Windows.
- Did not walk live main’s 28 extra commits for merge collision.
- Author-claimed 117/117 is not evidence.
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
left a comment
jackwener
left a comment
There was a problem hiding this comment.
Seat: kabi-grok (same GitHub owner as the coordinating review agent, different model). Review of exact head bdf6845f98f99c8d990e681c4550adebd0575f81.
I did not read existing pull-request comments. I am not approving. This head is a draft, CONFLICTING/DIRTY with main, 27 commits behind merge-base f02ac9433, and has zero check runs. Conflict resolution will change code; this review dies if the head moves. I will not rebase or merge it.
Frame of reference (two different questions)
A. Versus this SHA's merge-base (f02ac9433): does the change do what #5488 asked?
B. Versus live main today (compatibility epoch 166): what will a DIRTY merge do?
#5470's epoch collision lived in gap B. This PR does not touch RUNTIME_HOST_COMPATIBILITY_EPOCH. This SHA still contains epoch 162 because the branch is stale; that file is not in the 64-file diff, so a clean merge of that file should keep main's 166. The files that will fight main are the ones this PR actually edits — especially host-handoff.ts (main still has a single HostHandoffView with state: 'attention' | 'progress'; this SHA splits it) and the legacy file-lock deletion (main's CLI still calls withLegacyFileUpdateLockLease).
A — versus merge-base
The problem is real. A stuck handoff (staging → retry_required) behind StartupProgressWindow blocked the whole app. First paint should not wait for Host ready.
The split is the right layer:
main.tswaits only forfirstWindowConstructed, then dynamically importsruntime-host-boot.ts. The ~1100-file Host graph is not on the window-construction path.runtimeHostManager.start()is insidevoid (async () => { ... })()after IPC wiring — fire-and-forget, not a startup gate.- Attention-only overlay in the main window.
HostHandoffViewis a discriminated union, so aprogressview cannot carry areason. The slot keeps the newest attention view; a concurrent progress update cannot displace it (unit test pins this). retry_requiredis an attention reason. Progress stays silent. Window raise is once per attention revision.- Pre-ready Host reads on module-hub controllers are pending, not failed.
- Breaking: drop pre-dev.9 dir-lock / inherited-fd interop. On this SHA the CLI caller is gone with the helper. That is in-scope for the stated break, not smuggling.
I did not find a P0–P2 versus this merge-base.
[P3] this pull request — Host graph can still race the first paint
createWindow awaits loadMainRenderer and then resolves without waiting for ready-to-show. firstWindowConstructed also resolves on that promise, so import('./runtime-host-boot.js') can start evaluating the Host graph while Chromium is still painting the launch surface. Better than blocking window construction; still a shared-thread race with first paint. The 'show' listener is a second resolver and does not prevent the loadURL path from winning.
[P3] this pull request — lifetime tests are source-order greps
main-startup-lifetime.test.ts pins indexOf('await runtimeHostManager?.start()') after window create. That is the right order and would miss a later await start() reintroduced on the window path under a different spelling. The handoff-surface tests are the ones with discriminating power.
B — versus live main (merge action, not a severity upgrade)
- Do not keep this SHA's epoch 162 if a conflict appears in protocol files; live main is 166.
host-handoff.tswill need a manual merge: keep the discriminated union (attentionvsprogress) or #5476's leakedreasoncomes back.- Legacy lock: take this PR's storage+CLI pair together, or main's remaining
withLegacyFileUpdateLockLeasecaller will not compile.
Verification bounds
- Walked: boot order, handoff union + overlay slot, reconnecting 15s wait, onboarding async hydrate, legacy-lock removal, overlay vs LocaleProvider.
- Did not run desktop/cli/runtime-host suites or Storybook (no CI on this SHA).
- Did not boot Electron to watch first paint vs a stuck handoff.
- Did not read other reviews.
- Did not inspect every conflict hunk against live main beyond epoch,
host-handoff.tsshape, and the legacy lock caller.
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。只绑 bdf6845f9。draft、DIRTY、无 CI，不 approve。
相对 merge-base：问题真，窗先于 Host 图、start() 在 IIFE 里、attention-only overlay + 判别联合。无 P0–P2。
相对当前 main：本 PR 不改 epoch（分支上仍是 162 是落后，不在 64 文件里）；会打的是 host-handoff.ts 的联合类型，以及删掉的 legacy file-lock（main 的 CLI 还在调用）。
P3：Host 图仍可能和首帧抢主线程；lifetime 测试是源码顺序 grep。
没跑套件 / Electron。
jackwener
left a comment
jackwener
left a comment
There was a problem hiding this comment.
Review seat: kabi-sol. Reviewed bdf6845f98f99c8d990e681c4550adebd0575f81 independently against merge-base f02ac9433, without reading existing PR comments. Two P2 findings; I do not recommend merging this version.
The startup bottleneck is supported by the old await-before-window path and the direction agreed in #5488. However, the new path still hides draft editing when the Host has no identity, and a handoff refresh can resurrect a cancelled modal.
I rebuilt the complete Desktop dependency chain and Desktop main/preload/renderer/resources/overlays. All 81 focused startup/manager/recovery/handoff/onboarding tests passed. The first finding was reproduced in real Electron using the production preload and renderer with an injected unavailable-identity IPC response; the second uses the production handoff loop, surface, locale authority and disk-backed settings. These are controlled fault/race probes, not a full Host upgrade test.
I did not run all repository suites, Windows/Linux, real SSH/libp2p recovery or a conflict-resolved integration with current main. The fetched main was 0117d76c5 (28 commits ahead of the branch's base); the PR remains draft/conflicting with no CI results. The conclusions apply only to the reviewed head.
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-reviewer. Correction to review I independently read #5488 (comment) (Astro-Han, 2026-09-18). That later plan explicitly chooses silent background reconciliation and attention-only UI. I therefore reclassify the status-surface half of my P2 as an acceptance boundary of that plan, not a function defect against current author intent. The original issue body still asked for a non-modal status surface; the later comment supersedes that as the author's public plan. What remains [P2] 本次回归: first paint now precedes Host ready, so the 15 s reconnecting IPC wait (
|
|
Seat: kabi-grok-reviewer. Second correction to review I withdraw the remaining [P2] (15 s reconnecting IPC wait on first paint). Call chain on this head:
Silent progress remains an acceptance boundary of #5488 (comment). This seat has no open P0–P3 on this head. Other seats' findings are not mine.
|
zhiiw
left a comment
zhiiw
left a comment
There was a problem hiding this comment.
Independent review (blind — I have not read existing comments on this PR). Conclusions bind to bdf6845f98f99c8d990e681c4550adebd0575f81 (Draft, CONFLICTING vs main, zero CI checks — a code review, not a merge judgment).
Verified locally (real Windows 11, Node 24.18.1 — the version CI pins), the part no other seat can run: the startup/handoff suites that exercise this PR's core — main-startup-lifetime 7/7, runtime-host-desktop-manager 58/58, runtime-host-handoff-surface 3/3, use-onboarding-snapshot 11/11, runtime-host-default-recovery 2/2, startup-storage-repair 2/2, module-hub controllers 13/13, native-diagnostic-dialog 4/4, connection-settings-locale-render 21/21; packages side host-handoff + cli handoff-surface + file-update-lock 25/26 — the one failure is a pre-existing Windows fork-fd-inheritance test that fails identically on the base. runtime-host-local-remote-access.test.js fails 12/12 on my machine, but those fixtures are POSIX-shaped (the fake operator declares platform: 'posix' and posix-absolute paths); that suite is not Windows-targeted and CI never runs it there — not this PR's account.
The deletion is real and clean. Every removed symbol has zero remaining production consumers (verified by grep across apps/packages): the whole StartupProgressWindow / startup-presentation surface, the onboarding prefetch plumbing, and the legacy file-lock interop (withLegacyFileUpdateLockLease, withRuntimeHostManagedServiceLegacyOperatorLeases). RuntimeHostUpgradeCancelledError survives as manager API, deliberately. Deleted tests pinned deleted code, and their replacements exist where the behavior moved (cancel-no-longer-quits is re-pinned as unavailable + retry; the handoff progress/attention discrimination is a real discriminated union with typed helpers).
The boot reorder reads correct. Window creation moved ahead of the Runtime Host module graph in main.ts (the ~1100-file evaluation no longer starves first paint); early-window.ts holds the light slice; the manager is constructed synchronously but start()d in the background continuation, and start() itself degrades a failed first connect into unavailable instead of throwing — so the app staying up is structural, not a swallowed error. The quit coordinator moved into early-window before any handoff can exist. Fatal local errors no longer quit the app — consistent with the stated direction, and retryLocalStart gives the recovery path.
The handoff chain is sound. attention vs progress is a discriminated union enforced by types; main publishes only attention entries; the overlay double-filters; onOpenChange is a deliberate no-op so Escape/light-dismiss cannot drop a required decision; decide is validated main-side (revision + live attention + advertised action); focus raises once per revision.
One finding, inline: a P3 where the renderer meets the not-yet-registered WorkBoard IPC — an error banner that does not self-heal when the Host later becomes ready. Squarely on the slow-start path this PR is improving, so worth closing in it.
Not verified: the storybook smoke and the e2e suite (no display harness on this machine; CI coverage is also absent on this head — zero checks ran). The next head after the conflict resolution needs its own pass: main.ts, runtime-host-boot.ts, early-window.ts and the handoff surface are exactly the files main keeps touching.
COMMENT per this round's convention — no approval is being offered on a conflicting, CI-less draft.
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
left a comment
jackwener
left a comment
There was a problem hiding this comment.
Approving at exact head bdf6845f98f99c8d990e681c4550adebd0575f81, at the author's explicit request. Read the next paragraph before treating this as a quality signal.
What this approval is not. This PR is a draft, it is CONFLICTING, and it has no CI checks at all — the status rollup is empty, so nothing here has been validated by this repository's gates. It cannot merge in this state, and the moment the conflict is resolved the head changes and this approval stops describing the code. I normally require CI green on the exact head before approving; that condition is not met and is not waived quietly — it is recorded here.
Three [P2]s remain open. None is withdrawn or downgraded, and approving does not resolve any of them.
- An unavailable Host leaves the composer hidden after first paint.
shouldDeferErrorclassifiesidentity is unavailableas still connecting with no timeout and no way to express never, so the onboarding snapshot stays pending forever,isOnboardingLoadingstays true, and the real composer is hidden. Reproduced in Electron against the production preload/renderer, with a control that restores the composer by changing only the error classification. This is the case issue #5488's accepted direction names as an acceptance criterion — "even if the Host never becomes available, users can still edit and retain drafts, retry the connection, and quit normally." - A cancelled handoff can be resurrected. With the real one-second refresh, cancel produces
attention → null → the same attention, whilecurrent()is already null; the revived dialog's buttons are dead because its revision is stale. - The declared breaking change ships without detection (inline,
runtime-host-update-command.ts). The probe kept its shape but lostcapabilityRequest, so a pre-dev.9 operator passes it and is retired without the advisory lease, with no "unsupported update source" error. On currentmainthe legacy path is still live and called, so the conflict resolution will have both versions in front of it.
One [P3] worth reading beside the first [P2], because they are the same shape: workBoard:list is registered only inside the readiness === "ready" branch (runtime-host-boot.ts:1193), so a renderer that mounts first gets No handler registered — an error banner and a manual retry — and registration emits no change event to heal it. Both cases are first paint arriving at a surface whose backend is not in place yet, with not ready yet rendered as broken or loading forever. The plan in #5488 said "register all IPC up front"; this IPC is not.
What I did verify at this SHA: the legacy removals are complete and consistent (recoverSupervisedLegacyLock correctly outlives createSupervisionMarker; zero residue for six removed symbols; the Host-side capability declaration is rightly kept); the startup chain still catches, since main.ts returns the boot import into its promise chain. What I did not: run the app, any suite, Storybook, the smoke, or Windows — the numbers and screenshots in the description are unverified by me, and I reviewed against merge-base f02ac9433, not current main (28 commits ahead as measured at 17:00Z).
Five automated seats reviewed this head independently and published separately; two of them withdrew or narrowed their own findings after tracing the evidence further, which is why the count above is smaller than it was an hour ago.
Automated review notice: Approved by an automated review agent (Claude Opus 5) through the shared
jackweneraccount (seat: kabi-opus), at the PR author's explicit request. Not an independent human review, and not a substitute for one. The merge decision for a feature belongs to a human.
简体中文
在 exact head bdf6845f9 上批准,应作者明确要求。请先读下一段,再把它当作质量信号。
这个批准不是什么:本 PR 是 draft、CONFLICTING、且一个 CI check 都没有(rollup 为空)—— 本仓的任何门禁都没有验证过它。它当前合不进去;冲突一解决 head 就变,这个批准随即不再描述代码。我通常要求 CI 在该 exact head 上绿才批;这个条件没有满足,而且不是被悄悄豁免,是写在这里。
三条 [P2] 仍然开着,没有一条被撤回或降级,批准也不解决其中任何一条:
① Host 不可用时首屏之后 Composer 仍被隐藏 —— shouldDeferError 把 "identity is unavailable" 判为"仍在连接",无超时、无法表达"永不" ⇒ 快照恒 pending ⇒ Composer 被藏。真机 Electron 复现,对照实验只改错误分类就恢复。这正是 #5488 已接受方向写下的验收标准:"even if the Host never becomes available, users can still edit and retain drafts…"。
② 已取消的 handoff 会被复活:真实 1 秒刷新下,取消后输出 attention → null → 同一个 attention,而 current() 已为 null;复活弹窗的按钮因 revision 过期而失效。
③ 声明的破坏性变更没有配套检测(内联):探测保留了外壳但丢掉了 capabilityRequest ⇒ pre-dev.9 operator 能通过,随后无 advisory lease 被 retire 且无"不受支持"报错。当前 main 上 legacy 路径仍是现役并被真实调用,解冲突时会同时看到两版。
一条 [P3] 值得和第 ① 条并读,因为它们是同一个形状:workBoard:list 只在 readiness === "ready" 分支内注册(runtime-host-boot.ts:1193),先挂载的 renderer 会吃 No handler registered(错误横幅 + 手动重试),注册落地也不发变更事件来自愈。两者都是:首屏到达了一个后端尚未就位的界面,而"还没好"被渲染成"坏了"或"永远在转"。#5488 的方案写的是 "register all IPC up front",而这个 IPC 不是。
该 SHA 上我验过的:legacy 移除完整一致(recoverSupervisedLegacyLock 正确地比 createSupervisionMarker 活得久;六个被删符号零残留;Host 侧能力声明保留是对的);启动链仍被 catch(main.ts 把 boot 的 import 返回进了 promise 链)。我没验的:没跑应用、任何套件、Storybook、smoke 或 Windows —— 描述里的数字与截图我都没有验证;我是对着 merge-base f02ac9433 审的,不是当前 main(17:00Z 测得领先 28 个 commit)。
五个自动化席位独立审查了这个 head 并各自发布;其中两席在继续追证据后撤回或收窄了自己的结论,这也是上面计数比一小时前更小的原因。
Locale resolution makes each publication asynchronous, so a payload computed for a superseded or closed handoff could land after the newer state and resurrect the attention modal. Fence each publish with a monotonic ticket invalidated by update and close. Generated-by: Devin
The identity-unavailable deferral kept the snapshot pending forever when the default Host never acquired an identity, hiding the composer indefinitely. Gate the deferral on the authoritative profile readiness: only 'connecting'/'reconnecting' means still pending; 'unavailable', 'disabled' or a missing entry now surfaces the error so the shell exits its loading state. Generated-by: Devin
firstWindowConstructed could resolve at launch-settle before the first frame was composited, letting the Runtime Host module graph contend with the paint it was meant to follow. Resolve on ready-to-show instead — it fires at the first frame for every reveal mode, including hidden runs that never emit 'show'; launch-settle stays as the fallback resolver. Generated-by: Devin
The status probe dropped its capabilityRequest when the legacy lease path was removed, so a pre-lifetime-lock operator now passes the probe and gets retired without the advisory lease it needs. Restore the request and refuse operators that cannot echo it, so the failure is an explicit unsupported-operator error instead of a lock-free retirement. Generated-by: Devin
workBoard:list rejects with "No handler registered" while the Local Host is still connecting, and the panel's error state never self-heals because nothing re-triggers the load. Emit workBoard:changed after registration so a mounted panel reloads itself. Generated-by: Devin
|
⚠ My approval above is stale and does not describe the current code. It was given at I am flagging it rather than acting on it. Dismissing or renewing an approval moves a merge gate, so that decision belongs to a human, not to this automated seat. I will not dismiss my own approval and I will not silently re-approve. Two things worth being explicit about, because this approval was unusual to begin with:
If you want this head reviewed and approved, say so and I will review it as a fresh head and bind a new conclusion to it.
简体中文⚠ 上面那条批准已经过期,不描述当前代码。 它绑的是 我只标注,不处置。**撤销或续批都会动到合并门禁,那个决定属于人类,不属于这个自动化席位。**我不会撤销自己的批准,也不会悄悄重批。 有两点必须说清,因为这条批准本来就是特殊的:
需要我审这个新 head 并给结论,说一声,我按新 head 重审并重新绑定。 |
…artup Generated-by: Devin # Conflicts: # apps/desktop/renderer-architecture.json # apps/desktop/src/main/__tests__/module-hub-skills-controller.test.ts # apps/desktop/src/main/main.ts # apps/desktop/src/main/runtime-host-boot.ts
jackwener
left a comment
jackwener
left a comment
There was a problem hiding this comment.
Re-review at exact head aa62cb2fae58160500ab2da818ce729c1f69cda0 (34 commits past the SHA I approved; base now 0117d76c5, and the branch is MERGEABLE with real CI attached for the first time).
My [P2] is fixed, and the fix is better than what I proposed
capabilityRequest is restored on the probe and an operator that cannot echo the capability is refused outright. I asked for a guard; you also gave the user the next action — "predates capability reporting and cannot be safely retired; uninstall it before updating" — which my sketch did not. That message is the part that matters when someone actually hits it.
[P3] — the new guard is bypassed on the repair path
} catch (error) {
if (!activeTargetNeedsRepair) throw error;
currentOperatorUnavailable = true;
}This catch predates the guard, but it now swallows something new. Before, it could only absorb "I could not ask the operator". It now also absorbs "I asked, and the answer disqualifies it" — the deliberate RuntimeHostServiceManagerError thrown a few lines above. With activeTargetNeedsRepair, an operator that predates capability reporting is reclassified as merely unavailable, reaches the retirement_failed frame, and under --allow-interrupt-active-tasks is retired anyway through the forced backend.retire(). That is the outcome the guard exists to prevent, on the path where a damaged target makes it most likely.
It is a [P3] rather than higher because it needs repair mode and an operator that old, and because refusing to repair a broken install has its own cost — this is a trade-off worth stating rather than an obvious bug. Distinguishing the two failures would keep both properties: rethrow the capability rejection regardless of activeTargetNeedsRepair (or record it in a separate flag), so "cannot ask" still degrades to repair while "asked and it is unsupported" still refuses.
[P1] — test is red, and it is this PR's account
Check the hooks scoped to the whole shell:
app-shell.tsx: useEffect has 8 call sites in AppShellContent, inventory allows 7.
app-shell.tsx: useLayoutEffect has 1 call sites in AppShellContent, inventory says 2.
The gate warns that it might be miscounting, so I checked rather than assuming: the diff contains - useLayoutEffect(() => { / + useEffect(() => {. The hook really did move — which is a sensible thing to do in a PR about not blocking first paint — so the inventory in scripts/check-app-shell-hooks.mjs is what needs updating, not the gate. Same family as the ledger counters on the sibling PR: the change is right, the recorded numbers just have not followed it.
On my earlier approval
It is still bound to bdf6845f9 and still stale; my annotation above stands. I am not renewing it here — that is a human's call, and this head is still a draft with a red test.
I re-checked only my own finding and the CI state. The other seats' findings on this PR are theirs to re-verify.
Automated review notice: Posted by an automated review agent (Claude Opus 5) through the shared
jackweneraccount (seat: kabi-opus). Not an independent human review.
简体中文
在 exact head aa62cb2fa 上复审(距我批准的 SHA 已 34 个提交;base 现为 0117d76c5,分支 MERGEABLE,首次挂上真实 CI)。
我那条 [P2] 已修,而且比我提的写法更好:探测恢复了 capabilityRequest,无法回显该能力的 operator 被直接拒绝。我要的是一个守卫,你还给了用户下一步动作 —— "predates capability reporting and cannot be safely retired; uninstall it before updating" —— 这句我的草稿里没有,而真撞上的人需要的正是它。
[P3]:新守卫在 repair 路径上被绕过。那个 catch 比守卫更早存在,但它现在吞掉了新的东西:以前它只能吸收"我问不到 operator",现在它连"我问了,而答案说它不合格"(几行之上那个刻意抛出的 RuntimeHostServiceManagerError)一起吸收。于是在 activeTargetNeedsRepair 下,一个早于能力上报的 operator 会被重新归类为"不可用",走到 retirement_failed,并在 --allow-interrupt-active-tasks 时经由强制 backend.retire() 照样被 retire —— 正是该守卫要阻止的结果,而且发生在最容易发生的那条路径上。
定 P3 而非更高:它需要 repair 模式且足够老的 operator,而且"拒绝修复一个坏掉的安装"本身也有代价 —— 这是个值得写明的取舍,不是显然的 bug。把两种失败分开即可两者兼得:让能力拒绝无视 activeTargetNeedsRepair 继续上抛(或记到单独标志),"问不到"仍降级为修复,"问了且不受支持"仍然拒绝。
[P1] test 红,是本单的账:Check the hooks scoped to the whole shell 报 useEffect 8 个(清单允许 7)、useLayoutEffect 1 个(清单说 2)。那个检查自己提示"可能是它数错了",所以我核了:diff 里确实有 - useLayoutEffect(() => { / + useEffect(() => { —— hook 真的挪了(在一个"不阻塞首帧"的 PR 里这么做是合理的),所以要更新的是 scripts/check-app-shell-hooks.mjs 里的清单,不是这个 gate。与姊妹单的台账计数同一类:改动是对的,只是记录的数字没跟上。
关于我先前那条批准:它仍绑 bdf6845f9、仍然过期,上面的标注继续有效。我不在这里续批 —— 那是人的决定,而且这个 head 仍是 draft、test 仍红。
本次我只复核了自己的 finding 与 CI 状态;其他席位的 finding 由他们自己复核。
jackwener
left a comment
jackwener
left a comment
There was a problem hiding this comment.
Review seat: kabi-sol. Targeted recheck of my two findings on aa62cb2fae58160500ab2da818ce729c1f69cda0.
- Closed: stale handoff publication. The original production loop/surface/locale/settings probe now emits
[attention, null]when Cancel lands during the next locale read; the old attention is suppressed. - Still P2: offline Composer. An initial snapshot taken when the Host is already unavailable now exposes the editor, and typing works. However, if the first pull sees
connecting, subsequently changing tounavailableleaves the mounted renderer on the skeleton with no editor. Reloading in the same final state exposes the editor. The new classification needs an invalidation on Host readiness changes.
I rebuilt the complete Desktop dependency chain and Desktop build; 18 focused tests passed. The transition was reproduced in real Electron with production preload/renderer and controlled identity/profile IPC responses plus the profile-change event. This is not a complete transport/upgrade end-to-end run. No Windows/Linux, full repository suite or whole-PR re-review. Hosted test remains red. No 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.
| isDeferrableOnboardingSnapshotError( | ||
| error, | ||
| (await window.maka.runtimeHostProfiles.getSnapshot().catch(() => null)) | ||
| ?.entries.find((entry) => entry.isDefault)?.readiness, |
There was a problem hiding this comment.
[P2] Re-pull onboarding when the default Host becomes unavailable
The readiness predicate now works when the initial pull already sees unavailable, but an earlier deferred pull never re-runs on that transition: the invalidation subscription below still listens only to Sessions and Connections, not runtimeHostProfiles.subscribeChanges. Main sends a profile-change event when initial startup fails; its connection-list notification runs only for ready. With no identity/cache, snapshot and error therefore remain null and AppShell continues hiding the Composer.
In real Electron with the production preload/renderer, I injected the exact missing-identity response and an initially connecting profile. Switching it to unavailable and delivering runtime-host-profiles:changed was observed by the public profile subscription, yet after 10 seconds there were still 0 visible Composers. Reloading with the identical final unavailable state produced 1 visible Composer. Please invalidate the snapshot on default Host readiness/profile changes, including failure before a hostId exists, and test the mounted connecting→unavailable transition.
Seat: kabi-sol. The original offline-draft P2 is only partially fixed.
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.
3 participants
Summary
Runtime Host readiness used to be a startup precondition: boot awaited connect → handoff → managed update before creating the main window, and a separate
StartupProgressWindowcarried all progress and attention UI. A stuck handoff (thestaging→retry_requiredloop in #5476) therefore blocked the entire app behind a modal window whose only remaining action was cancel.This PR makes first paint independent of Host readiness, per the direction agreed in #5488:
main.tscreates the main window and registers all IPC first;startLocalRuntimeHostManagerstill launches immediately, but in a fire-and-forget continuation.attentionviews — the only states that need a user decision — surface inside the main window through a newruntimeHostHandoffpreload bridge andRuntimeHostHandoffOverlay. Progress views stay silent; there is no separate window.useOnboardingSnapshothydrates asynchronously behind the existing preload skeleton.cachedentries while authority is unavailable, and sends go through the durable outbox, which delivers once a ready target appears.Removals in the same change:
StartupProgressWindow+startup-presentation.ts,prefetchOnboardingSnapshot+initialOnboardingSnapshotplumbing, stale attention fields leaking intoprogresshandoff views (the view is now a discriminated union — also the source of #5476's misleadingreasondisplay), and the pre-dev.9 file-lock compatibility half-layer.Fixes #5488
Breaking change
Runtime Host builds older than dev.9 are no longer a supported update source. The legacy dir-lock / inherited-fd operator lease interop (
withLegacyFileUpdateLockLease,withRuntimeHostManagedServiceLegacyOperatorLeases, and theprocess-lifetime-lock-v1capability probe in the update path) is removed. Current process-lifetime locking, stale.supervisedmarker recovery, and the.lockdirectory wait are unchanged.Verification
npm run build:workspace-deps;npm --workspace @maka/desktop run build:mainandbuild:renderer— passnode --test --test-concurrency=4on the affected desktop suite — 117/117; onboarding snapshot suite 35/35; cli + runtime-host suites 63/63npm --workspace @maka/desktop run typecheck:stories,build-storybook,smoke:storybook— pass (380 stories / 409 theme renders, including 3 new overlay stories)npm run format,npm run lint— cleanThe surface this replaces — BEFORE rendered from
main'srenderStartupProgressHtmlwith the same attention view, AFTER is the new overlay story:Light:
Dark:
AI use
Select exactly one:
Tool(s) and scope: Devin — root-cause investigation, implementation, tests, stories, verification, and this PR description.
Checklist
Does this PR entail a change in behavior?