From c70d9e2df48fd8cb3bf2c9664682c81bcbd9ab0e Mon Sep 17 00:00:00 2001 From: Brad Ison Date: Fri, 7 Aug 2026 12:49:31 +0200 Subject: [PATCH] chore(agentnetwork): address automated review feedback Drain the peer update channels after the bootstrap in the realstack test so its fan-out assertions can only be satisfied by the operation under test; use assert with a length guard for diffMappings conditions under test; drop the duplicate labelgen package comment and two stale provider-bootstrap comments in the e2e suite; add context to the exclusion test's account-ID assertion. Co-Authored-By: Claude Fable 5 --- e2e/agentnetwork/guardrail_block_test.go | 2 +- .../guardrail_multipolicy_test.go | 2 +- .../agentnetwork/labelgen/adjectives.go | 16 +++++++-------- .../modules/agentnetwork/reconcile_test.go | 20 +++++++++++-------- .../domain/manager/manager_test.go | 3 ++- .../server/agentnetwork_realstack_test.go | 5 +++++ 6 files changed, 28 insertions(+), 20 deletions(-) diff --git a/e2e/agentnetwork/guardrail_block_test.go b/e2e/agentnetwork/guardrail_block_test.go index 1783d3f6b..302b5107e 100644 --- a/e2e/agentnetwork/guardrail_block_test.go +++ b/e2e/agentnetwork/guardrail_block_test.go @@ -113,7 +113,7 @@ func runPathRoutedGuardrailCase(t *testing.T, tc pathRoutedGuardrailCase) { // Catch-all provider (no models) so the router forwards any model; a static // bearer key means the router injects a static auth header instead of minting - // a GCP token. Bootstraps the cluster if it isn't already. + // a GCP token. staticKey := "static-e2e-token" prov, err := srv.CreateProvider(ctx, api.AgentNetworkProviderRequest{ Name: tc.name, diff --git a/e2e/agentnetwork/guardrail_multipolicy_test.go b/e2e/agentnetwork/guardrail_multipolicy_test.go index da07508ca..2bb56b3bc 100644 --- a/e2e/agentnetwork/guardrail_multipolicy_test.go +++ b/e2e/agentnetwork/guardrail_multipolicy_test.go @@ -61,7 +61,7 @@ func TestGuardrailMultiPolicyModelAllowlist(t *testing.T) { } // pRestricted declares the two guardrailed models so routing is deterministic - // (model -> provider). Created first, so it carries the bootstrap cluster. + // (model -> provider). pRestricted, err := srv.CreateProvider(ctx, api.AgentNetworkProviderRequest{ Name: "restricted", ProviderId: "openai_api", diff --git a/management/internals/modules/agentnetwork/labelgen/adjectives.go b/management/internals/modules/agentnetwork/labelgen/adjectives.go index b0d432d74..5058c2f1c 100644 --- a/management/internals/modules/agentnetwork/labelgen/adjectives.go +++ b/management/internals/modules/agentnetwork/labelgen/adjectives.go @@ -1,14 +1,12 @@ -// Package labelgen produces DNS-safe Agent Network subdomain labels. -// -// The adjective pool below pairs with the noun pool in words.go to form -// `-` labels. It is kept separate because words.go is almost -// entirely nouns — drawing both halves from it produced unreadable pairs like -// "millet-hammock". Entries are lowercase ASCII, 4-12 chars, free of hyphens -// and digits, screened for offensive/brand/region-specific terms, and disjoint -// from the noun pool (enforced by TestAdjectives_AreDisjointFromNouns). package labelgen -// adjectives is the descriptor half of a generated label. +// adjectives is the descriptor half of a generated label. It pairs with the +// noun pool in words.go to form `-` labels, and is kept +// separate because words.go is almost entirely nouns — drawing both halves +// from it produced unreadable pairs like "millet-hammock". Entries are +// lowercase ASCII, 4-12 chars, free of hyphens and digits, screened for +// offensive/brand/region-specific terms, and disjoint from the noun pool +// (enforced by TestAdjectives_AreDisjointFromNouns). var adjectives = []string{ "able", "active", "adept", "agile", "airy", "alert", "amiable", "ample", "ancient", "ardent", "artful", "astute", "balmy", "blithe", "bold", "bonny", diff --git a/management/internals/modules/agentnetwork/reconcile_test.go b/management/internals/modules/agentnetwork/reconcile_test.go index 097f53da4..cda3a9549 100644 --- a/management/internals/modules/agentnetwork/reconcile_test.go +++ b/management/internals/modules/agentnetwork/reconcile_test.go @@ -234,10 +234,12 @@ func TestDiffMappings_ServingProxyChange(t *testing.T) { creates, updates, deletes := diffMappings(previous, current) - require.Len(t, deletes, 1, "the old proxy must be told to drop the mapping") - assert.Equal(t, "proxy.example.com", deletes[0].cluster) - require.Len(t, creates, 1, "the new proxy must be told to add it") - assert.Equal(t, "brave-otter.gateway.example.com", creates[0].cluster) + if assert.Len(t, deletes, 1, "the old proxy must be told to drop the mapping") { + assert.Equal(t, "proxy.example.com", deletes[0].cluster) + } + if assert.Len(t, creates, 1, "the new proxy must be told to add it") { + assert.Equal(t, "brave-otter.gateway.example.com", creates[0].cluster) + } assert.Empty(t, updates, "a serving-proxy move is a delete plus a create, not an update") } @@ -261,8 +263,9 @@ func TestDiffMappings_UnchangedClusterIsAnUpdate(t *testing.T) { assert.Empty(t, creates) assert.Empty(t, deletes) - require.Len(t, updates, 1) - assert.Equal(t, "proxy.example.com", updates[0].cluster) + if assert.Len(t, updates, 1) { + assert.Equal(t, "proxy.example.com", updates[0].cluster) + } } // TestDiffMappings_RemovedServiceIsDeletedOnItsOwnCluster — a service that has @@ -280,6 +283,7 @@ func TestDiffMappings_RemovedServiceIsDeletedOnItsOwnCluster(t *testing.T) { assert.Empty(t, creates) assert.Empty(t, updates) - require.Len(t, deletes, 1) - assert.Equal(t, "brave-otter.gateway.example.com", deletes[0].cluster) + if assert.Len(t, deletes, 1) { + assert.Equal(t, "brave-otter.gateway.example.com", deletes[0].cluster) + } } diff --git a/management/internals/modules/reverseproxy/domain/manager/manager_test.go b/management/internals/modules/reverseproxy/domain/manager/manager_test.go index 3d952baf9..12281b447 100644 --- a/management/internals/modules/reverseproxy/domain/manager/manager_test.go +++ b/management/internals/modules/reverseproxy/domain/manager/manager_test.go @@ -220,7 +220,8 @@ func TestGetClusterAllowList_DedicatedGatewayAddressExcluded(t *testing.T) { } st := &stubStore{ getAgentNetworkSettingsFunc: func(_ context.Context, accountID string) (*agentnetworkTypes.Settings, error) { - assert.Equal(t, "acc-123", accountID) + assert.Equal(t, "acc-123", accountID, + "the exclusion must look up the requesting account's own settings") return &agentnetworkTypes.Settings{ AccountID: accountID, Domain: "brave-otter.gateway.example.com", diff --git a/management/server/agentnetwork_realstack_test.go b/management/server/agentnetwork_realstack_test.go index ef390d4cd..d4efb1607 100644 --- a/management/server/agentnetwork_realstack_test.go +++ b/management/server/agentnetwork_realstack_test.go @@ -94,6 +94,11 @@ func TestAgentNetwork_ProviderCRUD_FansOutToProxyAndClientPeers(t *testing.T) { _, err = agentMgr.CreateSettings(ctx, adminUserID, agenttypes.DefaultSettings(accountID), clusterAddr, "") require.NoError(t, err, "CreateSettings must bootstrap the endpoint") + // The bootstrap itself reconciles and queues updates on both channels; + // drain them so the fan-out assertions below can only be satisfied by the + // operation under test, not by this leftover. + drain(clientCh) + drain(proxyCh) provider, err := agentMgr.CreateProvider(ctx, adminUserID, &agenttypes.Provider{ AccountID: accountID,