Skip to content

fix(desktop): gate branch publish on workspace identity like cwd - #93216

Closed
ClintonEmok wants to merge 3 commits into
NousResearch:mainfrom
ClintonEmok:fix/kanban-worktree-composer
Closed

ClintonEmok wants to merge 3 commits into
NousResearch:mainfrom
ClintonEmok:fix/kanban-worktree-composer

Conversation

@ClintonEmok

Copy link
Copy Markdown
Contributor

What does this PR do?

A background Kanban worker running in a PR worktree could rewrite the FOREGROUND composer's workspace state while the default chat never moved (#92888): the composer's coding rail / branch flipped to the worker's checkout mid-conversation.

In handleSessionInfoEvent, the foreground cwd write was already guarded by sessionInfoDescribesSelectedSession() — an event may only claim cwd when its durable stored_session_id matches (or lineage-matches) the selected conversation. But setCurrentBranch() right beside it had NO such guard. session.info heartbeats from a worker profile therefore carried the worker's branch straight into the selected chat's composer.

The fix

  • Extracted the identity predicate into session-info-gate.ts (workspaceIdentityMatchesSelectedSession) — one resolver owns each policy, so the cwd and branch publishes now share ONE check.
  • The branch write is gated like the cwd write: a foreign stored_session_id can no longer touch either field.
  • Behavior otherwise unchanged: absent ids (lazy sessions) still apply ([Bug][Desktop]: Switching conversations keeps stale workspace folder and Git indicators #71254), direct matches still apply, compression-lineage rotation still matches.

Per-session cache updates via updateSessionState are untouched — background/tile state continues to update independently, exactly as the issue's invariant requires.

Related Issue

Fixes #92888
(#92811 is the roster-spinner fix and is unrelated; #90006 adjacency noted by the reporter.)

Type of Change

  • 🐛 Bug fix (non-breaking change that fixes an issue)

Changes Made

  • apps/desktop/src/app/session/hooks/use-message-stream/gateway-event/session-info-gate.ts (new) — workspaceIdentityMatchesSelectedSession(), the shared identity predicate
  • apps/desktop/src/app/session/hooks/use-message-stream/gateway-event/session-info.ts — branch publish gated; duplicate predicate removed in favor of the shared gate
  • apps/desktop/src/app/session/hooks/use-message-stream/gateway-event/session-info-gate.test.ts (new) — 5 cases: direct match, absent id, foreign worker id, fresh-draft rejection, lineage rotation

How to Test

  1. cd apps/desktop && npx vitest run session-info-gate session-info — 16 passed
  2. Repro on main: default chat foreground + a Kanban worker in a PR worktree → worker runtime updates flip the composer's branch to the worktree's. With this branch they don't; opening the worker's own chat still re-homes correctly.

Checklist

Code

  • I've read the Contributing Guide
  • My commit messages follow Conventional Commits (fix(scope):)
  • I searched existing PRs to make sure this isn't a duplicate
  • My PR contains only changes related to this fix
  • Targeted tests pass (16/16); tsc clean; eslint clean on touched files
  • I've added tests for my changes
  • Tested on macOS 26.5 (Apple Silicon)

Documentation and Housekeeping

  • Documentation updates — or N/A
  • cli-config.yaml.example — or N/A
  • CONTRIBUTING.md / AGENTS.md — or N/A
  • Cross-platform impact considered — renderer-only TypeScript
  • Tool descriptions/schemas — or N/A

A background Kanban worker's session.info carries the worker's
PR-worktree cwd AND branch. handleSessionInfoEvent guarded the
foreground cwd write with sessionInfoDescribesSelectedSession(), but
the branch write beside it was unguarded — a worker's runtime update
rewrote the default chat's composer branch/coding rail while the
conversation never moved (NousResearch#92888).

Extract the identity predicate into session-info-gate.ts (one resolver,
shared answer) and gate BOTH writes on it: a worker event with a
different stored_session_id can no longer touch the foreground
composer's cwd or branch, while absent ids (lazy sessions), direct
matches, and compression-lineage matches keep their existing behavior.

Covered by unit tests: direct match, absent id, foreign worker id,
fresh-draft rejection, lineage rotation match.
@alt-glitch alt-glitch added type/bug Something isn't working comp/desktop Electron desktop app (apps/desktop/*) P2 Medium — degraded but workaround exists sweeper:risk-session-state Sweeper risk: may lose/corrupt/mis-associate session or context state labels Aug 23, 2026
Replace the inline import() type annotation on importOriginal with a
top-level 'import type * as sessionStore' reference.
@Enough1122

Copy link
Copy Markdown
Contributor

AI code review — automated review for reference, author can ignore or act on any point.

Solid fix with the right shape: the unguarded branch publish was a genuine cross-session leak (a Kanban tile's PR-worktree branch rewrote the default chat's coding rail), and gating it on the same predicate as cwd closes the whole bug class rather than special-casing one field. Extracting workspaceIdentityMatchesSelectedSession into a DI-testable module is exactly the right move, and the tests assert behavior contracts (lazy-session absence, draft rejection, lineage rotation) instead of freezing data. A few observations:

  1. apps/desktop/src/app/session/hooks/use-message-stream/gateway-event/session-info-gate.ts:24-28: absent stored_session_id still publishes to the foreground by design ([Bug][Desktop]: Switching conversations keeps stale workspace folder and Git indicators #71254), which means any background session whose backend omits the id can still rehome cwd/branch. That's an accepted trade-off today, but consider emitting a debug log when a workspace write lands on an absent-id event — if this class of leak resurfaces via a lazy path, that line becomes the fastest way to see it.

  2. session-info-gate.ts:47-51: the lineage fallback requires one row in $sessions answering to both ids. At cold start (or right after a hard re-home) the list can briefly be empty, so a legitimate compression-rotated tip's branch update would be silently dropped during that window. The old branch path was unconditional, so this is newly possible; worth either a short-lived retry/queue or a comment acknowledging the startup-window drop.

  3. Duplication: the full rationale paragraph now lives twice — in the session-info-gate.ts docstring and again as an inline block at gateway-event/session-info.ts:100-105. One canonical home plus a pointer keeps them from drifting apart on the next edit.

Minor: the test file's hand-rolled { get: () => ... } atom doubles are fine here since the predicate only calls .get(), but typing them as unknown as ReadableAtom will get noisier if the gate ever subscribes.

… log absent-id publishes, note cold-start window

- The workspace-identity rationale now lives once in
  session-info-gate.ts; the call site points at it instead of restating.
- Absent-id publishes stay allowed (NousResearch#71254) but emit a debug line, so a
  recurrence of this leak via a lazy path is diagnosable from one log.
- Document the cold-start window where the lineage fallback drops a
  rotated-tip branch update until the first sessions refresh: accepted,
  self-healing on the next session.info heartbeat; a queue would need
  invalidation on every list mutation to stay honest.

Refs NousResearch#93216
@ClintonEmok

Copy link
Copy Markdown
Contributor Author

All three addressed in 2046cf8:

  1. Absent-id debug log — added a console.debug on the allowed absent-stored_session_id path, so if this leak class resurfaces via a lazy backend the log names the event immediately.

  2. Cold-start window — documented rather than queued: the lineage fallback drops a rotated-tip branch update only while $sessions is briefly empty after boot/re-home, and the next periodic session.info heartbeat re-publishes, so it self-heals. A retry queue would need invalidation on every sessions-list mutation to stay correct — more machinery than the one-heartbeat gap justifies. Comment now says all of that explicitly.

  3. Duplicated rationale — the call-site block now points at workspaceIdentityMatchesSelectedSession as the canonical home instead of restating it.

@ClintonEmok

Copy link
Copy Markdown
Contributor Author

Closing per request.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

comp/desktop Electron desktop app (apps/desktop/*) P2 Medium — degraded but workaround exists sweeper:risk-session-state Sweeper risk: may lose/corrupt/mis-associate session or context state type/bug Something isn't working

Projects

None yet

Development

Successfully merging this pull request may close these issues.

fix(desktop): prevent Kanban worker worktrees from contaminating the foreground composer

3 participants