You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@maka.apache.org by "liuxiaocs7 (via GitHub)" <gi...@apache.org> on 2026/09/01 21:05:18 UTC
[PR] fix(desktop): open side conversations without a settled turn [maka]
liuxiaocs7 opened a new pull request, #4508:
URL: https://github.com/apache/maka/pull/4508
## Summary
Opening the Desktop side conversation (quote-companion side chat) while the main session's **first turn** is still running failed with the banner "无法创建侧边对话，请稍后重试。" and stayed unusable until that turn settled. The panel forked the main session eagerly at mount and only from the latest **completed** turn (`latestSettledTurnId`); during the first turn there is no completed turn, so `ensureCompanionFork` returned `fork_setup_failed`.
This makes the side chat behave like comparable tools (Codex `/side`, Cursor branch): it snapshots whatever context is settled and is never blocked by the main session's state.
- The composer is usable immediately — no eager fork at mount, no "preparing" block, and the `fork_source_busy` retry loop is removed.
- The fork is created lazily on the **first send**. If a completed turn exists it forks through it (full context, as before); if not, it forks with an **empty context**.
- An empty copy inherits the source model/cwd/permission and records `parentSessionId`, but copies no messages and fabricates no `branchOfTurnId` lineage — so it never copies a mid-flight turn's partial output.
The conversation-copy protocol gains an explicit copy point (`{ kind: 'through_turn'; turnId } | { kind: 'empty' }`) in place of a mandatory `sourceTurnId`, threaded through core, storage (persistence lineage invariant + the recoverable cleanup lease), the Runtime Host protocol/coordinator, the desktop IPC/renderer, and the CLI. `RUNTIME_HOST_COMPATIBILITY_EPOCH` is bumped 90 → 91 for the new wire shape (the discriminator lets the persistence invariant tell a deliberate empty copy apart from a `through_turn` copy that lost its turn id).
Fixes #4507
## Verification
Ran locally (all green):
- `npm run build:workspace-deps` (core, storage, mcp, runtime, runtime-host, computer-use, ui) — 0 errors, incl. all package test compilation.
- `@maka/cli` build, and desktop `tsc` for main / renderer / preload — 0 errors each.
- `@maka/core` session tests; `@maka/storage` session-store + `session-copy-cleanup` + quiescent-snapshot suites (150+); `@maka/runtime-host` `protocol` + `session-revision-protocol` + the two-client / revision coordinator suites — the last exercises the **real coordinator + storage** creating an empty side conversation end-to-end (asserts it commits, keeps the `mode:side_conversation` label, sets `parentSessionId`, and leaves `branchOfTurnId` undefined).
- Desktop main suites for the changed areas — `quote-companion-retry` / `quote-companion-disposal` / `session-copy-attempt` / `runtime-host-session-execution-ipc-main` / `workbar-services-adapter` (59 pass), including new coverage for "first send during the first turn forks with an empty context" and "first send after a completed turn forks through the settled turn".
- Biome check on all changed source files.
## Review focus
Runtime Host protocol/persistence change: the copy-point discriminator alters `session.branch.create` / `session.revision.create` input and the persisted `conversationCopy` identity, hence the epoch bump. `isValidConversationCopyLineage` now accepts an empty branch (`parentSessionId` set, `branchOfTurnId` absent) while still rejecting a `through_turn` branch whose `branchOfTurnId` is missing/mismatched, and revisions remain `through_turn`-only (enforced at decode).
Known follow-up (edge case, not a regression): after a *failed* first send leaves a committed empty fork, a subsequent source-model rebind does not auto-replace that idle fork on the next send (the old eager effect used to). Flagged for a possible later fix in the hook.
## AI use
Select exactly one:
- [ ] No generative tool made a substantive contribution
- [x] Generative tooling made a substantive contribution
Tool(s) and scope: Claude Code (Anthropic) — implemented the change across the protocol/core/storage/desktop/CLI layers and the tests under human direction and review. The commit carries a `Generated-by: Claude Code` trailer.
## Checklist
- [x] Tests cover the change and fail without it
- [x] Lint, format, typecheck and the affected suites pass locally
Does this PR entail a change in behavior?
- [x] Yes — described under Summary above
- [ ] No
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@maka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@maka.apache.org
For additional commands, e-mail: commits-help@maka.apache.org
Re: [PR] fix(desktop): open side conversations without a settled turn [maka]
Posted by "liuxiaocs7 (via GitHub)" <gi...@apache.org>.
liuxiaocs7 commented on PR #4508:
URL: https://github.com/apache/maka/pull/4508#issuecomment-5506234059
Thanks — addressed the second review (rebased onto latest `main`; epoch is now 94):
**Spec (correctness)**
1. **Permission write failure still sent (P1)** — `send` now **fails closed**: if applying the staged permission mode returns false or throws, the send aborts (returns false), keeps the draft and staged mode, and surfaces an error, so a turn never runs under a wider inherited mode. New test: `fails closed when the staged permission write fails on the first send`.
2. **Ambiguous retry changed the copy boundary for the same copyId (P1)** — the copy point is now persisted in the retry lease and **replayed** (`copyPointFromBoundary(copyAttempt.sourceTurnId)`), never recomputed from live turns, so an ambiguous first `empty` send can't retry the same copyId as `through_turn`. New test: `replays the empty copy point across an ambiguous retry even after the source settles`.
3. **Stale empty fork couldn't actually re-fork (P2)** — the submit lock is no longer part of the "don't discard an existing fork" guard (with the eager effect gone, `send` is the sole `ensureFork` caller and owns that lock), so a resend replaces an idle empty fork. The test now performs the second `send()` and asserts the fork is replaced (`branchCount === 2`, old fork cleaned).
4. **Empty fork copied the current Todo (P2)** — the coordinator now sets `copyCurrent: false` for an empty copy point. The two-client UDS test asserts the empty side session has `items: []`.
**Standards**
2. **Docs contradiction** — `docs/side-conversation.md` no longer describes eager fork / loading tab / preparation spinner anywhere it speaks for Maka (lifecycle, panel-visibility, feature list, and visual-pass sections all updated to the lazy model; the Codex-reference contract and the active-turn spinner are kept as-is).
3. **PR description** — corrected to epoch 93 → 94 and removed the contradictory "known follow-up" (the stale-fork case is fixed above).
4. **Primitive Obsession (`SessionCopyAttempt.sourceTurnId: string`)** — deliberately kept string-based to hold the renderer architecture debt ledger flat for that legacy file (it can't gain a `@maka/core/session` dependency); the copy point is encoded/decoded at the non-legacy boundary and replayed, which resolves the retry defect (Spec 2) without the added dependency.
**Standards 1 (UI evidence)** — still pending: the behavior (immediately-usable composer, no preparation spinner) is covered by the hook/e2e tests above. I can attach before/after captures if preferred.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@maka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@maka.apache.org
For additional commands, e-mail: commits-help@maka.apache.org
Re: [PR] fix(desktop): open side conversations without a settled turn [maka]
Posted by "Astro-Han (via GitHub)" <gi...@apache.org>.
Astro-Han commented on code in PR #4508:
URL: https://github.com/apache/maka/pull/4508#discussion_r3915438558
##########
docs/side-conversation.md:
##########
@@ -108,14 +111,18 @@ a fixed menu wired to the side-chat feature.
## Desktop Side-Chat Lifecycle
-Opening a new side chat first installs a non-closable
-`sidechat-loading:<parent>:<index>` tab. It then forks the latest parent state
-and replaces the loading tab with `sidechat:<conversation-id>`.
+Codex installs a non-closable `sidechat-loading:<parent>:<index>` tab, forks the
+latest parent state, then swaps in `sidechat:<conversation-id>`.
-Maka now matches that user-visible loading contract while keeping its stable
-panel id: the tab shows a spinner and cannot be closed, dragged, reordered, or
-moved until the eager fork is ready. It then becomes the ordinary closeable
-Side Chat tab without remounting the panel draft.
+Maka diverges from that eager loading contract: opening a side chat installs a
+normal `sidechat:<conversation-id>` tab whose Composer is usable immediately —
Review Comment:
Not true on this head: `use-workbar-controller.ts:298` still creates the tab with `preparingOnCreate = true`, so one busy frame paints before the panel's passive effect clears it. `ensureFork`'s only caller passes `showPreparing: false`, so the whole `preparing` pipeline and the busy-tab mechanism in `workbar-surface.tsx` have no live producer; they go with this change (−101 lines on a scratch branch, suites green).
##########
packages/runtime-host/src/protocol/session-revision.ts:
##########
@@ -153,10 +181,16 @@ export function decodeSessionConversationCopyInput(value: unknown): SessionConve
if (input.intent !== undefined && input.intent !== 'side_conversation') {
throw invalidProtocolFrame('Invalid Session conversation-copy intent');
}
+ const copyPoint = decodeSessionConversationCopyPoint(input.copyPoint);
+ if (copyPoint.kind === 'empty' && input.intent !== 'side_conversation') {
Review Comment:
This line already decides "empty" from `intent`, and `session-store.ts:1515` does the same for lineage. The `kind` discriminator is a third representation of that fact, and it is what forces the two `SessionConversationCopyPoint` types, the helpers, the header and lease migrations and the `'\0empty'` sentinel. Optional `sourceTurnId` with absence meaning empty removes all of it: old headers and leases stay valid, fingerprints stay byte-identical, −350 lines with every suite green.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@maka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@maka.apache.org
For additional commands, e-mail: commits-help@maka.apache.org
Re: [PR] fix(desktop): open side conversations without a settled turn [maka]
Posted by "Astro-Han (via GitHub)" <gi...@apache.org>.
Astro-Han commented on code in PR #4508:
URL: https://github.com/apache/maka/pull/4508#discussion_r3920742078
##########
apps/desktop/src/main/permission-response-guard.ts:
##########
@@ -53,7 +53,13 @@ interface WorkspaceFileReferencePosition {
start: number;
}
-export type RuntimeHostBranchFromTurnInput = BranchFromTurnInput & { copyId: string };
+/** Branch input after normalization. Absent `sourceTurnId` forks empty. */
+export type NormalizedBranchFromTurnInput = {
Review Comment:
This is `BranchFromTurnInput` field for field, and it only became a duplicate because core made `sourceTurnId` optional in this PR. `BranchFromTurnInput` is still imported at `:21` and no longer used anywhere in the file. Use it as the return type and drop the local name.
##########
packages/runtime-host/src/protocol/session-revision.ts:
##########
@@ -116,6 +121,9 @@ function decodeSessionRevisionCopyInput(value: unknown): SessionConversationCopy
if (input.intent !== undefined) {
throw invalidProtocolFrame('Session revision copy does not support an intent');
}
+ if (input.sourceTurnId === undefined) {
Review Comment:
Unreachable: the shared `decodeSessionConversationCopyInput` runs first and throws either "An empty conversation copy requires the side_conversation intent" (no intent) or "does not support an intent" (with one), so this never sees a missing boundary. The `session-revision-protocol.test.ts` case that reads as pinning "A revision must carry a turn boundary" is passing on one of those other messages.
##########
apps/desktop/src/renderer/features/workbar/tools/side-chat/quote-companion-core.ts:
##########
@@ -108,11 +111,25 @@ export interface EnsureCompanionForkDeps {
/** The latest successfully completed turn of the source session.
* Failed and aborted turns are not reference context: branching through one
* makes a fresh side prompt look like a continuation of unfinished parent
- * work. If no completed turn exists, the side conversation starts empty. */
+ * work. Callers decide whether a missing completed turn is temporary (the
+ * first turn is still running) or a hard setup failure. */
export function latestSettledTurnId(turns: readonly TurnRecord[]): string | undefined {
return [...turns].reverse().find((turn) => turn.status === 'completed')?.turnId;
}
+/**
+ * Sentinel boundary stored in the retry lease for an empty copy. Turn ids match
+ * /^[A-Za-z0-9_-]{1,128}$/, so the NUL-prefixed token can never be one. This
+ * lets the retry lease keep a plain string field while still representing the
+ * absent (empty) source turn.
+ */
+const EMPTY_SOURCE_TURN_SENTINEL = '\0empty';
Review Comment:
Last second representation left in the change, for the same fact the rest of the PR now expresses by absence. It exists only because `SessionCopyAttempt.sourceTurnId` is still a required string; making it optional too takes the sentinel and `sourceTurnIdFromBoundary` with it. The lease is renderer `sessionStorage` and `isPersistedAttempt` only checks length, so nothing else depends on the string being non-empty.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@maka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@maka.apache.org
For additional commands, e-mail: commits-help@maka.apache.org
Re: [PR] fix(desktop): open side conversations without a settled turn [maka]
Posted by "liuxiaocs7 (via GitHub)" <gi...@apache.org>.
liuxiaocs7 commented on PR #4508:
URL: https://github.com/apache/maka/pull/4508#issuecomment-5520148306
Thanks for the thorough pass, @Astro-Han. Pushed `5074480` on top of the approved head:
1. **`permission-response-guard.ts`** — dropped the local `NormalizedBranchFromTurnInput` and now use the imported `BranchFromTurnInput` directly (as the `normalizeBranchFromTurnInput` return type and the base of `RuntimeHostBranchFromTurnInput`). It became field-for-field identical once this PR made `sourceTurnId` optional, and the import is live again.
2. **`session-revision.ts`** — removed the unreachable `sourceTurnId === undefined` throw in `decodeSessionRevisionCopyInput`. As you noted, a revision never carries an intent, so the shared `decodeSessionConversationCopyInput` already rejects a missing boundary ("an empty conversation copy requires the side_conversation intent"). The protocol test matches `isInvalidFrame`, so it stays green on the shared message.
3. **`'\0empty'` sentinel** — I did *not* take this one, and I'd like your read on the reasoning. `SessionCopyAttempt` is shared with the revision copy path, and its consumers (`app-shell-revision-actions.ts` → `commitRevisionDraft` / `reviseBeforeTurn`, where `ReviseBeforeTurnInput.sourceTurnId` is a required `string`) read `copyAttempt.sourceTurnId` back. Making it optional widens the shared type to `string | undefined`, which either forces non-null assertions at those revision sites or pulls revision code into this PR. The sentinel keeps the empty side-conversation boundary contained to its own lease without weakening the revision type; I've documented that rationale at the sentinel definition. Happy to do a follow-up that makes the attempt type optional across *both* paths if you'd rather have it gone here.
**Downgrade note** — added to the PR body: a header written by this build for an empty copy (omitting `sourceTurnId`) and read by an *older* release throws `malformed fields`, since `SESSION_CONVERSATION_COPY_SHAPE` relaxed a persisted closed shape from required to optional. The epoch doesn't cover the downgrade direction and there's no downgrade contract, so it's a note rather than a guarded path.
The branch was also rebased past the epoch churn on `main` (now epoch 100 against `main` 99); CI is green on the rebased head.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@maka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@maka.apache.org
For additional commands, e-mail: commits-help@maka.apache.org
Re: [PR] fix(desktop): open side conversations without a settled turn [maka]
Posted by "Astro-Han (via GitHub)" <gi...@apache.org>.
Astro-Han merged PR #4508:
URL: https://github.com/apache/maka/pull/4508
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@maka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@maka.apache.org
For additional commands, e-mail: commits-help@maka.apache.org
Re: [PR] fix(desktop): open side conversations without a settled turn [maka]
Posted by "liuxiaocs7 (via GitHub)" <gi...@apache.org>.
liuxiaocs7 commented on PR #4508:
URL: https://github.com/apache/maka/pull/4508#issuecomment-5507797420
Thanks — addressed the Standards findings (rebased onto latest `main`; epoch is now 95):
- **P2 real NUL byte (`quote-companion-core.ts`)** — the empty-copy sentinel is now the source escape `'\0empty'` (identical NUL runtime value, but the source is plain UTF-8 text, so the file is no longer a binary diff). Comment corrected to "NUL-prefixed". Verified with `file` (now `Unicode text`) and a NUL scan of the diff.
- **P3 unused `submitLocked` state** — removed; reentrancy is guarded by `submitLockRef` alone, so no state is set and the needless re-render on every lock toggle is gone.
- **P3 UI evidence** — still outstanding. The change removes a transient preparation state (the composer is usable immediately), so a meaningful before/after needs the running app rather than a static story, and it is already covered by the hook + `session-workbar` e2e tests. Happy to attach a capture if a maintainer would still like one.
Local: affected suites green (desktop main + runtime-host/storage), typecheck and Biome clean.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@maka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@maka.apache.org
For additional commands, e-mail: commits-help@maka.apache.org
Re: [PR] fix(desktop): open side conversations without a settled turn [maka]
Posted by "liuxiaocs7 (via GitHub)" <gi...@apache.org>.
liuxiaocs7 commented on PR #4508:
URL: https://github.com/apache/maka/pull/4508#issuecomment-5504138146
Rebased onto latest `main` (epoch now 93) and integrated with the newly-landed side-chat `confirmBypass` / `useSessionSettingIntent` permission flow. Addressed the review:
**Standards**
1. **Legacy data unreadable (blocker)** — `normalizeSessionHeader` now migrates a released `conversationCopy.sourceTurnId` to `copyPoint: { kind: 'through_turn', turnId }` at the decode boundary, so existing branch/revision/side-conversation headers still load. Covered by new tests in `session-store.test.ts`.
2. **Cleanup lease schema (high)** — `decodeLeaseRow` migrates a legacy `creation.sourceTurnId` lease to `copyPoint` on read, so an in-flight copy persisted before the upgrade still recovers instead of dereferencing `undefined.kind`. Covered by a new recovery test in `session-copy-cleanup.test.ts`.
3. **Failing test (blocker)** — `permission-response-ipc-boundary.test.ts` migrated to `copyPoint` (it exercised the normalizer at runtime); the `test` suite passes locally now.
4. **NUL byte (medium)** — the fingerprint sentinel is now the text escape `'\0empty'` (identical runtime value); `file` reports the source as text and the comment is corrected.
**Spec**
1. **Composer wedged after model rebind (high)** — an empty (no-content) fork no longer gates the composer (`companionModelReady` treats it as ready), so a stale-model empty fork is replaced by the next send instead of deadlocking. New hook test covers the failed-send + rebind path.
2. **Empty for plain branch (medium)** — an empty copy point is now rejected unless `intent === 'side_conversation'`, at both the protocol decoder and the persistence invariant. New protocol test.
3. **Permission picker before first send (medium)** — a mode chosen before the fork exists is now confirmed (bypass) and staged, then applied to the fork on the first send; the read-only chip reflects the staged choice. New hook test.
4. **Docs (medium)** — `docs/side-conversation.md` now describes the lazy, immediately-usable Composer instead of the eager-fork/preparation state.
**Confirmed OK:** empty copies still skip runtime runs/events and Session Todo.
Not yet included: before/after UI screenshots (Standards 5). The panel's ready/Composer behavior is exercised by the hook tests above; I can add captured evidence if preferred.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@maka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@maka.apache.org
For additional commands, e-mail: commits-help@maka.apache.org