chore(ci): add pathconcat custom linter plugin#3266
chore(ci): add pathconcat custom linter plugin#3266jakedoublev wants to merge 2 commits intomainfrom
Conversation
Integrates github.com/jakedoublev/pathconcat as a golangci-lint module plugin to detect string-based path/URL concatenation (x + "/" + y, fmt.Sprintf with path separators, strings.Join with "/") and suggest path.Join, filepath.Join, or url.JoinPath instead. - Add .custom-gcl.yml to build custom golangci-lint binary with plugin - Enable pathconcat in .golangci.yaml with ignore-strings for FQN patterns - Update Makefile go-lint target to build and use custom-gcl binary - Update CI workflow to build custom binary and restore --new-from-rev Signed-off-by: jakedoublev <jake.vanvorhis@virtru.com>
Summary of ChangesHello, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request enhances the project's static analysis capabilities by introducing a custom golangci-lint plugin designed to enforce safer path and URL construction practices. By automating the build of a custom linter binary and integrating it into the existing linting workflow, the changes ensure consistent code quality while allowing for necessary exceptions through configuration. Highlights
🧠 New Feature in Public Preview: You can now enable Memory to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console. Ignored Files
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for GitHub and other Google products, sign up here. Paths joined by strings can be a snare, Leading to bugs beyond compare. With a new plugin to check the way, Our code stays clean and safe today. Footnotes
|
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository UI Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughAdds a custom golangci-lint wrapper and enables a pathconcat plugin; integrates the custom linter into CI, the Makefile, and .gitignore, and provides plugin configuration in Changes
sequenceDiagram
participant Dev as Developer
participant GH as GitHub Actions
participant GoTool as Go toolchain
participant Build as Build/Wrapper (custom-gcl)
participant Linter as golangci-lint
participant Repo as Repository
Dev->>Repo: push PR / commit
GH->>Repo: checkout code
GH->>GoTool: go install golangci-lint@v2.8.0
GH->>GoTool: go build -o custom-gcl ./path/to/wrapper
GH->>Build: run ./custom-gcl run -c .golangci.yaml (with PR new-from-rev if applicable)
Build->>Linter: invoke golangci-lint with configured plugins (pathconcat)
Linter->>Build: lint results
Build->>GH: exit code / annotations
GH->>Dev: CI status/results
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request integrates a custom golangci-lint plugin called pathconcat to detect string path/URL concatenation. It introduces a .custom-gcl.yml configuration, updates .golangci.yaml to enable the linter, and modifies the Makefile to build and use the custom linter binary. Feedback was provided to optimize the Makefile by removing the custom-gcl target from .PHONY to prevent redundant builds and adding .custom-gcl.yml as a dependency for that target to ensure the binary is correctly updated when configuration changes.
| # To run all lint checks: `LINT_OPTIONS= make lint` | ||
|
|
||
| .PHONY: all build clean connect-wrapper-generate docker-build fix fmt go-lint license lint proto-generate proto-lint sdk/sdk test tidy toolcheck | ||
| .PHONY: all build clean connect-wrapper-generate custom-gcl docker-build fix fmt go-lint license lint proto-generate proto-lint sdk/sdk test tidy toolcheck |
There was a problem hiding this comment.
The custom-gcl target should not be marked as .PHONY. Since it produces a binary file named custom-gcl, marking it as .PHONY forces the custom linter to be rebuilt every time make lint or make go-lint is called. Building a custom golangci-lint binary is a relatively slow process as it involves compiling plugins, so avoiding unnecessary rebuilds will improve developer productivity and CI efficiency.
.PHONY: all build clean connect-wrapper-generate docker-build fix fmt go-lint license lint proto-generate proto-lint sdk/sdk test tidy toolcheck
| custom-gcl: | ||
| golangci-lint custom |
There was a problem hiding this comment.
To ensure the custom linter binary is rebuilt when its configuration changes, add .custom-gcl.yml as a dependency for the custom-gcl target. This, combined with removing it from .PHONY, ensures the binary is only built when necessary. Additionally, consider adding custom-gcl to your clean target (though it is outside the current diff) to ensure the binary is removed during cleanup.
custom-gcl: .custom-gcl.yml
golangci-lint custom
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.github/workflows/checks.yaml:
- Around line 90-93: Replace per-matrix-step compilation by adding a dedicated
prep job that builds the custom golangci-lint binary once and publishes it as an
artifact (e.g., "custom-gcl"); then change the matrix jobs that currently run
the steps named "install golangci-lint" and "build custom golangci-lint" to
instead download that artifact (or restore from cache) before running linting.
Ensure the prep job runs before the matrix (use needs or job dependencies),
upload the built binary with upload-artifact, and in each matrix job use
download-artifact (or cache) to reuse the same "custom-gcl" binary rather than
rebuilding in every leg.
In @.golangci.yaml:
- Around line 77-85: Add short inline rationale comments explaining why the
three broad ignore-strings ("/attr/", "/value/", "/obl/") exist so future
editors don't remove or change them inadvertently; update the .golangci.yaml
custom -> pathconcat -> settings -> ignore-strings block to include one-line
comments for each pattern describing what cases they suppress (e.g., internal
API paths, known URL fragments, or legacy routes) and a note about the risk of
narrowing them, referencing the keys "custom", "pathconcat", "settings", and
"ignore-strings" so reviewers can find the explanation easily.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 4c682650-d9a3-4635-b921-b78529b9dd42
📒 Files selected for processing (5)
.custom-gcl.yml.github/workflows/checks.yaml.gitignore.golangci.yamlMakefile
| - name: install golangci-lint | ||
| run: go install github.com/golangci/golangci-lint/v2/cmd/golangci-lint@v2.8.0 | ||
| - name: build custom golangci-lint | ||
| run: golangci-lint custom |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Build custom-gcl once per workflow and reuse across matrix jobs.
Compiling the custom binary in every matrix leg adds avoidable CI time. Consider a dedicated prep job that builds custom-gcl once and shares it via artifact/cache.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In @.github/workflows/checks.yaml around lines 90 - 93, Replace per-matrix-step
compilation by adding a dedicated prep job that builds the custom golangci-lint
binary once and publishes it as an artifact (e.g., "custom-gcl"); then change
the matrix jobs that currently run the steps named "install golangci-lint" and
"build custom golangci-lint" to instead download that artifact (or restore from
cache) before running linting. Ensure the prep job runs before the matrix (use
needs or job dependencies), upload the built binary with upload-artifact, and in
each matrix job use download-artifact (or cache) to reuse the same "custom-gcl"
binary rather than rebuilding in every leg.
| custom: | ||
| pathconcat: | ||
| type: module | ||
| description: "Detects string path/URL concatenation; suggests filepath.Join, path.Join, or url.JoinPath" | ||
| settings: | ||
| ignore-strings: | ||
| - "/attr/" | ||
| - "/value/" | ||
| - "/obl/" |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Add rationale comments for broad ignore-strings patterns.
Line 82–85 uses substring suppressions that are intentionally broad; a short inline note will reduce future accidental edits/regressions.
📝 Suggested clarity update
custom:
pathconcat:
type: module
description: "Detects string path/URL concatenation; suggests filepath.Join, path.Join, or url.JoinPath"
settings:
+ # Suppress known FQN token patterns that are not filesystem/URL path construction.
ignore-strings:
- "/attr/"
- "/value/"
- "/obl/"📝 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.
| custom: | |
| pathconcat: | |
| type: module | |
| description: "Detects string path/URL concatenation; suggests filepath.Join, path.Join, or url.JoinPath" | |
| settings: | |
| ignore-strings: | |
| - "/attr/" | |
| - "/value/" | |
| - "/obl/" | |
| custom: | |
| pathconcat: | |
| type: module | |
| description: "Detects string path/URL concatenation; suggests filepath.Join, path.Join, or url.JoinPath" | |
| settings: | |
| # Suppress known FQN token patterns that are not filesystem/URL path construction. | |
| ignore-strings: | |
| - "/attr/" | |
| - "/value/" | |
| - "/obl/" |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In @.golangci.yaml around lines 77 - 85, Add short inline rationale comments
explaining why the three broad ignore-strings ("/attr/", "/value/", "/obl/")
exist so future editors don't remove or change them inadvertently; update the
.golangci.yaml custom -> pathconcat -> settings -> ignore-strings block to
include one-line comments for each pattern describing what cases they suppress
(e.g., internal API paths, known URL fragments, or legacy routes) and a note
about the risk of narrowing them, referencing the keys "custom", "pathconcat",
"settings", and "ignore-strings" so reviewers can find the explanation easily.
Benchmark results, click to expandBenchmark authorization.GetDecisions Results:
Benchmark authorization.v2.GetMultiResourceDecision Results:
Benchmark Statistics
Bulk Benchmark Results
TDF3 Benchmark Results:
|
Signed-off-by: jakedoublev <jake.vanvorhis@virtru.com>
Benchmark results, click to expandBenchmark authorization.GetDecisions Results:
Benchmark authorization.v2.GetMultiResourceDecision Results:
Benchmark Statistics
Bulk Benchmark Results
TDF3 Benchmark Results:
|
|
1 participant
Summary
x + "/" + y,fmt.Sprintf("%s/%s", ...),strings.Join(parts, "/")) and suggestspath.Join,filepath.Join, orurl.JoinPath/attr/,/value/,/obl/), connection strings, and scheme prefixesChanges
.custom-gcl.yml— build config for custom golangci-lint binary with the plugin.golangci.yaml— enablepathconcatlinter withignore-stringsfor FQN patternsMakefile—go-linttarget now buildscustom-gclfirst, then uses it.github/workflows/checks.yaml— build custom binary in CI, restore--new-from-revfor PR-only issue reporting.gitignore— ignore thecustom-gclbinaryTest plan
make lintbuildscustom-gcland runs successfullyactions/setup-goproblem matcherlib/identifier/obligation.goare not flaggedSummary by CodeRabbit