test: Refactor and test AGWPE command handling in server.go by doismellburning · Pull Request #483 · doismellburning/samoyed

doismellburning · 2026-04-08T18:26:49Z

refactor: Extract AGWPE command dispatch into handleClientCommand
test: Add handleClientCommand tests including connect-via 'v' case

Move the DataKind switch out of the cmd_listen_thread goroutine into a standalone handleClientCommand function to improve testability. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>

Tests cover version reply (R), raw/monitor toggles (k/m), port capabilities (g), radio port listing (G), outstanding frames (y), callsign registration (X), and connect-via digipeater parsing (v). The 'v' test inspects the DLQ directly to verify that digipeater callsigns are correctly populated in the enqueued connect request. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>

Copilot

Pull request overview

This PR refactors AGWPE command dispatch in server.go by extracting the command switch into a new handleClientCommand helper, and adds a focused unit test suite for that handler (including the connect-via 'v' path).

Changes:

Refactor: move AGWPE command handling from cmd_listen_thread into handleClientCommand.
Test: add server_impl_test.go exercising several command kinds and reply formatting.
Test: add coverage for 'v' connect-via digipeater parsing into DLQ connect requests.

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 4 comments.

File	Description
`src/server.go`	Extracts AGWPE command dispatch into `handleClientCommand` and updates the listener loop to call it.
`src/server_impl_test.go`	Adds unit tests for `handleClientCommand` replies/toggles and DLQ connect-via behavior.

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

Copilot · 2026-04-08T18:33:28Z

src/server.go

+			var pp = ax25_from_text(stemp, true)
+
+			if pp == nil {
+				text_color_set(DW_COLOR_ERROR)
+				dw_printf("Failed to create frame from AGW 'M' message.\n")
+			}

-			// But another place says it depends on who initiated the connection.
-			//
-			//	"If we started the connection CallFrom=US and CallTo=THEM
-			//	If the other end started the connection CallFrom=THEM and CallTo=US"
-			//
-			// The response description says nothing about the order; it just mentions two addresses.
-			// If you are writing a client or server application, the order would
-			// be clear but right here it could be either case.
-			//
-			// Another version of the documentation mentioned the source address being optional.
-			//
+			ax25_set_info(pp, cmd.Data)
+			// Issue 527: NET/ROM routing broadcasts use PID 0xCF which was not preserved here.
+			ax25_set_pid(pp, pid)

-			// The only way to get this information is from inside the data link state machine.
-			// We will send a request to it and the result coming out will be used to
-			// send the reply back to the client application.
-			{
-				var callsigns [AX25_MAX_ADDRS]string
-				const num_calls = 2 // only first 2 used.
+			tq_append(int(cmd.Header.Portx), TQ_PRIO_1_LO, pp)
+		}


In the 'M' (UNPROTO) handler, if ax25_from_text returns nil, the code logs an error but then continues and calls ax25_set_info/ax25_set_pid/tq_append with a nil packet pointer. This will panic (or enqueue nil) on malformed input.

Add an early exit on pp == nil (e.g., break/return) before calling ax25_set_info/ax25_set_pid/tq_append.

Nice - filed a fix upstream at wb2osz/direwolf#641

Copilot · 2026-04-08T18:33:29Z

src/server.go

+			var pid = cmd.Header.PID
+			var stemp strings.Builder
+			stemp.WriteString(ByteArrayToString(cmd.Header.CallFrom[:]))
+			stemp.WriteString(">")
+			stemp.WriteString(ByteArrayToString(cmd.Header.CallTo[:]))
+
+			var ndigi = int(cmd.Data[0])
+
+			for k := range ndigi {
+				var offset = 1 + 10*k
+				stemp.WriteString("," + string(cmd.Data[offset:offset+10]))
 			}


The 'V' handler indexes/slices cmd.Data (e.g., cmd.Data[0], cmd.Data[offset:offset+10], cmd.Data[1+10*ndigi:]) without validating that the payload length is large enough. A client can send an undersized frame (or DataLen=0) and crash the server goroutine via panic.

Consider validating cmd.Header.DataLen/len(cmd.Data) against the minimum required for ndigi and the digipeater list before indexing/slicing, and treat invalid payloads as protocol errors instead of panicking.

Copilot · 2026-04-08T18:33:29Z

src/server_impl_test.go

+	cmd.Data = buf.Bytes()
+	cmd.Header.DataLen = uint32(via.NumDigi)*10 + 1 // expected size per protocol


