BED-6138 - StBernard Output and Error Handling Iteration #1662
Conversation
WalkthroughThis update refactors the analysis and command execution infrastructure, introducing a strongly typed severity system, new severity grouping utilities, and interactive command execution. It modifies analysis output handling to print formatted results directly, adds a new output flag, and updates related workflows and recipes to accommodate these changes. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant CLI
participant AnalysisCmd
participant Analyzers
participant GoAnalyzer
participant JSAnalyzer
User->>CLI: just analyze [-all]
CLI->>AnalysisCmd: Parse flags, call Run
AnalysisCmd->>Analyzers: Run(paths, env, outputAllSeverity)
Analyzers->>GoAnalyzer: Run(paths, env)
GoAnalyzer-->>Analyzers: SeverityMap
Analyzers->>JSAnalyzer: Run(paths, env)
JSAnalyzer-->>Analyzers: SeverityMap
Analyzers->>AnalysisCmd: Print grouped results, return error if needed
AnalysisCmd->>CLI: Exit with code based on severity
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (16)
🚧 Files skipped from review as they are similar to previous changes (15)
🧰 Additional context used🧠 Learnings (2)📓 Common learningspackages/go/stbernard/analyzers/codeclimate/codeclimate.go (3)⏰ 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)
🔇 Additional comments (9)
✨ 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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
packages/go/stbernard/cmdrunner/cmdrunner.go (1)
156-156: Remove incomplete comment.The comment
// ?appears to be incomplete or a placeholder.- // ?
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (16)
justfile(2 hunks)packages/go/stbernard/analyzers/analyzers.go(1 hunks)packages/go/stbernard/analyzers/codeclimate/codeclimate.go(2 hunks)packages/go/stbernard/analyzers/golang/golang.go(2 hunks)packages/go/stbernard/analyzers/js/js.go(2 hunks)packages/go/stbernard/cmdrunner/cmdrunner.go(3 hunks)packages/go/stbernard/command/analysis/analysis.go(3 hunks)packages/go/stbernard/git/git.go(1 hunks)packages/go/stbernard/workspace/golang/build.go(1 hunks)packages/go/stbernard/workspace/golang/generate.go(1 hunks)packages/go/stbernard/workspace/golang/goimports.go(1 hunks)packages/go/stbernard/workspace/golang/sync.go(2 hunks)packages/go/stbernard/workspace/golang/tester.go(1 hunks)packages/go/stbernard/workspace/redoc/redoc.go(1 hunks)packages/go/stbernard/workspace/workspace.go(1 hunks)packages/go/stbernard/workspace/yarn/yarn.go(4 hunks)
🧰 Additional context used
🧠 Learnings (15)
📓 Common learnings
Learnt from: zinic
PR: SpecterOps/BloodHound#1573
File: packages/go/dawgs/drivers/pg/query/sql/schema_up.sql:514-581
Timestamp: 2025-06-12T15:42:11.258Z
Learning: Contributor "zinic" prefers not to receive automated refactor suggestions/comments on their pull requests.
packages/go/stbernard/workspace/golang/tester.go (3)
Learnt from: superlinkx
PR: SpecterOps/BloodHound#1503
File: cmd/api/src/services/job/jobs_test.go:19-143
Timestamp: 2025-05-27T16:58:33.295Z
Learning: Tests in cmd/api/src/services/job/jobs_test.go have been found to be flaky in the past and are due for rewrite. They should be skipped with t.Skip() until they can be properly rewritten.
Learnt from: LawsonWillard
PR: SpecterOps/BloodHound#1595
File: cmd/api/src/api/v2/saved_queries_test.go:2594-2594
Timestamp: 2025-06-17T22:37:36.389Z
Learning: In Go table-driven tests, there's a distinction between main test function parallelism and subtest parallelism. Main test functions can safely use t.Parallel() for performance benefits, but individual subtests within table-driven tests may need to run sequentially to avoid race conditions with mocks, deferred functions, or shared resources.
Learnt from: LawsonWillard
PR: SpecterOps/BloodHound#1595
File: cmd/api/src/api/v2/saved_queries_test.go:2197-2197
Timestamp: 2025-06-17T22:37:50.192Z
Learning: In Go tests using table-driven patterns, t.Parallel() at the function level allows test functions to run in parallel with other test functions, which is generally beneficial. However, when subtests contain deferred goroutines, the individual t.Run() calls should not use t.Parallel() to avoid flakiness. The comment "Tests will be flaky if run in parallel due to use of go routine in deferred function" specifically refers to subtest parallelization, not function-level parallelization.
packages/go/stbernard/workspace/workspace.go (1)
Learnt from: elikmiller
PR: SpecterOps/BloodHound#1563
File: packages/go/graphschema/azure/azure.go:24-24
Timestamp: 2025-06-06T23:12:14.181Z
Learning: In BloodHound, files in packages/go/graphschema/*/`*.go` are generated from CUE schemas. When `just prepare-for-codereview` is run, it triggers code generation that may automatically add import aliases or other formatting changes. These changes are legitimate outputs of the generation process, not manual edits that would be overwritten.
packages/go/stbernard/workspace/golang/generate.go (3)
Learnt from: elikmiller
PR: SpecterOps/BloodHound#1563
File: packages/go/graphschema/azure/azure.go:24-24
Timestamp: 2025-06-06T23:12:14.181Z
Learning: In BloodHound, files in packages/go/graphschema/*/`*.go` are generated from CUE schemas. When `just prepare-for-codereview` is run, it triggers code generation that may automatically add import aliases or other formatting changes. These changes are legitimate outputs of the generation process, not manual edits that would be overwritten.
Learnt from: LawsonWillard
PR: SpecterOps/BloodHound#1595
File: cmd/api/src/api/v2/saved_queries_test.go:2197-2197
Timestamp: 2025-06-17T22:37:50.192Z
Learning: In Go tests using table-driven patterns, t.Parallel() at the function level allows test functions to run in parallel with other test functions, which is generally beneficial. However, when subtests contain deferred goroutines, the individual t.Run() calls should not use t.Parallel() to avoid flakiness. The comment "Tests will be flaky if run in parallel due to use of go routine in deferred function" specifically refers to subtest parallelization, not function-level parallelization.
Learnt from: LawsonWillard
PR: SpecterOps/BloodHound#1595
File: cmd/api/src/api/v2/saved_queries_test.go:2594-2594
Timestamp: 2025-06-17T22:37:36.389Z
Learning: In Go table-driven tests, there's a distinction between main test function parallelism and subtest parallelism. Main test functions can safely use t.Parallel() for performance benefits, but individual subtests within table-driven tests may need to run sequentially to avoid race conditions with mocks, deferred functions, or shared resources.
packages/go/stbernard/workspace/golang/build.go (1)
Learnt from: elikmiller
PR: SpecterOps/BloodHound#1563
File: packages/go/graphschema/azure/azure.go:24-24
Timestamp: 2025-06-06T23:12:14.181Z
Learning: In BloodHound, files in packages/go/graphschema/*/`*.go` are generated from CUE schemas. When `just prepare-for-codereview` is run, it triggers code generation that may automatically add import aliases or other formatting changes. These changes are legitimate outputs of the generation process, not manual edits that would be overwritten.
packages/go/stbernard/workspace/golang/goimports.go (1)
Learnt from: elikmiller
PR: SpecterOps/BloodHound#1563
File: packages/go/graphschema/azure/azure.go:24-24
Timestamp: 2025-06-06T23:12:14.181Z
Learning: In BloodHound, files in packages/go/graphschema/*/`*.go` are generated from CUE schemas. When `just prepare-for-codereview` is run, it triggers code generation that may automatically add import aliases or other formatting changes. These changes are legitimate outputs of the generation process, not manual edits that would be overwritten.
packages/go/stbernard/workspace/yarn/yarn.go (2)
Learnt from: superlinkx
PR: SpecterOps/BloodHound#1503
File: cmd/api/src/services/job/jobs_test.go:19-143
Timestamp: 2025-05-27T16:58:33.295Z
Learning: Tests in cmd/api/src/services/job/jobs_test.go have been found to be flaky in the past and are due for rewrite. They should be skipped with t.Skip() until they can be properly rewritten.
Learnt from: elikmiller
PR: SpecterOps/BloodHound#1563
File: packages/go/graphschema/azure/azure.go:24-24
Timestamp: 2025-06-06T23:12:14.181Z
Learning: In BloodHound, files in packages/go/graphschema/*/`*.go` are generated from CUE schemas. When `just prepare-for-codereview` is run, it triggers code generation that may automatically add import aliases or other formatting changes. These changes are legitimate outputs of the generation process, not manual edits that would be overwritten.
packages/go/stbernard/workspace/golang/sync.go (2)
Learnt from: elikmiller
PR: SpecterOps/BloodHound#1563
File: packages/go/graphschema/azure/azure.go:24-24
Timestamp: 2025-06-06T23:12:14.181Z
Learning: In BloodHound, files in packages/go/graphschema/*/`*.go` are generated from CUE schemas. When `just prepare-for-codereview` is run, it triggers code generation that may automatically add import aliases or other formatting changes. These changes are legitimate outputs of the generation process, not manual edits that would be overwritten.
Learnt from: superlinkx
PR: SpecterOps/BloodHound#1503
File: cmd/api/src/services/job/jobs_test.go:19-143
Timestamp: 2025-05-27T16:58:33.295Z
Learning: Tests in cmd/api/src/services/job/jobs_test.go have been found to be flaky in the past and are due for rewrite. They should be skipped with t.Skip() until they can be properly rewritten.
packages/go/stbernard/git/git.go (1)
Learnt from: zinic
PR: SpecterOps/BloodHound#1651
File: packages/go/stbernard/cmdrunner/cmdrunner.go:118-137
Timestamp: 2025-07-07T16:19:35.830Z
Learning: zinic designed the cmdrunner.ExecutionResult to implement the error interface as a safety measure to prevent nil dereference issues and eliminate the need for nil checks when commands fail. The error struct contains all necessary execution information, prioritizing API safety over Go conventions.
justfile (1)
Learnt from: elikmiller
PR: SpecterOps/BloodHound#1563
File: packages/go/graphschema/azure/azure.go:24-24
Timestamp: 2025-06-06T23:12:14.181Z
Learning: In BloodHound, files in packages/go/graphschema/*/`*.go` are generated from CUE schemas. When `just prepare-for-codereview` is run, it triggers code generation that may automatically add import aliases or other formatting changes. These changes are legitimate outputs of the generation process, not manual edits that would be overwritten.
packages/go/stbernard/analyzers/golang/golang.go (2)
Learnt from: superlinkx
PR: SpecterOps/BloodHound#1606
File: .golangci.json:3-5
Timestamp: 2025-06-25T18:23:53.103Z
Learning: In the BloodHound project's .golangci.json, errcheck linter is intentionally disabled because it was too noisy, but the severity override settings are preserved for when it gets re-enabled in the future.
Learnt from: elikmiller
PR: SpecterOps/BloodHound#1563
File: packages/go/graphschema/azure/azure.go:24-24
Timestamp: 2025-06-06T23:12:14.181Z
Learning: In BloodHound, files in packages/go/graphschema/*/`*.go` are generated from CUE schemas. When `just prepare-for-codereview` is run, it triggers code generation that may automatically add import aliases or other formatting changes. These changes are legitimate outputs of the generation process, not manual edits that would be overwritten.
packages/go/stbernard/command/analysis/analysis.go (1)
Learnt from: zinic
PR: SpecterOps/BloodHound#1651
File: packages/go/stbernard/cmdrunner/cmdrunner.go:118-137
Timestamp: 2025-07-07T16:19:35.830Z
Learning: zinic designed the cmdrunner.ExecutionResult to implement the error interface as a safety measure to prevent nil dereference issues and eliminate the need for nil checks when commands fail. The error struct contains all necessary execution information, prioritizing API safety over Go conventions.
packages/go/stbernard/cmdrunner/cmdrunner.go (6)
Learnt from: zinic
PR: SpecterOps/BloodHound#1651
File: packages/go/stbernard/cmdrunner/cmdrunner.go:118-137
Timestamp: 2025-07-07T16:19:35.830Z
Learning: zinic designed the cmdrunner.ExecutionResult to implement the error interface as a safety measure to prevent nil dereference issues and eliminate the need for nil checks when commands fail. The error struct contains all necessary execution information, prioritizing API safety over Go conventions.
Learnt from: superlinkx
PR: SpecterOps/BloodHound#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()).
Learnt from: superlinkx
PR: SpecterOps/BloodHound#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.
Learnt from: superlinkx
PR: SpecterOps/BloodHound#1606
File: cmd/api/src/analysis/azure/post.go:33-35
Timestamp: 2025-06-25T18:24:25.014Z
Learning: In BloodHound Go code, for error types in slog structured logging, prefer using slog.String("key", err.Error()) over slog.Any("key", err). The explicit string conversion with err.Error() is preferred over using slog.Any() for error types.
Learnt from: superlinkx
PR: SpecterOps/BloodHound#1509
File: packages/go/stbernard/command/license/internal/cmd.go:49-51
Timestamp: 2025-05-29T18:38:42.769Z
Learning: In the stbernard package, use structured slog logging with explicit parameter functions like slog.String(), slog.Int(), etc. instead of fmt.Sprintf() with slog for new logs. Example: slog.Warn("message", slog.String("key", value)) rather than slog.Warn(fmt.Sprintf("message %s", value)).
Learnt from: superlinkx
PR: SpecterOps/BloodHound#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.
packages/go/stbernard/analyzers/analyzers.go (1)
Learnt from: superlinkx
PR: SpecterOps/BloodHound#1606
File: .golangci.json:3-5
Timestamp: 2025-06-25T18:23:53.103Z
Learning: In the BloodHound project's .golangci.json, errcheck linter is intentionally disabled because it was too noisy, but the severity override settings are preserved for when it gets re-enabled in the future.
packages/go/stbernard/analyzers/codeclimate/codeclimate.go (1)
Learnt from: superlinkx
PR: SpecterOps/BloodHound#1606
File: .golangci.json:3-5
Timestamp: 2025-06-25T18:23:53.103Z
Learning: In the BloodHound project's .golangci.json, errcheck linter is intentionally disabled because it was too noisy, but the severity override settings are preserved for when it gets re-enabled in the future.
🧬 Code Graph Analysis (9)
packages/go/stbernard/workspace/golang/tester.go (1)
packages/go/stbernard/cmdrunner/cmdrunner.go (1)
RunInteractive(153-163)
packages/go/stbernard/workspace/workspace.go (1)
packages/go/stbernard/cmdrunner/cmdrunner.go (1)
RunInteractive(153-163)
packages/go/stbernard/workspace/redoc/redoc.go (1)
packages/go/stbernard/cmdrunner/cmdrunner.go (1)
RunInteractive(153-163)
packages/go/stbernard/workspace/golang/generate.go (1)
packages/go/stbernard/cmdrunner/cmdrunner.go (1)
RunInteractive(153-163)
packages/go/stbernard/workspace/golang/build.go (1)
packages/go/stbernard/cmdrunner/cmdrunner.go (1)
RunInteractive(153-163)
packages/go/stbernard/workspace/golang/goimports.go (1)
packages/go/stbernard/cmdrunner/cmdrunner.go (1)
RunInteractive(153-163)
packages/go/stbernard/workspace/yarn/yarn.go (1)
packages/go/stbernard/cmdrunner/cmdrunner.go (1)
RunInteractive(153-163)
packages/go/stbernard/workspace/golang/sync.go (1)
packages/go/stbernard/cmdrunner/cmdrunner.go (1)
RunInteractive(153-163)
packages/go/stbernard/git/git.go (1)
packages/go/stbernard/cmdrunner/cmdrunner.go (1)
ExecutionError(61-63)
⏰ 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: build-ui
- GitHub Check: run-tests
🔇 Additional comments (42)
packages/go/stbernard/workspace/golang/tester.go (1)
86-86: Good change to enable interactive test execution.The switch from
cmdrunner.Runtocmdrunner.RunInteractivewill provide real-time output streaming during test execution, improving the user experience by showing progress as tests run.packages/go/stbernard/git/git.go (1)
88-88: Correct update to align with cmdrunner refactoring.The change from
*cmdrunner.ExecutionResultto*cmdrunner.ExecutionErroris consistent with the cmdrunner package refactoring. SinceExecutionErrorembedsExecutionResult, the.ReturnCodefield remains accessible.packages/go/stbernard/workspace/redoc/redoc.go (1)
46-46: Good change to enable interactive OpenAPI doc generation.The switch from
cmdrunner.Runtocmdrunner.RunInteractivewill provide real-time output streaming during OpenAPI documentation generation, helping users track progress.packages/go/stbernard/workspace/workspace.go (1)
120-120: Good change to enable interactive schema generation.The switch from
cmdrunner.Runtocmdrunner.RunInteractivewill provide real-time output streaming during schema generation, improving visibility into the process.packages/go/stbernard/workspace/golang/sync.go (2)
49-49: LGTM: Interactive execution for go mod tidyThe switch to
RunInteractiveenables real-time output streaming forgo mod tidyoperations, which improves user experience during dependency management tasks.
75-75: LGTM: Interactive execution for go work syncThe switch to
RunInteractiveenables real-time output streaming forgo work syncoperations, providing better feedback during workspace synchronization.packages/go/stbernard/workspace/golang/goimports.go (1)
35-35: LGTM: Interactive execution for goimportsThe switch to
RunInteractiveenables real-time output streaming for thegoimportstool, providing better feedback during code formatting operations.packages/go/stbernard/workspace/golang/generate.go (1)
112-112: LGTM: Interactive execution for go generateThe switch to
RunInteractiveenables real-time output streaming forgo generateoperations, which is particularly beneficial for code generation tasks that can be time-consuming.justfile (2)
43-44: LGTM: Simplified analyze recipeThe changes streamline the analyze recipe by removing the jq processing step and enabling direct flag passing to the stbernard analysis command, which aligns with the refactored analysis output handling.
66-66: LGTM: Removed output redirection in prep stepsThe removal of output redirection aligns with the analysis command now handling output formatting internally, simplifying the preparation workflow.
packages/go/stbernard/workspace/yarn/yarn.go (4)
66-66: LGTM: Interactive execution for yarn installThe switch to
RunInteractiveenables real-time output streaming foryarn installoperations, providing better feedback during dependency installation.
81-81: LGTM: Interactive execution for yarn formatThe switch to
RunInteractiveenables real-time output streaming foryarn formatoperations, improving user experience during code formatting tasks.
95-95: LGTM: Interactive execution for yarn buildThe switch to
RunInteractiveenables real-time output streaming foryarn buildoperations, providing better feedback during build processes.
109-109: LGTM: Interactive execution for yarn testThe switch to
RunInteractiveenables real-time output streaming foryarn testoperations, improving user experience during test execution with coverage reporting.packages/go/stbernard/workspace/golang/build.go (1)
101-101: LGTM! Consistent with interactive execution pattern.The change to
RunInteractivealigns with the broader refactoring across workspace packages to enable real-time output streaming during build operations.packages/go/stbernard/command/analysis/analysis.go (4)
32-34: LGTM! Enhanced usage formatting.The addition of
UsageFmtconstant provides better structured usage output formatting.
38-39: LGTM! Clean addition of severity control flag.The new
outputAllSeverityfield properly extends the command structure to support the new functionality.
61-71: LGTM! Improved flag parsing with better variable naming.The refactoring from
cmdtoflagSetimproves code clarity, and the new-allflag is properly integrated with usage formatting.
82-82: LGTM! Correct integration with updated analyzers.Run signature.The function call properly passes the new
outputAllSeverityparameter and uses the updated signature.packages/go/stbernard/analyzers/golang/golang.go (6)
20-20: LGTM! Necessary import for buffer handling.The
bytesimport is required for the new output buffer handling pattern.
35-35: LGTM! Improved return type with severity mapping.The change to
codeclimate.SeverityMapenables better severity-based processing and aligns with the refactored analyzer architecture.
38-38: LGTM! Proper buffer initialization for output handling.The
outputbuffer variable correctly handles command output in both success and error cases.
47-57: LGTM! Robust error handling with output extraction.The error handling correctly uses
cmdrunner.ExecutionErrortype assertion to extract output from failed executions, ensuring JSON decoding can proceed in both success and error scenarios.
59-59: LGTM! Consistent JSON decoding from output buffer.The JSON decoding now uses the unified
outputbuffer regardless of command success or failure.
63-63: LGTM! Proper conversion to severity map.The return statement correctly converts the entries to a severity map using the new
codeclimate.NewSeverityMapfunction.packages/go/stbernard/analyzers/js/js.go (8)
20-20: LGTM! Necessary import for buffer handling.The
bytesimport is required for the new output buffer handling pattern, consistent with the golang analyzer.
47-47: LGTM! Improved return type with severity mapping.The change to
codeclimate.SeverityMapenables better severity-based processing and aligns with the refactored analyzer architecture.
54-54: LGTM! Consistent error handling.Returning
nilon error is appropriate for the newSeverityMapreturn type.
62-62: LGTM! Proper conversion to severity map.The return statement correctly converts the entries to a severity map using the new
codeclimate.NewSeverityMapfunction.
70-70: LGTM! Proper buffer initialization for output handling.The
outputbuffer variable correctly handles command output in both success and error cases, consistent with the golang analyzer pattern.
76-86: LGTM! Robust error handling with output extraction.The error handling correctly uses
cmdrunner.ExecutionErrortype assertion to extract output from failed executions, ensuring JSON decoding can proceed in both success and error scenarios.
88-88: LGTM! Consistent JSON decoding from output buffer.The JSON decoding now uses the unified
outputbuffer regardless of command success or failure.
94-94: LGTM! Improved type safety with severity constants.The change from string literals to typed
codeclimate.Severityconstants improves type safety and consistency across the codebase.Also applies to: 98-98, 100-100, 102-102
packages/go/stbernard/analyzers/analyzers.go (4)
23-23: LGTM! Necessary imports for the refactored architecture.The addition of
codeclimateandworkspaceimports supports the new severity-based processing and workspace path handling.Also applies to: 27-27
34-59: LGTM! Well-structured severity output handling.The
outputSeverityMapfunction correctly processes severity maps with proper priority ordering, conditional output based on severity levels, and appropriate exit error handling for high severity issues.
65-65: LGTM! Improved function signature with workspace integration.The new signature properly accepts workspace paths and environment, along with the output control flag, replacing the previous individual path parameters.
68-78: LGTM! Clean orchestration of analyzer execution.The refactored implementation correctly runs both Go and JavaScript analyzers with the new signature, processes their severity maps, and accumulates exit errors appropriately.
packages/go/stbernard/cmdrunner/cmdrunner.go (5)
21-21: LGTM: Context import added for logging level checks.The context import is properly added to support the logging level check in the RunInteractive function.
60-76: LGTM: Clean error handling refactor with ExecutionError wrapper.The introduction of ExecutionError as a wrapper around ExecutionResult properly separates concerns and follows Go conventions better than having ExecutionResult implement the error interface directly.
127-143: LGTM: Improved error handling in run function.The refactored run function now properly returns ExecutionError for exit errors and preserves other system errors, providing cleaner error handling semantics.
145-149: LGTM: Clean separation of concerns in Run function.The Run function now properly delegates to the run function, maintaining a clean separation between command preparation and execution.
151-163: LGTM: RunInteractive function provides real-time output streaming.The RunInteractive function correctly implements interactive command execution with conditional output streaming based on logging level. The MultiWriter approach properly maintains both buffered output and real-time streaming.
There was a problem hiding this comment.
Approving, the two cleanup comments are probably worth a final commit, but there's nothing functional needed at this stage. Local testing shows it works quite effectively.
Confirmed my direct file:line links in my integrated terminal work again, so I'm more than happy with these QoL upgrades.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
packages/go/stbernard/analyzers/codeclimate/codeclimate.go (1)
45-52: Verify if eslint severities need to be supported.Based on the previous review comment, eslint uses severities like "error" and "warning" that aren't included here. If this code needs to handle eslint output, consider adding these severities or implementing a mapping mechanism.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (16)
justfile(2 hunks)packages/go/stbernard/analyzers/analyzers.go(1 hunks)packages/go/stbernard/analyzers/codeclimate/codeclimate.go(2 hunks)packages/go/stbernard/analyzers/golang/golang.go(2 hunks)packages/go/stbernard/analyzers/js/js.go(2 hunks)packages/go/stbernard/cmdrunner/cmdrunner.go(3 hunks)packages/go/stbernard/command/analysis/analysis.go(3 hunks)packages/go/stbernard/git/git.go(1 hunks)packages/go/stbernard/workspace/golang/build.go(1 hunks)packages/go/stbernard/workspace/golang/generate.go(1 hunks)packages/go/stbernard/workspace/golang/goimports.go(1 hunks)packages/go/stbernard/workspace/golang/sync.go(2 hunks)packages/go/stbernard/workspace/golang/tester.go(1 hunks)packages/go/stbernard/workspace/redoc/redoc.go(1 hunks)packages/go/stbernard/workspace/workspace.go(1 hunks)packages/go/stbernard/workspace/yarn/yarn.go(4 hunks)
🚧 Files skipped from review as they are similar to previous changes (15)
- packages/go/stbernard/workspace/golang/tester.go
- packages/go/stbernard/workspace/golang/build.go
- packages/go/stbernard/workspace/redoc/redoc.go
- packages/go/stbernard/workspace/golang/goimports.go
- packages/go/stbernard/workspace/golang/sync.go
- packages/go/stbernard/workspace/workspace.go
- packages/go/stbernard/workspace/yarn/yarn.go
- justfile
- packages/go/stbernard/workspace/golang/generate.go
- packages/go/stbernard/git/git.go
- packages/go/stbernard/command/analysis/analysis.go
- packages/go/stbernard/analyzers/golang/golang.go
- packages/go/stbernard/analyzers/js/js.go
- packages/go/stbernard/analyzers/analyzers.go
- packages/go/stbernard/cmdrunner/cmdrunner.go
🧰 Additional context used
🧠 Learnings (2)
📓 Common learnings
Learnt from: zinic
PR: SpecterOps/BloodHound#1573
File: packages/go/dawgs/drivers/pg/query/sql/schema_up.sql:514-581
Timestamp: 2025-06-12T15:42:11.258Z
Learning: Contributor "zinic" prefers not to receive automated refactor suggestions/comments on their pull requests.
packages/go/stbernard/analyzers/codeclimate/codeclimate.go (3)
Learnt from: superlinkx
PR: SpecterOps/BloodHound#1606
File: .golangci.json:3-5
Timestamp: 2025-06-25T18:23:53.103Z
Learning: In the BloodHound project's .golangci.json, errcheck linter is intentionally disabled because it was too noisy, but the severity override settings are preserved for when it gets re-enabled in the future.
Learnt from: zinic
PR: SpecterOps/BloodHound#1573
File: packages/go/dawgs/drivers/pg/query/sql/schema_up.sql:514-581
Timestamp: 2025-06-12T15:42:11.258Z
Learning: Contributor "zinic" prefers not to receive automated refactor suggestions/comments on their pull requests.
Learnt from: elikmiller
PR: SpecterOps/BloodHound#1563
File: packages/go/graphschema/azure/azure.go:24-24
Timestamp: 2025-06-06T23:12:14.181Z
Learning: In BloodHound, files in packages/go/graphschema/*/`*.go` are generated from CUE schemas. When `just prepare-for-codereview` is run, it triggers code generation that may automatically add import aliases or other formatting changes. These changes are legitimate outputs of the generation process, not manual edits that would be overwritten.
⏰ 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 (1)
packages/go/stbernard/analyzers/codeclimate/codeclimate.go (1)
63-78: Priority method correctly handles all severity levels.Good job fixing the previously missing
SeverityMajorcase. The priority ordering is now complete and consistent.
…tighten up the execution result contract
Description
StBernard ergonomic work. See CodeRabbit's summary.
Motivation and Context
Resolves BED-6138
No broken windows.
How Has This Been Tested?
Unit tests updated and integration tests pass.
Types of changes
Checklist:
Summary by CodeRabbit
Summary by CodeRabbit
New Features
Bug Fixes
Refactor