fix(protocol): address screen wire review findings

commit: 4634437499fd206f43e13fe75c04f79e302de761
author: brice · 2mo ago · 2026-08-02 10:34 UTC
parent: c6e0a78
signature: GPG VerifiedCD88F9E564E0102646FB6077A1A07C7C7C3E349D · arch-gpg Verified for the committer email

2 files · +25 -27

M internal/ports/wire_screen.go +13 -12


@@ -66,10 +66,6 @@ const (
66	66		screenStyleLen = 18
67	67		screenSpanLimit = 4096
68	68
69		-	// The frame length includes MsgScreenUpdate's type byte. The codec takes
70		-	// only the payload, so one byte is reserved here.
71		-	screenPayloadLimit = MaxFrameLen - 1
72		-
73	69		screenFlagCursorVisible = 1 << 0
74	70		screenFlagCursorStyleSet = 1 << 1
75	71		screenKnownCursorFlags = screenFlagCursorVisible \| screenFlagCursorStyleSet

@@ -88,6 +84,10 @@ const (
88	84		styleKnownBits = (1 << 11) - 1
89	85		)
90	86
	87	+	// The frame length includes MsgScreenUpdate's type byte. The codec takes
	88	+	// only the payload, so one byte is reserved here.
	89	+	var screenPayloadLimit = MaxFrameLen - 1
	90	+
91	91		// MarshalScreenUpdate encodes a canonical structured screen update.
92	92		func MarshalScreenUpdate(m ScreenUpdate) ([]byte, error) {
93	93		if err := validateScreenUpdate(m); err != nil {

@@ -223,7 +223,7 @@ func UnmarshalScreenUpdate(data []byte) (ScreenUpdate, error) {
223	223		}
224	224		m.Cursor.Visible = cursorFlags&screenFlagCursorVisible != 0
225	225		m.Cursor.StyleSet = cursorFlags&screenFlagCursorStyleSet != 0
226		-	if !m.Cursor.StyleSet && m.Cursor.Style != 0 \|\| m.Cursor.StyleSet && m.Cursor.Style > 6 {
	226	+	if (!m.Cursor.StyleSet && m.Cursor.Style != 0) \|\| (m.Cursor.StyleSet && m.Cursor.Style > 6) {
227	227		return ScreenUpdate{}, ErrInvalidScreenUpdate
228	228		}
229	229		scrollPresent, err := r.u8()

@@ -353,15 +353,16 @@ func UnmarshalScreenUpdate(data []byte) (ScreenUpdate, error) {
353	353		}
354	354
355	355		func validateScreenUpdate(m ScreenUpdate) error {
356		-	if m.Kind == ScreenUpdateSnapshot {
	356	+	switch m.Kind {
	357	+	case ScreenUpdateSnapshot:
357	358		if m.BaseStateNum != 0 \|\| m.NewStateNum == 0 {
358	359		return ErrInvalidScreenUpdate
359	360		}
360		-	} else if m.Kind == ScreenUpdateDelta {
	361	+	case ScreenUpdateDelta:
361	362		if m.BaseStateNum == 0 \|\| m.BaseStateNum == math.MaxUint64 \|\| m.NewStateNum != m.BaseStateNum+1 {
362	363		return ErrInvalidScreenUpdate
363	364		}
364		-	} else {
	365	+	default:
365	366		return ErrInvalidScreenUpdate
366	367		}
367	368		if (m.Kind != ScreenUpdateSnapshot && m.Kind != ScreenUpdateDelta) \|\|

@@ -369,7 +370,7 @@ func validateScreenUpdate(m ScreenUpdate) error {
369	370		!screenAreaWithinLimit(m.Size) \|\| m.Cursor.Row >= uint16(m.Size.Rows) \|\| m.Cursor.Col >= uint16(m.Size.Cols) {
370	371		return ErrInvalidScreenUpdate
371	372		}
372		-	if !m.Cursor.StyleSet && m.Cursor.Style != 0 \|\| m.Cursor.StyleSet && m.Cursor.Style > 6 {
	373	+	if (!m.Cursor.StyleSet && m.Cursor.Style != 0) \|\| (m.Cursor.StyleSet && m.Cursor.Style > 6) {
373	374		return ErrInvalidScreenUpdate
374	375		}
375	376		if m.Kind == ScreenUpdateSnapshot && m.Scroll != nil {

@@ -405,7 +406,7 @@ func validateScreenSpans(m ScreenUpdate) error {
405	406		return ErrInvalidScreenUpdate
406	407		}
407	408		for _, cell := range span.Cells {
408		-	if cell.Continuation && cell.Rune != 0 \|\| !cell.Continuation && !utf8.ValidRune(cell.Rune) {
	409	+	if (cell.Continuation && cell.Rune != 0) \|\| (!cell.Continuation && !utf8.ValidRune(cell.Rune)) {
409	410		return ErrInvalidScreenUpdate
410	411		}
411	412		if err := validateScreenStyle(cell.Style); err != nil {

@@ -614,7 +615,7 @@ func readScreenStyle(r *screenReader) (renderer.Style, bool) {
614	615		return renderer.Style{}, false
615	616		}
616	617		style.Foreground = -1
617		-	} else if foregroundIndex != math.MaxUint16 && foregroundIndex > math.MaxUint8 \|\| fgRGB != (renderer.RGB{}) {
	618	+	} else if (foregroundIndex != math.MaxUint16 && foregroundIndex > math.MaxUint8) \|\| fgRGB != (renderer.RGB{}) {
618	619		return renderer.Style{}, false
619	620		}
620	621		if style.HasBackgroundRGB {

@@ -622,7 +623,7 @@ func readScreenStyle(r *screenReader) (renderer.Style, bool) {
622	623		return renderer.Style{}, false
623	624		}
624	625		style.Background = -1
625		-	} else if backgroundIndex != math.MaxUint16 && backgroundIndex > math.MaxUint8 \|\| bgRGB != (renderer.RGB{}) {
	626	+	} else if (backgroundIndex != math.MaxUint16 && backgroundIndex > math.MaxUint8) \|\| bgRGB != (renderer.RGB{}) {
626	627		return renderer.Style{}, false
627	628		}
628	629		if style.HasUnderlineColorRGB {

 	screenStyleLen  = 18
 	screenSpanLimit = 4096
-	// The frame length includes MsgScreenUpdate's type byte. The codec takes
-	// only the payload, so one byte is reserved here.
-	screenPayloadLimit = MaxFrameLen - 1
 	screenFlagCursorVisible  = 1 << 0
 	screenFlagCursorStyleSet = 1 << 1
 	screenKnownCursorFlags   = screenFlagCursorVisible | screenFlagCursorStyleSet
 	styleKnownBits            = (1 << 11) - 1
 )
+// The frame length includes MsgScreenUpdate's type byte. The codec takes
+// only the payload, so one byte is reserved here.
+var screenPayloadLimit = MaxFrameLen - 1
 // MarshalScreenUpdate encodes a canonical structured screen update.
 func MarshalScreenUpdate(m ScreenUpdate) ([]byte, error) {
 	if err := validateScreenUpdate(m); err != nil {
 	}
 	m.Cursor.Visible = cursorFlags&screenFlagCursorVisible != 0
 	m.Cursor.StyleSet = cursorFlags&screenFlagCursorStyleSet != 0
-	if !m.Cursor.StyleSet && m.Cursor.Style != 0 || m.Cursor.StyleSet && m.Cursor.Style > 6 {
+	if (!m.Cursor.StyleSet && m.Cursor.Style != 0) || (m.Cursor.StyleSet && m.Cursor.Style > 6) {
 		return ScreenUpdate{}, ErrInvalidScreenUpdate
 	}
 	scrollPresent, err := r.u8()
 }
 func validateScreenUpdate(m ScreenUpdate) error {
-	if m.Kind == ScreenUpdateSnapshot {
+	switch m.Kind {
+	case ScreenUpdateSnapshot:
 		if m.BaseStateNum != 0 || m.NewStateNum == 0 {
 			return ErrInvalidScreenUpdate
 		}
-	} else if m.Kind == ScreenUpdateDelta {
+	case ScreenUpdateDelta:
 		if m.BaseStateNum == 0 || m.BaseStateNum == math.MaxUint64 || m.NewStateNum != m.BaseStateNum+1 {
 			return ErrInvalidScreenUpdate
 		}
-	} else {
+	default:
 		return ErrInvalidScreenUpdate
 	}
 	if (m.Kind != ScreenUpdateSnapshot && m.Kind != ScreenUpdateDelta) ||
 		!screenAreaWithinLimit(m.Size) || m.Cursor.Row >= uint16(m.Size.Rows) || m.Cursor.Col >= uint16(m.Size.Cols) {
 		return ErrInvalidScreenUpdate
 	}
-	if !m.Cursor.StyleSet && m.Cursor.Style != 0 || m.Cursor.StyleSet && m.Cursor.Style > 6 {
+	if (!m.Cursor.StyleSet && m.Cursor.Style != 0) || (m.Cursor.StyleSet && m.Cursor.Style > 6) {
 		return ErrInvalidScreenUpdate
 	}
 	if m.Kind == ScreenUpdateSnapshot && m.Scroll != nil {
 			return ErrInvalidScreenUpdate
 		}
 		for _, cell := range span.Cells {
-			if cell.Continuation && cell.Rune != 0 || !cell.Continuation && !utf8.ValidRune(cell.Rune) {
+			if (cell.Continuation && cell.Rune != 0) || (!cell.Continuation && !utf8.ValidRune(cell.Rune)) {
 				return ErrInvalidScreenUpdate
 			}
 			if err := validateScreenStyle(cell.Style); err != nil {
 			return renderer.Style{}, false
 		}
 		style.Foreground = -1
-	} else if foregroundIndex != math.MaxUint16 && foregroundIndex > math.MaxUint8 || fgRGB != (renderer.RGB{}) {
+	} else if (foregroundIndex != math.MaxUint16 && foregroundIndex > math.MaxUint8) || fgRGB != (renderer.RGB{}) {
 		return renderer.Style{}, false
 	}
 	if style.HasBackgroundRGB {
 			return renderer.Style{}, false
 		}
 		style.Background = -1
-	} else if backgroundIndex != math.MaxUint16 && backgroundIndex > math.MaxUint8 || bgRGB != (renderer.RGB{}) {
+	} else if (backgroundIndex != math.MaxUint16 && backgroundIndex > math.MaxUint8) || bgRGB != (renderer.RGB{}) {
 		return renderer.Style{}, false
 	}
 	if style.HasUnderlineColorRGB {

M internal/ports/wire_screen_test.go +12 -15


@@ -146,10 +146,7 @@ func TestScreenUpdateStateNumbers(t *testing.T) {
146	146		func TestScreenUpdateGoldens(t *testing.T) {
147	147		for _, tt := range screenGoldenMessages() {
148	148		t.Run(tt.name, func(t *testing.T) {
149		-	want, err := hex.DecodeString(tt.hex)
150		-	if err != nil {
151		-	t.Fatal(err)
152		-	}
	149	+	want := mustDecodeHex(t, tt.hex)
153	150		got, err := MarshalScreenUpdate(tt.msg)
154	151		if err != nil {
155	152		t.Fatal(err)

@@ -188,10 +185,7 @@ func assertScreenEqual(t *testing.T, want, got ScreenUpdate) {
188	185
189	186		func TestScreenUpdateStrictTruncationAndTrailing(t *testing.T) {
190	187		for _, tt := range screenGoldenMessages() {
191		-	data, err := hex.DecodeString(tt.hex)
192		-	if err != nil {
193		-	t.Fatal(err)
194		-	}
	188	+	data := mustDecodeHex(t, tt.hex)
195	189		for n := 0; n < len(data); n++ {
196	190		if _, err := UnmarshalScreenUpdate(data[:n]); !errors.Is(err, ErrInvalidScreenUpdate) {
197	191		t.Fatalf("%s prefix %d: err = %v", tt.name, n, err)

@@ -205,7 +199,7 @@ func TestScreenUpdateStrictTruncationAndTrailing(t *testing.T) {
205	199		}
206	200
207	201		func TestScreenUpdateHostileFields(t *testing.T) {
208		-	golden, _ := hex.DecodeString(screenGoldenMessages()[0].hex)
	202	+	golden := mustDecodeHex(t, screenGoldenMessages()[0].hex)
209	203		tests := []struct {
210	204		name string
211	205		mut func([]byte)

@@ -289,7 +283,7 @@ func TestScreenUpdateSpanAndShapeValidation(t *testing.T) {
289	283		}
290	284
291	285		func TestScreenUpdateOwnership(t *testing.T) {
292		-	golden, _ := hex.DecodeString(screenGoldenMessages()[0].hex)
	286	+	golden := mustDecodeHex(t, screenGoldenMessages()[0].hex)
293	287		decoded, err := UnmarshalScreenUpdate(golden)
294	288		if err != nil {
295	289		t.Fatal(err)

@@ -309,6 +303,12 @@ func TestScreenUpdateLimits(t *testing.T) {
309	303		if _, err := UnmarshalScreenUpdate(make([]byte, MaxFrameLen)); !errors.Is(err, ErrScreenUpdateTooLarge) {
310	304		t.Fatalf("oversize err = %v", err)
311	305		}
	306	+	previousLimit := screenPayloadLimit
	307	+	t.Cleanup(func() { screenPayloadLimit = previousLimit })
	308	+	screenPayloadLimit = screenHeaderLen
	309	+	if _, err := MarshalScreenUpdate(screenGoldenMessages()[0].msg); !errors.Is(err, ErrScreenUpdateTooLarge) {
	310	+	t.Fatalf("marshal oversize err = %v", err)
	311	+	}
312	312		tooMany := screenGoldenMessages()[0].msg
313	313		tooMany.Spans = make([]ScreenSpan, screenSpanLimit+1)
314	314		if _, err := MarshalScreenUpdate(tooMany); !errors.Is(err, ErrInvalidScreenUpdate) {

@@ -363,7 +363,7 @@ func TestScreenUpdateScreenAreaLimit(t *testing.T) {
363	363		}
364	364		}
365	365
366		-	func mustDecodeHex(t *testing.T, s string) []byte {
	366	+	func mustDecodeHex(t testing.TB, s string) []byte {
367	367		t.Helper()
368	368		b, err := hex.DecodeString(s)
369	369		if err != nil {

@@ -374,10 +374,7 @@ func mustDecodeHex(t *testing.T, s string) []byte {
374	374
375	375		func FuzzScreenUpdate(f *testing.F) {
376	376		for _, tt := range screenGoldenMessages() {
377		-	b, err := hex.DecodeString(tt.hex)
378		-	if err != nil {
379		-	f.Fatalf("seed %s: %v", tt.name, err)
380		-	}
	377	+	b := mustDecodeHex(f, tt.hex)
381	378		f.Add(b)
382	379		}
383	380		f.Fuzz(func(t *testing.T, data []byte) {

 func TestScreenUpdateGoldens(t *testing.T) {
 	for _, tt := range screenGoldenMessages() {
 		t.Run(tt.name, func(t *testing.T) {
-			want, err := hex.DecodeString(tt.hex)
+			want := mustDecodeHex(t, tt.hex)
-			if err != nil {
-				t.Fatal(err)
-			}
 			got, err := MarshalScreenUpdate(tt.msg)
 			if err != nil {
 				t.Fatal(err)
 func TestScreenUpdateStrictTruncationAndTrailing(t *testing.T) {
 	for _, tt := range screenGoldenMessages() {
-		data, err := hex.DecodeString(tt.hex)
+		data := mustDecodeHex(t, tt.hex)
-		if err != nil {
-			t.Fatal(err)
-		}
 		for n := 0; n < len(data); n++ {
 			if _, err := UnmarshalScreenUpdate(data[:n]); !errors.Is(err, ErrInvalidScreenUpdate) {
 				t.Fatalf("%s prefix %d: err = %v", tt.name, n, err)
 }
 func TestScreenUpdateHostileFields(t *testing.T) {
-	golden, _ := hex.DecodeString(screenGoldenMessages()[0].hex)
+	golden := mustDecodeHex(t, screenGoldenMessages()[0].hex)
 	tests := []struct {
 		name string
 		mut  func([]byte)
 }
 func TestScreenUpdateOwnership(t *testing.T) {
-	golden, _ := hex.DecodeString(screenGoldenMessages()[0].hex)
+	golden := mustDecodeHex(t, screenGoldenMessages()[0].hex)
 	decoded, err := UnmarshalScreenUpdate(golden)
 	if err != nil {
 		t.Fatal(err)
 	if _, err := UnmarshalScreenUpdate(make([]byte, MaxFrameLen)); !errors.Is(err, ErrScreenUpdateTooLarge) {
 		t.Fatalf("oversize err = %v", err)
 	}
+	previousLimit := screenPayloadLimit
+	t.Cleanup(func() { screenPayloadLimit = previousLimit })
+	screenPayloadLimit = screenHeaderLen
+	if _, err := MarshalScreenUpdate(screenGoldenMessages()[0].msg); !errors.Is(err, ErrScreenUpdateTooLarge) {
+		t.Fatalf("marshal oversize err = %v", err)
+	}
 	tooMany := screenGoldenMessages()[0].msg
 	tooMany.Spans = make([]ScreenSpan, screenSpanLimit+1)
 	if _, err := MarshalScreenUpdate(tooMany); !errors.Is(err, ErrInvalidScreenUpdate) {
 	}
 }
-func mustDecodeHex(t *testing.T, s string) []byte {
+func mustDecodeHex(t testing.TB, s string) []byte {
 	t.Helper()
 	b, err := hex.DecodeString(s)
 	if err != nil {
 func FuzzScreenUpdate(f *testing.F) {
 	for _, tt := range screenGoldenMessages() {
-		b, err := hex.DecodeString(tt.hex)
+		b := mustDecodeHex(f, tt.hex)
-		if err != nil {
-			f.Fatalf("seed %s: %v", tt.name, err)
-		}
 		f.Add(b)
 	}
 	f.Fuzz(func(t *testing.T, data []byte) {