Skip to content

fix(dashboard): chat profile scoping by rotating attach token on profile change - #62581

Open
iosub wants to merge 3 commits into
NousResearch:mainfrom
iosub:fix/dashboard-chat-profile-attach-token-rotation
Open

fix(dashboard): chat profile scoping by rotating attach token on profile change#62581
iosub wants to merge 3 commits into
NousResearch:mainfrom
iosub:fix/dashboard-chat-profile-attach-token-rotation

Conversation

@iosub

@iosub iosub commented Jul 11, 2026

Copy link
Copy Markdown

Summary

This PR fixes a dashboard chat regression where switching the selected profile could still reattach to a keep-alive PTY created under the previous profile.

Problem

The chat keep-alive attach token was rotated only for forced-fresh sessions. On profile switches without fresh=1, the client could reattach to an old PTY and continue under stale profile scope.

Root Cause

Profile transitions were not treated as attachment identity boundaries.

Changes

  • Track profile transitions in ChatPage.
  • Rotate the PTY attach token when either:
    • forced fresh session is requested, or
    • selected profile changes.
  • Reset the profile-change flag after token generation.
  • Keep existing reattach behavior for reconnects when profile is unchanged.

Impact

  • Prevents cross-profile PTY reattachment.
  • Ensures profile-scoped sessions use the selected profile consistently.
  • No server API or persistence contract changes.

Testing

  • Patch applies cleanly on current main.
  • Manual validation:
    1. Start chat under profile A.
    2. Switch to profile B.
    3. Confirm no reattach to profile A PTY.
    4. Refresh while staying on profile B and confirm normal reattach.

Risk

Low. Change is limited to token rotation decision logic in chat page attach parameter generation.

Copilot AI review requested due to automatic review settings July 11, 2026 10:24

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR fixes a dashboard chat regression where switching the selected management profile could still reattach to an existing keep-alive PTY that was spawned under the previous profile, by treating profile transitions as an attachment identity boundary and rotating the attach token accordingly.

Changes:

  • Track profile transitions in ChatPage to detect when the selected profile changes.
  • Rotate the PTY attach token when either a forced-fresh session is requested or the selected profile changes.
  • Reset the profile-change flag after generating the attach token to preserve normal reconnect/reattach behavior when the profile is unchanged.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment thread web/src/pages/ChatPage.tsx Outdated
@alt-glitch alt-glitch added type/bug Something isn't working P2 Medium — degraded but workaround exists comp/dashboard Web dashboard / control panel UI (dashboard/, landing) sweeper:risk-session-state Sweeper risk: may lose/corrupt/mis-associate session or context state sweeper:risk-message-delivery Sweeper risk: may drop, duplicate, misroute, or suppress messages labels Jul 11, 2026
@iosub iosub changed the title Fix dashboard chat profile scoping by rotating attach token on profile change fix(dashboard) chat profile scoping by rotating attach token on profile change Jul 11, 2026
@iosub iosub changed the title fix(dashboard) chat profile scoping by rotating attach token on profile change fix(dashboard): chat profile scoping by rotating attach token on profile change Jul 11, 2026
@teknium1

Copy link
Copy Markdown
Contributor

Thanks for isolating the attachment identity boundary. The normal profile-switch premise is confirmed on current main: web/src/pages/ChatPage.tsx:805 reuses the attach token independently of scopedProfile, while hermes_cli/web_server.py:15452 reattaches solely by that token.

Problems

  • A rapid profile transition can still open a stale socket. web/src/pages/ChatPage.tsx:810 awaits api.buildWsUrl, cleanup sets unmounting = true at line 1022, and line 811 creates the socket without a post-await cancellation check. If the scope changes during that await, the cleaned-up effect can still assign wsRef.current for its old profile.
  • The PR has no regression test (gh pr diff 62581 changes only web/src/pages/ChatPage.tsx). This path needs coverage for A → B rotation, unchanged-profile reconnect reuse, and a deferred URL-build/profile-change race.

Suggested changes

  • Recheck unmounting immediately after await api.buildWsUrl(...) and before constructing the WebSocket.
  • Add the focused regression coverage above.

Automated hermes-sweeper review.

@teknium1 teknium1 added the sweeper:blast-moderate Sweeper blast radius: moderate — a subsystem or single platform label Jul 11, 2026
Recheck unmounting after await buildWsUrl before opening the WebSocket,
and add focused regression coverage for attach-token rotation and the
deferred URL-build race called out in review.
@iosub

iosub commented Jul 11, 2026

Copy link
Copy Markdown
Author

Thanks for the review — both points addressed in f08c129:

  1. Stale socket race — after await api.buildWsUrl(...), we now recheck the effect teardown flag via shouldOpenPtySocketAfterUrlBuild(unmounting) and return before constructing the WebSocket / assigning wsRef.
  2. Regression coverage — added focused helpers + vitest coverage in web/src/lib/pty-attach.ts / web/src/lib/pty-attach.test.ts for:
    • A → B attach-token rotation
    • same-profile reconnect token reuse
    • forced-fresh rotation
    • deferred URL-build / profile-change race (abort when unmounting)

npm test -- src/lib/pty-attach.test.ts src/lib/pty-reconnect.test.ts → 15 passed.

@iosub

iosub commented Jul 12, 2026

Copy link
Copy Markdown
Author

Do I need to do something else?

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

Labels

area/profiles Multi-profile isolation, HERMES_HOME scoping comp/dashboard Web dashboard / control panel UI (dashboard/, landing) P2 Medium — degraded but workaround exists sweeper:blast-moderate Sweeper blast radius: moderate — a subsystem or single platform sweeper:risk-message-delivery Sweeper risk: may drop, duplicate, misroute, or suppress messages 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.

4 participants