Skip to content

Conversation

@seanjSO
Copy link
Contributor

@seanjSO seanjSO commented Jan 5, 2026

Description

Modifies default behavior of stbernard's license tool to 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:

  • -full: Disables the branch context filter and runs against the whole codebase (previous behavior)
  • -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 to

Motivation and Context

Resolves BED-7086

Why is this change required? What problem does it solve?

license being a part of just prepare-for-codereview can 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 option

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.

Using -dry-run flag of license w/ SB_LOG_LEVEL=DEBUG to see which files are selected for scanning under various conditions, for example:

SB_LOG_LEVEL=DEBUG go tool stbernard license -dry-run 2>&1 | grep -v 'skipped'
time=2026-01-05T17:18:10.453-06:00 level=DEBUG message="Loaded branch changeset" changeset="[packages/go/stbernard/command/license/internal/cmd.go packages/go/stbernard/command/license/internal/utils.go packages/go/stbernard/command/license/license.go]"
time=2026-01-05T17:18:10.467-06:00 level=DEBUG message="add file to scan queue" path=/Users/sjohnson/code/BloodHound/packages/go/stbernard/command/license/internal/cmd.go
time=2026-01-05T17:18:10.467-06:00 level=DEBUG message="add file to scan queue" path=/Users/sjohnson/code/BloodHound/packages/go/stbernard/command/license/internal/utils.go
time=2026-01-05T17:18:10.467-06:00 level=DEBUG message="add file to scan queue" path=/Users/sjohnson/code/BloodHound/packages/go/stbernard/command/license/license.go
time=2026-01-05T17:18:10.481-06:00 level=INFO message="Running scans on bhce" execution_time=33.525084ms
time=2026-01-05T17:18:10.481-06:00 level=INFO message="Command `license` completed successfully"

Screenshots (optional):

Types of changes

  • Chore (a change that does not modify the application functionality)

Checklist:

Summary by CodeRabbit

  • New Features

    • Added CLI flags: --base-branch, --dry-run, and --full to control scope and behavior.
    • Selective file scanning based on branch changes when not in full-run mode; dry-run previews enqueued files.
  • Improvements

    • Symlinks, directories, and disallowed extensions are now skipped with explanatory logs.
    • Enhanced logging for scan queueing and skip reasons.
    • Updated file permissions for generated LICENSE headers.

✏️ Tip: You can customize this high-level summary in your review settings.

@seanjSO seanjSO self-assigned this Jan 5, 2026
@seanjSO seanjSO added the tooling This updates developer tooling label Jan 5, 2026
@coderabbitai
Copy link
Contributor

coderabbitai bot commented Jan 5, 2026

Important

Review skipped

Draft detected.

Please check the settings in the CodeRabbit UI or the .coderabbit.yaml file in this repository. To trigger a single review, invoke the @coderabbitai review command.

You can disable this status message by setting the reviews.review_status to false in the CodeRabbit configuration file.

Walkthrough

Adds an Args type and CLI flags for base-branch, dry-run, and full; Run now accepts Args. When not full-run, gathers changed files from git, skips symlinks/dirs/disallowed extensions and files not in the changeset, enqueues files honoring DryRun, and uses 0o644 for written license files.

Changes

Cohort / File(s) Summary
CLI Flag Integration
packages/go/stbernard/command/license/license.go
Added CLI flags --base-branch, --dry-run, --full; new package-level flag variables; build an Args instance and pass it into license.Run.
Core License Command Logic
packages/go/stbernard/command/license/internal/cmd.go
Introduced Args type (BaseBranchName, DryRun, FullRunMode); changed Run to accept Args; when not full-run call git diff to get changeset; per-file checks added (skip symlinks, directories, disallowed extensions, skip files not in changeset); enqueue respects DryRun; permission literals updated to 0o644; added detailed skip/add logging.
Git Diff Utility
packages/go/stbernard/command/license/internal/utils.go
Added getBranchDiff(baseBranchName string) ([]string, error) which runs git diff-index --cached --diff-filter=ACMR <base> to collect changed file paths; imported os/exec.

Sequence Diagram(s)

