Conversation
Contributor
📝 WalkthroughWalkthroughThe PR adds PostgreSQL-backed asset-group tag resolution, graph node lookup, and OpenGraph schema environment lookup. It also exposes public adapters and extends feature-flag interfaces with ChangesAsset group tag resolution
Graph node lookup
OpenGraph schema environment lookup
Feature flag lookup
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant Caller
participant AssetGroupsRequestAdapter
participant Service
participant Store
participant PostgreSQL
Caller->>AssetGroupsRequestAdapter: ResolveTagIDsWithFallback
AssetGroupsRequestAdapter->>Service: ResolveTagIDsWithFallback
Service->>Store: GetAssetGroupTagByID or GetTierZeroTag
Store->>PostgreSQL: Query active asset-group tags
PostgreSQL-->>Store: Return tag rows
Store-->>Service: Return AssetGroupTag
Service-->>Caller: Return resolved tag IDs
Possibly related PRs
Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
Comment |
Flake85
marked this pull request as ready for review
August 6, 2026 15:55
coderabbitai
Bot
added
go
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
server/assetgroups/internal/services/services.go (1)
35-41: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueUse descriptive database field names consistently.
Both stores use the abbreviated field name
db. Rename each field todatabaseand update its method reads.
server/assetgroups/internal/services/services.go#L35-L41: renameService.dbtoService.database.server/assetgroups/internal/appdb/store.go#L39-L45: renameStore.dbtoStore.database.As per coding guidelines, prefer descriptive variable names, such as
databaseInterface, instead of abbreviated names such asdiordbi.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@server/assetgroups/internal/services/services.go` around lines 35 - 41, Rename Service.db to Service.database in server/assetgroups/internal/services/services.go and update all Service method references. Rename Store.db to Store.database in server/assetgroups/internal/appdb/store.go and update all Store method references; retain descriptive constructor parameter names such as databaseInterface.Source: Coding guidelines
server/assetgroups/assetgroups.go (1)
30-41: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick winExpose the asset-group not-found errors from the public package.
AssetGroupsRequestAdapterpropagates both sentinels, but external consumers cannot useerrors.Iswithout importing the internal package. Add public aliases forErrAssetGroupTagNotFoundandErrTierZeroTagNotFound.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@server/assetgroups/assetgroups.go` around lines 30 - 41, Expose public aliases for services.ErrAssetGroupTagNotFound and services.ErrTierZeroTagNotFound alongside AssetGroupTag and AssetGroupsRequestAdapter, preserving their identity so external consumers can use errors.Is without importing the internal package.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@server/featureflags/featureflags.go`:
- Line 41: Update FeatureFlagRequestAdapter’s GetFlagByKey contract to return a
FeatureFlag type from a public package instead of services.FeatureFlag. Define
the public model and translate the service-layer value at the adapter boundary,
updating the public mock and Return signatures to use the new type.
---
Nitpick comments:
In `@server/assetgroups/assetgroups.go`:
- Around line 30-41: Expose public aliases for services.ErrAssetGroupTagNotFound
and services.ErrTierZeroTagNotFound alongside AssetGroupTag and
AssetGroupsRequestAdapter, preserving their identity so external consumers can
use errors.Is without importing the internal package.
In `@server/assetgroups/internal/services/services.go`:
- Around line 35-41: Rename Service.db to Service.database in
server/assetgroups/internal/services/services.go and update all Service method
references. Rename Store.db to Store.database in
server/assetgroups/internal/appdb/store.go and update all Store method
references; retain descriptive constructor parameter names such as
databaseInterface.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Organization UI (inherited)
Review profile: CHILL
Plan: Pro
Run ID: e5b77b2d-56b3-480b-affb-83f0c2caa5c1
📒 Files selected for processing (29)
server/assetgroups/assetgroups.goserver/assetgroups/internal/appdb/store.goserver/assetgroups/internal/appdb/tag.goserver/assetgroups/internal/appdb/tag_integration_test.goserver/assetgroups/internal/appdb/tag_test.goserver/assetgroups/internal/services/mocks/database.goserver/assetgroups/internal/services/services.goserver/assetgroups/internal/services/tag.goserver/assetgroups/internal/services/tag_test.goserver/assetgroups/mocks/assetgroupsrequestadapter.goserver/featureflags/featureflags.goserver/featureflags/internal/handlers/handlers.goserver/featureflags/internal/handlers/mocks/featureflag.goserver/featureflags/mocks/featureflagrequestadapter.goserver/graphdb/graphdb.goserver/graphdb/internal/appdb/node.goserver/graphdb/internal/appdb/node_integration_test.goserver/graphdb/internal/services/mocks/database.goserver/graphdb/internal/services/node_test.goserver/graphdb/internal/services/services.goserver/graphdb/mocks/graphdbrequestadapter.goserver/opengraphschema/internal/appdb/store.goserver/opengraphschema/internal/appdb/store_integration_test.goserver/opengraphschema/internal/appdb/store_test.goserver/opengraphschema/internal/services/mocks/database.goserver/opengraphschema/internal/services/services.goserver/opengraphschema/internal/services/services_test.goserver/opengraphschema/mocks/opengraphschemarequestadapter.goserver/opengraphschema/opengraphschema.go
|
|
||
| type FeatureFlagRequestAdapter interface { | ||
| IsEnabled(ctx context.Context, key string) (bool, error) | ||
| GetFlagByKey(ctx context.Context, key string) (services.FeatureFlag, error) |
Contributor
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy lift
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
ast-grep outline server/featureflags/featureflags.go --items all
rg -n -C 4 --type go \
'FeatureFlagRequestAdapter|NewFeatureFlagRequestAdapter|GetFlagByKey\s*\(|MockFeatureFlagRequestAdapter|EXPECT\(\)\.GetFlagByKey' .
rg -n -C 3 --type go \
'"github\.com/specterops/bloodhound/server/featureflags/internal/services"' .Repository: SpecterOps/BloodHound
Length of output: 50378
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '%s\n' '--- adapter and type definitions ---'
sed -n '1,90p' server/featureflags/featureflags.go
sed -n '1,150p' server/featureflags/internal/services/services.go
sed -n '1,140p' server/featureflags/mocks/featureflagrequestadapter.go
printf '%s\n' '--- module and external adapter usage ---'
sed -n '1,40p' go.mod
rg -n --type go \
'featureflags\.(FeatureFlagRequestAdapter|NewFeatureFlagRequestAdapter)|mocks\.NewMockFeatureFlagRequestAdapter|FeatureFlagRequestAdapter' \
--glob '!server/featureflags/**' .
printf '%s\n' '--- minimal Go internal-package API probe ---'
probe_dir="$(mktemp -d)"
trap 'rm -rf "$probe_dir"' EXIT
mkdir -p "$probe_dir/parent/internal/model" "$probe_dir/parent/api" "$probe_dir/client"
cat >"$probe_dir/go.mod" <<'EOF'
module example.com/probe
go 1.22
EOF
cat >"$probe_dir/parent/internal/model/model.go" <<'EOF'
package model
type Value struct{ Enabled bool }
EOF
cat >"$probe_dir/parent/api/api.go" <<'EOF'
package api
import "example.com/probe/parent/internal/model"
type Provider interface {
Get() (model.Value, error)
}
EOF
cat >"$probe_dir/client/main.go" <<'EOF'
package main
import "example.com/probe/parent/api"
type implementation struct{}
func (implementation) Get() (struct{ Enabled bool }, error) { return struct{ Enabled bool }{}, nil }
var _ api.Provider = implementation{}
func main() {}
EOF
(
cd "$probe_dir"
if go test ./client 2>&1; then
echo 'external implementation unexpectedly compiled'
else
echo 'external implementation did not compile, as expected'
fi
)
cat >"$probe_dir/client/main.go" <<'EOF'
package main
import "example.com/probe/parent/api"
func main() {
var provider api.Provider
_, _ = provider.Get()
}
EOF
(
cd "$probe_dir"
if go test ./client 2>&1; then
echo 'external method call compiled'
else
echo 'external method call did not compile'
fi
)Repository: SpecterOps/BloodHound
Length of output: 12575
🏁 Script executed:
#!/bin/bash
set -u
printf '%s\n' '--- package declarations and direct imports ---'
rg -n --type go \
'^(package |[[:space:]]*"github\.com/specterops/bloodhound/server/featureflags"|[[:space:]]*featureflags "[^"]+")' \
server/featureflags cmd packages \
| head -200
printf '%s\n' '--- minimal Go internal-package API probe ---'
probe_dir="$(mktemp -d)"
trap 'rm -rf "$probe_dir"' EXIT
mkdir -p "$probe_dir/parent/internal/model" "$probe_dir/parent/api" "$probe_dir/client"
cat >"$probe_dir/go.mod" <<'EOF'
module example.com/probe
go 1.22
EOF
cat >"$probe_dir/parent/internal/model/model.go" <<'EOF'
package model
type Value struct{ Enabled bool }
EOF
cat >"$probe_dir/parent/api/api.go" <<'EOF'
package api
import "example.com/probe/parent/internal/model"
type Provider interface {
Get() (model.Value, error)
}
func New() Provider { return provider{} }
type provider struct{}
func (provider) Get() (model.Value, error) { return model.Value{}, nil }
EOF
printf '%s\n' 'case 1: external package calls method without naming internal type'
cat >"$probe_dir/client/main.go" <<'EOF'
package main
import "example.com/probe/parent/api"
func main() {
var provider api.Provider = api.New()
value, err := provider.Get()
_ = value
_ = err
}
EOF
(cd "$probe_dir" && go test ./client)
printf '%s\n' 'case 1: compiled'
printf '%s\n' 'case 2: external package implements interface with structurally similar type'
cat >"$probe_dir/client/main.go" <<'EOF'
package main
import "example.com/probe/parent/api"
type implementation struct{}
func (implementation) Get() (struct{ Enabled bool }, error) {
return struct{ Enabled bool }{}, nil
}
var _ api.Provider = implementation{}
func main() {}
EOF
if (cd "$probe_dir" && go test ./client); then
echo 'case 2: unexpectedly compiled'
else
echo 'case 2: failed as expected'
fi
printf '%s\n' 'case 3: external package imports the internal package directly'
cat >"$probe_dir/client/main.go" <<'EOF'
package main
import (
"example.com/probe/parent/api"
"example.com/probe/parent/internal/model"
)
var _ model.Value
var _ api.Provider
func main() {}
EOF
if (cd "$probe_dir" && go test ./client); then
echo 'case 3: unexpectedly compiled'
else
echo 'case 3: failed as expected'
fiRepository: SpecterOps/BloodHound
Length of output: 13161
Do not expose services.FeatureFlag in FeatureFlagRequestAdapter.
External packages can call the method, but they cannot import the internal type or implement the interface with a compatible declared return type. The public mock also requires this internal type in Return. Define FeatureFlag in a public package and translate the service value at the adapter boundary.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@server/featureflags/featureflags.go` at line 41, Update
FeatureFlagRequestAdapter’s GetFlagByKey contract to return a FeatureFlag type
from a public package instead of services.FeatureFlag. Define the public model
and translate the service-layer value at the adapter boundary, updating the
public mock and Return signatures to use the new type.
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
1 participant
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.
Description
Describe your changes in detail
Moved
finding-trendsto the new architecture.Motivation and Context
Resolves BED-9018
Why is this change required? What problem does it solve?
How Has This Been Tested?
Please describe in detail how you tested your changes.
Include details of your testing environment, and the tests you ran to
see how your change affects other areas of the code, etc.
Screenshots (optional):
Types of changes
Checklist:
Summary by CodeRabbit