fix(daemon): address PR 156 review findings

Fix a Hello.Proxied compile error in session_core_test.go by removing the
unsupported field (no proxied/remote attachment mechanism exists on this
branch yet) and repairing the test's teardown so it exercises the real
graceful-skip behavior of an invalid active-tab index during first paint.

Guard nil session cores before dereferencing registry IDs in the
attachment-transition validators, resolve req.target.core() into a single
local for the target-init block, and wrap the session-registry rejection
path in createSessionLocked with the same domain.UserErr/NoticeSessionSpawn
used by its sibling failure paths.

commit: 38bcf153ab4885d0a3e205ffc75800506fb23680
author: brice · 2mo ago · 2026-07-31 07:30 UTC
parent: 951a71d
signature: GPG VerifiedCD88F9E564E0102646FB6077A1A07C7C7C3E349D · arch-gpg Verified for the committer email

4 files · +31 -15

M internal/usecase/daemon/attachment_transition.go +13 -5


@@ -84,19 +84,27 @@ func (d *Daemon) snapshotAttachmentTransition(req attachmentTransitionRequest) (
84	84		if source == nil {
85	85		source = req.target
86	86		}
87		-	if source == nil \|\| (req.target == nil && req.createTargetLocked == nil) \|\| req.next == nil \|\| d.closing \|\|
88		-	d.sessions[source.core().id] != source \|\| req.target != nil && d.sessions[req.target.core().id] != req.target \|\|
	87	+	if source == nil \|\| (req.target == nil && req.createTargetLocked == nil) {
	88	+	return attachmentTransitionRequest{}, attachmentTransitionParticipants{}, errAttachmentTransition
	89	+	}
	90	+	sourceCore := source.core()
	91	+	var targetCore *sessionCore
	92	+	if req.target != nil {
	93	+	targetCore = req.target.core()
	94	+	}
	95	+	if sourceCore == nil \|\| req.target != nil && targetCore == nil \|\| req.next == nil \|\| d.closing \|\|
	96	+	d.sessions[sourceCore.id] != source \|\| req.target != nil && d.sessions[targetCore.id] != req.target \|\|
89	97		req.activationBarrier && (req.sourceToken == nil \|\| source != req.target \|\|
90	98		req.expectedRole != attachmentSnatched \|\| req.targetRole != attachmentActive) {
91	99		return attachmentTransitionRequest{}, attachmentTransitionParticipants{}, errAttachmentTransition
92	100		}
93	101		if req.target != nil {
94		-	req.target.core().mu.Lock()
95		-	req.expectedTargetCurrent = req.target.core().client
	102	+	targetCore.mu.Lock()
	103	+	req.expectedTargetCurrent = targetCore.client
96	104		if req.expectedTargetCurrent != nil && req.expectedTargetCurrent != req.next && req.targetRole == attachmentActive {
97	105		req.expectedTargetTransport = req.expectedTargetCurrent.transportSnapshot()
98	106		}
99		-	req.target.core().mu.Unlock()
	107	+	targetCore.mu.Unlock()
100	108		}
101	109		if req.sourceToken != nil && !transitionSourceTokenMatchesRequest(*req.sourceToken, source, req) {
102	110		return attachmentTransitionRequest{}, attachmentTransitionParticipants{}, errAttachmentTransition

 	if source == nil {
 		source = req.target
 	}
-	if source == nil || (req.target == nil && req.createTargetLocked == nil) || req.next == nil || d.closing ||
+	if source == nil || (req.target == nil && req.createTargetLocked == nil) {
-		d.sessions[source.core().id] != source || req.target != nil && d.sessions[req.target.core().id] != req.target ||
+		return attachmentTransitionRequest{}, attachmentTransitionParticipants{}, errAttachmentTransition
+	}
+	sourceCore := source.core()
+	var targetCore *sessionCore
+	if req.target != nil {
+		targetCore = req.target.core()
+	}
+	if sourceCore == nil || req.target != nil && targetCore == nil || req.next == nil || d.closing ||
+		d.sessions[sourceCore.id] != source || req.target != nil && d.sessions[targetCore.id] != req.target ||
 		req.activationBarrier && (req.sourceToken == nil || source != req.target ||
 			req.expectedRole != attachmentSnatched || req.targetRole != attachmentActive) {
 		return attachmentTransitionRequest{}, attachmentTransitionParticipants{}, errAttachmentTransition
 	}
 	if req.target != nil {
-		req.target.core().mu.Lock()
+		targetCore.mu.Lock()
-		req.expectedTargetCurrent = req.target.core().client
+		req.expectedTargetCurrent = targetCore.client
 		if req.expectedTargetCurrent != nil && req.expectedTargetCurrent != req.next && req.targetRole == attachmentActive {
 			req.expectedTargetTransport = req.expectedTargetCurrent.transportSnapshot()
 		}
-		req.target.core().mu.Unlock()
+		targetCore.mu.Unlock()
 	}
 	if req.sourceToken != nil && !transitionSourceTokenMatchesRequest(*req.sourceToken, source, req) {
 		return attachmentTransitionRequest{}, attachmentTransitionParticipants{}, errAttachmentTransition

M internal/usecase/daemon/attachment_transition_publish.go +11 -5


@@ -103,10 +103,16 @@ func (d *Daemon) validateAttachmentTransitionPrelocked(req attachmentTransitionR
103	103		if source == nil {
104	104		source = req.target
105	105		}
106		-	if !req.preflighted \|\| !req.roleEffectsFrozen \|\|
	106	+	if source == nil \|\| req.target == nil {
	107	+	return nil, errAttachmentTransition
	108	+	}
	109	+	sourceCore := source.core()
	110	+	targetCore := req.target.core()
	111	+	if sourceCore == nil \|\| targetCore == nil \|\|
	112	+	!req.preflighted \|\| !req.roleEffectsFrozen \|\|
107	113		!validAttachmentTransitionRole(req.expectedRole, true) \|\| !validAttachmentTransitionRole(req.targetRole, false) \|\|
108		-	source == nil \|\| req.target == nil \|\| req.next == nil \|\| d.closing \|\|
109		-	d.sessions[source.core().id] != source \|\| d.sessions[req.target.core().id] != req.target \|\|
	114	+	req.next == nil \|\| d.closing \|\|
	115	+	d.sessions[sourceCore.id] != source \|\| d.sessions[targetCore.id] != req.target \|\|
110	116		req.expectedTransport.transport == nil \|\| !req.next.transportSnapshotCurrent(req.expectedTransport) \|\|
111	117		req.preserveRole && req.sourceToken == nil {
112	118		return nil, errAttachmentTransition

@@ -118,13 +124,13 @@ func (d *Daemon) validateAttachmentTransitionPrelocked(req attachmentTransitionR
118	124		targetCoordinator = d.ensureRenderCoordinatorPrelocked(target)
119	125		}
120	126		}
121		-	sourceCoordinator := source.core().coordinator.Load()
	127	+	sourceCoordinator := sourceCore.coordinator.Load()
122	128		if attachmentSessionRoleLocked(source, req.next) != req.expectedRole \|\|
123	129		!req.next.transportSnapshotCurrent(req.expectedTransport) {
124	130		return nil, errAttachmentTransition
125	131		}
126	132
127		-	old := req.target.core().client
	133	+	old := targetCore.client
128	134		if old != req.expectedTargetCurrent \|\| source != req.target && old == req.next {
129	135		return nil, errAttachmentTransition
130	136		}

 	if source == nil {
 		source = req.target
 	}
-	if !req.preflighted || !req.roleEffectsFrozen ||
+	if source == nil || req.target == nil {
+		return nil, errAttachmentTransition
+	}
+	sourceCore := source.core()
+	targetCore := req.target.core()
+	if sourceCore == nil || targetCore == nil ||
+		!req.preflighted || !req.roleEffectsFrozen ||
 		!validAttachmentTransitionRole(req.expectedRole, true) || !validAttachmentTransitionRole(req.targetRole, false) ||
-		source == nil || req.target == nil || req.next == nil || d.closing ||
+		req.next == nil || d.closing ||
-		d.sessions[source.core().id] != source || d.sessions[req.target.core().id] != req.target ||
+		d.sessions[sourceCore.id] != source || d.sessions[targetCore.id] != req.target ||
 		req.expectedTransport.transport == nil || !req.next.transportSnapshotCurrent(req.expectedTransport) ||
 		req.preserveRole && req.sourceToken == nil {
 		return nil, errAttachmentTransition
 			targetCoordinator = d.ensureRenderCoordinatorPrelocked(target)
 		}
 	}
-	sourceCoordinator := source.core().coordinator.Load()
+	sourceCoordinator := sourceCore.coordinator.Load()
 	if attachmentSessionRoleLocked(source, req.next) != req.expectedRole ||
 		!req.next.transportSnapshotCurrent(req.expectedTransport) {
 		return nil, errAttachmentTransition
 	}
-	old := req.target.core().client
+	old := targetCore.client
 	if old != req.expectedTargetCurrent || source != req.target && old == req.next {
 		return nil, errAttachmentTransition
 	}

M internal/usecase/daemon/session.go +1 -1


@@ -363,7 +363,7 @@ func (d *Daemon) createSessionLocked(name string, ephemeral bool, cwd string, sz
363	363		}
364	364		closeTabs(tabs)
365	365		cancel()
366		-	return nil, err
	366	+	return nil, domain.UserErr(domain.NoticeSessionSpawn, "couldn't create session", err)
367	367		}
368	368		if !ephemeral {
369	369		// Keep stopped lifecycle authority intact until runtime publication and


@@ -363,7 +363,7 @@ func (d *Daemon) createSessionLocked(name string, ephemeral bool, cwd string, sz
363		}	363		}
364		closeTabs(tabs)	364		closeTabs(tabs)
365		cancel()	365		cancel()
366	-	return nil, err	366	+	return nil, domain.UserErr(domain.NoticeSessionSpawn, "couldn't create session", err)
367		}	367		}
368		if !ephemeral {	368		if !ephemeral {
369		// Keep stopped lifecycle authority intact until runtime publication and	369		// Keep stopped lifecycle authority intact until runtime publication and

M internal/usecase/daemon/session_core_test.go +6 -4


@@ -134,12 +134,11 @@ func TestSessionCoreLockOrderUsesImmutableIDs(t *testing.T) {
134	134		second.core().mu.Unlock()
135	135		}
136	136
137		-	func TestProxiedInitialMetadataSkipsInvalidSnapshot(t *testing.T) {
	137	+	func TestInitialMetadataSkipsInvalidSnapshot(t *testing.T) {
138	138		d := newTestDaemon(t, newFactory(t, newQuietPTY()), stubClock{})
139	139		hello := ports.Hello{
140	140		Version: ports.ProtocolVersion,
141	141		Intent: ports.IntentNew,
142		-	Proxied: true,
143	142		Name: "remote-work",
144	143		Size: defaultSize,
145	144		}

@@ -150,13 +149,16 @@ func TestProxiedInitialMetadataSkipsInvalidSnapshot(t *testing.T) {
150	149		close(done)
151	150		}()
152	151
153		-	awaitTestCompletion(t, tr.welcomeEntered, "proxied handshake did not send Welcome")
	152	+	awaitTestCompletion(t, tr.welcomeEntered, "handshake did not send Welcome")
154	153		sess := firstSession(d)
155	154		require.NotNil(t, sess)
156	155		sess.mu.Lock()
157	156		sess.active = len(sess.tabs)
158	157		sess.mu.Unlock()
159		-	tr.release()
	158	+	// finish (not just release) also closes recvDone, so runConnLoop's Recv
	159	+	// fails immediately after the first paint and the handshake goroutine
	160	+	// returns instead of blocking on further input.
	161	+	tr.finish()
160	162		awaitTestCompletion(t, done, "invalid metadata snapshot did not end the handshake")
161	163
162	164		welcome := <-tr.sends

 	second.core().mu.Unlock()
 }
-func TestProxiedInitialMetadataSkipsInvalidSnapshot(t *testing.T) {
+func TestInitialMetadataSkipsInvalidSnapshot(t *testing.T) {
 	d := newTestDaemon(t, newFactory(t, newQuietPTY()), stubClock{})
 	hello := ports.Hello{
 		Version: ports.ProtocolVersion,
 		Intent:  ports.IntentNew,
-		Proxied: true,
 		Name:    "remote-work",
 		Size:    defaultSize,
 	}
 		close(done)
 	}()
-	awaitTestCompletion(t, tr.welcomeEntered, "proxied handshake did not send Welcome")
+	awaitTestCompletion(t, tr.welcomeEntered, "handshake did not send Welcome")
 	sess := firstSession(d)
 	require.NotNil(t, sess)
 	sess.mu.Lock()
 	sess.active = len(sess.tabs)
 	sess.mu.Unlock()
-	tr.release()
+	// finish (not just release) also closes recvDone, so runConnLoop's Recv
+	// fails immediately after the first paint and the handshake goroutine
+	// returns instead of blocking on further input.
+	tr.finish()
 	awaitTestCompletion(t, done, "invalid metadata snapshot did not end the handshake")
 	welcome := <-tr.sends