From 1c12195186a792dac2fe9096e941c14c837cbf3f Mon Sep 17 00:00:00 2001 From: Costa Tsaousis Date: Thu, 4 Jun 2026 05:40:32 +0300 Subject: [PATCH] refactor entity detail selection --- ...quality-complexity-duplication-coverage.md | 139 ++++++++++++- pkg/engine/entity_detail_selection.go | 190 ++++++++++++++++++ pkg/engine/entity_feed_sidecar.go | 123 +----------- pkg/engine/entity_surgical_test.go | 37 ++++ 4 files changed, 367 insertions(+), 122 deletions(-) create mode 100644 pkg/engine/entity_detail_selection.go diff --git a/.agents/sow/current/SOW-0102-20260603-quality-complexity-duplication-coverage.md b/.agents/sow/current/SOW-0102-20260603-quality-complexity-duplication-coverage.md index bab0fba..6376c1d 100644 --- a/.agents/sow/current/SOW-0102-20260603-quality-complexity-duplication-coverage.md +++ b/.agents/sow/current/SOW-0102-20260603-quality-complexity-duplication-coverage.md @@ -4,7 +4,7 @@ Status: in-progress -Sub-state: twenty-third implementation slice validated; operator status meaning PR pending +Sub-state: twenty-fourth implementation slice validated; entity detail selection PR pending ## Requirements @@ -2989,11 +2989,144 @@ Artifact maintenance gate: - Specs: no update needed; operator status semantics are unchanged. - End-user/operator docs: no update needed. - End-user/operator skills: no update needed. -- SOW lifecycle: remains in `.agents/sow/current/`; Slice 23 is validated and pending PR merge. +- SOW lifecycle: remains in `.agents/sow/current/`; Slice 23 merged through PR #27 as merge commit `87843397dbb295a536b740b2f72aafd6bc7e9033`. + +## Pre-Implementation Gate - Slice 24 + +Status: ready. + +Problem / root-cause model: + +- Facts: after the Slice 23 merge, local `tools/archposture` reports `pkg/engine/entity_feed_sidecar.go:370` `(*Engine).buildSelectedEntityDetailSidecarsFromFeedSidecars` at 122 lines with complexity 39. +- Facts: `go test -coverprofile=/tmp/update-ipsets-engine-slice24-baseline.cover -covermode=atomic ./pkg/engine` reports `pkg/engine` coverage at `71.6%`; `go tool cover` reports `buildSelectedEntityDetailSidecarsFromFeedSidecars` at `77.0%`, `feedEntitySidecarHasCountries` at `0.0%`, and `feedEntitySidecarHasASNs` at `0.0%`. +- Working theory: the function is large because it combines provider setup, target-builder initialization, stable feed ordering, per-feed selection, country detail aggregation, ASN feed aggregation, ASN country-distribution aggregation, and final sidecar map construction. + +Evidence reviewed: + +- `pkg/engine/entity_feed_sidecar.go` +- `pkg/engine/entity_surgical_test.go` +- `pkg/engine/home_detail_test.go` +- `pkg/engine/entity_artifact_selected_repair.go` +- `/tmp/update-ipsets-archposture-after-pr27.json` +- `/tmp/update-ipsets-engine-slice24-baseline.cover` +- Project coding, testing, hygiene, Go best-practices, Go behavioral-testing, and content-surface skills. + +Affected contracts and surfaces: + +- Selected country detail sidecars rebuilt from committed feed sidecars. +- Selected ASN detail sidecars rebuilt from committed feed sidecars. +- Full rebuild behavior versus targeted repair behavior. +- Provider metadata assigned to country and ASN detail sidecars. +- Country/ASN feed contributions, ASN names, country distribution, maintainer URLs, and stable feed processing order. +- SOW only; no docs or specs are expected to change because the slice is behavior-preserving. + +Existing patterns to reuse: + +- Existing `countryDetailBuilder` and `asnDetailBuilder` builder APIs. +- Existing `indexFeedEntitySidecar`, `feedEntitySidecarHasCountries`, and `feedEntitySidecarHasASNs` helpers. +- Existing provider lookup helpers: `preferredGeoProvider`, `preferredASNProvider`, `lookupSource`, `providerDisplayLabel`, and `sourceMaintainerURL`. +- Existing entity detail tests that validate sidecar materialization, repair, promoted pending sidecars, and detail payload behavior. + +Risk and blast radius: + +- Entity detail sidecars feed public country/ASN detail payloads and integrity repair paths. +- Targeted repair must not accidentally rebuild unrelated countries or ASNs. +- Full rebuild must still discover all countries and ASNs present in feed sidecars. +- ASN builders must keep the first non-empty ASN name and add country distribution after feed rows exist. +- Country detail builders must keep country feed rows and nested ASN contributions. +- No downloader, scheduler queueing, public serving fallback, install behavior, or UI behavior should change. + +Sensitive data handling plan: + +- This slice uses only local source code, synthetic test fixtures, temporary paths, and local posture/coverage metrics. +- No secrets, tokens, cookies, private endpoints, customer data, or personal data are needed. +- Durable artifacts will record only file paths, metrics, validation outcomes, and sanitized command evidence. + +Implementation plan: + +1. Introduce a focused builder state for selected entity details that owns provider metadata, target maps, full/targeted mode, and output builders. +2. Extract stable feed-name ordering. +3. Extract per-feed decision and country detail contribution logic. +4. Extract ASN feed contribution and ASN country-distribution contribution logic. +5. Extract final country and ASN sidecar map construction. +6. Add focused tests for country/ASN target detection helpers and preserve existing entity detail behavior through existing engine tests. + +Validation plan: + +- Run `go test ./pkg/engine`. +- Run `go test -coverprofile=/tmp/update-ipsets-engine-slice24.cover -covermode=atomic ./pkg/engine` and inspect `go tool cover -func`. +- Run `go run ./tools/archposture -root . > /tmp/update-ipsets-archposture-slice24.json` and confirm `buildSelectedEntityDetailSidecarsFromFeedSidecars` no longer appears as a large-function target. +- Run `make lint`, `make staticcheck`, `make golangci-lint`, `CI=true make coverage`, and `make test-strict`. +- Run whitespace and durable-artifact forbidden-name scans over the changed files before commit. + +Artifact impact plan: + +- AGENTS.md: no update expected. +- Runtime project skills: update only if a repeatable entity-sidecar lesson is found. +- Specs: no update expected because selected entity detail semantics are intended to stay unchanged. +- End-user/operator docs: no update expected. +- End-user/operator skills: no update expected. +- SOW lifecycle: this SOW remains in `.agents/sow/current/`; Slice 24 results will be recorded after validation. + +Open decisions: + +- No new user design decision is required because the slice is behavior-preserving quality work under the previously approved quality plan. + +## Slice 24 Results + +Changes made: + +- Moved selected entity detail sidecar aggregation into `pkg/engine/entity_detail_selection.go`. +- Reduced `buildSelectedEntityDetailSidecarsFromFeedSidecars` to a thin wrapper that creates a selected-detail builder and returns built country/ASN sidecars. +- Split the selected-detail aggregation flow into focused helpers for: + - provider metadata setup; + - target-builder initialization; + - stable feed-name ordering; + - per-feed selection decisions; + - country detail contributions; + - ASN feed contributions; + - ASN country-distribution contributions; + - final country/ASN sidecar map construction. +- Added focused tests for country and ASN target-detection helpers, including normalization, misses, zero ASN filtering, and nil sidecars. +- Preserved full versus targeted behavior, provider metadata, maintainer URLs, country feed rows, nested ASN contributions, ASN names, and ASN country distributions. + +Measured result: + +- Baseline: `pkg/engine/entity_feed_sidecar.go:370` `(*Engine).buildSelectedEntityDetailSidecarsFromFeedSidecars` was 122 lines with complexity 39 and `77.0%` direct coverage. +- Baseline helper coverage: `feedEntitySidecarHasCountries` `0.0%`; `feedEntitySidecarHasASNs` `0.0%`. +- After refactor: no `pkg/engine/entity_feed_sidecar*.go` selected-detail function appears in `tools/archposture` large-function output. +- `feedEntitySidecarHasCountries` and `feedEntitySidecarHasASNs` are each `100.0%` covered. +- `pkg/engine` coverage moved from `71.6%` to `71.7%`. +- Root coverage by `go tool cover -func=coverage.out` is `72.9%`. +- `tools/archposture` after this slice: source files `630`, source lines `127073`, large files `49`, large functions `26`, and production large functions `1`. +- Remaining top production complexity target: `pkg/engine/asn.go:39` `(*Engine).processASNDatabases`, 121 lines, complexity 22. + +Tests or equivalent validation: + +- `go test ./pkg/engine`: passed. +- `go test -coverprofile=/tmp/update-ipsets-engine-slice24.cover -covermode=atomic ./pkg/engine`: passed, `71.7%`. +- `go tool cover -func=/tmp/update-ipsets-engine-slice24.cover`: passed; target-detection helpers `100.0%`, package total `71.7%`. +- `go run ./tools/archposture -root . > /tmp/update-ipsets-archposture-slice24.json`: passed. +- `make lint`: passed. +- `make staticcheck`: passed. +- `make golangci-lint`: passed with `0 issues`. +- `CI=true make coverage`: passed, root total `72.9%`. +- `make test-strict`: passed. +- `git diff --check`: passed. +- Durable-artifact forbidden-name scan over the changed SOW and Go files found no newly added personal name, authorship, tool, or vendor-attribution text. + +Artifact maintenance gate: + +- AGENTS.md: no update needed. +- Runtime project skills: no update needed; no new durable process rule was found. +- Specs: no update needed; selected entity detail semantics are unchanged. +- End-user/operator docs: no update needed. +- End-user/operator skills: no update needed. +- SOW lifecycle: remains in `.agents/sow/current/`; Slice 24 is validated and pending PR merge. ## Outcome -First through twenty-second implementation slices are complete, validated locally, and merged. The twenty-third implementation slice is complete and validated locally. The SOW remains open for the next focused coverage, complexity, or duplication slice. +First through twenty-third implementation slices are complete, validated locally, and merged. The twenty-fourth implementation slice is complete and validated locally. The SOW remains open for the next focused coverage, complexity, or duplication slice. ## Lessons Extracted diff --git a/pkg/engine/entity_detail_selection.go b/pkg/engine/entity_detail_selection.go new file mode 100644 index 0000000..637b298 --- /dev/null +++ b/pkg/engine/entity_detail_selection.go @@ -0,0 +1,190 @@ +package engine + +import ( + "fmt" + "slices" + "strings" +) + +type selectedEntityDetailSidecarBuilder struct { + e *Engine + sidecars map[string]*feedEntitySidecar + targetCountries map[string]struct{} + targetASNs map[uint32]struct{} + full bool + + geoProvider HomeSummaryProvider + asnProvider HomeSummaryProvider + + countryBuilders map[string]*countryDetailBuilder + asnBuilders map[uint32]*asnDetailBuilder +} + +func (e *Engine) newSelectedEntityDetailSidecarBuilder(sidecars map[string]*feedEntitySidecar, targetCountries map[string]struct{}, targetASNs map[uint32]struct{}, full bool) (*selectedEntityDetailSidecarBuilder, error) { + if e == nil || e.cfg == nil { + return nil, fmt.Errorf("engine is not configured") + } + builder := &selectedEntityDetailSidecarBuilder{ + e: e, + sidecars: sidecars, + targetCountries: targetCountries, + targetASNs: targetASNs, + full: full, + geoProvider: e.homeSummaryProvider(e.preferredGeoProvider()), + asnProvider: e.homeSummaryProvider(e.preferredASNProvider()), + countryBuilders: map[string]*countryDetailBuilder{}, + asnBuilders: map[uint32]*asnDetailBuilder{}, + } + builder.initTargetBuilders() + return builder, nil +} + +func (e *Engine) homeSummaryProvider(name string) HomeSummaryProvider { + return HomeSummaryProvider{ + Name: name, + Label: providerDisplayLabel(e.lookupSource(name)), + } +} + +func (b *selectedEntityDetailSidecarBuilder) initTargetBuilders() { + if b.full { + return + } + for code := range b.targetCountries { + b.countryBuilders[code] = newCountryDetailBuilder(code) + } + for asn := range b.targetASNs { + b.asnBuilders[asn] = newASNDetailBuilder(asn) + } +} + +func (b *selectedEntityDetailSidecarBuilder) build() (map[string]*countryDetailSidecar, map[uint32]*asnDetailSidecar, error) { + for _, name := range sortedFeedEntitySidecarNames(b.sidecars) { + b.addFeedSidecar(name, b.sidecars[name]) + } + return b.countrySidecars(), b.asnSidecars(), nil +} + +func sortedFeedEntitySidecarNames(sidecars map[string]*feedEntitySidecar) []string { + names := make([]string, 0, len(sidecars)) + for name := range sidecars { + names = append(names, name) + } + slices.Sort(names) + return names +} + +func (b *selectedEntityDetailSidecarBuilder) addFeedSidecar(name string, sidecar *feedEntitySidecar) { + if sidecar == nil { + return + } + needCountryDetail := b.full || feedEntitySidecarHasCountries(sidecar, b.targetCountries) + needASNDetail := b.full || feedEntitySidecarHasASNs(sidecar, b.targetASNs) + if !needCountryDetail && !needASNDetail { + return + } + index := indexFeedEntitySidecar(sidecar) + maintainerURL := sourceMaintainerURL(b.e.lookupSource(name)) + if needCountryDetail { + b.addCountryDetails(sidecar, maintainerURL) + } + if needASNDetail { + b.addASNFeedDetails(sidecar, maintainerURL) + b.addASNCountryDetails(sidecar, index) + } +} + +func (b *selectedEntityDetailSidecarBuilder) addCountryDetails(sidecar *feedEntitySidecar, maintainerURL string) { + for _, country := range sidecar.Countries { + code := strings.ToUpper(strings.TrimSpace(country.Code)) + if code == "" { + continue + } + if !b.full { + if _, ok := b.targetCountries[code]; !ok { + continue + } + } + builder := b.countryBuilder(code) + builder.addFeed(sidecar.countryRow(country), maintainerURL) + for _, row := range country.ASNs { + builder.addASN(row.ASN, row.Name, row.Count) + } + } +} + +func (b *selectedEntityDetailSidecarBuilder) addASNFeedDetails(sidecar *feedEntitySidecar, maintainerURL string) { + for _, row := range sidecar.ASNs { + if row.ASN == 0 || row.AttributedIPs == 0 { + continue + } + if !b.full { + if _, ok := b.targetASNs[row.ASN]; !ok { + continue + } + } + builder := b.asnBuilder(row.ASN) + if builder.name == "" && row.Name != "" { + builder.name = row.Name + } + builder.addFeed(sidecar.asnRow(row), maintainerURL, row.Name) + } +} + +func (b *selectedEntityDetailSidecarBuilder) addASNCountryDetails(sidecar *feedEntitySidecar, index feedEntitySidecarIndex) { + for _, row := range sidecar.ASNs { + if row.ASN == 0 { + continue + } + if !b.full { + if _, ok := b.targetASNs[row.ASN]; !ok { + continue + } + } + builder := b.asnBuilders[row.ASN] + if builder == nil { + continue + } + for _, country := range index.asnCountries(row.ASN) { + builder.addCountry(country.code, country.count) + } + } +} + +func (b *selectedEntityDetailSidecarBuilder) countryBuilder(code string) *countryDetailBuilder { + builder := b.countryBuilders[code] + if builder == nil { + builder = newCountryDetailBuilder(code) + b.countryBuilders[code] = builder + } + return builder +} + +func (b *selectedEntityDetailSidecarBuilder) asnBuilder(asn uint32) *asnDetailBuilder { + builder := b.asnBuilders[asn] + if builder == nil { + builder = newASNDetailBuilder(asn) + b.asnBuilders[asn] = builder + } + return builder +} + +func (b *selectedEntityDetailSidecarBuilder) countrySidecars() map[string]*countryDetailSidecar { + sidecars := make(map[string]*countryDetailSidecar, len(b.countryBuilders)) + for code, builder := range b.countryBuilders { + if sidecar := builder.build(b.geoProvider, b.asnProvider); sidecar != nil { + sidecars[code] = sidecar + } + } + return sidecars +} + +func (b *selectedEntityDetailSidecarBuilder) asnSidecars() map[uint32]*asnDetailSidecar { + sidecars := make(map[uint32]*asnDetailSidecar, len(b.asnBuilders)) + for asn, builder := range b.asnBuilders { + if sidecar := builder.build(b.asnProvider, b.geoProvider); sidecar != nil { + sidecars[asn] = sidecar + } + } + return sidecars +} diff --git a/pkg/engine/entity_feed_sidecar.go b/pkg/engine/entity_feed_sidecar.go index a2b0dce..46d12c0 100644 --- a/pkg/engine/entity_feed_sidecar.go +++ b/pkg/engine/entity_feed_sidecar.go @@ -368,126 +368,11 @@ func feedEntitySidecarHasASNs(sidecar *feedEntitySidecar, targets map[uint32]str } func (e *Engine) buildSelectedEntityDetailSidecarsFromFeedSidecars(sidecars map[string]*feedEntitySidecar, targetCountries map[string]struct{}, targetASNs map[uint32]struct{}, full bool) (map[string]*countryDetailSidecar, map[uint32]*asnDetailSidecar, error) { - if e == nil || e.cfg == nil { - return nil, nil, fmt.Errorf("engine is not configured") - } - - geoProvider := HomeSummaryProvider{ - Name: e.preferredGeoProvider(), - Label: providerDisplayLabel(e.lookupSource(e.preferredGeoProvider())), - } - asnProvider := HomeSummaryProvider{ - Name: e.preferredASNProvider(), - Label: providerDisplayLabel(e.lookupSource(e.preferredASNProvider())), - } - - countryBuilders := map[string]*countryDetailBuilder{} - asnBuilders := map[uint32]*asnDetailBuilder{} - if !full { - for code := range targetCountries { - countryBuilders[code] = newCountryDetailBuilder(code) - } - for asn := range targetASNs { - asnBuilders[asn] = newASNDetailBuilder(asn) - } - } - - feedNames := make([]string, 0, len(sidecars)) - for name := range sidecars { - feedNames = append(feedNames, name) - } - slices.Sort(feedNames) - - for _, name := range feedNames { - sidecar := sidecars[name] - if sidecar == nil { - continue - } - needCountryDetail := full || feedEntitySidecarHasCountries(sidecar, targetCountries) - needASNDetail := full || feedEntitySidecarHasASNs(sidecar, targetASNs) - if !needCountryDetail && !needASNDetail { - continue - } - index := indexFeedEntitySidecar(sidecar) - maintainerURL := sourceMaintainerURL(e.lookupSource(name)) - - if needCountryDetail { - for _, country := range sidecar.Countries { - code := strings.ToUpper(strings.TrimSpace(country.Code)) - if code == "" { - continue - } - if !full { - if _, ok := targetCountries[code]; !ok { - continue - } - } - builder := countryBuilders[code] - if builder == nil { - builder = newCountryDetailBuilder(code) - countryBuilders[code] = builder - } - builder.addFeed(sidecar.countryRow(country), maintainerURL) - for _, row := range country.ASNs { - builder.addASN(row.ASN, row.Name, row.Count) - } - } - } - - if needASNDetail { - for _, row := range sidecar.ASNs { - if row.ASN == 0 || row.AttributedIPs == 0 { - continue - } - if !full { - if _, ok := targetASNs[row.ASN]; !ok { - continue - } - } - builder := asnBuilders[row.ASN] - if builder == nil { - builder = newASNDetailBuilder(row.ASN) - asnBuilders[row.ASN] = builder - } - if builder.name == "" && row.Name != "" { - builder.name = row.Name - } - builder.addFeed(sidecar.asnRow(row), maintainerURL, row.Name) - } - for _, row := range sidecar.ASNs { - if row.ASN == 0 { - continue - } - if !full { - if _, ok := targetASNs[row.ASN]; !ok { - continue - } - } - builder := asnBuilders[row.ASN] - if builder == nil { - continue - } - for _, country := range index.asnCountries(row.ASN) { - builder.addCountry(country.code, country.count) - } - } - } - } - - countrySidecars := make(map[string]*countryDetailSidecar, len(countryBuilders)) - for code, builder := range countryBuilders { - if sidecar := builder.build(geoProvider, asnProvider); sidecar != nil { - countrySidecars[code] = sidecar - } - } - - asnSidecars := make(map[uint32]*asnDetailSidecar, len(asnBuilders)) - for asn, builder := range asnBuilders { - if sidecar := builder.build(asnProvider, geoProvider); sidecar != nil { - asnSidecars[asn] = sidecar - } + builder, err := e.newSelectedEntityDetailSidecarBuilder(sidecars, targetCountries, targetASNs, full) + if err != nil { + return nil, nil, err } - return countrySidecars, asnSidecars, nil + return builder.build() } func (e *Engine) buildCountryIndexFromFeedSidecars(sidecars map[string]*feedEntitySidecar) *CountryIndexPayload { diff --git a/pkg/engine/entity_surgical_test.go b/pkg/engine/entity_surgical_test.go index f47c94d..f6327fb 100644 --- a/pkg/engine/entity_surgical_test.go +++ b/pkg/engine/entity_surgical_test.go @@ -52,6 +52,43 @@ func TestIndexFeedEntityJointSidecarProvidesConstantTimePatchRows(t *testing.T) } } +func TestFeedEntitySidecarTargetDetection(t *testing.T) { + sidecar := &feedEntitySidecar{ + Countries: []feedEntityCountryContribution{ + {Code: " us ", AttributedIPs: 10}, + {Code: "", AttributedIPs: 1}, + {Code: "DE", AttributedIPs: 5}, + }, + ASNs: []feedEntityASNContribution{ + {ASN: 0, AttributedIPs: 1}, + {ASN: 13335, AttributedIPs: 15}, + {ASN: 15169, AttributedIPs: 20}, + }, + } + + countryTargets := map[string]struct{}{"US": {}} + if !feedEntitySidecarHasCountries(sidecar, countryTargets) { + t.Fatal("expected country target detection to match normalized US code") + } + if feedEntitySidecarHasCountries(sidecar, map[string]struct{}{"FR": {}}) { + t.Fatal("unexpected country target match") + } + if feedEntitySidecarHasCountries(nil, countryTargets) { + t.Fatal("nil sidecar should not match country targets") + } + + asnTargets := map[uint32]struct{}{13335: {}} + if !feedEntitySidecarHasASNs(sidecar, asnTargets) { + t.Fatal("expected ASN target detection to match 13335") + } + if feedEntitySidecarHasASNs(sidecar, map[uint32]struct{}{64512: {}}) { + t.Fatal("unexpected ASN target match") + } + if feedEntitySidecarHasASNs(nil, asnTargets) { + t.Fatal("nil sidecar should not match ASN targets") + } +} + func TestLoadFeedEntitySidecarAcceptsLegacyMembershipArrays(t *testing.T) { eng := newEngineFixture(t) path := filepath.Join(eng.entityFeedsDir(), "sample.json")