test(runnerhub): cover the Pin relay arm and gate arm coverage on the oneof (RIG-3527) - #1014
Merged
Merged
Conversation
… oneof (RIG-3527) `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 <matt@rigel.build>
|
😎 Merged successfully - details. |
|
Compass engineering docs preview: https://compass-comms-rig3527-relay.compass-eng-docs.pages.dev Deployed from |
…RIG-3527) 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 <matt@rigel.build>
…G-3527) 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 <matt@rigel.build>
…527) 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 <matt@rigel.build>
rigel-mintaka
marked this pull request as ready for review
September 8, 2026 06:27
mattwilkinsonn
approved these changes
Sep 8, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
This PR is part of a stack containing 3 PRs:
mainCommsCallRequest_Pinhad zero tests at any tier: 0 constructions and 0.pinreads across every
_test.goin the module, so nothing asserted the attributionof the one arm dispatching
UpdatePinnedBoardAsAccount. The eight other armseach had at least one dispatching test.
Adds
relay_arm_coverage_test.go:TestRelayCommsPinDispatchesAsBoundAccount— the pin arm forwards the exactrequest under the session's bound account, wraps the pin result, round-trips
call_id.TestRelayCommsPinToolErrorIsInBandNotStreamError— a pin tool failure isrendered as an in-band
CommsCallError, never a transport teardown.TestRelayCommsEveryArmAttributesToBoundAccount— a table over all nine armsasserting bound-account attribution, exact request forwarding, and that the
response is wrapped in the result arm MATCHING the request. Coverage is gated
on the
CommsCallRequestcalloneof descriptor in both directions, so anewly 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 agentmay raise an ask but never answer one, so the request oneof cannot express
RespondToAsk(answering is an operator action onCommsService).Tests only; no production change. No podman and no Postgres — in-package against
the existing
fakeCommsCaller, so this is independent of the e2e fixture workand 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 matt@rigel.build