fix(desktop): preserve profile across worktree handoffs - #95045
Tomenatore wants to merge 1 commit into
Conversation
ddc734e to
63ede45
Compare
andrexibiza
left a comment
There was a problem hiding this comment.
Reviewing exact head 63ede45eddab2db15b31d76247af7bf5fea82f8e against its base main@a9351543789baabdd2897ee2ed73087d488f9b6c. Live main is now f751a8c5467c41500e505d90cb0eb8b70929080f, two updater-only commits ahead of this base; those intervening commits do not change the Desktop routing/worktree surfaces reviewed here.
The core direction is right: ownership is captured before the async worktree boundary, StartWorkSessionRequest carries it through the controller, tiles preserve exact connection routes, and the project/worktree entry points stop relying on whatever gateway happens to be active later. I found two merge-blocking ownership holes that still make the new carrier lossy/fail-open.
1. Ambiguous ownership is encoded as the same undefined used for genuinely ownerless fresh intent.
knownOwnerForSession() deliberately returns undefined when multiple connection-qualified claims exist. That is a valid resolution result ("this real source session is ambiguous"), but useComposerBranch immediately turns it into an ownerless worktree request:
requestStartWorkSession(path, text, owner ? { owner } : undefined)The new regression even locks this behavior in as "keeps an unresolved owner unresolved" by expecting requestStartWorkSession(..., undefined).
That is not fail-closed at the actual mutation boundary. startFreshSessionDraft() only applies setNewChatOwner() when an owner is present, and desktopSessionCreateParams() subsequently resolves from the mutable new-chat/active-gateway state. So a same-stored-id collision across two connections can be correctly recognized as ambiguous and then silently reclassified as a normal ownerless fresh draft, allowing session.create to run under unrelated ambient/stale routing state.
This also contradicts the contract already documented in session-request-router.ts: ambient is for a genuinely unknown fresh draft/global surface; callers operating on a real session are expected to resolve its owner before reaching that case.
Required fix: preserve the distinction between no source owner exists and a source owner exists but cannot be uniquely resolved. A session-originated worktree handoff with ambiguous ownership must either resolve to one exact owner through authoritative evidence or refuse the handoff; it must not emit an ownerless StartWorkSessionRequest. Please cover the same-id/two-connection case through the controller bridge and assert that no ambient session.create occurs.
2. activeNewChatOwner() drops targetProfile, so it is not an exact route for Desktop aliases.
AgentProfileRoute correctly includes targetProfile, and the existing route machinery uses it when constructing the backend request. But the new capture helper reconstructs the active owner as only:
return connectionId ? { connectionId, profile } : profileThat loses a required identity dimension for configured alias routes. The existing Desktop route contract explicitly supports shapes like:
{ connectionId: C, profile: 'moxie', targetProfile: 'default' }for per-profile Cloud/URL overrides; plugin-profile-routes.ts derives targetProfile = 'default' for exactly that case, and merged #92860 depends on the same connection-exact alias→backend-target distinction.
This PR now uses activeNewChatOwner() in project/open-folder/worktree entry points. After capture, desktopSessionCreateParams() sends capturedRoute?.targetProfile || profile. With the target discarded, the request is pinned to the correct connection but sends the Desktop alias (moxie) as the backend profile instead of the backend namespace (default). In other words, the patch fixes source attribution while still permitting profile-namespace attribution to drift.
Required fix: capture the full active route descriptor, including backend targetProfile, rather than reconstructing ownership from activeGatewayConnectionId() + $activeGatewayProfile. Add a regression for an active alias route where display profile != backend target and prove the worktree/project handoff calls session.create on the exact connection with the target backend profile.
Architecture/interlocks checked:
- #89736 / #89731 remains complementary: it projects profile metadata only after a row was created under the correct owner; it cannot repair either creation-time routing hole above.
- #81817 remains distinct: it handles the mutable profile-switch/Cmd+N race, while this PR is trying to make an initiating surface's owner sticky across a worktree transition.
- #86822 is adjacent and overlaps the fresh-session/worktree creation seam. Whichever lands second must preserve both invariants: automatic worktree isolation and the full exact owner carrier; do not re-derive owner from foreground state during the merge adaptation.
- #91684 is the same broader owner-routed-session defect family: a live session operation must reach the runtime/source that owns it, never a same-named/ambient gateway.
- #92860 / #89131 establish why
targetProfileis part of route identity rather than optional decoration; this PR should compose with that merged contract, not narrow it. - #92888 is the other side of the worktree ownership shape: background worker runtime state must not contaminate the foreground composer, while this PR carries foreground source authority into a new worktree session. They are complementary, not duplicates.
Exact-head hosted evidence is not green: CI, Docker Build/Test/Publish, and Nix flake check are all action_required; the CI run exposes zero jobs. The local focused/typecheck/build receipts in the PR body are useful, but they do not exercise the two adversarial ownership cases above.
Once ambiguity cannot collapse into ambient authority and targetProfile survives the capture, this becomes the right shape: one immutable initiating-owner carrier from interaction boundary through worktree creation to session.create, without reconstructing authority from mutable presentation state.
63ede45 to
cddbd0e
Compare
What does this PR do?
Keeps a new Hermes Desktop session on the profile/backend that owned the session surface which initiated a Git worktree hand-off.
The failing sequence was:
requestStartWorkSession()carries onlypath,draft, andopenTab.session.createfalls back to mutable new-chat/active-gateway state and can run ondefault.In the reproduced case, the named-profile WebSocket opened but sent no request;
session.createran on the default backend. The resulting row was present only in defaultstate.dbwithprofile_name = NULL, so its worktree cwd produced a default-profile auto-project and no named-profile glyph.This change captures an immutable owner at the interaction boundary (a profile name or exact connection route), carries it through the worktree request and fresh-draft transition, and applies it before
session.create. Main composers, session tiles, the worktree dialog, command-palette worktree entries, and project/open-folder entry points all preserve the appropriate owner.Existing misrouted sessions are intentionally not migrated or restarted. This fixes future hand-offs without interrupting active work.
Related work
projects.treefor rows already stored under the correct named profile. It cannot repair this case because the row was created in the default database with no profile identity; this PR does not duplicate its projection change.Type of Change
Changes Made
NewChatOwnerboundary for profile names and exact connection routes.StartWorkSessionRequestandstartWorkspaceSessioninto the fresh draft.How to Test
Regression before the production change:
A mutation run that temporarily removed only the new owner application reproduced the same failure; restoring the line made the test pass again.
An independent staged review additionally caught two follow-ups before publication: an
undefinedowner snapshot could be re-resolved after an await, and the first integration test bypassed the controller bridge. Dedicated regressions now keep an unresolved snapshot unresolved and exercise the samehandleStartWorkSessionRequestbridge used by the wiring effect.Automated verification on current upstream
main(f751a8c546):Full Desktop UI run on this Windows host before the subsequent conflict-free upstream rebases (starting from
5b82658b3c;--maxWorkers=2to limit the host's worker-saturation timeouts):The failures are not introduced by this diff:
1,234,567,5,000, and currency spacing) reproduce identically on a clean current-upstream worktree under this host'sde-DElocale (4 failed / 110 passed).Checklist
Code
Documentation & Housekeeping
cli-config.yaml.example: N/A; no config keys changedCONTRIBUTING.md/AGENTS.md: N/A; no workflow changed