Conversation
mistahj67
added
the
api
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
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 (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughA single exported constant ChangesAlert Webhook URI Constant
Estimated code review effort🎯 1 (Trivial) | ⏱️ ~2 minutes 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
![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 (3)
cmd/api/src/api/router/router.go (1)
106-108: ⚡ Quick winRename
dbto match the service abstraction.Line 106 no longer accepts a database-specific dependency, so
dbobscures the new contract. UsefeatureFlagsconsistently.As per coding guidelines, “Prefer rich variable names, for example:
databaseInterfaceinstead ofdiordbi.”Suggested rename
-func (s *Route) CheckFeatureFlag(db featureflags.Service, flagKey string) *Route { - s.handler.Use(middleware.FeatureFlagMiddleware(db, flagKey)) +func (s *Route) CheckFeatureFlag(featureFlags featureflags.Service, flagKey string) *Route { + s.handler.Use(middleware.FeatureFlagMiddleware(featureFlags, flagKey)) return s }🤖 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 `@cmd/api/src/api/router/router.go` around lines 106 - 108, Rename the parameter `db` to `featureFlags` in the CheckFeatureFlag method to accurately reflect that it accepts a featureflags.Service abstraction rather than a database-specific dependency. Update the parameter name in the function signature from `db featureflags.Service` to `featureFlags featureflags.Service`, and then update the reference within the middleware call to pass `featureFlags` instead of `db` to the FeatureFlagMiddleware function.Source: Coding guidelines
cmd/api/src/model/appcfg/flag.go (1)
103-104: ⚡ Quick winDocument the
IsEnabledcontract directly.“shared slice method” does not explain the public interface behavior. Prefer a behavior-focused comment.
Suggested comment update
- // Implements the new feature flag shared slice method + // IsEnabled reports whether the feature flag identified by key is enabled. IsEnabled(ctx context.Context, key string) (bool, error)🤖 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 `@cmd/api/src/model/appcfg/flag.go` around lines 103 - 104, The comment for the IsEnabled method on line 103 is vague and uses implementation-focused language ("shared slice method") rather than explaining the actual behavior and contract of the public interface. Replace the current comment with a behavior-focused description that clearly explains what the IsEnabled method does, what parameters it takes, what it returns, and any relevant error conditions or behavior users of this interface should understand.cmd/api/src/database/featureflags.go (1)
37-44: ⚡ Quick winDelegate to
GetFlagByKeyto avoid lookup drift.
IsEnabledrepeats the same key lookup from Line 32. Keep the query/error-mapping contract centralized and make this method a thin boolean wrapper.Suggested refactor
func (s *BloodhoundDB) IsEnabled(ctx context.Context, key string) (bool, error) { - var flag appcfg.FeatureFlag - if err := s.db.WithContext(ctx).Where("key = ?", key).First(&flag); err.Error != nil { - return false, CheckError(err) - } else { - return flag.Enabled, nil + var ( + flag appcfg.FeatureFlag + err error + ) + + if flag, err = s.GetFlagByKey(ctx, key); err != nil { + return false, err } + + return flag.Enabled, nil }🤖 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 `@cmd/api/src/database/featureflags.go` around lines 37 - 44, The IsEnabled method duplicates the key lookup logic that already exists in the GetFlagByKey method, which can lead to inconsistencies if the lookup logic needs to change. Refactor the IsEnabled method to call GetFlagByKey internally to fetch the feature flag, then return the Enabled boolean field from the result. This keeps the query and error-handling logic centralized in GetFlagByKey while making IsEnabled a thin wrapper that simply extracts and returns the Enabled value.
🤖 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 `@cmd/api/src/api/middleware/middleware.go`:
- Line 37: The import statement for
github.com/specterops/bloodhound-enterprise/server/shared/featureflags is
unresolved because no corresponding module entry exists in go.mod. Replace this
enterprise module dependency with the compatible local appcfg interface by
removing the featureflags import from both middleware.go and router.go, adding
the import for github.com/specterops/bloodhound/cmd/api/src/model/appcfg
instead, and updating all function signatures that use featureflags.Service type
to use appcfg.FeatureFlagService type instead (look for these type references in
the function parameters around the middleware and router initialization
functions).
---
Nitpick comments:
In `@cmd/api/src/api/router/router.go`:
- Around line 106-108: Rename the parameter `db` to `featureFlags` in the
CheckFeatureFlag method to accurately reflect that it accepts a
featureflags.Service abstraction rather than a database-specific dependency.
Update the parameter name in the function signature from `db
featureflags.Service` to `featureFlags featureflags.Service`, and then update
the reference within the middleware call to pass `featureFlags` instead of `db`
to the FeatureFlagMiddleware function.
In `@cmd/api/src/database/featureflags.go`:
- Around line 37-44: The IsEnabled method duplicates the key lookup logic that
already exists in the GetFlagByKey method, which can lead to inconsistencies if
the lookup logic needs to change. Refactor the IsEnabled method to call
GetFlagByKey internally to fetch the feature flag, then return the Enabled
boolean field from the result. This keeps the query and error-handling logic
centralized in GetFlagByKey while making IsEnabled a thin wrapper that simply
extracts and returns the Enabled value.
In `@cmd/api/src/model/appcfg/flag.go`:
- Around line 103-104: The comment for the IsEnabled method on line 103 is vague
and uses implementation-focused language ("shared slice method") rather than
explaining the actual behavior and contract of the public interface. Replace the
current comment with a behavior-focused description that clearly explains what
the IsEnabled method does, what parameters it takes, what it returns, and any
relevant error conditions or behavior users of this interface should understand.
🪄 Autofix (Beta)
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: acf8f066-5eb2-4a8f-844a-51e18de34857
📒 Files selected for processing (5)
cmd/api/src/api/constant.gocmd/api/src/api/middleware/middleware.gocmd/api/src/api/router/router.gocmd/api/src/database/featureflags.gocmd/api/src/model/appcfg/flag.go
| "github.com/gorilla/mux" | ||
| "github.com/prometheus/client_golang/prometheus" | ||
| "github.com/prometheus/client_golang/prometheus/promhttp" | ||
| "github.com/specterops/bloodhound-enterprise/server/shared/featureflags" |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Description: Confirm whether the enterprise featureflags package is available to this module.
echo "Imports:"
rg -n 'github\.com/specterops/bloodhound-enterprise/server/shared/featureflags' --type go -C2 || true
echo
echo "go.mod references:"
fd '^go\.mod$' -x sh -c 'printf "\n# %s\n" "$1"; rg -n "bloodhound-enterprise|server/shared/featureflags|replace .*bloodhound-enterprise" "$1" || true' sh {} \;Repository: SpecterOps/BloodHound
Length of output: 940
🏁 Script executed:
#!/bin/bash
# Examine middleware.go to see the FeatureFlagMiddleware signature and usage
echo "=== middleware.go lines 295-315 ==="
sed -n '295,315p' cmd/api/src/api/middleware/middleware.go | cat -n
echo ""
echo "=== Check if appcfg.FeatureFlagService exists ==="
rg -n 'FeatureFlagService' --type go cmd/api/src/api/middleware/ cmd/api/src/model/ 2>/dev/null || echo "Not found in middleware or model"
echo ""
echo "=== Check router.go usage of featureflags ==="
sed -n '20,40p' cmd/api/src/api/router/router.go | cat -nRepository: SpecterOps/BloodHound
Length of output: 2784
🏁 Script executed:
#!/bin/bash
# Check if appcfg.FeatureFlagService has IsEnabled method
echo "=== appcfg.FeatureFlagService definition ==="
sed -n '90,110p' cmd/api/src/model/appcfg/flag.go | cat -n
echo ""
echo "=== Check router.go for actual featureflags usage ==="
rg -n 'featureflags\.' cmd/api/src/api/router/router.go || echo "No direct featureflags usage found"Repository: SpecterOps/BloodHound
Length of output: 1216
🏁 Script executed:
#!/bin/bash
# Verify all usages of the enterprise featureflags in the codebase
echo "=== All featureflags.Service usages ==="
rg -n 'featureflags\.Service' --type go
echo ""
echo "=== Confirm no go.mod entry or replace for bloodhound-enterprise ==="
rg -n 'bloodhound-enterprise' go.mod || echo "No bloodhound-enterprise references in go.mod"Repository: SpecterOps/BloodHound
Length of output: 485
Resolve the unresolved featureflags module import—this is a critical build blocker.
The import github.com/specterops/bloodhound-enterprise/server/shared/featureflags at line 37 has no corresponding module entry or replace directive in go.mod. The codebase defines a compatible appcfg.FeatureFlagService interface in cmd/api/src/model/appcfg/flag.go with the same IsEnabled(ctx context.Context, key string) (bool, error) method signature.
Update both middleware.go and router.go:
- Remove the enterprise import (line 37 in middleware.go, line 23 in router.go)
- Add
"github.com/specterops/bloodhound/cmd/api/src/model/appcfg"to imports - Change function signatures:
featureflags.Service→appcfg.FeatureFlagServiceat middleware.go:301 and router.go:106
Suggested diff
- "github.com/specterops/bloodhound-enterprise/server/shared/featureflags"
"github.com/unrolled/secure"
"github.com/specterops/bloodhound/cmd/api/src/api"
"github.com/specterops/bloodhound/cmd/api/src/bhctx"
"github.com/specterops/bloodhound/cmd/api/src/config"
"github.com/specterops/bloodhound/cmd/api/src/model"
+ "github.com/specterops/bloodhound/cmd/api/src/model/appcfg"
"github.com/specterops/bloodhound/cmd/api/src/utils"
@@
-func FeatureFlagMiddleware(featureFlags featureflags.Service, flagKey string) mux.MiddlewareFunc {
+func FeatureFlagMiddleware(featureFlags appcfg.FeatureFlagService, flagKey string) mux.MiddlewareFunc {Also applies to router.go line 23 (remove import) and router.go line 106 (change signature).
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| "github.com/specterops/bloodhound-enterprise/server/shared/featureflags" | |
| "github.com/unrolled/secure" | |
| "github.com/specterops/bloodhound/cmd/api/src/api" | |
| "github.com/specterops/bloodhound/cmd/api/src/bhctx" | |
| "github.com/specterops/bloodhound/cmd/api/src/config" | |
| "github.com/specterops/bloodhound/cmd/api/src/model" | |
| "github.com/specterops/bloodhound/cmd/api/src/model/appcfg" | |
| "github.com/specterops/bloodhound/cmd/api/src/utils" |
Suggested change
| "github.com/specterops/bloodhound-enterprise/server/shared/featureflags" | |
| func FeatureFlagMiddleware(featureFlags appcfg.FeatureFlagService, flagKey string) mux.MiddlewareFunc { |
🧰 Tools
🪛 GitHub Actions: Run Go Unit Tests / 0_run-go-unit-tests.txt
[error] 37-37: go test failed: no required module provides package github.com/specterops/bloodhound-enterprise/server/shared/featureflags (import in cmd/api/src/api/middleware/middleware.go:37:2). Suggestion from Go: 'to add it:'
🪛 GitHub Actions: Run Go Unit Tests / run-go-unit-tests
[error] 37-37: Go test failed: no required module provides package github.com/specterops/bloodhound-enterprise/server/shared/featureflags; to add it: (go module dependency missing).
🤖 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 `@cmd/api/src/api/middleware/middleware.go` at line 37, The import statement
for github.com/specterops/bloodhound-enterprise/server/shared/featureflags is
unresolved because no corresponding module entry exists in go.mod. Replace this
enterprise module dependency with the compatible local appcfg interface by
removing the featureflags import from both middleware.go and router.go, adding
the import for github.com/specterops/bloodhound/cmd/api/src/model/appcfg
instead, and updating all function signatures that use featureflags.Service type
to use appcfg.FeatureFlagService type instead (look for these type references in
the function parameters around the middleware and router initialization
functions).
Source: Pipeline failures
coderabbitai
Bot
added
documentation
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
cmd/api/src/database/featureflags.go (1)
37-44: ⚡ Quick winReuse
GetFlagByKeyinsideIsEnabledto avoid query/error-path drift.
IsEnabledreimplements the same lookup path already present inGetFlagByKey. Reusing the existing method keeps behavior centralized and simpler.Proposed refactor
func (s *BloodhoundDB) IsEnabled(ctx context.Context, key string) (bool, error) { - var flag appcfg.FeatureFlag - if err := s.db.WithContext(ctx).Where("key = ?", key).First(&flag); err.Error != nil { - return false, CheckError(err) - } else { - return flag.Enabled, nil - } + flag, err := s.GetFlagByKey(ctx, key) + if err != nil { + return false, err + } + return flag.Enabled, nil }🤖 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 `@cmd/api/src/database/featureflags.go` around lines 37 - 44, The IsEnabled method duplicates the same database lookup logic that already exists in GetFlagByKey, which creates maintenance overhead and potential for behavior divergence. Refactor the IsEnabled method to call the existing GetFlagByKey method to retrieve the feature flag, then simply return the Enabled field from the result. This centralizes the lookup logic in one place and ensures consistent behavior and error handling across both methods.
🤖 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/internal/appdb/appdb.go`:
- Around line 112-114: The error handling in the "reading rows" error case is
using %s format specifier which does not properly wrap the error and prevents
callers from using errors.Is or errors.As on the underlying error. Change the
fmt.Errorf format string from "reading rows: %s" to "reading rows: %w" to
properly wrap and preserve the error chain, allowing callers to inspect
underlying pgx errors.
- Around line 73-75: The NewStore constructor function accepts a nil pgxQuerier
without validation, which defers failures to runtime when the Store is actually
used. Add a nil check at the beginning of the NewStore function to validate the
db parameter is not nil, either by panicking immediately with a descriptive
message for fail-fast behavior or by modifying the function signature to return
an error so wiring failures surface at startup rather than during operation.
In `@server/featureflags/internal/services/services.go`:
- Around line 26-32: The constants in the services.go file are documented as
mirroring keys defined in cmd/api/src/model/appcfg/flag.go, but FeatureAlerts
exists in services.go and is missing from the appcfg file while
FeatureOpenHoundSupport is present in both. Resolve this parity issue by either
adding the FeatureAlerts constant to cmd/api/src/model/appcfg/flag.go to match
the constants in services.go, removing FeatureAlerts from the constants block in
services.go if it is not actually used, or updating the comment in services.go
to accurately reflect which constants are mirrored and which are not.
---
Nitpick comments:
In `@cmd/api/src/database/featureflags.go`:
- Around line 37-44: The IsEnabled method duplicates the same database lookup
logic that already exists in GetFlagByKey, which creates maintenance overhead
and potential for behavior divergence. Refactor the IsEnabled method to call the
existing GetFlagByKey method to retrieve the feature flag, then simply return
the Enabled field from the result. This centralizes the lookup logic in one
place and ensures consistent behavior and error handling across both methods.
🪄 Autofix (Beta)
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: 7eaede80-2089-4efe-baad-dc7aef4453fe
📒 Files selected for processing (14)
cmd/api/src/api/constant.gocmd/api/src/api/middleware/middleware.gocmd/api/src/api/router/router.gocmd/api/src/database/featureflags.gocmd/api/src/database/mocks/db.gocmd/api/src/model/appcfg/flag.goserver/featureflags/internal/appdb/appdb.goserver/featureflags/internal/appdb/appdb_integration_test.goserver/featureflags/internal/appdb/appdb_test.goserver/featureflags/internal/services/mocks/database.goserver/featureflags/internal/services/services.goserver/featureflags/internal/services/services_test.goserver/featureflags/mocks/service.goserver/featureflags/register.go
✅ Files skipped from review due to trivial changes (2)
- cmd/api/src/database/mocks/db.go
- server/featureflags/mocks/service.go
🚧 Files skipped from review as they are similar to previous changes (3)
- cmd/api/src/api/middleware/middleware.go
- cmd/api/src/api/constant.go
- cmd/api/src/model/appcfg/flag.go
Comment on lines
+73
to
+75
| func NewStore(db pgxQuerier) *Store { | ||
| return &Store{db: db} | ||
| } |
Contributor
There was a problem hiding this comment.
Fail fast on nil querier in NewStore.
NewStore accepts a nil pgxQuerier, which allows constructing a Store that will panic later when GetFlagByKey calls s.db.Query(...). Add a constructor precondition to panic immediately (or change API to return error) so wiring failures surface at startup.
Suggested fix
func NewStore(db pgxQuerier) *Store {
+ if db == nil {
+ panic("feature-flag: store requires a non-nil pgx querier")
+ }
return &Store{db: db}
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| func NewStore(db pgxQuerier) *Store { | |
| return &Store{db: db} | |
| } | |
| func NewStore(db pgxQuerier) *Store { | |
| if db == nil { | |
| panic("feature-flag: store requires a non-nil pgx querier") | |
| } | |
| return &Store{db: db} | |
| } |
🤖 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/internal/appdb/appdb.go` around lines 73 - 75, The
NewStore constructor function accepts a nil pgxQuerier without validation, which
defers failures to runtime when the Store is actually used. Add a nil check at
the beginning of the NewStore function to validate the db parameter is not nil,
either by panicking immediately with a descriptive message for fail-fast
behavior or by modifying the function signature to return an error so wiring
failures surface at startup rather than during operation.
Comment on lines
+112
to
+114
| if err != nil { | ||
| return services.FeatureFlag{}, fmt.Errorf("reading rows: %s", err) | ||
| } |
Contributor
There was a problem hiding this comment.
Wrap row-read errors with %w to preserve error identity.
Using %s drops the unwrap chain, so callers cannot use errors.Is/As on underlying pgx errors.
Suggested fix
- return services.FeatureFlag{}, fmt.Errorf("reading rows: %s", err)
+ return services.FeatureFlag{}, fmt.Errorf("reading rows: %w", err)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| if err != nil { | |
| return services.FeatureFlag{}, fmt.Errorf("reading rows: %s", err) | |
| } | |
| if err != nil { | |
| return services.FeatureFlag{}, fmt.Errorf("reading rows: %w", err) | |
| } |
🤖 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/internal/appdb/appdb.go` around lines 112 - 114, The
error handling in the "reading rows" error case is using %s format specifier
which does not properly wrap the error and prevents callers from using errors.Is
or errors.As on the underlying error. Change the fmt.Errorf format string from
"reading rows: %s" to "reading rows: %w" to properly wrap and preserve the error
chain, allowing callers to inspect underlying pgx errors.
f60f8d7 to
d1f9fbf
Compare
Useinovski
approved these changes
Jul 2, 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.
Description
Adds uri constant for webhook id
Describe your changes in detail
Motivation and Context
Resolves BED-8016
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
alert_webhook_id) to improve how alert webhook requests are interpreted and handled.