Conversation
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository YAML (base), Organization UI (inherited) Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (8)
🚧 Files skipped from review as they are similar to previous changes (4)
📝 WalkthroughWalkthroughThe PR adds asset-group tag, graph node, and OpenGraph schema adapters with PostgreSQL-backed services and tests. It also adds feature-flag lookup by key and direct JSON response writing. ChangesAsset group tag resolution
Graph node lookup
OpenGraph schema environment lookup
Feature flag lookup
Direct JSON responses
Estimated code review effort: 4 (Complex) | ~45 minutes 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.
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
packages/go/responses/responses.go (1)
79-80: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueHoist the local declarations into a
varblock.Declare
rawDataanderrat the start ofWriteJSON. Then assign theJSONViewresult.Proposed change
func WriteJSON(ctx context.Context, data JSONViewer, statusCode int, response http.ResponseWriter) { - rawData, err := data.JSONView() + var ( + rawData []byte + err error + ) + + rawData, err = data.JSONView()As per coding guidelines, “When possible, group variable initializations in a
var (...)block and hoist them to the top of the function.”🤖 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 `@packages/go/responses/responses.go` around lines 79 - 80, At the start of WriteJSON, declare rawData and err together in a var block, then assign the result of data.JSONView() to them afterward. Preserve the existing JSON serialization and error-handling behavior.Source: Coding guidelines
🤖 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.
Nitpick comments:
In `@packages/go/responses/responses.go`:
- Around line 79-80: At the start of WriteJSON, declare rawData and err together
in a var block, then assign the result of data.JSONView() to them afterward.
Preserve the existing JSON serialization and error-handling behavior.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Organization UI (inherited)
Review profile: CHILL
Plan: Pro
Run ID: 1299d389-ee04-475d-8d76-bb8d0c21d01a
📒 Files selected for processing (2)
packages/go/responses/responses.gopackages/go/responses/responses_test.go
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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/assetgrouptags/mocks/assetgrouptagsrequestadapter.go`:
- Around line 1-4: Add the current LICENSE.header contents at the beginning of
the generated assetgrouptagsrequestadapter mock, before the mockery-generated
notice. Update the mock generation template or configuration so regenerating the
file preserves the license header.
🪄 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: 80241fd4-8669-4466-8044-4a8e534ec6f2
📒 Files selected for processing (31)
server/alerts/mocks/publisher.goserver/analysis/internal/handlers/mocks/analysis.goserver/analysis/internal/services/mocks/database.goserver/analysis/mocks/analysisrequestadapter.goserver/assetgrouptags/assetgrouptags.goserver/assetgrouptags/internal/appdb/store.goserver/assetgrouptags/internal/appdb/tag.goserver/assetgrouptags/internal/appdb/tag_integration_test.goserver/assetgrouptags/internal/appdb/tag_test.goserver/assetgrouptags/internal/services/mocks/database.goserver/assetgrouptags/internal/services/services.goserver/assetgrouptags/internal/services/tag.goserver/assetgrouptags/internal/services/tag_test.goserver/assetgrouptags/mocks/assetgrouptagsrequestadapter.goserver/etac/internal/services/mocks/appdatabase.goserver/etac/mocks/service.goserver/extensions/internal/handlers/mocks/extensions.goserver/extensions/internal/services/mocks/database.goserver/featureflags/internal/handlers/mocks/featureflag.goserver/featureflags/internal/services/mocks/database.goserver/featureflags/mocks/featureflagrequestadapter.goserver/graphdb/internal/handlers/mocks/graphdb.goserver/graphdb/internal/handlers/mocks/nodeauthorizer.goserver/graphdb/internal/services/mocks/database.goserver/graphdb/mocks/graphdbrequestadapter.goserver/identity/internal/handlers/mocks/identity.goserver/identity/internal/services/mocks/database.goserver/opengraphschema/internal/services/mocks/database.goserver/opengraphschema/mocks/opengraphschemarequestadapter.goserver/users/mocks/resolver.goserver/users/mocks/user.go
💤 Files with no reviewable changes (21)
- server/analysis/internal/handlers/mocks/analysis.go
- server/users/mocks/user.go
- server/featureflags/internal/services/mocks/database.go
- server/users/mocks/resolver.go
- server/identity/internal/handlers/mocks/identity.go
- server/analysis/mocks/analysisrequestadapter.go
- server/graphdb/internal/handlers/mocks/nodeauthorizer.go
- server/etac/mocks/service.go
- server/identity/internal/services/mocks/database.go
- server/graphdb/internal/handlers/mocks/graphdb.go
- server/alerts/mocks/publisher.go
- server/extensions/internal/services/mocks/database.go
- server/analysis/internal/services/mocks/database.go
- server/opengraphschema/internal/services/mocks/database.go
- server/extensions/internal/handlers/mocks/extensions.go
- server/etac/internal/services/mocks/appdatabase.go
- server/featureflags/mocks/featureflagrequestadapter.go
- server/featureflags/internal/handlers/mocks/featureflag.go
- server/graphdb/internal/services/mocks/database.go
- server/graphdb/mocks/graphdbrequestadapter.go
- server/opengraphschema/mocks/opengraphschemarequestadapter.go
coderabbitai
Bot
removed
documentation
stephanieslamb
approved these changes
Aug 11, 2026
stephanieslamb
left a comment
stephanieslamb
left a comment
Contributor
There was a problem hiding this comment.
Looks good! Nice work 👏
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 subscribe to this conversation on GitHub.
Already have an account?
Sign in.
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.
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
New Features
Tests