chore: Add Code Static Analysis GH Workflow#56
Conversation
|
Warning Rate limit exceeded
Your organization is not enrolled in usage-based pricing. Contact your admin to enable usage-based pricing to continue reviews beyond the rate limit, or try again in 46 minutes and 37 seconds. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (2)
WalkthroughAdded CI static-analysis workflow and GolangCI-Lint config, updated go.mod tooling/dependencies, migrated Neo4j driver usage to v6 (context-aware APIs) across driver and transaction-related code, refactored relationship creation in opengraph, and applied small local code cleanups. Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant DriverLib as Neo4jDriver(v6)
participant Session
participant Transaction
participant Database
Client->>DriverLib: NewSession(ctx, cfg)
DriverLib->>Session: return session(handle)
Client->>Session: BeginTransaction(ctx, opts)
Session->>Transaction: return explicit transaction
Client->>Transaction: Run(ctx, statement, params)
Transaction->>Database: execute statement
Database-->>Transaction: result
Client->>Transaction: Commit(ctx)
Transaction->>Database: commit
Client->>Session: Close(ctx)
DriverLib->>Client: Close(ctx)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 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 |
![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
🧹 Nitpick comments (3)
.github/workflows/run-static-analysis.yml (2)
42-45: Consider adding output formatting for better CI integration.Adding output formatting (e.g.,
--out-format=github-actions) provides better integration with GitHub's annotation system, making issues easier to discover directly in the PR diff view.💡 Proposed enhancement
- name: Run Analysis run: | - go tool golangci-lint run ./... + go tool golangci-lint run --out-format=github-actions ./...🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.github/workflows/run-static-analysis.yml around lines 42 - 45, Update the "Run Analysis" workflow step so the golangci-lint invocation emits GitHub Actions annotations: modify the command "go tool golangci-lint run ./..." to include the output format flag (e.g., add --out-format=github-actions) so lint findings surface as PR annotations; ensure the step name "Run Analysis" keeps continue-on-error: true if desired.
24-26: Consider addingreopenedevent type for completeness.The workflow triggers on
openedandsynchronizeevents but notreopened. Addingreopenedensures static analysis runs when a closed PR is reopened.✨ Proposed addition
types: - "opened" - "synchronize" + - "reopened"🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.github/workflows/run-static-analysis.yml around lines 24 - 26, Add the "reopened" pull_request type to the workflow trigger types so the static analysis job also runs when a previously closed PR is reopened; update the types array (currently containing "opened" and "synchronize") to include "reopened" alongside those entries to ensure the workflow triggers on reopened PRs as well.go.mod (1)
3-3: Consider updating Go version to 1.25.8 or later.Go 1.25.0 is a valid, released version. However, Go 1.25.8 is available as a more recent patch in the same minor version series, and Go 1.26.1 is the latest stable release. Using an older patch version may miss important security fixes and improvements. Update
go 1.25.0to a more current version.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@go.mod` at line 3, Update the Go toolchain version in go.mod by replacing the existing directive "go 1.25.0" with a newer patch/minor release (e.g., "go 1.25.8" or "go 1.26.1") so the module uses a Go release that includes recent security fixes and improvements; locate the "go 1.25.0" directive in go.mod and change it to the chosen newer version.
🤖 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/run-static-analysis.yml:
- Around line 42-45: The CI step named "Run Analysis" currently sets
continue-on-error: true which allows golangci-lint failures to pass; remove the
continue-on-error: true key from that step so lint failures fail the job, and
optionally update the run command (the golangci-lint invocation) to use
--new-from-rev=<rev> or other flags if you want to enforce only new issues;
ensure you only modify the "Run Analysis" step and retain the existing
golangci-lint run ./... invocation otherwise.
In @.golangci.yml:
- Around line 3-4: The configuration currently disables the entire errcheck
linter; re-enable errcheck (remove the "- errcheck" entry) and, if certain
unchecked errors must be ignored, add targeted exclusions instead using
golangci-lint's configuration (e.g., add linters-settings for errcheck or use
exclude/exclude-rules or excludePatterns to silence specific files, functions,
or error-return patterns). Ensure the symbol "errcheck" is not listed under
disable and create precise exclude rules for known safe exceptions rather than
disabling the linter globally.
---
Nitpick comments:
In @.github/workflows/run-static-analysis.yml:
- Around line 42-45: Update the "Run Analysis" workflow step so the
golangci-lint invocation emits GitHub Actions annotations: modify the command
"go tool golangci-lint run ./..." to include the output format flag (e.g., add
--out-format=github-actions) so lint findings surface as PR annotations; ensure
the step name "Run Analysis" keeps continue-on-error: true if desired.
- Around line 24-26: Add the "reopened" pull_request type to the workflow
trigger types so the static analysis job also runs when a previously closed PR
is reopened; update the types array (currently containing "opened" and
"synchronize") to include "reopened" alongside those entries to ensure the
workflow triggers on reopened PRs as well.
In `@go.mod`:
- Line 3: Update the Go toolchain version in go.mod by replacing the existing
directive "go 1.25.0" with a newer patch/minor release (e.g., "go 1.25.8" or "go
1.26.1") so the module uses a Go release that includes recent security fixes and
improvements; locate the "go 1.25.0" directive in go.mod and change it to the
chosen newer version.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: dc996af0-15d4-41ee-aaa3-ba850ae459f3
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
📒 Files selected for processing (3)
.github/workflows/run-static-analysis.yml.golangci.ymlgo.mod
| disable: | ||
| - errcheck |
There was a problem hiding this comment.
Critical: Do not disable the errcheck linter.
Disabling errcheck removes detection of unchecked error returns, which is a common source of correctness bugs and security vulnerabilities in Go. Unchecked errors can lead to silent failures, data corruption, resource leaks, and security issues.
If specific error checks need to be excluded, use targeted exclusions rather than disabling the entire linter.
✅ Proposed fix: Enable errcheck with targeted exclusions if needed
-linters:
- disable:
- - errcheck
+linters:
+ enable:
+ - errcheck
+# If specific exclusions are needed:
+# linters-settings:
+# errcheck:
+# exclude-functions:
+# - fmt.Print.*📝 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.
| disable: | |
| - errcheck | |
| linters: | |
| enable: | |
| - errcheck | |
| # If specific exclusions are needed: | |
| # linters-settings: | |
| # errcheck: | |
| # exclude-functions: | |
| # - fmt.Print.* |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In @.golangci.yml around lines 3 - 4, The configuration currently disables the
entire errcheck linter; re-enable errcheck (remove the "- errcheck" entry) and,
if certain unchecked errors must be ignored, add targeted exclusions instead
using golangci-lint's configuration (e.g., add linters-settings for errcheck or
use exclude/exclude-rules or excludePatterns to silence specific files,
functions, or error-return patterns). Ensure the symbol "errcheck" is not listed
under disable and create precise exclude rules for known safe exceptions rather
than disabling the linter globally.
| check-latest: true | ||
|
|
||
| - name: Run Analysis | ||
| continue-on-error: true |
There was a problem hiding this comment.
I agree with the 🐇 here in that we would benefit from hard failing given that the static analysis is of value. We may need to tackle the current findings in order to enable merging this check into main
There was a problem hiding this comment.
Thank you, @urangel, for your feedback. Going forward, our CI will fail on errors. I also pushed changes to address the major issues flagged by staticcheck and to fix formatting.
There was a problem hiding this comment.
🧹 Nitpick comments (1)
drivers/neo4j/result.go (1)
46-48: Consider propagating context through the result interface.Using
context.Background()here means that result iteration won't respect the caller's context cancellation or timeout. If the upstreamneo4jTransactionhas a deadline and the database is slow to return rows, this iteration will continue regardless.This appears to be a design constraint of the current
graph.Resultinterface, but worth noting as a potential improvement area for timeout-sensitive workloads.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@drivers/neo4j/result.go` around lines 46 - 48, The Next method on internalResult currently calls s.driverResult.Next(context.Background()), which ignores caller cancellation; update the graph.Result contract and implementation to propagate context: change the graph.Result.Next signature to Next(ctx context.Context) (or add NextContext(ctx context.Context) if you need backward compatibility), then update internalResult.Next to call s.driverResult.Next(ctx) and adjust all callers (including neo4jTransaction usage sites) to pass through the originating context/deadline. Ensure all other Result implementations and tests are updated to match the new signature so iteration respects cancellation/timeouts.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@drivers/neo4j/result.go`:
- Around line 46-48: The Next method on internalResult currently calls
s.driverResult.Next(context.Background()), which ignores caller cancellation;
update the graph.Result contract and implementation to propagate context: change
the graph.Result.Next signature to Next(ctx context.Context) (or add
NextContext(ctx context.Context) if you need backward compatibility), then
update internalResult.Next to call s.driverResult.Next(ctx) and adjust all
callers (including neo4jTransaction usage sites) to pass through the originating
context/deadline. Ensure all other Result implementations and tests are updated
to match the new signature so iteration respects cancellation/timeouts.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 311fe747-77b7-4a40-b839-036e54dc374e
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
📒 Files selected for processing (22)
.github/workflows/run-static-analysis.yml.golangci.ymlcmd/benchmark/main.gocontainer/bfs.gocontainer/pacmap.gocypher/frontend/query.gocypher/models/pgsql/translate/expression.godrivers/neo4j/batch.godrivers/neo4j/driver.godrivers/neo4j/mapper.godrivers/neo4j/neo4j.godrivers/neo4j/node.godrivers/neo4j/relationship.godrivers/neo4j/result.godrivers/neo4j/result_internal_test.godrivers/neo4j/transaction.godrivers/neo4j/wrapper.gogo.modgraph/properties.goopengraph/load.goutil/errors.goutil/errors_test.go
✅ Files skipped from review due to trivial changes (7)
- container/pacmap.go
- cypher/models/pgsql/translate/expression.go
- util/errors_test.go
- container/bfs.go
- cypher/frontend/query.go
- cmd/benchmark/main.go
- .golangci.yml
🚧 Files skipped from review as they are similar to previous changes (2)
- .github/workflows/run-static-analysis.yml
- go.mod
2 participants
Description
golangci-lintas tool dependency ingo.modstaticchecklinter and formatting error.v5.28.4tov6.0.0Resolves: PQE-404
Type of Change
Testing
go test -tags manual_integration ./integration/...)Screenshots (if appropriate):
Driver Impact
drivers/pg)drivers/neo4j)Checklist
go.mod/go.sumare up to date if dependencies changedSummary by CodeRabbit