feat(session): migrate incompatible daemons safely - #933
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub. |
Greptile SummaryThe PR authenticates local session producers and CLI controls, minimizes public daemon health, and replaces unsafe PID-based daemon migration with quiescent authenticated reconnection.
Confidence Score: 4/5The PR appears safe to merge, with only a non-blocking environment-access consistency issue in the launch-fingerprint helper. The authenticated migration and request-routing paths are coherently tested; the remaining accepted concern is that one new helper bypasses the repository's validated environment configuration boundary. Files Needing Attention: src/session/broker/brokerLauncher.ts Important Files Changed
Sequence DiagramsequenceDiagram
participant Window as Hunk window
participant Incumbent as Incompatible daemon
participant Successor as Successor daemon
participant CLI as Session CLI
Window->>Incumbent: Signed producer hello
Incumbent-->>Window: Pre-auth compatibility refusal
Window->>Window: Wait for daemon generation change
Incumbent-->>Incumbent: Exit when quiescent
Window->>Successor: Signed producer hello
Successor-->>Window: Authenticate and register session
CLI->>Successor: Signed capabilities request
Successor-->>CLI: Signed capabilities
CLI->>Successor: Authorized session action
Successor-->>CLI: Signed result
Prompt To Fix All With AI### Issue 1
src/session/broker/brokerLauncher.ts:418
**Direct environment access**
The new `readSessionBrokerLaunchFingerprint` parameter defaults directly to `process.env`, bypassing the repository's validated environment configuration boundary and making configuration behavior harder to validate and test consistently.
---
For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.Reviews (1): Last reviewed commit: "feat(session): migrate incompatible daem..." | Re-trigger Greptile |
| @@ -376,6 +416,27 @@ export function parseSessionBrokerHealth(value: unknown): SessionBrokerHealth | | |||
| } | |||
| } | |||
|
|
|||
There was a problem hiding this comment.
The new readSessionBrokerLaunchFingerprint parameter defaults directly to process.env, bypassing the repository's validated environment configuration boundary and making configuration behavior harder to validate and test consistently.
Context Used: guidelines.mdc Cursor rule (source)
Prompt To Fix With AI
This is a comment left during a code review.
Path: src/session/broker/brokerLauncher.ts
Line: 418
Comment:
**Direct environment access**
The new `readSessionBrokerLaunchFingerprint` parameter defaults directly to `process.env`, bypassing the repository's validated environment configuration boundary and making configuration behavior harder to validate and test consistently.
**Context Used:** guidelines.mdc Cursor rule ([source](https://github.com/modem-dev/modem/blob/main/.cursor/rules/guidelines.mdc))
---
For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
There was a problem hiding this comment.
Not changing this one. Hunk has no repository-wide validated-environment API, and this helper follows the adjacent injectable-environment convention: tests may pass an environment, while production defaults to process.env. The fingerprint is only a reconnect hint; the signed hello remains the compatibility and identity authority.
Responded by Pi using gpt-5.6-sol.
This comment was generated by Pi using gpt-5.6-sol
There was a problem hiding this comment.
That context makes sense. Given there is no repository-wide validated-environment API and the helper follows the established injectable-environment convention, defaulting production calls to process.env is reasonable here. Since the fingerprint is only a reconnect hint and the signed hello remains the compatibility and identity authority, I agree this does not require a change.
1b84e69 to
5e359fd
Compare
5e359fd to
c292d8a
Compare
Stack
#932 → 2 of 3 → #934. Merge bottom-to-top.
Problem
Hunk windows and one-shot session commands need authenticated access to the shared local daemon. Upgraded interactive windows must recover from an incompatible incumbent without trusting health metadata or terminating a process that may still own live sessions.
Approach
{ "ok": true }and remove PID-based replacement;SessionBrokerConnectionacross daemon generations so queued/executing command budgets and registration state remain stable;Forced handoff, coordinator endpoints, random ports, remote hosts, and updater-controlled daemon lifecycle remain non-goals.
Verification
bun run typecheckbun run deps:checkbun run changeset:statusgit diff --checkNo visual evidence is included because this changes session lifecycle rather than terminal rendering.
Known limitations
Hung or idle-disabled incompatible incumbents still require manual termination. Native Windows credential-path validation and durable key rotation remain publication gates.
This PR description was generated by Pi using gpt-5.6-sol