Adds a linter for the ocm logger #1101
Conversation
WalkthroughAdds an "ocmlogger" linter plugin that checks OCM SDK logger calls for format-placeholder vs variadic-argument mismatches, includes analyzer tests and test fixtures (logger stub and sample code), a Ginkgo test bootstrap, and updates several transitive Go module dependencies. Changes
Sequence Diagram(s)sequenceDiagram
participant Test as Linter Test
participant Reg as Plugin Registry
participant Linter as OcmLoggerLinter
participant Analyzer as Analyzer
participant AST as AST Walker
Test->>Reg: GetPlugin("ocmlogger")
Reg->>Linter: New(...)
Linter-->>Reg: plugin instance
Test->>Linter: BuildAnalyzers()
Linter->>Analyzer: create Analyzer with Run callback
Analyzer-->>Linter: Analyzer ready
Test->>Analyzer: Run on testdata
Analyzer->>AST: Walk AST nodes
loop for each log-call node
AST->>Linter: isDisabled(node)?
alt disabled via nolint
Linter-->>AST: skip node
else
AST->>Linter: extractString(formatExpr)
Linter-->>AST: format string or dynamic
AST->>Linter: countPlaceholders(format)
Linter-->>Analyzer: placeholderCount
Analyzer->>AST: compare with variadic args
alt mismatch
Analyzer-->>Test: report diagnostic
else match or dynamic
Analyzer-->>Test: no report
end
end
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Tip 📝 Customizable high-level summaries are now available in beta!You can now customize how CodeRabbit generates the high-level summary in your pull requests — including its content, structure, tone, and formatting.
Example instruction:
Note: This feature is currently in beta for Pro-tier users, and pricing will be announced later. Comment |
|
The linting step is failing because the CI is not executing the Makefile from the PR (which adds the new custom linter) |
![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
🧹 Nitpick comments (3)
.golangci.yml (1)
5-11: Consider adding a meaningful description for the ocmlogger linter.The description field is set to "none", which doesn't provide useful information about the linter's purpose.
Apply this diff to improve the description:
custom: ocmlogger: type: "module" - description: "none" + description: "Validates OCM logger format strings match argument counts"linters/ocm_logging_linter_test.go (1)
20-22: Verify that only one analyzer is expected, or add an assertion for analyzer count.The test uses
analyzers[0]without checking the length of the analyzers slice. If the plugin returns zero or multiple analyzers, this could cause issues.Apply this diff to add a safety check:
analyzers, err := plugin.BuildAnalyzers() gomega.Expect(err).NotTo(gomega.HaveOccurred()) + gomega.Expect(analyzers).To(gomega.HaveLen(1), "Expected exactly one analyzer") analysistest.Run(GinkgoT(), td, analyzers[0], "data")linters/ocm_logging_linter.go (1)
45-49: Consider removing the commented code example from documentation.The commented-out analyzer variable definition within the documentation comment is somewhat confusing. The struct documentation would be clearer without this example code, as the actual implementation is visible in the BuildAnalyzers method.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Cache: Disabled due to data retention organization setting
Knowledge base: Disabled due to Reviews -> Disable Knowledge Base setting
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
📒 Files selected for processing (9)
.custom-gcl.yml(1 hunks).golangci.yml(1 hunks)Makefile(1 hunks)go.mod(2 hunks)linters/linters_suite_test.go(1 hunks)linters/ocm_logging_linter.go(1 hunks)linters/ocm_logging_linter_test.go(1 hunks)linters/testdata/src/data/data.go(1 hunks)linters/testdata/src/github.com/openshift-online/ocm-sdk-go/logging/logging.go(1 hunks)
🧰 Additional context used
🪛 OSV Scanner (2.2.4)
go.mod
[HIGH] 24-24: golang.org/x/oauth2 0.15.0: Unexpected memory consumption during token parsing in golang.org/x/oauth2
(GO-2025-3488)
[HIGH] 24-24: golang.org/x/oauth2 0.15.0: golang.org/x/oauth2 Improper Validation of Syntactic Correctness of Input vulnerability
⏰ 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: Generate
- GitHub Check: Test (1.21, windows-latest)
- GitHub Check: Test (1.21, macos-latest)
- GitHub Check: Test (1.21, ubuntu-latest)
🔇 Additional comments (9)
linters/testdata/src/github.com/openshift-online/ocm-sdk-go/logging/logging.go (1)
1-13: LGTM!The Logger stub correctly defines the expected signature for the linter to analyze. The no-op implementations are appropriate for test data.
linters/linters_suite_test.go (1)
1-13: LGTM!Standard Ginkgo test suite bootstrap that follows best practices.
linters/testdata/src/data/data.go (1)
1-41: LGTM! Comprehensive test data covering positive, negative, and suppression scenarios.The test data correctly covers:
- Valid format strings with various placeholder types and escaped percent signs
- Mismatched placeholder/argument counts with
// wantassertions- Dynamic (non-literal) format strings that should be ignored
- Both inline and previous-line nolint suppression
.custom-gcl.yml (1)
1-10: Module Plugin System unavailable in installed golangci-lint version.The .custom-gcl.yml file structure correctly follows the golangci-lint Module Plugin System configuration format. However, the installed golangci-lint v2.5.0 does not support the required
golangci-lint customcommand. The script output confirms: "unknown command 'custom' for 'golangci-lint help'", indicating thegolangci-lint customcommand is not available in this binary.Additionally, the configuration specifies
version: v2.1.6, but the installed binary is v2.5.0. Either downgrade golangci-lint to v2.1.6 or verify that your installation properly includes Module Plugin System support (available in versions >= v1.57.0, though availability may depend on how the binary was built).linters/ocm_logging_linter.go (5)
14-16: LGTM: Plugin registration follows standard pattern.The init function correctly registers the plugin with an appropriate name.
52-58: LGTM: Load mode and constructor are correctly implemented.The LoadModeSyntax is appropriate for AST analysis, and the constructor signature matches the expected plugin interface.
70-90: LGTM: Comment-based disabling is correctly implemented.The logic properly checks for nolint directives on the same line or the line above, and handles both comment styles.
92-130: LGTM: String extraction and placeholder counting are well-implemented.The extractString method appropriately handles literal strings and concatenation while skipping dynamic cases. The countPlaceholders method correctly counts format specifiers and handles the escaped %% sequence.
132-194: LGTM: Main analysis logic is correct and well-structured.The run method properly validates OCM logger calls by:
- Filtering to only the target logger methods
- Verifying the receiver type matches the OCM Logger
- Extracting and validating format strings against argument counts
- Respecting nolint directives
The logic correctly assumes the OCM logger API signature:
method(ctx, formatString, ...args).
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
leadership/flag.go (1)
385-389: Consider standardizing error message format.The error message format here differs from similar error messages in the file. For consistency:
- Line 327:
"error running precheck: process '%s' name '%s' err %v"- Line 387:
"Process '%s' can't create initial state for flag: name %s err %v"- Line 469:
"Process '%s' can't update holder for flag '%s': %v"Consider adopting a uniform format for all error messages, such as:
"Process '%s' can't create initial state for flag '%s': %v"
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Cache: Disabled due to data retention organization setting
Knowledge base: Disabled due to Reviews -> Disable Knowledge Base setting
📒 Files selected for processing (1)
leadership/flag.go(3 hunks)
🔇 Additional comments (2)
leadership/flag.go (2)
330-332: LGTM! Behavioral improvement for consistency.Adding
f.lower(ctx)on precheck error makes this error path consistent with all other error paths in the function. This ensures the process immediately releases the leadership flag when precheck fails, which is the correct behavior.
466-470: LGTM! Error parameter now properly included.The error parameter is now correctly included in the log message. The format string and arguments match properly (3 placeholders, 3 arguments).
Note: Format consistency across error messages was already noted in the previous comment.
| f.ctx, | ||
| "error running precheck: %v", | ||
| "error running precheck: process '%s' name '%s' err %v", | ||
| f.process, f.name, err, | ||
| ) |
There was a problem hiding this comment.
Pre-existing inconsistency: Use ctx instead of f.ctx for logging.
This logger call uses f.ctx (Line 326) while all other logger calls in this function use the ctx parameter. The ctx parameter is enriched with the leadershipFlag context value (Line 317) and should be used consistently for all logging operations in this function.
Apply this diff to use the correct context:
if err != nil {
f.logger.Error(
- f.ctx,
+ ctx,
"error running precheck: process '%s' name '%s' err %v",
f.process, f.name, err,
)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| f.ctx, | |
| "error running precheck: %v", | |
| "error running precheck: process '%s' name '%s' err %v", | |
| f.process, f.name, err, | |
| ) | |
| ctx, | |
| "error running precheck: process '%s' name '%s' err %v", | |
| f.process, f.name, err, | |
| ) |
🤖 Prompt for AI Agents
In leadership/flag.go around lines 326 to 329, the logger call uses f.ctx
instead of the enriched ctx parameter (created at line 317); replace f.ctx with
ctx in that log call so all logging in this function consistently uses the
context carrying the leadershipFlag value.
ziccardi
force-pushed
the
OCM_LOGGER_LINTER
branch
from
775580e to
c22f6a5
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Cache: Disabled due to data retention organization setting
Knowledge base: Disabled due to Reviews -> Disable Knowledge Base setting
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
📒 Files selected for processing (10)
.custom-gcl.yml(1 hunks).golangci.yml(1 hunks)Makefile(1 hunks)go.mod(2 hunks)leadership/flag.go(3 hunks)linters/linters_suite_test.go(1 hunks)linters/ocm_logging_linter.go(1 hunks)linters/ocm_logging_linter_test.go(1 hunks)linters/testdata/src/data/data.go(1 hunks)linters/testdata/src/github.com/openshift-online/ocm-sdk-go/logging/logging.go(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (5)
- linters/testdata/src/data/data.go
- linters/linters_suite_test.go
- .golangci.yml
- .custom-gcl.yml
- linters/ocm_logging_linter_test.go
🧰 Additional context used
🪛 OSV Scanner (2.2.4)
go.mod
[HIGH] 24-24: golang.org/x/oauth2 0.15.0: Unexpected memory consumption during token parsing in golang.org/x/oauth2
(GO-2025-3488)
[HIGH] 24-24: golang.org/x/oauth2 0.15.0: golang.org/x/oauth2 Improper Validation of Syntactic Correctness of Input vulnerability
⏰ 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: Test (1.21, ubuntu-latest)
- GitHub Check: Test (1.21, macos-latest)
- GitHub Check: Test (1.21, windows-latest)
- GitHub Check: Generate
🔇 Additional comments (7)
leadership/flag.go (1)
327-329: LGTM! Format strings now match argument counts.The updated error log messages correctly align the number of format placeholders with the number of arguments, which will satisfy the new OCM logger linter requirements.
Also applies to: 387-388, 468-469
linters/testdata/src/github.com/openshift-online/ocm-sdk-go/logging/logging.go (1)
1-13: LGTM! Appropriate minimal test stub.This provides the necessary Logger interface for testing the OCM logger linter without introducing unnecessary complexity.
linters/ocm_logging_linter.go (3)
70-90: Well-implemented nolint suppression.The logic correctly checks for suppression comments on both the same line and the line immediately above, supporting both
nolint:ocmloggerandocm-linter:ignoreformats.
114-130: Correct placeholder counting with %% escape handling.The implementation properly counts format placeholders while correctly skipping escaped
%%sequences.
158-163: Precise receiver type checking.The exact type-string comparison ensures the linter only analyzes calls on the intended Logger type, avoiding false positives on similarly-named methods.
go.mod (2)
29-29: Linter plugin infrastructure dependencies look good.The additions of
github.com/golangci/plugin-module-registerandgolang.org/x/toolsalign with the PR objective to add a custom OCM logger linter plugin. These provide the necessary tooling infrastructure for plugin registration and analysis.Also applies to: 33-33
22-23: Version bumps are compatible with linter plugin—no breaking changes detected.No API-breaking changes exist in golang.org/x/crypto v0.37.0 or golang.org/x/net v0.39.0.
go mod verifyconfirms the go.mod is consistent, and the dependency graph shows no conflicts. The linter tooling integration (golangci-lint/plugin-module-register) has no reported incompatibilities with these versions. The changes are safe to proceed with.
ziccardi
force-pushed
the
OCM_LOGGER_LINTER
branch
from
c22f6a5 to
ca93b84
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
linters/ocm_logging_linter.go (1)
52-54: Fix the load mode - TypesInfo will be unavailable.The method returns
register.LoadModeSyntax, but line 153 callspass.TypesInfo.TypeOf(sel.X)which requires type information. With syntax-only mode,TypesInfowill not be populated, causing the type check to fail.Apply this diff to use the correct load mode:
func (f *OcmLoggerLinter) GetLoadMode() string { - return register.LoadModeSyntax + return register.LoadModeTypesInfo }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Cache: Disabled due to data retention organization setting
Knowledge base: Disabled due to Reviews -> Disable Knowledge Base setting
⛔ Files ignored due to path filters (2)
examples/go.sumis excluded by!**/*.sumgo.sumis excluded by!**/*.sum
📒 Files selected for processing (11)
.custom-gcl.yml(1 hunks).golangci.yml(1 hunks)Makefile(1 hunks)examples/go.mod(1 hunks)go.mod(2 hunks)leadership/flag.go(3 hunks)linters/linters_suite_test.go(1 hunks)linters/ocm_logging_linter.go(1 hunks)linters/ocm_logging_linter_test.go(1 hunks)linters/testdata/src/data/data.go(1 hunks)linters/testdata/src/github.com/openshift-online/ocm-sdk-go/logging/logging.go(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- leadership/flag.go
🧰 Additional context used
🪛 OSV Scanner (2.2.4)
go.mod
[HIGH] 24-24: golang.org/x/oauth2 0.15.0: Unexpected memory consumption during token parsing in golang.org/x/oauth2
(GO-2025-3488)
[HIGH] 24-24: golang.org/x/oauth2 0.15.0: golang.org/x/oauth2 Improper Validation of Syntactic Correctness of Input vulnerability
⏰ 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: Generate
- GitHub Check: Test (1.21, ubuntu-latest)
- GitHub Check: Test (1.21, windows-latest)
- GitHub Check: Test (1.21, macos-latest)
🔇 Additional comments (17)
.golangci.yml (1)
5-11: LGTM! Custom linter configuration is correct.The ocmlogger linter is properly configured as a module-type custom linter and enabled in the linters list. This aligns with the module plugin approach defined in
.custom-gcl.yml.examples/go.mod (1)
53-57: LGTM! Dependency upgrades align with parent module.The golang.org/x/* dependency upgrades are consistent with the changes in the root
go.modand maintain proper version alignment between the examples module and the parent SDK module..custom-gcl.yml (1)
1-10: LGTM! Plugin configuration is properly structured.The
.custom-gcl.ymlcorrectly defines the ocmlogger plugin as a local module plugin, specifying the import path and using a local build path. Thegolangci-lint customcommand will use this to build theocm-lintbinary in./bin/.Makefile (1)
100-101: LGTM! Lint workflow correctly builds and executes custom linter.The updated lint target properly:
- Builds the custom
ocm-lintbinary with the ocmlogger plugin viagolangci-lint custom- Executes the custom linter with
$(LOCAL_BIN_PATH)/ocm-lint runThis follows the golangci-lint module plugin pattern and was confirmed correct in previous review discussion.
go.mod (1)
22-23: LGTM! Dependency updates support the new linter infrastructure.The additions of
plugin-module-registerandgolang.org/x/tools, along with the golang.org/x/* upgrades, properly support the new ocmlogger custom linter plugin and analyzer framework.Also applies to: 29-29, 33-33, 69-73
linters/linters_suite_test.go (1)
1-13: LGTM! Standard Ginkgo test suite setup.The test suite bootstrap properly initializes Ginkgo v2 and Gomega for the linters package tests, following standard Go testing conventions.
linters/ocm_logging_linter_test.go (1)
11-24: LGTM! Analyzer test follows standard patterns.The test properly exercises the ocmlogger plugin through the plugin registration framework and uses
analysistest.Runto verify diagnostic output against the// wantcomments in testdata. Error handling with Gomega assertions is appropriate.linters/testdata/src/data/data.go (1)
14-41: LGTM! Comprehensive test cases for the linter.The testdata covers all critical scenarios:
- Valid format/argument combinations including edge cases with
%%(lines 17-19)- Mismatched placeholder/argument counts with expected diagnostics (lines 23-24)
- Dynamic format strings that are intentionally not validated (line 29)
- Both inline and previous-line
nolintsuppression patterns (lines 34, 40)The test cases effectively validate the linter's behavior across normal, error, and suppression paths.
linters/testdata/src/github.com/openshift-online/ocm-sdk-go/logging/logging.go (1)
1-13: LGTM! Test stub is well-formed.The Logger stub correctly defines the method signatures that the linter will validate. The no-op implementations are appropriate for testdata.
linters/ocm_logging_linter.go (8)
1-16: LGTM! Plugin registration is correct.The imports are appropriate and the
init()function properly registers the plugin.
18-50: LGTM! Excellent documentation.The documentation clearly explains the linter's purpose, provides valid and invalid examples, and documents the suppression mechanisms.
56-58: LGTM! Constructor is straightforward.The
Newfunction correctly instantiates the linter plugin.
60-68: LGTM! Analyzer metadata is correct.The analyzer name and documentation are now properly set (previous issue with template values has been resolved).
70-90: LGTM! Comment suppression logic is correct.The method correctly identifies disabled checks by examining comments on the same line or the line immediately above. It handles both
nolint:ocmloggerandocm-linter:ignoredirectives.
92-112: LGTM! String extraction handles literal concatenation.The method correctly extracts string literals and supports concatenation with the
+operator. Intentionally returnsfalsefor non-literal expressions, which is appropriate since dynamic format strings cannot be statically validated.
114-130: LGTM! Placeholder counting logic is correct.The method correctly counts format placeholders, properly handling escaped
%%sequences by skipping both characters.
132-194: LGTM! Main analyzer logic is sound.The
runmethod correctly:
- Identifies calls to Logger methods (Debug, Info, Warn, Error, Fatal)
- Validates receiver type strictly (exact match on OCM SDK Logger type)
- Extracts and validates literal format strings
- Counts placeholders and arguments accurately
- Respects suppression comments
Note: The critical issue with
TypesInfoavailability (due toLoadModeSyntaxat line 53) must be addressed for this code to function correctly.
| golang.org/x/net v0.21.0 | ||
| golang.org/x/crypto v0.37.0 // indirect | ||
| golang.org/x/net v0.39.0 | ||
| golang.org/x/oauth2 v0.15.0 |
There was a problem hiding this comment.
Critical: Update golang.org/x/oauth2 to address high-severity vulnerability.
The current version v0.15.0 is affected by CVE-2025-22868 (GO-2025-3488), a high-severity DoS vulnerability where a malformed token can cause unexpected memory consumption during parsing. This was flagged in a previous review but remains unresolved.
Apply this fix:
- golang.org/x/oauth2 v0.15.0
+ golang.org/x/oauth2 v0.30.0Then run:
go get golang.org/x/[email protected]
go mod tidyVerify compatibility with your tests before merging.
🧰 Tools
🪛 OSV Scanner (2.2.4)
[HIGH] 24-24: golang.org/x/oauth2 0.15.0: Unexpected memory consumption during token parsing in golang.org/x/oauth2
(GO-2025-3488)
[HIGH] 24-24: golang.org/x/oauth2 0.15.0: golang.org/x/oauth2 Improper Validation of Syntactic Correctness of Input vulnerability
🤖 Prompt for AI Agents
In go.mod around line 24, the dependency golang.org/x/oauth2 is pinned to
v0.15.0 which is vulnerable (CVE-2025-22868); update the module to v0.30.0, then
run "go get golang.org/x/[email protected]" followed by "go mod tidy" to update
go.sum and module graph, and run the test suite/build to verify compatibility
and fix any API changes.
ca93b84 to
4208c4d
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
linters/ocm_logging_linter.go (1)
52-54: Critical: LoadMode must include type information for the analyzer to work.The method returns
register.LoadModeSyntax, but line 153 requirespass.TypesInfo.TypeOf(sel.X)to check receiver types. Without type information,pass.TypesInfowill be nil or incomplete, causing the analyzer to skip all checks.Apply this diff to fix the load mode:
func (f *OcmLoggerLinter) GetLoadMode() string { - return register.LoadModeSyntax + return register.LoadModeTypesInfo }
🧹 Nitpick comments (1)
linters/ocm_logging_linter.go (1)
45-49: Consider removing the commented-out analyzer definition.Lines 45-49 contain a commented-out analyzer definition that appears to be leftover template or example code. Since the actual analyzer is built in
BuildAnalyzers(), this commented section can be removed to improve code clarity.Apply this diff to clean up the documentation:
// Comments can be placed on the same line (or the line immediately above) the call. -// -// var OcmLoggerLinter = &analysis.Analyzer{ -// Name: "ocmlogger", -// Doc: "checks that log calls have matching format strings and arguments", -// Run: run, -// } type OcmLoggerLinter struct{}
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Cache: Disabled due to data retention organization setting
Knowledge base: Disabled due to Reviews -> Disable Knowledge Base setting
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
📒 Files selected for processing (11)
.custom-gcl.yml(1 hunks).golangci.yml(1 hunks)Makefile(1 hunks)examples/go.mod(1 hunks)go.mod(1 hunks)leadership/flag.go(3 hunks)linters/linters_suite_test.go(1 hunks)linters/ocm_logging_linter.go(1 hunks)linters/ocm_logging_linter_test.go(1 hunks)linters/testdata/src/data/data.go(1 hunks)linters/testdata/src/github.com/openshift-online/ocm-sdk-go/logging/logging.go(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (6)
- go.mod
- leadership/flag.go
- linters/testdata/src/data/data.go
- examples/go.mod
- linters/linters_suite_test.go
- .golangci.yml
⏰ 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: Test (1.21, macos-latest)
- GitHub Check: Generate
- GitHub Check: Test (1.21, windows-latest)
- GitHub Check: Test (1.21, ubuntu-latest)
🔇 Additional comments (11)
.custom-gcl.yml (1)
1-10: LGTM! Configuration correctly defines the custom linter plugin.The module-based plugin configuration properly declares the ocmlogger linter and specifies the destination matching the Makefile's
LOCAL_BIN_PATH. Thegolangci-lint customcommand will build the custom binary with the plugin compiled in.Makefile (1)
98-101: LGTM! Lint workflow correctly integrates the custom linter.The two-step process (build custom binary via
golangci-lint custom, then executeocm-lint run) properly implements the custom linter integration. As confirmed in previous discussion,golangci-lint customreads.custom-gcl.ymland creates$(LOCAL_BIN_PATH)/ocm-lintbefore the second command executes.linters/ocm_logging_linter_test.go (1)
11-24: LGTM! Test properly exercises the analyzer using the standard framework.The test correctly uses the
analysistestframework to load the plugin, build analyzers, and validate diagnostics against the testdata. The use ofGinkgoT()appropriately bridges Ginkgo with the analysis test runner.linters/testdata/src/github.com/openshift-online/ocm-sdk-go/logging/logging.go (1)
1-13: LGTM! Logger stub provides the necessary test interface.The no-op Logger implementation with the correct method signatures (context, format string, variadic args) is appropriate for analyzer test data.
linters/ocm_logging_linter.go (7)
14-16: LGTM! Plugin registration follows the standard pattern.The initialization correctly registers the "ocmlogger" plugin with the factory function, aligning with the plugin name used in configuration and tests.
56-58: LGTM! Factory function correctly instantiates the linter.
60-68: LGTM! Analyzer metadata is correct.The analyzer is properly configured with the correct name and documentation.
70-90: LGTM! Directive suppression logic is correctly implemented.The function properly checks for disable comments on the same line or line above, and correctly handles both
nolint:ocmloggerandocm-linter:ignoredirectives.
92-112: LGTM! String extraction handles literals and concatenation appropriately.The method correctly extracts string literals and handles concatenation with the
+operator. Returningfalsefor non-literal expressions is the intended behavior, as documented in the analyzer's logic (lines 170-176).
114-130: LGTM! Placeholder counting correctly handles escaped sequences.The logic properly counts format placeholders while correctly handling escaped
%%sequences. The implementation works for standard format specifiers including those with width/precision modifiers.
132-194: LGTM! Analyzer logic correctly validates format strings and arguments.The implementation properly:
- Identifies calls to logger methods (Debug, Info, Warn, Error, Fatal)
- Verifies receiver type matches the OCM Logger (handling both value and pointer receivers)
- Extracts format strings from the second argument
- Counts placeholders and compares with variadic argument count
- Respects disable directives before reporting mismatches
The logic is sound and the implementation is clear.
ziccardi
force-pushed
the
OCM_LOGGER_LINTER
branch
3 times, most recently
from
be4af62 to
39ab6b9
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (2)
go.mod (1)
24-24: Critical: Update golang.org/x/oauth2 to address high-severity vulnerability.The dependency
golang.org/x/oauth2 v0.15.0remains vulnerable to CVE-2025-22868 (GO-2025-3488), a high-severity DoS vulnerability. This was flagged in a previous review but is still unresolved.Apply this fix:
- golang.org/x/oauth2 v0.15.0 + golang.org/x/oauth2 v0.30.0Then run:
go get golang.org/x/[email protected] go mod tidylinters/ocm_logging_linter.go (1)
52-54: Critical: Fix the load mode to enable type information.
GetLoadMode()returnsregister.LoadModeSyntax, but line 153 usespass.TypesInfo.TypeOf(sel.X)to check receiver types. Type information is unavailable in syntax-only mode, which will cause the analyzer to fail or skip all checks.Apply this diff:
func (f *OcmLoggerLinter) GetLoadMode() string { - return register.LoadModeSyntax + return register.LoadModeTypesInfo }
🧹 Nitpick comments (3)
linters/ocm_logging_linter.go (3)
70-90: Consider caching comment positions for better performance.The
isDisabledmethod scans all comments in all files for every node checked. For large codebases, this could be inefficient.Consider building a map of disabled line numbers once per file:
// In run(), before ast.Inspect: disabledLines := make(map[string]map[int]bool) // filename -> line -> disabled for _, file := range pass.Files { filename := pass.Fset.Position(file.Pos()).Filename disabledLines[filename] = make(map[int]bool) for _, cg := range file.Comments { for _, c := range cg.List { if strings.Contains(c.Text, "nolint:ocmlogger") || strings.Contains(c.Text, "ocm-linter:ignore") { pos := pass.Fset.Position(c.Pos()) disabledLines[filename][pos.Line] = true } } } } // Then check: disabledLines[filename][line] || disabledLines[filename][line-1]
114-130: Consider more robust format string parsing.The current implementation counts any
%followed by a non-%character as a placeholder. This doesn't account for:
- Indexed verbs like
%[1]s(which don't consume sequential arguments)- Width arguments like
%*s(which consume an extra argument)- Invalid format sequences
For more accurate analysis, consider using
fmt's internal format parsing logic or at least handle the width specifier case:func (f *OcmLoggerLinter) countPlaceholders(fmtString string) int { count := 0 for i := 0; i < len(fmtString); i++ { if fmtString[i] != '%' { continue } if i+1 < len(fmtString) && fmtString[i+1] == '%' { i++ // skip %% continue } // Skip to verb (simplified - real parsing is complex) i++ if i < len(fmtString) && fmtString[i] == '*' { count++ // width argument i++ } count++ // the actual placeholder } return count }Note: Full format string parsing is complex. This approach is acceptable for initial implementation.
165-168: Minor: Clarify the comment.The comment states "If the number of parameters is less than 2, it is a compilation error", but 2 parameters (
ctxandmsg) is actually valid—only fewer than 2 would be an error.Consider rewording:
- if len(call.Args) < 2 { - // If the number of parameters is less than 2, it is a compilation error - return true - } + if len(call.Args) < 2 { + // Logger methods require at least 2 arguments (ctx, msg); fewer would be a compilation error + return true + }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Cache: Disabled due to data retention organization setting
Knowledge base: Disabled due to Reviews -> Disable Knowledge Base setting
⛔ Files ignored due to path filters (2)
examples/go.sumis excluded by!**/*.sumgo.sumis excluded by!**/*.sum
📒 Files selected for processing (7)
examples/go.mod(1 hunks)go.mod(2 hunks)linters/linters_suite_test.go(1 hunks)linters/ocm_logging_linter.go(1 hunks)linters/ocm_logging_linter_test.go(1 hunks)linters/testdata/src/data/data.go(1 hunks)linters/testdata/src/github.com/openshift-online/ocm-sdk-go/logging/logging.go(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- linters/testdata/src/data/data.go
- linters/ocm_logging_linter_test.go
🧰 Additional context used
🪛 OSV Scanner (2.2.4)
go.mod
[HIGH] 24-24: golang.org/x/oauth2 0.15.0: Unexpected memory consumption during token parsing in golang.org/x/oauth2
(GO-2025-3488)
[HIGH] 24-24: golang.org/x/oauth2 0.15.0: golang.org/x/oauth2 Improper Validation of Syntactic Correctness of Input vulnerability
⏰ 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: Test (1.21, macos-latest)
- GitHub Check: Test (1.21, windows-latest)
🔇 Additional comments (6)
examples/go.mod (1)
53-57: LGTM! Dependency upgrades aligned with root module.The transitive dependency updates for
golang.org/x/net,golang.org/x/sys,golang.org/x/term, andgolang.org/x/textcorrectly align with the upgrades in the rootgo.mod.go.mod (1)
22-23: LGTM! Dependency updates support the new linter plugin.The additions of
github.com/golangci/plugin-module-registerandgolang.org/x/tools, along with the upgrades to variousgolang.org/x/*packages, appropriately support the new OCM logger linter plugin infrastructure.Also applies to: 29-29, 33-33, 69-73
linters/linters_suite_test.go (1)
1-13: LGTM! Standard Ginkgo test suite bootstrap.The test suite setup follows standard Ginkgo v2 patterns and correctly integrates with the testing framework.
linters/testdata/src/github.com/openshift-online/ocm-sdk-go/logging/logging.go (1)
1-13: LGTM! Appropriate test fixture.The minimal
Loggerstub provides the necessary method signatures (Debug,Info,Warn,Error,Fatal) with correct parameters(context.Context, string, ...any)for testing the linter.linters/ocm_logging_linter.go (2)
132-194: Core analyzer logic is sound (pending LoadMode fix).The main analysis logic correctly:
- Identifies logger method calls
- Validates receiver type against the expected logger type
- Extracts format strings from literals
- Counts placeholders and compares to argument count
- Respects nolint directives
Once the
LoadModeissue (line 52-53) is resolved, this should function correctly.
102-109: Remove or clarify the binary expression handling.Go does not support the
+operator for string literal concatenation at the source level. Adjacent string literals like"hello" "world"are automatically concatenated by the parser without creating aBinaryExprnode. This branch appears unreachable for valid Go code and may cause confusion.If this was intended to handle adjacent literals, they're already concatenated in the
BasicLit. If you want to support string variables being concatenated, note that those wouldn't be analyzable at compile-time anyway. Consider removing this code:func (f *OcmLoggerLinter) extractString(expr ast.Expr) (string, bool) { switch e := expr.(type) { case *ast.BasicLit: if e.Kind == token.STRING { s, err := strconv.Unquote(e.Value) if err != nil { return "", false } return s, true } - case *ast.BinaryExpr: - if e.Op == token.ADD { - left, ok1 := f.extractString(e.X) - right, ok2 := f.extractString(e.Y) - if ok1 && ok2 { - return left + right, true - } - } } return "", false }Likely an incorrect or invalid review comment.
ziccardi
force-pushed
the
OCM_LOGGER_LINTER
branch
2 times, most recently
from
592311b to
a3f2ed4
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
go.mod (1)
24-24:⚠️ Critical: Update golang.org/x/oauth2 to address high-severity vulnerability (UNRESOLVED from prior review).The version v0.15.0 is affected by CVE-2025-22868 (GO-2025-3488) and GHSA-6v2p-p543-phr9—high-severity DoS vulnerabilities in token parsing. This issue was flagged in a previous review and remains unresolved. This is a blocker and must be fixed before merging.
Apply this fix:
- golang.org/x/oauth2 v0.15.0 + golang.org/x/oauth2 v0.30.0Then run:
go get golang.org/x/[email protected] go mod tidyVerify compatibility with your tests before merging.
🧹 Nitpick comments (1)
linters/ocm_logging_linter_test.go (1)
22-22: Consider testing all analyzers instead of just the first.The test currently only runs
analyzers[0]. If the plugin provides multiple analyzers, consider iterating through all of them to ensure complete test coverage.If multiple analyzers are expected, apply this diff:
- analysistest.Run(GinkgoT(), td, analyzers[0], "data") + for _, analyzer := range analyzers { + analysistest.Run(GinkgoT(), td, analyzer, "data") + }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Cache: Disabled due to data retention organization setting
Knowledge base: Disabled due to Reviews -> Disable Knowledge Base setting
⛔ Files ignored due to path filters (2)
examples/go.sumis excluded by!**/*.sumgo.sumis excluded by!**/*.sum
📒 Files selected for processing (7)
examples/go.mod(1 hunks)go.mod(2 hunks)linters/linters_suite_test.go(1 hunks)linters/ocm_logging_linter.go(1 hunks)linters/ocm_logging_linter_test.go(1 hunks)linters/testdata/src/data/data.go(1 hunks)linters/testdata/src/github.com/openshift-online/ocm-sdk-go/logging/logging.go(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (5)
- linters/testdata/src/data/data.go
- linters/linters_suite_test.go
- linters/ocm_logging_linter.go
- linters/testdata/src/github.com/openshift-online/ocm-sdk-go/logging/logging.go
- examples/go.mod
🧰 Additional context used
🪛 OSV Scanner (2.2.4)
go.mod
[HIGH] 24-24: golang.org/x/oauth2 0.15.0: Unexpected memory consumption during token parsing in golang.org/x/oauth2
(GO-2025-3488)
[HIGH] 24-24: golang.org/x/oauth2 0.15.0: golang.org/x/oauth2 Improper Validation of Syntactic Correctness of Input vulnerability
⏰ 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: Test (1.21, windows-latest)
- GitHub Check: Test (1.21, macos-latest)
- GitHub Check: Test (1.21, ubuntu-latest)
🔇 Additional comments (3)
go.mod (2)
22-23: Verify compatibility of golang.org/x/crypto and golang.org/x/net updates.The updates from v0.20.0 → v0.37.0 (crypto) and v0.21.0 → v0.39.0 (net) are substantial. Ensure these version jumps do not introduce breaking changes with your OAuth2/TLS usage patterns or the linter infrastructure.
29-29: ✅ New linter infrastructure dependencies look appropriate.The additions of
github.com/golangci/plugin-module-register v0.1.2andgolang.org/x/tools v0.32.0align well with the PR's objective to add a custom OCM logger linter. Placing them in the secondrequireblock is the correct pattern for toolchain dependencies. The indirect dependency updates (lines 69–73) are expected transitive consequences.Also applies to: 33-33, 69-73
linters/ocm_logging_linter_test.go (1)
17-17: No issues found. The code is correct.The nil parameter is intentionally handled by the plugin's
Newfunction signature acceptinganytype. TheBuildAnalyzers()method always returns a slice with exactly one analyzer, making the access toanalyzers[0]safe and the test coverage complete.
| analyzers, err := plugin.BuildAnalyzers() | ||
| gomega.Expect(err).NotTo(gomega.HaveOccurred()) | ||
| analysistest.Run(GinkgoT(), td, analyzers[0], "data") |
There was a problem hiding this comment.
Add bounds check before accessing analyzers[0].
The code accesses analyzers[0] without verifying the slice is non-empty, which will panic if BuildAnalyzers() returns an empty slice.
Apply this diff to add a bounds check:
analyzers, err := plugin.BuildAnalyzers()
gomega.Expect(err).NotTo(gomega.HaveOccurred())
+ gomega.Expect(analyzers).NotTo(gomega.BeEmpty())
analysistest.Run(GinkgoT(), td, analyzers[0], "data")📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| analyzers, err := plugin.BuildAnalyzers() | |
| gomega.Expect(err).NotTo(gomega.HaveOccurred()) | |
| analysistest.Run(GinkgoT(), td, analyzers[0], "data") | |
| analyzers, err := plugin.BuildAnalyzers() | |
| gomega.Expect(err).NotTo(gomega.HaveOccurred()) | |
| gomega.Expect(analyzers).NotTo(gomega.BeEmpty()) | |
| analysistest.Run(GinkgoT(), td, analyzers[0], "data") |
🤖 Prompt for AI Agents
In linters/ocm_logging_linter_test.go around lines 20 to 22, the test indexes
analyzers[0] without ensuring the slice is non-empty; add a bounds check by
asserting the slice has at least one element (e.g.
gomega.Expect(analyzers).NotTo(gomega.BeEmpty()) or
gomega.Expect(len(analyzers)).To(gomega.BeNumerically(">", 0))) immediately
after the BuildAnalyzers call, then proceed to call analysistest.Run with
analyzers[0]; ensure the test fails clearly if the slice is empty.
openshift-ci
bot
added
the
approved
openshift-merge-robot
added
the
needs-rebase
|
@ziccardi, can you please resolve the conflicts? |
Linting issues in using ocm logger were not detected by the linter because the logger methods are not ending with `f` and the first parameter is not the string format. This PR adds a custom linter to detect them.
ziccardi
force-pushed
the
OCM_LOGGER_LINTER
branch
from
a3f2ed4 to
5dbf4ac
Compare
openshift-merge-robot
removed
the
needs-rebase
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (2)
go.mod (1)
22-24: Still using vulnerable golang.org/x/oauth2 v0.15.0 – please bump to a fixed release.Static analysis (and a prior review) flag v0.15.0 as affected by high‑severity issues (e.g. GO-2025-3488 / GHSA-6v2p-p543-phr9). It’s safer to update this along with the other x/* bumps, e.g. to v0.30.0 (or the current recommended version) and re‑run tests.
A concrete change you can apply:
- golang.org/x/oauth2 v0.15.0 + golang.org/x/oauth2 v0.30.0Then locally run:
go get golang.org/x/[email protected] go mod tidy go test ./...to verify compatibility and adjust if newer API changes require it.
linters/ocm_logging_linter.go (1)
47-54: Load mode must include type info since run() relies on pass.TypesInfo.
runcallspass.TypesInfo.TypeOf(sel.X), butGetLoadModecurrently returnsregister.LoadModeSyntax, which may not populateTypesInfoand can lead to missing type information or panics depending on the loader. This linter needs a load mode that provides type info.Suggested fix:
-func (l *OcmLoggerLinter) GetLoadMode() string { return register.LoadModeSyntax } +func (l *OcmLoggerLinter) GetLoadMode() string { return register.LoadModeTypesInfo }After this change, verify that the plugin still loads correctly under golangci-lint with the updated
golang.org/x/toolsand thatpass.TypesInfois non‑nil in your tests.Also applies to: 158-193
🧹 Nitpick comments (1)
linters/ocm_logging_linter.go (1)
140-156: Placeholder counting is simple and may flag advanced fmt patterns.
countPlaceholderstreats every non‑%%%as a placeholder, which is fine for typical log messages but will miscount patterns like%*dor indexed verbs (%[2]d) where a single placeholder consumes multiple or reused arguments. If such patterns are common in logs, consider tightening the parser to better approximatefmtsemantics; otherwise this is acceptable as a deliberate trade‑off.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Cache: Disabled due to data retention organization setting
Knowledge base: Disabled due to Reviews -> Disable Knowledge Base setting
⛔ Files ignored due to path filters (2)
examples/go.sumis excluded by!**/*.sumgo.sumis excluded by!**/*.sum
📒 Files selected for processing (7)
examples/go.mod(1 hunks)go.mod(2 hunks)linters/linters_suite_test.go(1 hunks)linters/ocm_logging_linter.go(1 hunks)linters/ocm_logging_linter_test.go(1 hunks)linters/testdata/src/data/data.go(1 hunks)linters/testdata/src/github.com/openshift-online/ocm-sdk-go/logging/logging.go(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- linters/linters_suite_test.go
- linters/ocm_logging_linter_test.go
🧰 Additional context used
🧬 Code graph analysis (1)
linters/testdata/src/data/data.go (1)
linters/testdata/src/github.com/openshift-online/ocm-sdk-go/logging/logging.go (6)
Logger(7-7)Logger(9-9)Logger(10-10)Logger(11-11)Logger(12-12)Logger(13-13)
🪛 OSV Scanner (2.2.4)
go.mod
[HIGH] 24-24: golang.org/x/oauth2 0.15.0: Unexpected memory consumption during token parsing in golang.org/x/oauth2
(GO-2025-3488)
[HIGH] 24-24: golang.org/x/oauth2 0.15.0: golang.org/x/oauth2 Improper Validation of Syntactic Correctness of Input vulnerability
⏰ 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). (5)
- GitHub Check: Generate
- GitHub Check: Test (1.21, ubuntu-latest)
- GitHub Check: Test (1.21, windows-latest)
- GitHub Check: Test (1.21, macos-latest)
- GitHub Check: Lint
🔇 Additional comments (3)
examples/go.mod (1)
53-57: Examples module deps aligned with root go.mod.Updating the x/net, x/sys, x/term, and x/text versions here to match the root module keeps the examples in lockstep with the main SDK and looks good.
linters/testdata/src/data/data.go (1)
1-50: Testdata nicely exercises formatting, %% handling, and nolint behavior.These cases cover literal vs dynamic formats, correct counting of placeholders with
%%, and both same‑line and previous‑line//nolint:ocmloggersuppression, which matches the linter logic and should give good confidence in behavior.linters/testdata/src/github.com/openshift-online/ocm-sdk-go/logging/logging.go (1)
1-13: Logger stub matches the linter’s expected receiver type.The stub’s type and method signatures line up with the string type checks in the linter (
logging.Logger/*logging.Logger), so it’s a good fit for testdata without adding runtime behavior.
|
/lgtm |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: andreadecorte, oriAdler, ziccardi The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
|
security/snyk failure seems like a known issue https://redhat-internal.slack.com/archives/C06M02W2L01/p1753702448730799 |
|
@ziccardi, thanks for adding this enhancement :) |
|
Verified the changes locally: |
4 participants
Linting issues in using ocm logger were not detected by the linter because the logger methods are not ending with
fand the first parameter is not the string format. This PR adds a custom linter to detect them.This PR just adds the linter code. A following PR will enable it into the Makefile
linter output example:
How to run it locally
This MR does not enable the linter because otherwise the linting step of the CI will fail. The reason is that the linter runs with the linters from
masterand doesn't see the new one introduced by this PR.That means that if you want to test this PR locally you will have to add the missing configuration manually
linterssection:linttarget: