chore: make stbernard/license tool aware of branch context BED-7086 #2218
Conversation
WalkthroughAdds an Args struct and CLI flags ( Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant CLI
participant LicenseRun as License.Run
participant Git as git (getBranchDiff)
participant Walker as FileWalker
participant FS as FileSystem
CLI->>LicenseRun: invoke with Args (--base-branch, --dry-run, --changes-only)
alt ChangesOnlyMode = true
LicenseRun->>Git: getBranchDiff(BaseBranchName)
Git-->>LicenseRun: changed file list
end
LicenseRun->>Walker: iterate workspace files
loop per-file
Walker->>FS: stat file (symlink/dir/extension)
alt symlink/dir/invalid extension
Walker-->>LicenseRun: log skipped
else if ChangesOnlyMode and file not in changeset
Walker-->>LicenseRun: log skipped (not in changeset)
else
Walker->>LicenseRun: enqueue file
alt DryRun = true
LicenseRun-->>Walker: log intended processing (skip write)
else
LicenseRun->>FS: write/update license header (0o644)
end
end
end
LicenseRun-->>CLI: complete
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
seanjSO
force-pushed
the
seanj/BED-7086
branch
from
1c6a4c5 to
c2d2cb2
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
Fix all issues with AI Agents 🤖
In @packages/go/stbernard/command/license/internal/cmd.go:
- Around line 66-74: The code dereferences args.FullRunMode and
args.BaseBranchName without nil checks; either (A) update the Args type to use
value fields (e.g., FullRunMode bool, BaseBranchName string) and adjust callers
to pass values, or (B) add defensive nil checks before dereferencing in the
conditional and when calling getBranchDiff: verify args != nil and
args.FullRunMode != nil (and args.BaseBranchName != nil) and return a clear
error if any are nil; update tests/constructors that build Args accordingly to
avoid panics.
In @packages/go/stbernard/command/license/internal/utils.go:
- Around line 223-249: In getBranchDiff, remove the "--cached" argument so the
git command compares the working tree against the base branch (capture staged +
unstaged changes) and make the parser robust by checking the split result before
indexing: when reading diffCmdStdout lines from the git output (the loop in
getBranchDiff), split on "\t" and ensure the resulting slice has at least 2
elements before appending [1] (skip or log lines that don't match the expected
format) to avoid a panic; update the exec.Command invocation to drop "--cached"
and keep the rest of the args the same.
🧹 Nitpick comments (1)
packages/go/stbernard/command/license/internal/cmd.go (1)
35-43: Consider using values instead of pointers for Args fields.The Args struct uses pointer fields for all three configuration options. While this pattern can indicate optional parameters, all three flags have default values in the CLI layer (lines 63-65 of license.go). Using plain values would eliminate nil pointer risks throughout the codebase and simplify the code.
🔎 Proposed refactor
In this file:
type Args struct { - // BaseBranchName defines which base branch should be compared against for deriving - // the changeset of the current branch - BaseBranchName *string - // DryRun will make license not make any changes, just print the files to be changed - DryRun *bool - // FullRunMode makes license run across the whole codebase instead of the current changeset - FullRunMode *bool + BaseBranchName string + DryRun bool + FullRunMode bool }In packages/go/stbernard/command/license/license.go:
func (s *command) Run() error { args := &license.Args{ - BaseBranchName: baseBranchName, - DryRun: dryRun, - FullRunMode: fullRunMode, + BaseBranchName: *baseBranchName, + DryRun: *dryRun, + FullRunMode: *fullRunMode, }Then update all usage sites to remove the
*dereference operators.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
packages/go/stbernard/command/license/internal/cmd.gopackages/go/stbernard/command/license/internal/utils.gopackages/go/stbernard/command/license/license.go
🧰 Additional context used
🧠 Learnings (3)
📚 Learning: 2025-05-29T18:26:28.958Z
Learnt from: superlinkx
Repo: SpecterOps/BloodHound PR: 1509
File: packages/go/stbernard/command/license/internal/utils.go:0-0
Timestamp: 2025-05-29T18:26:28.958Z
Learning: When rewriting existing files in Go, always preserve the original file permissions by using os.Stat() to get the file's Mode() before reading, then use that mode when writing the file back with os.WriteFile(). Avoid hardcoded permissions like 0666 or 0644 when modifying existing files.
Applied to files:
packages/go/stbernard/command/license/internal/cmd.go
📚 Learning: 2025-05-29T18:26:02.775Z
Learnt from: superlinkx
Repo: SpecterOps/BloodHound PR: 1509
File: packages/go/stbernard/command/license/internal/utils.go:0-0
Timestamp: 2025-05-29T18:26:02.775Z
Learning: When modifying existing files, preserve the original file permissions by using os.Stat() to get the file info and then using fileInfo.Mode().Perm() when writing the file back, rather than hardcoding permissions like 0644 or 0666.
Applied to files:
packages/go/stbernard/command/license/internal/cmd.go
📚 Learning: 2025-05-31T00:26:12.575Z
Learnt from: superlinkx
Repo: SpecterOps/BloodHound PR: 1509
File: packages/go/stbernard/command/license/internal/cmd.go:115-117
Timestamp: 2025-05-31T00:26:12.575Z
Learning: In Go filepath operations, `!info.IsDir()` already filters out directories effectively. Files without extensions are legitimate files (like Dockerfile, Makefile, justfile) and should not be filtered out solely based on lacking an extension. The absence of a file extension does not indicate that a path is a directory.
Applied to files:
packages/go/stbernard/command/license/internal/cmd.go
🧬 Code graph analysis (1)
packages/go/stbernard/command/license/license.go (1)
packages/go/stbernard/command/license/internal/cmd.go (2)
Args(35-43)Run(45-190)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: Build BloodHound Container Image / Build and Package Container
- GitHub Check: run-tests
- GitHub Check: run-analysis
🔇 Additional comments (3)
packages/go/stbernard/command/license/license.go (2)
33-37: LGTM!Package-level flag variables are appropriate here given the single-threaded command execution model.
63-65: LGTM!Flag registration and Args construction are correct. The flag package ensures the pointers returned by
cmd.String()andcmd.Bool()are never nil.Also applies to: 82-86
packages/go/stbernard/command/license/internal/cmd.go (1)
85-85: LGTM!The file mode syntax updates from
0644to0o644use the explicit octal prefix introduced in Go 1.13, which improves readability. These permissions are appropriate for new file creation.Also applies to: 216-216
seanjSO
force-pushed
the
seanj/BED-7086
branch
from
c2d2cb2 to
b3e197e
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
Fix all issues with AI Agents 🤖
In @packages/go/stbernard/command/license/internal/utils.go:
- Around line 223-249: In getBranchDiff, remove the "--cached" flag from the git
diff-index invocation so the command compares the working tree against the base
branch, and harden the scanner loop by using strings.SplitN(line, "\t", 2) (or
strings.Index to locate the tab) and only access the path if the split yields
two parts to avoid an index-out-of-range panic; after the scan loop, check
diffOutputScanner.Err() and return a wrapped error if non-nil so I/O errors are
not ignored, and ensure the git command's Start/Wait error handling remains
intact.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
packages/go/stbernard/command/license/internal/cmd.gopackages/go/stbernard/command/license/internal/utils.gopackages/go/stbernard/command/license/license.go
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/go/stbernard/command/license/internal/cmd.go
🧰 Additional context used
🧬 Code graph analysis (1)
packages/go/stbernard/command/license/license.go (1)
packages/go/stbernard/command/license/internal/cmd.go (2)
Args(35-43)Run(45-190)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
- GitHub Check: Build BloodHound Container Image / Build and Package Container
- GitHub Check: run-tests
- GitHub Check: run-analysis
- GitHub Check: build-ui
🔇 Additional comments (3)
packages/go/stbernard/command/license/license.go (3)
33-37: LGTM!Package-level variables for CLI flags are idiomatic in Go. The pointers returned by
flag.String/flag.Boolare guaranteed non-nil after parsing.
60-78: LGTM!Flag definitions are clear and align with the PR objectives:
-base-branchdefaults to"main"for sensible out-of-box behavior.-dry-runand-fulldefault tofalse, making branch-scoped, non-destructive runs the default.
81-91: LGTM!The
Argsstruct cleanly encapsulates the parsed flags and is passed to the internalRunfunction, keeping the public API stable while allowing the internal implementation to evolve.
seanjSO
force-pushed
the
seanj/BED-7086
branch
2 times, most recently
from
e1845a2 to
e5fe14d
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI Agents
In @packages/go/stbernard/command/license/internal/cmd.go:
- Around line 66-74: The code dereferences args.BaseBranchName when
args.FullRunMode is non-nil and false, which can panic if BaseBranchName is nil;
update the conditional to check args.BaseBranchName != nil before using
*args.BaseBranchName (e.g., if args.FullRunMode != nil && !*args.FullRunMode &&
args.BaseBranchName != nil) and if BaseBranchName is nil return a clear error
(or handle a sensible default) before calling getBranchDiff; ensure you
reference args.BaseBranchName, args.FullRunMode, getBranchDiff and
branchChangeset in the fix so there is no unchecked dereference.
🧹 Nitpick comments (1)
packages/go/stbernard/command/license/internal/cmd.go (1)
35-43: Consider Args validation or using value types.The Args struct uses pointer fields, which creates nil-pointer risks. While the current caller via the flag package (license.go lines 82-86) ensures these pointers are non-nil, tests or future code paths could construct Args with nil pointers.
The nil checks at lines 66, 166, and 172 provide some protection, but they also introduce unclear semantics: if
FullRunModeis nil, is that full mode or branch mode?💡 Two approaches to consider
Option 1 (Preferred): Use value types
type Args struct { - BaseBranchName *string - DryRun *bool - FullRunMode *bool + BaseBranchName string + DryRun bool + FullRunMode bool }Then update callers in license.go:
args := license.Args{ - BaseBranchName: baseBranchName, - DryRun: dryRun, - FullRunMode: fullRunMode, + BaseBranchName: *baseBranchName, + DryRun: *dryRun, + FullRunMode: *fullRunMode, }Option 2: Add validation at function entry
func Run(env environment.Environment, args Args) error { + if args.BaseBranchName == nil || args.DryRun == nil || args.FullRunMode == nil { + return fmt.Errorf("invalid args: all fields must be non-nil") + } + var ( ignoreDir = []string{...}
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
packages/go/stbernard/command/license/internal/cmd.gopackages/go/stbernard/command/license/internal/utils.gopackages/go/stbernard/command/license/license.go
🧰 Additional context used
🧠 Learnings (3)
📚 Learning: 2025-05-29T18:26:28.958Z
Learnt from: superlinkx
Repo: SpecterOps/BloodHound PR: 1509
File: packages/go/stbernard/command/license/internal/utils.go:0-0
Timestamp: 2025-05-29T18:26:28.958Z
Learning: When rewriting existing files in Go, always preserve the original file permissions by using os.Stat() to get the file's Mode() before reading, then use that mode when writing the file back with os.WriteFile(). Avoid hardcoded permissions like 0666 or 0644 when modifying existing files.
Applied to files:
packages/go/stbernard/command/license/internal/cmd.go
📚 Learning: 2025-05-29T18:26:02.775Z
Learnt from: superlinkx
Repo: SpecterOps/BloodHound PR: 1509
File: packages/go/stbernard/command/license/internal/utils.go:0-0
Timestamp: 2025-05-29T18:26:02.775Z
Learning: When modifying existing files, preserve the original file permissions by using os.Stat() to get the file info and then using fileInfo.Mode().Perm() when writing the file back, rather than hardcoding permissions like 0644 or 0666.
Applied to files:
packages/go/stbernard/command/license/internal/cmd.go
📚 Learning: 2025-05-31T00:26:12.575Z
Learnt from: superlinkx
Repo: SpecterOps/BloodHound PR: 1509
File: packages/go/stbernard/command/license/internal/cmd.go:115-117
Timestamp: 2025-05-31T00:26:12.575Z
Learning: In Go filepath operations, `!info.IsDir()` already filters out directories effectively. Files without extensions are legitimate files (like Dockerfile, Makefile, justfile) and should not be filtered out solely based on lacking an extension. The absence of a file extension does not indicate that a path is a directory.
Applied to files:
packages/go/stbernard/command/license/internal/cmd.go
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
- GitHub Check: Build BloodHound Container Image / Build and Package Container
- GitHub Check: run-analysis
- GitHub Check: run-tests
- GitHub Check: build-ui
🔇 Additional comments (2)
packages/go/stbernard/command/license/internal/cmd.go (1)
153-174: Well-structured filtering logic.The filtering cascade is well-organized:
- Symlink and directory exclusion with appropriate logging
- Extension-based filtering aligned with the disallowed list
- Branch changeset filtering when not in full-run mode
- Dry-run respected before enqueueing files
The debug logging at each decision point aids troubleshooting.
packages/go/stbernard/command/license/license.go (1)
33-37: Flag integration looks correct.The flag definitions at lines 63-65 and Args construction at lines 82-86 work correctly together. The flag package guarantees non-nil pointers after
Parse(), so the Args struct will receive valid pointers in normal execution.The coupling between flag pointers and Args pointer fields has been addressed in the comment on cmd.go regarding Args struct design.
Also applies to: 63-65, 82-86
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Fix all issues with AI Agents
In @packages/go/stbernard/command/license/internal/cmd.go:
- Line 73: Fix the syntax error in the slog.Debug call by adding the missing
closing parenthesis: locate the slog.Debug invocation that logs "Loaded branch
changeset" (which currently uses slog.Int("size", len(branchChangeset)) and
close the call properly so the statement compiles (ensure both the slog.Int call
and the slog.Debug call have their matching closing parentheses).
- Around line 173-176: The dry-run gating in cmd.go currently prevents files
from being enqueued (pathChan) when args.DryRun is true, so workers never call
isHeaderPresent and never detect candidates; instead always enqueue files to
pathChan in the worker loop and move the dry-run conditional into the write
path: thread the args.DryRun flag (or a bool) through processFile into writeFile
(utils.go) so processFile and isHeaderPresent run for every file but writeFile
only performs writes when DryRun is false (and when DryRun true, record/log the
would-be change). Update references: pathChan, args.DryRun, processFile,
isHeaderPresent, writeFile to implement this change.
- Around line 66-74: The code dereferences args.BaseBranchName inside the branch
handling block (when args.FullRunMode != nil && !*args.FullRunMode) without
checking for nil, which can panic; update the condition to verify
args.BaseBranchName != nil before using *args.BaseBranchName (or return a clear
error like "base branch name required when FullRunMode is false"), then call
getBranchDiff(*args.BaseBranchName) and assign branchChangeset; also fix the
slog.Debug call for "Loaded branch changeset" to close its parentheses properly
(slog.Debug(..., slog.Int("size", len(branchChangeset)))).
In @packages/go/stbernard/command/license/internal/utils.go:
- Around line 223-260: The current getBranchDiff uses git diff-index with the
--cached flag (diffCmd) which only returns staged changes; replace that
invocation with a git diff command that compares the working tree/HEAD to the
base branch (for example: exec.Command("git", "diff", "--name-status",
"--diff-filter=ACMR", baseBranchName+"...HEAD") or equivalent) so unstaged
working-tree changes are included, and adjust the parsing logic around
diffOutputScanner/recordParts if the output format changes (git diff
--name-status yields "<status>\t<path>") while keeping the existing error
handling around diffCmd.StdoutPipe(), diffCmd.Start(), scanner errors, and
diffCmd.Wait().
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
packages/go/stbernard/command/license/internal/cmd.gopackages/go/stbernard/command/license/internal/utils.gopackages/go/stbernard/command/license/license.go
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/go/stbernard/command/license/license.go
🧰 Additional context used
🧠 Learnings (3)
📚 Learning: 2025-05-29T18:26:28.958Z
Learnt from: superlinkx
Repo: SpecterOps/BloodHound PR: 1509
File: packages/go/stbernard/command/license/internal/utils.go:0-0
Timestamp: 2025-05-29T18:26:28.958Z
Learning: When rewriting existing files in Go, always preserve the original file permissions by using os.Stat() to get the file's Mode() before reading, then use that mode when writing the file back with os.WriteFile(). Avoid hardcoded permissions like 0666 or 0644 when modifying existing files.
Applied to files:
packages/go/stbernard/command/license/internal/cmd.go
📚 Learning: 2025-05-29T18:26:02.775Z
Learnt from: superlinkx
Repo: SpecterOps/BloodHound PR: 1509
File: packages/go/stbernard/command/license/internal/utils.go:0-0
Timestamp: 2025-05-29T18:26:02.775Z
Learning: When modifying existing files, preserve the original file permissions by using os.Stat() to get the file info and then using fileInfo.Mode().Perm() when writing the file back, rather than hardcoding permissions like 0644 or 0666.
Applied to files:
packages/go/stbernard/command/license/internal/cmd.go
📚 Learning: 2025-05-31T00:26:12.575Z
Learnt from: superlinkx
Repo: SpecterOps/BloodHound PR: 1509
File: packages/go/stbernard/command/license/internal/cmd.go:115-117
Timestamp: 2025-05-31T00:26:12.575Z
Learning: In Go filepath operations, `!info.IsDir()` already filters out directories effectively. Files without extensions are legitimate files (like Dockerfile, Makefile, justfile) and should not be filtered out solely based on lacking an extension. The absence of a file extension does not indicate that a path is a directory.
Applied to files:
packages/go/stbernard/command/license/internal/cmd.go
🪛 GitHub Actions: Run Go Unit Tests
packages/go/stbernard/command/license/internal/cmd.go
[error] 73-73: syntax error: unexpected newline in argument list; possibly missing comma or ) during 'go tool stbernard test -g -r' execution.
🪛 GitHub Actions: Run UI Tests
packages/go/stbernard/command/license/internal/cmd.go
[error] 73-73: Syntax error: unexpected newline in argument list; possibly missing comma or )
🪛 GitHub Actions: Static Code Analysis
packages/go/stbernard/command/license/internal/cmd.go
[error] 73-73: go tool stbernard deps: syntax error: unexpected newline in argument list; possibly missing comma or )
🪛 GitHub Check: run-analysis
packages/go/stbernard/command/license/internal/cmd.go
[failure] 73-73:
syntax error: unexpected newline in argument list; possibly missing comma or )
🪛 GitHub Check: run-go-unit-tests
packages/go/stbernard/command/license/internal/cmd.go
[failure] 73-73:
syntax error: unexpected newline in argument list; possibly missing comma or )
🪛 GitHub Check: run-tests
packages/go/stbernard/command/license/internal/cmd.go
[failure] 73-73:
syntax error: unexpected newline in argument list; possibly missing comma or )
🪛 golangci-lint (2.5.0)
packages/go/stbernard/command/license/internal/cmd.go
[major] 73-73: : # github.com/specterops/bloodhound/packages/go/stbernard/command/license/internal
packages/go/stbernard/command/license/internal/cmd.go:73:79: syntax error: unexpected newline in argument list; possibly missing comma or )
(typecheck)
[major] 73-73: missing ',' before newline in argument list
(typecheck)
[major] 74-74: expected operand, found '}'
(typecheck)
[major] 85-85: expected operand, found 'if'
(typecheck)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: Build BloodHound Container Image / Build and Package Container
- GitHub Check: build-ui
seanjSO
force-pushed
the
seanj/BED-7086
branch
2 times, most recently
from
3ff57c5 to
2b6b73b
Compare
seanjSO
force-pushed
the
seanj/BED-7086
branch
from
2b6b73b to
91b4f91
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In @packages/go/stbernard/command/license/internal/utils.go:
- Around line 223-260: getBranchDiff currently runs `git diff-index --cached
...` which only compares the index to baseBranchName; replace that with a
branch-to-branch diff using `git diff --name-only --diff-filter=ACMR
baseBranchName...HEAD` so you get all files changed on the current branch
relative to the base. Update the exec.Command call (diffCmd) to use "git",
"diff", "--name-only", "--diff-filter=ACMR", baseBranchName+"...HEAD", and
simplify the scanner loop (diffOutputScanner) to treat each scanned line as a
path (remove the tab-split/recordParts logic and related malformed-line
handling); keep the same error handling around Start(), Wait(), and scanning
errors and return the collected paths from getBranchDiff.
🧹 Nitpick comments (2)
packages/go/stbernard/command/license/internal/cmd.go (2)
117-120: LGTM: Dry-run implementation is correct.The logic properly skips file processing when
DryRunis enabled and logs the intent at debug level.💡 Optional: Add summary count for dry-run
Consider tracking and reporting the total number of files that would be processed:
var ( // ... existing vars ... filesProcessed int filesSkipped int ) // In the worker, after line 119: if args.DryRun != nil && *args.DryRun { slog.Debug("Would process file", slog.String("path", path)) errsMu.Lock() filesSkipped++ errsMu.Unlock() continue } // After successful processing (line 128): errsMu.Lock() filesProcessed++ errsMu.Unlock() // Before final log (line 195): if args.DryRun != nil && *args.DryRun { slog.Info("Dry-run complete", slog.Int("would_process", filesSkipped), slog.Duration("execution_time", diff)) } else { slog.Info("Processing complete", slog.Int("files_processed", filesProcessed), slog.Duration("execution_time", diff)) }
164-179: File filtering logic is sound, with helpful debug logging.The filtering correctly:
- Skips symbolic links (defensive against edge cases)
- Skips files with disallowed extensions
- Filters to changeset when not in full mode
- Logs skip decisions for debugging
Note: The
info.IsDir()check on line 164 is likely redundant becausefilepath.Walkalready skips directory contents when you returnfilepath.SkipDir(line 153), and directories wouldn't typically reach this point. However, keeping it as defensive coding is harmless and may help if Walk behavior changes or edge cases arise.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
packages/go/stbernard/command/license/internal/cmd.gopackages/go/stbernard/command/license/internal/utils.gopackages/go/stbernard/command/license/license.go
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/go/stbernard/command/license/license.go
🧰 Additional context used
🧠 Learnings (7)
📚 Learning: 2025-05-29T18:24:23.227Z
Learnt from: superlinkx
Repo: SpecterOps/BloodHound PR: 1509
File: packages/go/stbernard/command/license/internal/cmd.go:49-51
Timestamp: 2025-05-29T18:24:23.227Z
Learning: In the stbernard package, use slog for all logging prints instead of fmt.Printf/fmt.Fprintf to maintain consistency with the codebase's logging standards.
Applied to files:
packages/go/stbernard/command/license/internal/cmd.go
📚 Learning: 2025-06-25T17:52:33.291Z
Learnt from: superlinkx
Repo: SpecterOps/BloodHound PR: 1606
File: cmd/api/src/analysis/azure/post.go:33-35
Timestamp: 2025-06-25T17:52:33.291Z
Learning: In BloodHound Go code, prefer using explicit slog type functions like slog.Any(), slog.String(), slog.Int(), etc. over simple key-value pairs for structured logging. This provides better type safety and makes key-value pairs more visually distinct. For error types, use slog.Any("key", err) or slog.String("key", err.Error()).
Applied to files:
packages/go/stbernard/command/license/internal/cmd.go
📚 Learning: 2025-05-27T17:28:37.963Z
Learnt from: superlinkx
Repo: SpecterOps/BloodHound PR: 1503
File: cmd/api/src/services/upload/upload.go:70-84
Timestamp: 2025-05-27T17:28:37.963Z
Learning: The current pattern in upload.go of passing both io.ReadCloser and *os.File to validation functions creates unclear resource ownership and makes lifecycle management error-prone. The user identified this as a "logical mess" that should be refactored to separate validation concerns from file I/O management.
Applied to files:
packages/go/stbernard/command/license/internal/cmd.go
📚 Learning: 2025-05-29T18:26:28.958Z
Learnt from: superlinkx
Repo: SpecterOps/BloodHound PR: 1509
File: packages/go/stbernard/command/license/internal/utils.go:0-0
Timestamp: 2025-05-29T18:26:28.958Z
Learning: When rewriting existing files in Go, always preserve the original file permissions by using os.Stat() to get the file's Mode() before reading, then use that mode when writing the file back with os.WriteFile(). Avoid hardcoded permissions like 0666 or 0644 when modifying existing files.
Applied to files:
packages/go/stbernard/command/license/internal/cmd.go
📚 Learning: 2025-05-29T18:26:02.775Z
Learnt from: superlinkx
Repo: SpecterOps/BloodHound PR: 1509
File: packages/go/stbernard/command/license/internal/utils.go:0-0
Timestamp: 2025-05-29T18:26:02.775Z
Learning: When modifying existing files, preserve the original file permissions by using os.Stat() to get the file info and then using fileInfo.Mode().Perm() when writing the file back, rather than hardcoding permissions like 0644 or 0666.
Applied to files:
packages/go/stbernard/command/license/internal/cmd.go
📚 Learning: 2025-05-27T17:28:46.899Z
Learnt from: superlinkx
Repo: SpecterOps/BloodHound PR: 1503
File: cmd/api/src/services/graphify/tasks.go:114-118
Timestamp: 2025-05-27T17:28:46.899Z
Learning: When processing files in a loop where resources need to be cleaned up, extract the file processing into a separate function that uses `defer` for resource management rather than trying to handle cleanup in the loop body. This ensures proper resource cleanup while maintaining error handling robustness.
Applied to files:
packages/go/stbernard/command/license/internal/cmd.go
📚 Learning: 2025-05-31T00:26:12.575Z
Learnt from: superlinkx
Repo: SpecterOps/BloodHound PR: 1509
File: packages/go/stbernard/command/license/internal/cmd.go:115-117
Timestamp: 2025-05-31T00:26:12.575Z
Learning: In Go filepath operations, `!info.IsDir()` already filters out directories effectively. Files without extensions are legitimate files (like Dockerfile, Makefile, justfile) and should not be filtered out solely based on lacking an extension. The absence of a file extension does not indicate that a path is a directory.
Applied to files:
packages/go/stbernard/command/license/internal/cmd.go
🔇 Additional comments (3)
packages/go/stbernard/command/license/internal/cmd.go (3)
35-43: LGTM: Well-designed Args struct.The use of pointer types for flag fields allows distinguishing between "not set" (nil) and explicitly set to false/empty, which is appropriate for CLI flag handling.
90-90: LGTM: Modern octal notation for new file permissions.The update from
0644to0o644uses Go's modern octal literal syntax (introduced in Go 1.13). The0o644permission is appropriate for newly created files.Based on learnings, these permission values are correct because both are new file creations. The learnings note that when modifying existing files, original permissions should be preserved—but that guidance doesn't apply here.
Also applies to: 223-223
67-79: The implementation is correct. The concern in the original comment is based on a misunderstanding of howflag.Bool()works.
cmd.Bool("full", false, ...)on line 65 of license.go returns a non-nil pointer to a bool value, never nil. Therefore:
- Default (no
-fullflag):*fullRunMode = false→ condition on line 67 is true → changeset is loaded → only changed files processed ✓- With
-fullflag:*fullRunMode = true→ condition on line 67 is false → changeset is NOT loaded → all files processed ✓This matches the stated behavior: filtered mode by default, with
-fullas an opt-in for whole-codebase scans.
| func getBranchDiff(baseBranchName string) ([]string, error) { | ||
| // Exec git to get the difference between the currently checked out branch and | ||
| // the base branch. | ||
| diffCmd := exec.Command("git", "diff-index", "--cached", "--diff-filter=ACMR", baseBranchName) | ||
| diffCmdStdout, err := diffCmd.StdoutPipe() | ||
| if err != nil { | ||
| return nil, fmt.Errorf("could not get command stdout pipe: %w", err) | ||
| } | ||
|
|
||
| if err := diffCmd.Start(); err != nil { | ||
| return nil, fmt.Errorf("error running `git diff-index`: %w", err) | ||
| } | ||
|
|
||
| paths := []string{} | ||
| diffOutputScanner := bufio.NewScanner(diffCmdStdout) | ||
| for diffOutputScanner.Scan() { | ||
| line := diffOutputScanner.Text() | ||
| // :<srcMode> <dstMode> <srcHash> <dstHash> <status>\t<path> | ||
| recordParts := strings.Split(line, "\t") | ||
| if len(recordParts) != 2 { | ||
| // One-shot a Wait() to ensure the process gets cleaned up before bailing | ||
| diffCmd.Wait() | ||
| return nil, fmt.Errorf("`git diff-index` returned malformed status line: %s", line) | ||
| } | ||
|
|
||
| paths = append(paths, recordParts[1]) | ||
| } | ||
|
|
||
| if err := diffOutputScanner.Err(); err != nil { | ||
| return nil, fmt.Errorf("error scanning `git diff-index` output: %w", err) | ||
| } | ||
|
|
||
| if err := diffCmd.Wait(); err != nil { | ||
| return nil, fmt.Errorf("error waiting for `git diff-index` to exit: %w", err) | ||
| } | ||
|
|
||
| return paths, nil | ||
| } |
There was a problem hiding this comment.
Critical: --cached flag compares index to base branch, not branch-to-branch changes.
The --cached flag makes git diff-index compare the index (staging area) to baseBranchName, not the current branch's commits to the base branch. This contradicts the PR objective: "scan files added, copied, renamed, or modified in the current branch relative to a base branch."
In practice, this will only detect staged changes, meaning:
- In a typical development workflow with an empty index, no files will be processed.
- Changed but unstaged files in the branch will be ignored.
- The tool won't fulfill its purpose of processing branch-relative changes.
🔧 Recommended fix: Compare branch HEADs
-func getBranchDiff(baseBranchName string) ([]string, error) {
- // Exec git to get the difference between the currently checked out branch and
- // the base branch.
- diffCmd := exec.Command("git", "diff-index", "--cached", "--diff-filter=ACMR", baseBranchName)
+func getBranchDiff(baseBranchName string) ([]string, error) {
+ // Exec git to get the difference between the current branch and the base branch.
+ diffCmd := exec.Command("git", "diff", "--name-only", "--diff-filter=ACMR", baseBranchName+"...HEAD")
diffCmdStdout, err := diffCmd.StdoutPipe()
if err != nil {
return nil, fmt.Errorf("could not get command stdout pipe: %w", err)
}
if err := diffCmd.Start(); err != nil {
- return nil, fmt.Errorf("error running `git diff-index`: %w", err)
+ return nil, fmt.Errorf("error running `git diff`: %w", err)
}
paths := []string{}
diffOutputScanner := bufio.NewScanner(diffCmdStdout)
for diffOutputScanner.Scan() {
- line := diffOutputScanner.Text()
- // :<srcMode> <dstMode> <srcHash> <dstHash> <status>\t<path>
- recordParts := strings.Split(line, "\t")
- if len(recordParts) != 2 {
- // One-shot a Wait() to ensure the process gets cleaned up before bailing
- diffCmd.Wait()
- return nil, fmt.Errorf("`git diff-index` returned malformed status line: %s", line)
- }
-
- paths = append(paths, recordParts[1])
+ paths = append(paths, diffOutputScanner.Text())
}
if err := diffOutputScanner.Err(); err != nil {
- return nil, fmt.Errorf("error scanning `git diff-index` output: %w", err)
+ return nil, fmt.Errorf("error scanning `git diff` output: %w", err)
}
if err := diffCmd.Wait(); err != nil {
- return nil, fmt.Errorf("error waiting for `git diff-index` to exit: %w", err)
+ return nil, fmt.Errorf("error waiting for `git diff` to exit: %w", err)
}
return paths, nil
}The --name-only flag outputs only filenames (one per line), eliminating the need to parse tab-delimited records. The triple-dot syntax (baseBranch...HEAD) shows files that differ between the merge-base and HEAD, which correctly captures branch changes.
🤖 Prompt for AI Agents
In @packages/go/stbernard/command/license/internal/utils.go around lines 223 -
260, getBranchDiff currently runs `git diff-index --cached ...` which only
compares the index to baseBranchName; replace that with a branch-to-branch diff
using `git diff --name-only --diff-filter=ACMR baseBranchName...HEAD` so you get
all files changed on the current branch relative to the base. Update the
exec.Command call (diffCmd) to use "git", "diff", "--name-only",
"--diff-filter=ACMR", baseBranchName+"...HEAD", and simplify the scanner loop
(diffOutputScanner) to treat each scanned line as a path (remove the
tab-split/recordParts logic and related malformed-line handling); keep the same
error handling around Start(), Wait(), and scanning errors and return the
collected paths from getBranchDiff.
There was a problem hiding this comment.
I'm thinking that license should only check/apply changes to files that are currently staged and ignore anything that is not explicitly pulled in to the index.
There was a problem hiding this comment.
@seanjSO, thank you for clarifying! I misunderstood the intent—I now see that limiting the license check to staged files is the desired behavior. This makes sense for a workflow where you explicitly control which files get processed by staging them first. I appreciate the explanation!
✏️ Learnings added
Learnt from: seanjSO
Repo: SpecterOps/BloodHound PR: 2218
File: packages/go/stbernard/command/license/internal/utils.go:223-260
Timestamp: 2026-01-08T20:15:42.718Z
Learning: In the BloodHound stbernard license tool (packages/go/stbernard/command/license/), the getBranchDiff function intentionally uses `git diff-index --cached` to compare only staged (indexed) files against the base branch, not all branch changes. This design allows users to explicitly control which files are processed by staging them.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
There was a problem hiding this comment.
That doesn't match the intent of the language used in the ticket or the names in the code. It also would lead to engineers missing checks if they don't run them every commit (many run during PFC, which is typically right before or after sending a PR). I'd rather see a full check against the branch for this feature
There was a problem hiding this comment.
Seems like the humans are having a chat. I'll hop back into my burrow for now. If you need me again, just tag @coderabbitai in a new comment, and I'll come hopping out!
seanjSO
force-pushed
the
seanj/BED-7086
branch
from
91b4f91 to
3dc5d71
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In @packages/go/stbernard/command/license/license.go:
- Around line 63-65: The flag fullRunMode is currently created with
cmd.Bool("full", true, ...) making full-codebase mode the default; change its
default to false so the tool scans only changed files by default
(cmd.Bool("full", false, ...)) and ensure the help text for the "full" flag
still states that full repository scanning is opt-in; update any related
references to fullRunMode if they assume true by default.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
packages/go/stbernard/command/license/internal/cmd.gopackages/go/stbernard/command/license/internal/utils.gopackages/go/stbernard/command/license/license.go
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/go/stbernard/command/license/internal/utils.go
🧰 Additional context used
🧠 Learnings (7)
📚 Learning: 2025-05-29T18:24:23.227Z
Learnt from: superlinkx
Repo: SpecterOps/BloodHound PR: 1509
File: packages/go/stbernard/command/license/internal/cmd.go:49-51
Timestamp: 2025-05-29T18:24:23.227Z
Learning: In the stbernard package, use slog for all logging prints instead of fmt.Printf/fmt.Fprintf to maintain consistency with the codebase's logging standards.
Applied to files:
packages/go/stbernard/command/license/internal/cmd.go
📚 Learning: 2025-06-25T17:52:33.291Z
Learnt from: superlinkx
Repo: SpecterOps/BloodHound PR: 1606
File: cmd/api/src/analysis/azure/post.go:33-35
Timestamp: 2025-06-25T17:52:33.291Z
Learning: In BloodHound Go code, prefer using explicit slog type functions like slog.Any(), slog.String(), slog.Int(), etc. over simple key-value pairs for structured logging. This provides better type safety and makes key-value pairs more visually distinct. For error types, use slog.Any("key", err) or slog.String("key", err.Error()).
Applied to files:
packages/go/stbernard/command/license/internal/cmd.go
📚 Learning: 2025-05-27T17:28:37.963Z
Learnt from: superlinkx
Repo: SpecterOps/BloodHound PR: 1503
File: cmd/api/src/services/upload/upload.go:70-84
Timestamp: 2025-05-27T17:28:37.963Z
Learning: The current pattern in upload.go of passing both io.ReadCloser and *os.File to validation functions creates unclear resource ownership and makes lifecycle management error-prone. The user identified this as a "logical mess" that should be refactored to separate validation concerns from file I/O management.
Applied to files:
packages/go/stbernard/command/license/internal/cmd.go
📚 Learning: 2025-05-29T18:26:28.958Z
Learnt from: superlinkx
Repo: SpecterOps/BloodHound PR: 1509
File: packages/go/stbernard/command/license/internal/utils.go:0-0
Timestamp: 2025-05-29T18:26:28.958Z
Learning: When rewriting existing files in Go, always preserve the original file permissions by using os.Stat() to get the file's Mode() before reading, then use that mode when writing the file back with os.WriteFile(). Avoid hardcoded permissions like 0666 or 0644 when modifying existing files.
Applied to files:
packages/go/stbernard/command/license/internal/cmd.go
📚 Learning: 2025-05-29T18:26:02.775Z
Learnt from: superlinkx
Repo: SpecterOps/BloodHound PR: 1509
File: packages/go/stbernard/command/license/internal/utils.go:0-0
Timestamp: 2025-05-29T18:26:02.775Z
Learning: When modifying existing files, preserve the original file permissions by using os.Stat() to get the file info and then using fileInfo.Mode().Perm() when writing the file back, rather than hardcoding permissions like 0644 or 0666.
Applied to files:
packages/go/stbernard/command/license/internal/cmd.go
📚 Learning: 2025-05-27T17:28:46.899Z
Learnt from: superlinkx
Repo: SpecterOps/BloodHound PR: 1503
File: cmd/api/src/services/graphify/tasks.go:114-118
Timestamp: 2025-05-27T17:28:46.899Z
Learning: When processing files in a loop where resources need to be cleaned up, extract the file processing into a separate function that uses `defer` for resource management rather than trying to handle cleanup in the loop body. This ensures proper resource cleanup while maintaining error handling robustness.
Applied to files:
packages/go/stbernard/command/license/internal/cmd.go
📚 Learning: 2025-05-31T00:26:12.575Z
Learnt from: superlinkx
Repo: SpecterOps/BloodHound PR: 1509
File: packages/go/stbernard/command/license/internal/cmd.go:115-117
Timestamp: 2025-05-31T00:26:12.575Z
Learning: In Go filepath operations, `!info.IsDir()` already filters out directories effectively. Files without extensions are legitimate files (like Dockerfile, Makefile, justfile) and should not be filtered out solely based on lacking an extension. The absence of a file extension does not indicate that a path is a directory.
Applied to files:
packages/go/stbernard/command/license/internal/cmd.go
🧬 Code graph analysis (1)
packages/go/stbernard/command/license/license.go (1)
packages/go/stbernard/command/license/internal/cmd.go (2)
Args(35-43)Run(45-197)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
- GitHub Check: Build BloodHound Container Image / Build and Package Container
- GitHub Check: build-ui
- GitHub Check: run-analysis
- GitHub Check: run-tests
🔇 Additional comments (7)
packages/go/stbernard/command/license/license.go (1)
82-87: Correct Args construction and invocation.The Args struct is properly populated with all three flag values and passed to the internal Run function.
packages/go/stbernard/command/license/internal/cmd.go (6)
35-43: Well-designed Args struct with clear documentation.The pointer types allow for proper nil-checking to distinguish between unset and explicitly-set values, and the comments clearly explain each field's purpose.
56-57: Sensible default values for branch-aware mode.The default branch name of "main" is appropriate, and the empty changeset slice will be populated when not in full-run mode.
67-79: Correct branch changeset loading with proper error handling.The logic correctly loads the branch changeset only when not in full-run mode, uses the provided or default base branch name, and returns descriptive errors. The debug logging follows the codebase's structured logging standards.
90-90: Good practice: Explicit octal notation.The change from
0644to0o644adds the explicit octal prefix, improving code clarity. Since this creates a new file rather than modifying an existing one, a fixed permission is appropriate.
117-120: Correct dry-run implementation with appropriate logging.The dry-run logic properly prevents file modification and uses structured logging to indicate which files would be processed.
163-179: Effective skip logic with helpful debug logging.The three skip conditions (symlinks/directories, disallowed extensions, and changeset filtering) are correctly implemented. The debug logging provides visibility into filtering decisions and follows structured logging best practices. The changeset check correctly applies only when not in full-run mode.
seanjSO
force-pushed
the
seanj/BED-7086
branch
from
3dc5d71 to
e57a7f3
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
packages/go/stbernard/command/license/internal/utils.go (1)
223-260: Consider validating git availability and adding command timeout.The
getBranchDifffunction executesgit diff-indexwithout first checking that git is available or that the current directory is within a git repository. If git is unavailable or the command hangs, users will experience unclear errors or timeouts.Consider:
- Adding a validation check that git is available (e.g.,
exec.LookPath("git"))- Using
context.WithTimeoutto prevent indefinite hangs on the git command♻️ Recommended additions
Before executing the git command, validate git availability:
func getBranchDiff(baseBranchName string) ([]string, error) { + // Validate git is available + if _, err := exec.LookPath("git"); err != nil { + return nil, fmt.Errorf("git command not found: %w", err) + } + // Exec git to get the difference between the currently checked out branch and // the base branch. diffCmd := exec.Command("git", "diff-index", "--cached", "--diff-filter=ACMR", baseBranchName)For timeout protection, consider adding context:
ctx, cancel := context.WithTimeout(context.Background(), 30*time.Second) defer cancel() diffCmd := exec.CommandContext(ctx, "git", "diff-index", "--cached", "--diff-filter=ACMR", baseBranchName)packages/go/stbernard/command/license/internal/cmd.go (1)
164-179: Consider optimizing changeset lookup for large repositories.The current implementation uses
slices.Containsto check if each file is in thebranchChangeset, which is O(n) per file. For repositories with many files and large changesets, this could accumulate.♻️ Optional optimization
Convert
branchChangesetfrom[]stringtomap[string]struct{}for O(1) lookup:var ( ignoreDir = []string{...} ignorePaths = []string{...} disallowedExtensions = []string{...} now = time.Now() baseBranchName = "main" - branchChangeset = []string{} + branchChangeset = map[string]struct{}{} ... ) if args.ChangesOnlyMode != nil && *args.ChangesOnlyMode { var err error if args.BaseBranchName != nil { baseBranchName = *args.BaseBranchName } - branchChangeset, err = getBranchDiff(baseBranchName) + paths, err := getBranchDiff(baseBranchName) if err != nil { return fmt.Errorf("could not load branch changeset from git: %w", err) } + + // Convert to map for O(1) lookup + for _, p := range paths { + branchChangeset[p] = struct{}{} + } slog.Debug("Loaded branch changeset", slog.Int("size", len(branchChangeset))) } // Later in the walk function: -if args.ChangesOnlyMode != nil && *args.ChangesOnlyMode && !slices.Contains(branchChangeset, relPath) { +_, inChangeset := branchChangeset[relPath] +if args.ChangesOnlyMode != nil && *args.ChangesOnlyMode && !inChangeset { slog.Debug("Skipped file: not in changeset", slog.String("path", relPath)) return nil }
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
packages/go/stbernard/command/license/internal/cmd.gopackages/go/stbernard/command/license/internal/utils.gopackages/go/stbernard/command/license/license.go
🧰 Additional context used
🧠 Learnings (9)
📓 Common learnings
Learnt from: seanjSO
Repo: SpecterOps/BloodHound PR: 2218
File: packages/go/stbernard/command/license/internal/utils.go:223-260
Timestamp: 2026-01-08T20:15:42.718Z
Learning: In the BloodHound stbernard license tool (packages/go/stbernard/command/license/), the getBranchDiff function intentionally uses `git diff-index --cached` to compare only staged (indexed) files against the base branch, not all branch changes. This design allows users to explicitly control which files are processed by staging them.
📚 Learning: 2026-01-08T20:15:42.718Z
Learnt from: seanjSO
Repo: SpecterOps/BloodHound PR: 2218
File: packages/go/stbernard/command/license/internal/utils.go:223-260
Timestamp: 2026-01-08T20:15:42.718Z
Learning: In the BloodHound stbernard license tool (packages/go/stbernard/command/license/), the getBranchDiff function intentionally uses `git diff-index --cached` to compare only staged (indexed) files against the base branch, not all branch changes. This design allows users to explicitly control which files are processed by staging them.
Applied to files:
packages/go/stbernard/command/license/internal/cmd.gopackages/go/stbernard/command/license/internal/utils.gopackages/go/stbernard/command/license/license.go
📚 Learning: 2025-05-29T18:24:23.227Z
Learnt from: superlinkx
Repo: SpecterOps/BloodHound PR: 1509
File: packages/go/stbernard/command/license/internal/cmd.go:49-51
Timestamp: 2025-05-29T18:24:23.227Z
Learning: In the stbernard package, use slog for all logging prints instead of fmt.Printf/fmt.Fprintf to maintain consistency with the codebase's logging standards.
Applied to files:
packages/go/stbernard/command/license/internal/cmd.go
📚 Learning: 2025-06-25T17:52:33.291Z
Learnt from: superlinkx
Repo: SpecterOps/BloodHound PR: 1606
File: cmd/api/src/analysis/azure/post.go:33-35
Timestamp: 2025-06-25T17:52:33.291Z
Learning: In BloodHound Go code, prefer using explicit slog type functions like slog.Any(), slog.String(), slog.Int(), etc. over simple key-value pairs for structured logging. This provides better type safety and makes key-value pairs more visually distinct. For error types, use slog.Any("key", err) or slog.String("key", err.Error()).
Applied to files:
packages/go/stbernard/command/license/internal/cmd.go
📚 Learning: 2025-05-27T17:28:37.963Z
Learnt from: superlinkx
Repo: SpecterOps/BloodHound PR: 1503
File: cmd/api/src/services/upload/upload.go:70-84
Timestamp: 2025-05-27T17:28:37.963Z
Learning: The current pattern in upload.go of passing both io.ReadCloser and *os.File to validation functions creates unclear resource ownership and makes lifecycle management error-prone. The user identified this as a "logical mess" that should be refactored to separate validation concerns from file I/O management.
Applied to files:
packages/go/stbernard/command/license/internal/cmd.go
📚 Learning: 2025-05-29T18:26:28.958Z
Learnt from: superlinkx
Repo: SpecterOps/BloodHound PR: 1509
File: packages/go/stbernard/command/license/internal/utils.go:0-0
Timestamp: 2025-05-29T18:26:28.958Z
Learning: When rewriting existing files in Go, always preserve the original file permissions by using os.Stat() to get the file's Mode() before reading, then use that mode when writing the file back with os.WriteFile(). Avoid hardcoded permissions like 0666 or 0644 when modifying existing files.
Applied to files:
packages/go/stbernard/command/license/internal/cmd.go
📚 Learning: 2025-05-29T18:26:02.775Z
Learnt from: superlinkx
Repo: SpecterOps/BloodHound PR: 1509
File: packages/go/stbernard/command/license/internal/utils.go:0-0
Timestamp: 2025-05-29T18:26:02.775Z
Learning: When modifying existing files, preserve the original file permissions by using os.Stat() to get the file info and then using fileInfo.Mode().Perm() when writing the file back, rather than hardcoding permissions like 0644 or 0666.
Applied to files:
packages/go/stbernard/command/license/internal/cmd.go
📚 Learning: 2025-05-27T17:28:46.899Z
Learnt from: superlinkx
Repo: SpecterOps/BloodHound PR: 1503
File: cmd/api/src/services/graphify/tasks.go:114-118
Timestamp: 2025-05-27T17:28:46.899Z
Learning: When processing files in a loop where resources need to be cleaned up, extract the file processing into a separate function that uses `defer` for resource management rather than trying to handle cleanup in the loop body. This ensures proper resource cleanup while maintaining error handling robustness.
Applied to files:
packages/go/stbernard/command/license/internal/cmd.go
📚 Learning: 2025-05-31T00:26:12.575Z
Learnt from: superlinkx
Repo: SpecterOps/BloodHound PR: 1509
File: packages/go/stbernard/command/license/internal/cmd.go:115-117
Timestamp: 2025-05-31T00:26:12.575Z
Learning: In Go filepath operations, `!info.IsDir()` already filters out directories effectively. Files without extensions are legitimate files (like Dockerfile, Makefile, justfile) and should not be filtered out solely based on lacking an extension. The absence of a file extension does not indicate that a path is a directory.
Applied to files:
packages/go/stbernard/command/license/internal/cmd.go
🧬 Code graph analysis (1)
packages/go/stbernard/command/license/license.go (1)
packages/go/stbernard/command/license/internal/cmd.go (2)
Args(35-43)Run(45-197)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
- GitHub Check: Build BloodHound Container Image / Build and Package Container
- GitHub Check: build-ui
- GitHub Check: run-analysis
- GitHub Check: run-tests
🔇 Additional comments (6)
packages/go/stbernard/command/license/license.go (2)
33-37: LGTM!The flag definitions follow standard Go CLI patterns with sensible defaults and clear descriptions.
Also applies to: 63-65
82-87: LGTM!The Args construction and Run invocation are clean and properly pass the flag values to the internal implementation.
packages/go/stbernard/command/license/internal/cmd.go (4)
35-43: LGTM!The Args struct uses pointer fields to distinguish between unset and false values, which is appropriate for optional CLI flags where nil indicates the flag was not provided.
67-79: LGTM!The ChangesOnlyMode initialization correctly loads the branch changeset via
getBranchDiffwhen enabled, with appropriate error handling and debug logging. Based on learnings, the use of--cachedin the underlying git command is intentional to allow users to control which files are processed by staging them.
90-90: LGTM!The use of
0o644permissions is appropriate for creating new license files. ThewriteFilefunction properly preserves original permissions when modifying existing files by usingO_RDWRwithout mode changes.Also applies to: 223-223
117-120: LGTM!The dry-run implementation correctly skips file modifications while logging intended actions, providing a safe preview mode.
superlinkx
left a comment
superlinkx
left a comment
There was a problem hiding this comment.
I'd like to reexamine the code rabbit note tomorrow with you. I've added some additional notes for what might be improved, can also go over those in the morning if you'd like, they shouldn't require much in the way of changes. Overall I love the cleanup of existing logic, it was definitely needed
| func (s *command) Parse(cmdIndex int) error { | ||
| cmd := flag.NewFlagSet(Name, flag.ExitOnError) | ||
|
|
||
| baseBranchName = cmd.String("base-branch", "main", "Base branch to compare against for change tracking") |
There was a problem hiding this comment.
Using StringVar and BoolVar will allow you to not use pointers for the types, which will make other code safer and simpler. I would avoid having your arguments to the code use pointer form and instead allow the defaults set in your flags here be the zero value
| type Args struct { | ||
| // BaseBranchName defines which base branch should be compared against for deriving | ||
| // the changeset of the current branch | ||
| BaseBranchName *string |
There was a problem hiding this comment.
As noted, using StringVar/BoolVar upstream will allow you to avoid pointers, which will simplify other aspects here
| pathChan = make(chan string, numWorkers) | ||
| ) | ||
|
|
||
| if args.ChangesOnlyMode != nil && *args.ChangesOnlyMode { |
There was a problem hiding this comment.
The nil check would go away without the pointers, and you won't have to dereference the value either
| func getBranchDiff(baseBranchName string) ([]string, error) { | ||
| // Exec git to get the difference between the currently checked out branch and | ||
| // the base branch. | ||
| diffCmd := exec.Command("git", "diff-index", "--cached", "--diff-filter=ACMR", baseBranchName) |
There was a problem hiding this comment.
We have a command runner function in St Bernard that can do some of the wiring you're doing here. Is there a reason we needed to manually manage this command?
Description
Gives stbernard's license tool the ability scan/update only the files that are added/copied/renamed/modified in the current branch, with respect to the base branch (main, by default).
Adds three flags:
-changes-only: Runs against the changes from the current branch instead of the full codebase-dry-run: Don't actually scan/update any files, only collects the files that would otherwise be changed-base-branch [name]: Set the branch the current index should be compared toMotivation and Context
Resolves BED-7086
Why is this change required? What problem does it solve?
licensebeing a part ofjust prepare-for-codereviewcan modify many files and will dirty the working tree each run. This makes license prefer to just change the files associated with a branch as opposed to the entire codebase, with the full-codebase license update being put behind an optionHow 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.
Using
-dry-runflag oflicensew/SB_LOG_LEVEL=DEBUGto see which files are selected for scanning under various conditions, for example:Screenshots (optional):
Types of changes
Checklist:
Summary by CodeRabbit
New Features
Improvements
✏️ Tip: You can customize this high-level summary in your review settings.