TestHandleClientCommand_v_PopulatesDigipeaters builds cmd.Data from binary.Write(via), which encodes the full fixed-size struct (71 bytes), but sets cmd.Header.DataLen to NumDigi*10+1 (21 bytes). In real execution, cmd_listen_thread reads exactly DataLen bytes from the socket into cmd.Data, so this mismatch means the test isn't exercising the same shape of input that production code will see.

Make DataLen consistent with the provided Data (and match cmd_listen_thread’s behavior), or construct cmd.Data with only the bytes that would actually be read for the chosen DataLen.

Suggested change

cmd.Data = buf.Bytes()

cmd.Header.DataLen = uint32(via.NumDigi)*10 + 1 // expected size per protocol

cmd.Header.DataLen = uint32(via.NumDigi)*10 + 1 // expected size per protocol

cmd.Data = buf.Bytes()[:int(cmd.Header.DataLen)]

src/server_impl_test.go

doismellburning · 2026-04-08T22:14:57Z

So this is actually quite neat. What's happened here is:

After finding a bug in AGWPE command handling (fix: Export struct fields so binary.Decode can populate them #479) I wanted to improve tests here (by which I mean "have more than zero")
Adding tests wasn't easy because the command handling was part of a ~600 line function in server.go that was designed to be run in a goroutine and infinitely loop
So I (ok, mostly Claude) refactored out the logic in 03d1136 and added a bunch of tests in 125445a
Then because the refactor touched (purely by whitespace) a lot of lines, Copilot reviewed them and identified a bunch of other potential issues
So now I'm fixing those too

This feels like a really neat demonstration of some of the things that excite me about using LLMs in software development

For posterity, a demonstration that the refactor was minimal (helped by using the right git diff flags):

$ git show --word-diff --ignore-all-space 03d11360c620cc2a5c8c0f797a82289577f143a1
commit 03d11360c620cc2a5c8c0f797a82289577f143a1
Author: Kristian Glass <git@doismellburning.co.uk>
Date:   Mon Apr 6 21:46:44 2026 +0100

    refactor: Extract AGWPE command dispatch into handleClientCommand

    Move the DataKind switch out of the cmd_listen_thread goroutine into a
    standalone handleClientCommand function to improve testability.

    Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>

diff --git a/src/server.go b/src/server.go
index a940fba..e69d502 100644
--- a/src/server.go
+++ b/src/server.go
@@ -989,6 +989,11 @@ func cmd_listen_thread(client int) {
                        debug_print(FROM_CLIENT, client, cmd)
                }

                {+handleClientCommand(client, cmd)+}
{+      }+}
{+} /* end cmd_listen_thread */+}

{+func handleClientCommand(client int, cmd *AGWPEMessage) {+}
        switch cmd.Header.DataKind {
        case 'R': /* Request for version number */
                {
@@ -1525,5 +1530,4 @@ func cmd_listen_thread(client int) {
                debug_print(FROM_CLIENT, client, cmd)

        }
}[-}-] /* end [-send_to_client-]{+handleClientCommand+} */

N.B. To be clear, the issues Copilot identified were with semantically unchanged code, i.e. they predated this PR - either from my tradcoded port, or from the original upstream - this is "while we're at it, I found more opportunities for improvement"

doismellburning and others added 2 commits April 8, 2026 19:22

refactor: Extract AGWPE command dispatch into handleClientCommand

03d1136

Move the DataKind switch out of the cmd_listen_thread goroutine into a standalone handleClientCommand function to improve testability. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>

Copilot AI review requested due to automatic review settings April 8, 2026 18:26

Copilot started reviewing on behalf of doismellburning April 8, 2026 18:27 View session

Copilot AI reviewed Apr 8, 2026

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

test: Refactor and test AGWPE command handling in server.go#483

test: Refactor and test AGWPE command handling in server.go#483
doismellburning wants to merge 2 commits intomainfrom
test/server

doismellburning commented Apr 8, 2026

Uh oh!

Copilot AI left a comment

Uh oh!

Copilot AI Apr 8, 2026

Uh oh!

doismellburning Apr 8, 2026

Uh oh!

Copilot AI Apr 8, 2026

Uh oh!

Copilot AI Apr 8, 2026

Uh oh!

Uh oh!

doismellburning commented Apr 8, 2026 •

edited

Loading

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

		cmd.Data = buf.Bytes()
		cmd.Header.DataLen = uint32(via.NumDigi)*10 + 1 // expected size per protocol

Conversation

doismellburning commented Apr 8, 2026

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Copilot AI Apr 8, 2026

Choose a reason for hiding this comment

Uh oh!

doismellburning Apr 8, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Apr 8, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Apr 8, 2026

Choose a reason for hiding this comment

Uh oh!

Uh oh!

doismellburning commented Apr 8, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

doismellburning commented Apr 8, 2026 •

edited

Loading