From 3c74bb11a208b82088ae9da289d259a081b69dcb Mon Sep 17 00:00:00 2001 From: mintaka Date: Tue, 8 Sep 2026 00:12:09 -0400 Subject: [PATCH 1/4] test(runnerhub): cover the Pin relay arm and gate arm coverage on the oneof (RIG-3527) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `CommsCallRequest_Pin` had zero tests at any tier: 0 constructions and 0 `.pin` reads across every `_test.go` in the module, so nothing asserted the attribution of the one arm dispatching `UpdatePinnedBoardAsAccount`. The eight other arms each had at least one dispatching test. Adds `relay_arm_coverage_test.go`: - `TestRelayCommsPinDispatchesAsBoundAccount` — the pin arm forwards the exact request under the session's bound account, wraps the pin result, round-trips `call_id`. - `TestRelayCommsPinToolErrorIsInBandNotStreamError` — a pin tool failure is rendered as an in-band `CommsCallError`, never a transport teardown. - `TestRelayCommsEveryArmAttributesToBoundAccount` — a table over all nine arms asserting bound-account attribution, exact request forwarding, and that the response is wrapped in the result arm MATCHING the request. Coverage is gated on the `CommsCallRequest` `call` oneof descriptor in both directions, so a newly added arm fails until listed and a stale case fails when its arm goes away. That self-enforcement is the property whose absence let Pin go untested. - `TestCommsCallRequestHasNoAskAnsweringArm` — the structural negative: an agent may raise an ask but never answer one, so the request oneof cannot express `RespondToAsk` (answering is an operator action on `CommsService`). Tests only; no production change. No podman and no Postgres — in-package against the existing `fakeCommsCaller`, so this is independent of the e2e fixture work and lands first. Each assertion was verified to bite against a real mutation rather than assumed from a green run: renaming an arm in the table fails the sweep naming it, pointing the descriptor lookup at a missing oneof reports a lost descriptor instead of panicking, and swapping the Pin arm's result wrapper to Roster fails with `pin wrapped its response in the "roster" result arm`. Ledger-impact: none Refs RIG-3527 Co-authored-by: Matt Wilkinson --- .../runnerhub/relay_arm_coverage_test.go | 189 ++++++++++++++++++ .../runnerhub/relay_roster_setstatus_test.go | 2 +- 2 files changed, 190 insertions(+), 1 deletion(-) create mode 100644 go/internal/runnerhub/relay_arm_coverage_test.go diff --git a/go/internal/runnerhub/relay_arm_coverage_test.go b/go/internal/runnerhub/relay_arm_coverage_test.go new file mode 100644 index 000000000..153903233 --- /dev/null +++ b/go/internal/runnerhub/relay_arm_coverage_test.go @@ -0,0 +1,189 @@ +//go:build unix + +package runnerhub + +import ( + "context" + "errors" + "strings" + "testing" + + "connectrpc.com/connect" + compassv1 "github.com/RigelBuild/compass/go/gen/compass/v1" + compassv1internal "github.com/RigelBuild/compass/go/internal/gen/compass/v1" +) + +// relayPin builds a RelayCommsCallRequest carrying a pin variant under callID. +func relayPin(sessionID, callID string, req *compassv1.UpdatePinnedBoardRequest) *compassv1internal.RelayCommsCallRequest { + return &compassv1internal.RelayCommsCallRequest{ + SessionId: sessionID, + Call: &compassv1internal.CommsCallRequest{ + CallId: callID, + Call: &compassv1internal.CommsCallRequest_Pin{Pin: req}, + }, + } +} + +// TestRelayCommsPinDispatchesAsBoundAccount: a pin call forwards the exact +// request under the bound account and wraps the pin result with call_id intact. +func TestRelayCommsPinDispatchesAsBoundAccount(t *testing.T) { + hub, comms := newHubWithComms() + comms.pinResp = &compassv1.UpdatePinnedBoardResponse{} + bindLiveSession(hub) + + req := &compassv1.UpdatePinnedBoardRequest{ChannelId: "ch-1"} + resp, err := hub.RelayCommsCall(context.Background(), relayPin("sess-1", "tc-pin", req)) + if err != nil { + t.Fatalf("RelayCommsCall(pin) = %v, want success", err) + } + calls := comms.snapshot() + if len(calls) != 1 { + t.Fatalf("caller invoked %d times, want 1", len(calls)) + } + if calls[0].account != testAgentAccount { + t.Fatalf("pin attributed to %q, want bound %q", calls[0].account, testAgentAccount) + } + if calls[0].pin != req { + t.Fatalf("caller received a different UpdatePinnedBoardRequest than relayed") + } + if resp.GetResult().GetPin() != comms.pinResp { + t.Fatalf("result oneof = %T, want the caller's pin response", resp.GetResult().GetResult()) + } + if got := resp.GetResult().GetCallId(); got != "tc-pin" { + t.Fatalf("response call_id = %q, want tc-pin", got) + } +} + +// TestRelayCommsPinToolErrorIsInBandNotStreamError: a pin tool failure is +// rendered as a CommsCallError while RelayCommsCall itself remains successful. +func TestRelayCommsPinToolErrorIsInBandNotStreamError(t *testing.T) { + hub, comms := newHubWithComms() + comms.pinErr = connect.NewError(connect.CodePermissionDenied, errors.New("pin denied")) + bindLiveSession(hub) + + resp, err := hub.RelayCommsCall(context.Background(), relayPin("sess-1", "tc-pin-err", &compassv1.UpdatePinnedBoardRequest{ChannelId: "ch-1"})) + if err != nil { + t.Fatalf("RelayCommsCall returned a stream error %v, want in-band tool error", err) + } + toolErr := resp.GetResult().GetError() + if toolErr == nil { + t.Fatal("response has no in-band CommsCallError, want the tool failure rendered in-band") + } + if got := toolErr.GetCode(); got != connect.CodePermissionDenied.String() { + t.Fatalf("in-band error code = %q, want %q", got, connect.CodePermissionDenied.String()) + } + if got := resp.GetResult().GetCallId(); got != "tc-pin-err" { + t.Fatalf("response call_id = %q, want tc-pin-err", got) + } +} + +// TestRelayCommsEveryArmAttributesToBoundAccount: every CommsCallRequest arm +// forwards its exact request under the session's bound account. +func TestRelayCommsEveryArmAttributesToBoundAccount(t *testing.T) { + type armCase struct { + name string + request *compassv1internal.RelayCommsCallRequest + seed func(*fakeCommsCaller) + field func(commsCall) any + want any + } + post := &compassv1.PostMessageRequest{} + list := &compassv1.ListMessagesRequest{} + roster := &compassv1.GetRosterRequest{} + pin := &compassv1.UpdatePinnedBoardRequest{} + createChannel := &compassv1.CreateChannelRequest{} + updateMembers := &compassv1.UpdateChannelMembersRequest{} + createChannelGroup := &compassv1.CreateChannelGroupRequest{} + openDM := &compassv1.OpenDMRequest{} + cases := []armCase{ + {name: "post", request: relayPost("sess-1", "tc-post", post), seed: func(c *fakeCommsCaller) { c.postResp = &compassv1.PostMessageResponse{} }, field: func(c commsCall) any { return c.post }, want: post}, + {name: "list", request: relayList("sess-1", "tc-list", list), seed: func(c *fakeCommsCaller) { c.listResp = &compassv1.ListMessagesResponse{} }, field: func(c commsCall) any { return c.list }, want: list}, + {name: "roster", request: relayRoster("sess-1", "tc-roster", roster), seed: func(c *fakeCommsCaller) { c.rosterResp = &compassv1.GetRosterResponse{} }, field: func(c commsCall) any { return c.roster }, want: roster}, + {name: "set_status", request: relaySetStatus("sess-1", "tc-status", "status"), seed: func(c *fakeCommsCaller) {}, field: func(c commsCall) any { return c.setStatus }, want: "status"}, + {name: "pin", request: relayPin("sess-1", "tc-pin", pin), seed: func(c *fakeCommsCaller) { c.pinResp = &compassv1.UpdatePinnedBoardResponse{} }, field: func(c commsCall) any { return c.pin }, want: pin}, + {name: "create_channel", request: relayCreateChannel("sess-1", "tc-channel", createChannel), seed: func(c *fakeCommsCaller) { c.createChannelResp = &compassv1.CreateChannelResponse{} }, field: func(c commsCall) any { return c.createChannel }, want: createChannel}, + {name: "update_members", request: relayUpdateMembers("sess-1", "tc-members", updateMembers), seed: func(c *fakeCommsCaller) { c.updateMembersResp = &compassv1.UpdateChannelMembersResponse{} }, field: func(c commsCall) any { return c.updateMembers }, want: updateMembers}, + {name: "create_channel_group", request: relayCreateChannelGroup("sess-1", "tc-group", createChannelGroup), seed: func(c *fakeCommsCaller) { c.createChannelGroupResp = &compassv1.CreateChannelGroupResponse{} }, field: func(c commsCall) any { return c.createChannelGroup }, want: createChannelGroup}, + {name: "open_dm", request: relayOpenDM("sess-1", "tc-dm", openDM), seed: func(c *fakeCommsCaller) { c.openDMResp = &compassv1.OpenDMResponse{} }, field: func(c commsCall) any { return c.openDM }, want: openDM}, + } + + oneof := (&compassv1internal.CommsCallRequest{}).ProtoReflect().Descriptor().Oneofs().ByName("call") + // Vacuity guards, in the order they can fail. A nil descriptor must be + // caught BEFORE any method call on it: ranging a nil oneof panics, which + // reads as a confusing crash rather than "this gate stopped measuring". + if oneof == nil { + t.Fatal(`CommsCallRequest has no oneof named "call" — the arm-coverage gate lost its descriptor and is measuring nothing`) + } + arms := oneof.Fields() + if arms.Len() == 0 { + t.Fatal(`CommsCallRequest "call" oneof yielded zero fields — the arm-coverage gate is vacuous`) + } + for i := range arms.Len() { + name := string(arms.Get(i).Name()) + found := false + for _, arm := range cases { + if arm.name == name { + found = true + break + } + } + if !found { + t.Fatalf("CommsCallRequest oneof arm %q is not covered; add a table case", name) + } + } + // The converse direction: a table case naming an arm the oneof no longer + // has would otherwise sit here forever, asserting nothing. + if len(cases) != arms.Len() { + t.Fatalf("table covers %d arms but the oneof declares %d — remove the stale case(s)", len(cases), arms.Len()) + } + + for _, arm := range cases { + t.Run(arm.name, func(t *testing.T) { + hub, comms := newHubWithComms() + arm.seed(comms) + bindLiveSession(hub) + resp, err := hub.RelayCommsCall(context.Background(), arm.request) + if err != nil { + t.Fatalf("RelayCommsCall(%s) = %v, want success", arm.name, err) + } + calls := comms.snapshot() + if len(calls) != 1 { + t.Fatalf("caller invoked %d times, want 1", len(calls)) + } + if calls[0].account != testAgentAccount { + t.Fatalf("%s attributed to %q, want bound %q", arm.name, calls[0].account, testAgentAccount) + } + if got := arm.field(calls[0]); got != arm.want { + t.Fatalf("%s recorded field = %v, want %v", arm.name, got, arm.want) + } + // The result must be wrapped in the arm MATCHING the request. + // Attribution and forwarding both pass under a mis-wrapped + // response, so without this a swapped result oneof is invisible. + // CommsCallResult reuses the request's arm names + // (agent_gateway.proto:144-155), so the check is generic. + result := resp.GetResult().ProtoReflect() + set := result.WhichOneof(result.Descriptor().Oneofs().ByName("result")) + if set == nil { + t.Fatalf("%s returned no result arm set", arm.name) + } + if got := string(set.Name()); got != arm.name { + t.Fatalf("%s wrapped its response in the %q result arm, want %q", arm.name, got, arm.name) + } + }) + } +} + +// TestCommsCallRequestHasNoAskAnsweringArm: the relay oneof cannot answer an +// ask because agents raise asks, while operators answer through CommsService.RespondToAsk. +func TestCommsCallRequestHasNoAskAnsweringArm(t *testing.T) { + oneof := (&compassv1internal.CommsCallRequest{}).ProtoReflect().Descriptor().Oneofs().ByName("call") + for i := range oneof.Fields().Len() { + field := oneof.Fields().Get(i) + if field.Name() == "respond_to_ask" { + t.Fatalf("CommsCallRequest oneof contains forbidden arm %q", field.Name()) + } + if field.Message() != nil && strings.Contains(string(field.Message().Name()), "RespondToAsk") { + t.Fatalf("CommsCallRequest oneof arm %q uses forbidden message type %q", field.Name(), field.Message().Name()) + } + } +} diff --git a/go/internal/runnerhub/relay_roster_setstatus_test.go b/go/internal/runnerhub/relay_roster_setstatus_test.go index 60419013e..f8eabc132 100644 --- a/go/internal/runnerhub/relay_roster_setstatus_test.go +++ b/go/internal/runnerhub/relay_roster_setstatus_test.go @@ -32,7 +32,7 @@ func relayRoster(sessionID, callID string, roster *compassv1.GetRosterRequest) * } // relaySetStatus builds a RelayCommsCallRequest carrying a set_status variant. -func relaySetStatus(sessionID, callID, activity string) *compassv1internal.RelayCommsCallRequest { +func relaySetStatus(sessionID, callID, activity string) *compassv1internal.RelayCommsCallRequest { //nolint:unparam // read-clarity signature: sessionID names WHICH session the call is relayed for at each call site, and every sibling relay* builder in this package takes it — dropping it here alone would break that symmetry and hide the session→account binding this leg's assertions turn on. return &compassv1internal.RelayCommsCallRequest{ SessionId: sessionID, Call: &compassv1internal.CommsCallRequest{ From 3825b59a92468dc83765177502981d96049a1819 Mon Sep 17 00:00:00 2001 From: mintaka Date: Tue, 8 Sep 2026 00:58:46 -0400 Subject: [PATCH 2/4] test(runnerhub): assert relay arms return the caller's own response (RIG-3527) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Review findings on #1014, all three mediums — test-adequacy in the new sweep. The arm-name check was weaker than it read. `WhichOneof` reports an arm as set whenever the wrapper struct is present, even when the inner message pointer is nil, so an arm that forwarded the request correctly but dropped the caller's response still passed. Proof the reviewer supplied: neutralising all nine `seed` closures left the whole test green, which means the seeds were decorative. - Add `wantResp`/`gotResp` per arm and assert the returned payload is the seeded instance, not merely that some arm is set. Seeds are now load-bearing: with every seed removed the sweep fails. `set_status` keeps no response identity by design — its arm returns a fresh empty response and its string value is what the row asserts. - Fix the roster arm's own test to assert response identity (`!= comms.rosterResp`) instead of a nil check, matching the seven sibling per-arm tests. Roster was the lone arm with no identity coverage anywhere: mutating its production arm to discard the caller's response left the entire package green. - Hoist the oneof lookup into `commsCallOneofArms`, so the structural negative gets the same nil-descriptor guard the sweep already had rather than panicking on a renamed oneof — the file was holding two standards for one lookup. - Soften the structural negative's doc comment: it guards the literal `RespondToAsk` shape, not every ask-answering spelling. The oneof's arm count is what catches an unreviewed new arm under any name. - Split the table into `commsArmCases` (funlen; the expanded rows pushed the test past 120 lines) and give each row one field per line. Mutation-verified after the split: dropping a response fails per arm, renaming a table arm fails naming it, a duplicate arm name fails by pigeonhole, and a missing oneof reports a lost descriptor instead of panicking. Ledger-impact: none Refs RIG-3527 Co-authored-by: Matt Wilkinson --- .../runnerhub/relay_arm_coverage_test.go | 206 +++++++++++++++--- .../runnerhub/relay_roster_setstatus_test.go | 4 +- 2 files changed, 172 insertions(+), 38 deletions(-) diff --git a/go/internal/runnerhub/relay_arm_coverage_test.go b/go/internal/runnerhub/relay_arm_coverage_test.go index 153903233..4f14b5dc7 100644 --- a/go/internal/runnerhub/relay_arm_coverage_test.go +++ b/go/internal/runnerhub/relay_arm_coverage_test.go @@ -9,6 +9,8 @@ import ( "testing" "connectrpc.com/connect" + "google.golang.org/protobuf/reflect/protoreflect" + compassv1 "github.com/RigelBuild/compass/go/gen/compass/v1" compassv1internal "github.com/RigelBuild/compass/go/internal/gen/compass/v1" ) @@ -77,16 +79,29 @@ func TestRelayCommsPinToolErrorIsInBandNotStreamError(t *testing.T) { } } -// TestRelayCommsEveryArmAttributesToBoundAccount: every CommsCallRequest arm -// forwards its exact request under the session's bound account. -func TestRelayCommsEveryArmAttributesToBoundAccount(t *testing.T) { - type armCase struct { - name string - request *compassv1internal.RelayCommsCallRequest - seed func(*fakeCommsCaller) - field func(commsCall) any - want any - } +// armCase is one relay arm's coverage row: the request to relay, the response to +// seed on the fake, and the accessors reading back what the hub recorded and +// returned. +// +// seed installs the response the fake returns for this arm, and wantResp is that +// same instance. gotResp reads the arm the hub actually wrapped. Together they +// make the seed load-bearing: without the identity assertion an arm could wrap +// nil and still pass, because WhichOneof reports an arm as set whenever the +// wrapper struct exists even when the inner message pointer is nil. +type armCase struct { + name string + request *compassv1internal.RelayCommsCallRequest + seed func(*fakeCommsCaller) + field func(commsCall) any + want any + gotResp func(*compassv1internal.CommsCallResult) any + wantResp any +} + +// commsArmCases is the per-arm coverage table. It lives beside the test rather +// than inside it so the table reads as data and the assertions read as logic; +// commsCallOneofArms gates it against the oneof so a new arm cannot be missed. +func commsArmCases() []armCase { post := &compassv1.PostMessageRequest{} list := &compassv1.ListMessagesRequest{} roster := &compassv1.GetRosterRequest{} @@ -95,29 +110,113 @@ func TestRelayCommsEveryArmAttributesToBoundAccount(t *testing.T) { updateMembers := &compassv1.UpdateChannelMembersRequest{} createChannelGroup := &compassv1.CreateChannelGroupRequest{} openDM := &compassv1.OpenDMRequest{} - cases := []armCase{ - {name: "post", request: relayPost("sess-1", "tc-post", post), seed: func(c *fakeCommsCaller) { c.postResp = &compassv1.PostMessageResponse{} }, field: func(c commsCall) any { return c.post }, want: post}, - {name: "list", request: relayList("sess-1", "tc-list", list), seed: func(c *fakeCommsCaller) { c.listResp = &compassv1.ListMessagesResponse{} }, field: func(c commsCall) any { return c.list }, want: list}, - {name: "roster", request: relayRoster("sess-1", "tc-roster", roster), seed: func(c *fakeCommsCaller) { c.rosterResp = &compassv1.GetRosterResponse{} }, field: func(c commsCall) any { return c.roster }, want: roster}, - {name: "set_status", request: relaySetStatus("sess-1", "tc-status", "status"), seed: func(c *fakeCommsCaller) {}, field: func(c commsCall) any { return c.setStatus }, want: "status"}, - {name: "pin", request: relayPin("sess-1", "tc-pin", pin), seed: func(c *fakeCommsCaller) { c.pinResp = &compassv1.UpdatePinnedBoardResponse{} }, field: func(c commsCall) any { return c.pin }, want: pin}, - {name: "create_channel", request: relayCreateChannel("sess-1", "tc-channel", createChannel), seed: func(c *fakeCommsCaller) { c.createChannelResp = &compassv1.CreateChannelResponse{} }, field: func(c commsCall) any { return c.createChannel }, want: createChannel}, - {name: "update_members", request: relayUpdateMembers("sess-1", "tc-members", updateMembers), seed: func(c *fakeCommsCaller) { c.updateMembersResp = &compassv1.UpdateChannelMembersResponse{} }, field: func(c commsCall) any { return c.updateMembers }, want: updateMembers}, - {name: "create_channel_group", request: relayCreateChannelGroup("sess-1", "tc-group", createChannelGroup), seed: func(c *fakeCommsCaller) { c.createChannelGroupResp = &compassv1.CreateChannelGroupResponse{} }, field: func(c commsCall) any { return c.createChannelGroup }, want: createChannelGroup}, - {name: "open_dm", request: relayOpenDM("sess-1", "tc-dm", openDM), seed: func(c *fakeCommsCaller) { c.openDMResp = &compassv1.OpenDMResponse{} }, field: func(c commsCall) any { return c.openDM }, want: openDM}, - } - oneof := (&compassv1internal.CommsCallRequest{}).ProtoReflect().Descriptor().Oneofs().ByName("call") - // Vacuity guards, in the order they can fail. A nil descriptor must be - // caught BEFORE any method call on it: ranging a nil oneof panics, which - // reads as a confusing crash rather than "this gate stopped measuring". - if oneof == nil { - t.Fatal(`CommsCallRequest has no oneof named "call" — the arm-coverage gate lost its descriptor and is measuring nothing`) - } - arms := oneof.Fields() - if arms.Len() == 0 { - t.Fatal(`CommsCallRequest "call" oneof yielded zero fields — the arm-coverage gate is vacuous`) + postResp := &compassv1.PostMessageResponse{} + listResp := &compassv1.ListMessagesResponse{} + rosterResp := &compassv1.GetRosterResponse{} + pinResp := &compassv1.UpdatePinnedBoardResponse{} + createChannelResp := &compassv1.CreateChannelResponse{} + updateMembersResp := &compassv1.UpdateChannelMembersResponse{} + createChannelGroupResp := &compassv1.CreateChannelGroupResponse{} + openDMResp := &compassv1.OpenDMResponse{} + + return []armCase{ + { + name: "post", + request: relayPost("sess-1", "tc-post", post), + seed: func(c *fakeCommsCaller) { c.postResp = postResp }, + field: func(c commsCall) any { return c.post }, + want: post, + gotResp: func(r *compassv1internal.CommsCallResult) any { return r.GetPost() }, + wantResp: postResp, + }, + { + name: "list", + request: relayList("sess-1", "tc-list", list), + seed: func(c *fakeCommsCaller) { c.listResp = listResp }, + field: func(c commsCall) any { return c.list }, + want: list, + gotResp: func(r *compassv1internal.CommsCallResult) any { return r.GetList() }, + wantResp: listResp, + }, + { + name: "roster", + request: relayRoster("sess-1", "tc-roster", roster), + seed: func(c *fakeCommsCaller) { c.rosterResp = rosterResp }, + field: func(c commsCall) any { return c.roster }, + want: roster, + gotResp: func(r *compassv1internal.CommsCallResult) any { return r.GetRoster() }, + wantResp: rosterResp, + }, + { + // set_status is the one arm with no canned response to seed: the + // production arm returns the server-truncated activity string and + // wraps a FRESH empty response, so its identity check is against + // that empty value's presence, not the caller's instance. `want` + // is a string VALUE here, compared through `any` — which catches a + // production bug forwarding "" or the call_id instead. + name: "set_status", + request: relaySetStatus("sess-1", "tc-status", "status"), + seed: func(c *fakeCommsCaller) {}, + field: func(c commsCall) any { return c.setStatus }, + want: "status", + }, + { + name: "pin", + request: relayPin("sess-1", "tc-pin", pin), + seed: func(c *fakeCommsCaller) { c.pinResp = pinResp }, + field: func(c commsCall) any { return c.pin }, + want: pin, + gotResp: func(r *compassv1internal.CommsCallResult) any { return r.GetPin() }, + wantResp: pinResp, + }, + { + name: "create_channel", + request: relayCreateChannel("sess-1", "tc-channel", createChannel), + seed: func(c *fakeCommsCaller) { c.createChannelResp = createChannelResp }, + field: func(c commsCall) any { return c.createChannel }, + want: createChannel, + gotResp: func(r *compassv1internal.CommsCallResult) any { return r.GetCreateChannel() }, + wantResp: createChannelResp, + }, + { + name: "update_members", + request: relayUpdateMembers("sess-1", "tc-members", updateMembers), + seed: func(c *fakeCommsCaller) { c.updateMembersResp = updateMembersResp }, + field: func(c commsCall) any { return c.updateMembers }, + want: updateMembers, + gotResp: func(r *compassv1internal.CommsCallResult) any { return r.GetUpdateMembers() }, + wantResp: updateMembersResp, + }, + { + name: "create_channel_group", + request: relayCreateChannelGroup("sess-1", "tc-group", createChannelGroup), + seed: func(c *fakeCommsCaller) { c.createChannelGroupResp = createChannelGroupResp }, + field: func(c commsCall) any { return c.createChannelGroup }, + want: createChannelGroup, + gotResp: func(r *compassv1internal.CommsCallResult) any { return r.GetCreateChannelGroup() }, + wantResp: createChannelGroupResp, + }, + { + name: "open_dm", + request: relayOpenDM("sess-1", "tc-dm", openDM), + seed: func(c *fakeCommsCaller) { c.openDMResp = openDMResp }, + field: func(c commsCall) any { return c.openDM }, + want: openDM, + gotResp: func(r *compassv1internal.CommsCallResult) any { return r.GetOpenDm() }, + wantResp: openDMResp, + }, } +} + +// TestRelayCommsEveryArmAttributesToBoundAccount: every CommsCallRequest arm +// forwards its exact request under the session's bound account and returns the +// caller's own response wrapped in the matching result arm. Coverage is gated on +// the oneof descriptor in both directions, so a newly added arm fails here until +// it is listed in commsArmCases. +func TestRelayCommsEveryArmAttributesToBoundAccount(t *testing.T) { + cases := commsArmCases() + arms := commsCallOneofArms(t) for i := range arms.Len() { name := string(arms.Get(i).Name()) found := false @@ -169,16 +268,51 @@ func TestRelayCommsEveryArmAttributesToBoundAccount(t *testing.T) { if got := string(set.Name()); got != arm.name { t.Fatalf("%s wrapped its response in the %q result arm, want %q", arm.name, got, arm.name) } + // The arm name alone is not enough: WhichOneof reports an arm as + // set whenever the wrapper struct exists, even wrapping a NIL + // message — so an arm that drops the caller's response passes the + // name check. Asserting the payload is the seeded instance is what + // makes each seed load-bearing and kills a dropped-response bug. + if arm.gotResp != nil { + if got := arm.gotResp(resp.GetResult()); got != arm.wantResp { + t.Fatalf("%s returned response %v, want the caller's seeded instance %v", arm.name, got, arm.wantResp) + } + } }) } } -// TestCommsCallRequestHasNoAskAnsweringArm: the relay oneof cannot answer an -// ask because agents raise asks, while operators answer through CommsService.RespondToAsk. -func TestCommsCallRequestHasNoAskAnsweringArm(t *testing.T) { +// commsCallOneofArms resolves the CommsCallRequest `call` oneof's fields, and is +// shared so every structural test over the oneof applies ONE standard for the +// lookup. A nil descriptor is caught before any method call on it: ranging a nil +// oneof panics, which reads as a confusing crash rather than "this gate stopped +// measuring". Both vacuity guards live here for the same reason. +func commsCallOneofArms(t *testing.T) protoreflect.FieldDescriptors { + t.Helper() oneof := (&compassv1internal.CommsCallRequest{}).ProtoReflect().Descriptor().Oneofs().ByName("call") - for i := range oneof.Fields().Len() { - field := oneof.Fields().Get(i) + if oneof == nil { + t.Fatal(`CommsCallRequest has no oneof named "call" — the arm-coverage gate lost its descriptor and is measuring nothing`) + } + arms := oneof.Fields() + if arms.Len() == 0 { + t.Fatal(`CommsCallRequest "call" oneof yielded zero fields — the arm-coverage gate is vacuous`) + } + return arms +} + +// TestCommsCallRequestHasNoAskAnsweringArm: the relay oneof cannot answer an ask +// because agents raise asks, while operators answer through +// CommsService.RespondToAsk (comms.proto:108). This guards the LITERAL +// RespondToAsk shape — a field named respond_to_ask, or any arm carrying a +// RespondToAsk-named message — not every conceivable ask-answering spelling; an +// arm named answer_ask would pass. It is an executable statement of the +// structural claim, not an airtight semantic gate. The oneof's arm count is +// pinned separately by the sweep above, which is what catches an unreviewed new +// arm under any name. +func TestCommsCallRequestHasNoAskAnsweringArm(t *testing.T) { + arms := commsCallOneofArms(t) + for i := range arms.Len() { + field := arms.Get(i) if field.Name() == "respond_to_ask" { t.Fatalf("CommsCallRequest oneof contains forbidden arm %q", field.Name()) } diff --git a/go/internal/runnerhub/relay_roster_setstatus_test.go b/go/internal/runnerhub/relay_roster_setstatus_test.go index f8eabc132..0ec000847 100644 --- a/go/internal/runnerhub/relay_roster_setstatus_test.go +++ b/go/internal/runnerhub/relay_roster_setstatus_test.go @@ -64,8 +64,8 @@ func TestRelayCommsCallRosterArmForwardsUnderBoundAccount(t *testing.T) { if calls[0].roster != req { t.Fatalf("caller received a different GetRosterRequest than relayed") } - if resp.GetResult().GetRoster() == nil { - t.Fatalf("result oneof = %T, want a roster result", resp.GetResult().GetResult()) + if resp.GetResult().GetRoster() != comms.rosterResp { + t.Fatalf("result oneof = %T, want the caller's roster response", resp.GetResult().GetResult()) } if got := resp.GetResult().GetCallId(); got != "tc-r" { t.Fatalf("response call_id = %q, want tc-r", got) From 5746157f2026bfb52eb7ce985269982c2269fac6 Mon Sep 17 00:00:00 2001 From: mintaka Date: Tue, 8 Sep 2026 01:31:41 -0400 Subject: [PATCH 3/4] test(runnerhub): hold every relay arm to a non-nil result payload (RIG-3527) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Round-2 review findings on #1014. The round-1 payload fix closed the hole it aimed at but made the check opt-IN, and nothing enforced that a row opts in — so the self-enforcing property held on the arm-name axis and not on the payload axis. Proof from the reviewer: strip an arm's `gotResp`/`wantResp` (the shape set_status legitimately ships) and mutate that arm to wrap nil, and the sweep passes. set_status itself was covered only by luck of a sibling test. - Add a generic `IsValid()` gate every arm pays, opted-in or not, before the identity check. `IsValid` separates a real empty message from a nil pointer, which neither `WhichOneof` nor `Has` can. The identity assertion stays as the ceiling the eight arms with a caller-owned response reach; this is the floor. A future arm that declines the pair now still cannot drop its payload silently. - Guard the RESULT oneof's descriptor lookup. Round 1 fixed exactly this on the request side; the result-side lookup sat inline in the sweep and still panicked on a rename, so the file applied its stated one-standard rule to two of three lookups. - Document the opt-out on `armCase`: a row declines seed/gotResp/wantResp only when the production arm returns a fresh response rather than the caller's. The comment previously asserted the seeds were load-bearing without noting the exception, which is what let the gap land. - Assertion messages: `%#v` and `%p` instead of `%v`, which rendered empty for zero-valued proto fixtures and named only the arm. - Move the 392-char nolint justification above the function, keeping a short directive on the signature. Mutation-verified: set_status wrapping nil now fails the sweep alone; an opted-out row plus a nil-wrapping arm fails; a renamed result oneof reports a lost descriptor with zero panics. Deferred with an issue: roster and list have no in-band error-path test, so dropping the caller's error in either arm is green today. Pre-existing and outside T13's dispatch/attribution scope — filed as RIG-3549. Ledger-impact: none Refs RIG-3527 Co-authored-by: Matt Wilkinson --- .../runnerhub/relay_arm_coverage_test.go | 63 +++++++++++++------ .../runnerhub/relay_roster_setstatus_test.go | 8 ++- 2 files changed, 51 insertions(+), 20 deletions(-) diff --git a/go/internal/runnerhub/relay_arm_coverage_test.go b/go/internal/runnerhub/relay_arm_coverage_test.go index 4f14b5dc7..4e48002be 100644 --- a/go/internal/runnerhub/relay_arm_coverage_test.go +++ b/go/internal/runnerhub/relay_arm_coverage_test.go @@ -88,6 +88,12 @@ func TestRelayCommsPinToolErrorIsInBandNotStreamError(t *testing.T) { // make the seed load-bearing: without the identity assertion an arm could wrap // nil and still pass, because WhichOneof reports an arm as set whenever the // wrapper struct exists even when the inner message pointer is nil. +// +// A row omits seed/gotResp/wantResp ONLY when the production arm returns a FRESH +// response rather than the caller's, so there is no instance to be identical to +// — today that is set_status alone, whose CommsCaller method returns a string. +// Such a row still pays the sweep's generic non-nil (IsValid) gate, which is why +// declining the pair cannot silently drop an arm's payload coverage. type armCase struct { name string request *compassv1internal.RelayCommsCallRequest @@ -209,13 +215,13 @@ func commsArmCases() []armCase { } } -// TestRelayCommsEveryArmAttributesToBoundAccount: every CommsCallRequest arm -// forwards its exact request under the session's bound account and returns the -// caller's own response wrapped in the matching result arm. Coverage is gated on -// the oneof descriptor in both directions, so a newly added arm fails here until -// it is listed in commsArmCases. -func TestRelayCommsEveryArmAttributesToBoundAccount(t *testing.T) { - cases := commsArmCases() +// requireEveryArmCovered gates the hand-maintained table against the oneof +// descriptor in BOTH directions: every declared arm has a case, and no case +// names an arm the oneof no longer declares. Both are needed — the forward loop +// alone passes a table that lost an arm to a duplicate name, and the count alone +// passes a table covering the wrong nine. +func requireEveryArmCovered(t *testing.T, cases []armCase) { + t.Helper() arms := commsCallOneofArms(t) for i := range arms.Len() { name := string(arms.Get(i).Name()) @@ -230,11 +236,19 @@ func TestRelayCommsEveryArmAttributesToBoundAccount(t *testing.T) { t.Fatalf("CommsCallRequest oneof arm %q is not covered; add a table case", name) } } - // The converse direction: a table case naming an arm the oneof no longer - // has would otherwise sit here forever, asserting nothing. if len(cases) != arms.Len() { t.Fatalf("table covers %d arms but the oneof declares %d — remove the stale case(s)", len(cases), arms.Len()) } +} + +// TestRelayCommsEveryArmAttributesToBoundAccount: every CommsCallRequest arm +// forwards its exact request under the session's bound account and returns the +// caller's own response wrapped in the matching result arm. Coverage is gated on +// the oneof descriptor in both directions, so a newly added arm fails here until +// it is listed in commsArmCases. +func TestRelayCommsEveryArmAttributesToBoundAccount(t *testing.T) { + cases := commsArmCases() + requireEveryArmCovered(t, cases) for _, arm := range cases { t.Run(arm.name, func(t *testing.T) { @@ -253,29 +267,40 @@ func TestRelayCommsEveryArmAttributesToBoundAccount(t *testing.T) { t.Fatalf("%s attributed to %q, want bound %q", arm.name, calls[0].account, testAgentAccount) } if got := arm.field(calls[0]); got != arm.want { - t.Fatalf("%s recorded field = %v, want %v", arm.name, got, arm.want) + t.Fatalf("%s recorded field = %#v, want %#v", arm.name, got, arm.want) } // The result must be wrapped in the arm MATCHING the request. // Attribution and forwarding both pass under a mis-wrapped // response, so without this a swapped result oneof is invisible. - // CommsCallResult reuses the request's arm names - // (agent_gateway.proto:144-155), so the check is generic. + // CommsCallResult reuses CommsCallRequest.call's arm names + // (agent_gateway.proto, `message CommsCallResult`), so the check is + // generic over the oneof rather than per-arm. result := resp.GetResult().ProtoReflect() - set := result.WhichOneof(result.Descriptor().Oneofs().ByName("result")) + resultOneof := result.Descriptor().Oneofs().ByName("result") + if resultOneof == nil { + t.Fatal(`CommsCallResult has no oneof named "result" — the arm-coverage gate lost its descriptor and is measuring nothing`) + } + set := result.WhichOneof(resultOneof) if set == nil { t.Fatalf("%s returned no result arm set", arm.name) } if got := string(set.Name()); got != arm.name { t.Fatalf("%s wrapped its response in the %q result arm, want %q", arm.name, got, arm.name) } - // The arm name alone is not enough: WhichOneof reports an arm as - // set whenever the wrapper struct exists, even wrapping a NIL - // message — so an arm that drops the caller's response passes the - // name check. Asserting the payload is the seeded instance is what - // makes each seed load-bearing and kills a dropped-response bug. + // The floor EVERY arm pays, including one that declines wantResp: + // WhichOneof reports an arm as set whenever the wrapper struct + // exists, even wrapping a NIL message, so the name check alone + // passes an arm that drops the caller's response. IsValid is what + // separates a real empty message from a nil pointer — Has cannot. + if !result.Get(set).Message().IsValid() { + t.Fatalf("%s wrapped a NIL message in the %q result arm", arm.name, arm.name) + } + // The ceiling the eight arms with a caller-owned response reach: + // the payload is the seeded instance, which is what makes each + // seed load-bearing and kills a wrong-instance swap. if arm.gotResp != nil { if got := arm.gotResp(resp.GetResult()); got != arm.wantResp { - t.Fatalf("%s returned response %v, want the caller's seeded instance %v", arm.name, got, arm.wantResp) + t.Fatalf("%s returned response %p, want the caller's seeded instance %p", arm.name, got, arm.wantResp) } } }) diff --git a/go/internal/runnerhub/relay_roster_setstatus_test.go b/go/internal/runnerhub/relay_roster_setstatus_test.go index 0ec000847..b64da6f82 100644 --- a/go/internal/runnerhub/relay_roster_setstatus_test.go +++ b/go/internal/runnerhub/relay_roster_setstatus_test.go @@ -32,7 +32,13 @@ func relayRoster(sessionID, callID string, roster *compassv1.GetRosterRequest) * } // relaySetStatus builds a RelayCommsCallRequest carrying a set_status variant. -func relaySetStatus(sessionID, callID, activity string) *compassv1internal.RelayCommsCallRequest { //nolint:unparam // read-clarity signature: sessionID names WHICH session the call is relayed for at each call site, and every sibling relay* builder in this package takes it — dropping it here alone would break that symmetry and hide the session→account binding this leg's assertions turn on. +// +// sessionID is retained though every current call site passes "sess-1": it names +// WHICH session the call is relayed for, every sibling relay* builder takes it, +// and dropping it here alone would hide the session→account binding this leg's +// assertions turn on (relay_comms_test.go passes "never-bound" to relayPost for +// exactly that reason). +func relaySetStatus(sessionID, callID, activity string) *compassv1internal.RelayCommsCallRequest { //nolint:unparam // read-clarity signature: see above return &compassv1internal.RelayCommsCallRequest{ SessionId: sessionID, Call: &compassv1internal.CommsCallRequest{ From c9285c5231fd8a6221018286e85e8598c530d94f Mon Sep 17 00:00:00 2001 From: mintaka Date: Tue, 8 Sep 2026 01:59:45 -0400 Subject: [PATCH 4/4] test(runnerhub): make the payload opt-out earned, not asserted (RIG-3527) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Round-3 review finding on #1014. The IsValid floor closed the nil-wrapping half of the payload gap but not the other half: IsValid separates non-nil from nil and cannot tell the caller's instance from a fresh empty one. So a row that declined the identity pair could still let its arm drop the whole response. Proof: strip pin's gotResp/wantResp (the opt-out shape the comment sanctioned) and return a fresh empty UpdatePinnedBoardResponse instead of the caller's, and the sweep stayed green. Worse, the doc comment added in the previous commit stated the opposite — that declining the pair "cannot silently drop an arm's payload coverage" — which was true only by the accident that set_status's response type is empty. That sentence was the thing a future arm author would have read as permission. - Reject a row that declines the identity check while its result type has any field, naming the type and field count. The opt-out is now earned by a zero-field response type rather than taken on trust, so a future arm that forgets the pair fails here instead of going uncovered. set_status still qualifies (SetAgentStatusResponse has zero fields), so no existing row changes. - Correct the armCase doc comment to state that actual rule. - Split the recorded-field diagnostic by row shape. `%#v` renders two zero-valued proto pointers byte-identically — a 446-char wall reading "got = X, want = X" — while `%p` on the string row is an error token. Pointer rows now print addresses, the set_status row prints the quoted activity. Mutation-verified: the previously-passing opted-out mutant now fails with "pin declines the identity check but its result type UpdatePinnedBoardResponse has 1 field(s)"; the pointer diagnostic prints distinct addresses; the string diagnostic prints got="" want="status". Ledger-impact: none Refs RIG-3527 Co-authored-by: Matt Wilkinson --- .../runnerhub/relay_arm_coverage_test.go | 28 +++++++++++++++++-- 1 file changed, 25 insertions(+), 3 deletions(-) diff --git a/go/internal/runnerhub/relay_arm_coverage_test.go b/go/internal/runnerhub/relay_arm_coverage_test.go index 4e48002be..b2f535733 100644 --- a/go/internal/runnerhub/relay_arm_coverage_test.go +++ b/go/internal/runnerhub/relay_arm_coverage_test.go @@ -92,8 +92,11 @@ func TestRelayCommsPinToolErrorIsInBandNotStreamError(t *testing.T) { // A row omits seed/gotResp/wantResp ONLY when the production arm returns a FRESH // response rather than the caller's, so there is no instance to be identical to // — today that is set_status alone, whose CommsCaller method returns a string. -// Such a row still pays the sweep's generic non-nil (IsValid) gate, which is why -// declining the pair cannot silently drop an arm's payload coverage. +// That opt-out is EARNED, not taken on trust: the sweep rejects a row that +// declines the pair while its result type has any field, because IsValid alone +// cannot tell a fresh empty message from the caller's and such a row could drop +// a real payload silently. set_status qualifies because SetAgentStatusResponse +// has zero fields. type armCase struct { name string request *compassv1internal.RelayCommsCallRequest @@ -266,8 +269,16 @@ func TestRelayCommsEveryArmAttributesToBoundAccount(t *testing.T) { if calls[0].account != testAgentAccount { t.Fatalf("%s attributed to %q, want bound %q", arm.name, calls[0].account, testAgentAccount) } + // One verb cannot serve both row shapes: the eight message rows are + // pointer-identity, and %#v renders two zero-valued proto pointers + // byte-identically (a 446-char wall reading "got = X, want = X"); + // set_status's row is a string VALUE, for which %p is an error + // token. Split on the row's kind so the failure names the mismatch. if got := arm.field(calls[0]); got != arm.want { - t.Fatalf("%s recorded field = %#v, want %#v", arm.name, got, arm.want) + if want, ok := arm.want.(string); ok { + t.Fatalf("%s recorded activity = %#v, want %#v", arm.name, got, want) + } + t.Fatalf("%s recorded request %p, want the relayed instance %p", arm.name, got, arm.want) } // The result must be wrapped in the arm MATCHING the request. // Attribution and forwarding both pass under a mis-wrapped @@ -295,6 +306,17 @@ func TestRelayCommsEveryArmAttributesToBoundAccount(t *testing.T) { if !result.Get(set).Message().IsValid() { t.Fatalf("%s wrapped a NIL message in the %q result arm", arm.name, arm.name) } + // The opt-out must be EARNED, not asserted in a comment: IsValid + // separates non-nil from nil and cannot tell the caller's instance + // from a fresh empty one, so a row that declines the identity check + // while its result type HAS fields could drop the whole response + // silently. Only a zero-field response type has nothing to lose, + // which is why set_status alone qualifies — and a future arm that + // forgets the pair fails here instead of going uncovered. + if arm.gotResp == nil && set.Message().Fields().Len() != 0 { + t.Fatalf("%s declines the identity check but its result type %s has %d field(s) that could be silently dropped; add gotResp/wantResp", + arm.name, set.Message().FullName().Name(), set.Message().Fields().Len()) + } // The ceiling the eight arms with a caller-owned response reach: // the payload is the seeded instance, which is what makes each // seed load-bearing and kills a wrong-instance swap.