sequenceDiagram
  autonumber
  participant CLI
  participant LicenseRun as License.Run
  participant Git as git (getBranchDiff)
  participant Scanner as FileScanner
  participant FS as FileSystem

  CLI->>LicenseRun: invoke with Args (--base-branch, --dry-run, --full)
  alt FullRunMode = false
    LicenseRun->>Git: getBranchDiff(BaseBranchName)
    Git-->>LicenseRun: changed file list
  end
  LicenseRun->>Scanner: iterate repository files
  loop per-file
    Scanner->>FS: stat file (symlink/dir/check ext)
    alt disallowed (symlink/dir/extension)
      Scanner-->>LicenseRun: log skipped
    else if not in changeset (and not full)
      Scanner-->>LicenseRun: log skipped (not in changeset)
    else
      Scanner->>LicenseRun: enqueue file
      alt DryRun = true
        LicenseRun-->>Scanner: skip actual enqueue/write
      end
    end
  end
  LicenseRun->>FS: write root LICENSE/header with 0o644 (when applicable)
  LicenseRun-->>CLI: complete
Loading

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~25 minutes

Suggested reviewers

  • kpowderly
  • sircodemane
  • AD7ZJ

Poem

🐰 I hopped through flags and git-diff trails,
Skipped links and folders on soft rabbit tails.
Dry runs held back, full runs roam free,
Octal perms snug — 0o644 for me.
A tiny hop — licenses in harmony. ✨

Pre-merge checks and finishing touches

❌ Failed checks (1 warning)
Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. You can run @coderabbitai generate docstrings to improve docstring coverage.
✅ Passed checks (2 passed)
Check name Status Explanation
Title check ✅ Passed The title clearly summarizes the main change: making the stbernard/license tool aware of branch context, with the relevant ticket reference BED-7086.
Description check ✅ Passed The description covers all required template sections: detailed change description, motivation/context with ticket reference, testing methodology with debug output examples, and completed checklist items.

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

📥 Commits

Reviewing files that changed from the base of the PR and between b1e1e1e and c2d2cb2.

📒 Files selected for processing (3)
  • packages/go/stbernard/command/license/internal/cmd.go
  • packages/go/stbernard/command/license/internal/utils.go
  • 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
🧬 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() and cmd.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 0644 to 0o644 use 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

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

📥 Commits

Reviewing files that changed from the base of the PR and between c2d2cb2 and b3e197e.

📒 Files selected for processing (3)
  • packages/go/stbernard/command/license/internal/cmd.go
  • packages/go/stbernard/command/license/internal/utils.go
  • packages/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.Bool are guaranteed non-nil after parsing.


60-78: LGTM!

Flag definitions are clear and align with the PR objectives:

  • -base-branch defaults to "main" for sensible out-of-box behavior.
  • -dry-run and -full default to false, making branch-scoped, non-destructive runs the default.

81-91: LGTM!

The Args struct cleanly encapsulates the parsed flags and is passed to the internal Run function, keeping the public API stable while allowing the internal implementation to evolve.

@seanjSO seanjSO force-pushed the seanj/BED-7086 branch 2 times, most recently from e1845a2 to e5fe14d Compare January 6, 2026 17:54
Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 FullRunMode is 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

📥 Commits

Reviewing files that changed from the base of the PR and between b3e197e and e1845a2.

📒 Files selected for processing (3)
  • packages/go/stbernard/command/license/internal/cmd.go
  • packages/go/stbernard/command/license/internal/utils.go
  • 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
⏰ 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

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

📥 Commits

Reviewing files that changed from the base of the PR and between e1845a2 and e5fe14d.

📒 Files selected for processing (3)
  • packages/go/stbernard/command/license/internal/cmd.go
  • packages/go/stbernard/command/license/internal/utils.go
  • packages/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

Comment on lines +223 to +260
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
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🔴 Critical

Previous review concern about --cached flag remains unaddressed.

The --cached flag on line 226 limits git diff-index to staged changes only, not all working tree modifications. Per the PR objectives, the tool should operate on "files added/copied/renamed/modified in the current branch relative to a base branch," which includes unstaged working tree changes.

Lines 241-246 and 251-253 now properly handle the malformed-line panic and scanner errors flagged in previous reviews ✓

🔎 Proposed fix
-	diffCmd := exec.Command("git", "diff-index", "--cached", "--diff-filter=ACMR", baseBranchName)
+	diffCmd := exec.Command("git", "diff-index", "--diff-filter=ACMR", baseBranchName)
🤖 Prompt for AI Agents
In @packages/go/stbernard/command/license/internal/utils.go around lines 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().

@seanjSO seanjSO force-pushed the seanj/BED-7086 branch 2 times, most recently from 3ff57c5 to 2b6b73b Compare January 6, 2026 18:03
@seanjSO seanjSO marked this pull request as draft January 6, 2026 18:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

tooling This updates developer tooling

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants