Conversation
WalkthroughThis pull request performs a comprehensive library version upgrade across the entire codebase, updating all import paths from Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 minutes Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 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 |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #549 +/- ##
=======================================
Coverage 28.80% 28.80%
=======================================
Files 175 175
Lines 7062 7062
=======================================
Hits 2034 2034
Misses 4911 4911
Partials 117 117 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
temporal/logger.go (1)
29-31:⚠️ Potential issue | 🔴 CriticalFix
Warnto callWarninstead ofErrorf.The
Warnmethod currently escalates warnings to error level. The temporal SDK'slog.Loggerinterface uses structured logging with aWarn(msg string, keyvals ...interface{})method—notWarnf. Update the call accordingly:Fix
func (l logger) Warn(msg string, keyvals ...interface{}) { - l.logger.WithFields(keyvalsToMap(keyvals...)).Errorf(msg) + l.logger.WithFields(keyvalsToMap(keyvals...)).Warn(msg) }Note: The internal
logging.Loggerinterface also lacks aWarnfmethod, so this change requires either addingWarnto that interface or temporarily mapping warnings to errors until the underlying logger is updated.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@temporal/logger.go` around lines 29 - 31, The Warn method currently escalates warnings by calling l.logger.Errorf; change it to call the underlying logger's Warn (not Warnf/Errorf) so warnings are logged at the correct level—update the implementation in function logger.Warn (and the call that uses keyvalsToMap) to invoke Warn on the embedded logging.Logger; if logging.Logger does not yet declare a Warn method, either add Warn(msg string, keyvals ...interface{}) to that interface or document/implement a temporary mapping that forwards Warn to an appropriate existing method without using Errorf.
🧹 Nitpick comments (2)
queue/listener_test.go (1)
4-4: Misplaced//nolint: gosecon a stdlibcontextimport.
gosechas no rule that fires on a bare"context"import. This suppression is a no-op and is likely a copy-paste artifact from a file that suppressesgosecon a crypto import (e.g.,crypto/md5as seen inlocalstack_integration_test.goline 5). Safe to remove.🧹 Proposed cleanup
- "context" //nolint: gosec + "context"🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@queue/listener_test.go` at line 4, Remove the unnecessary gosec suppression on the standard library context import in queue/listener_test.go by deleting the "//nolint: gosec" comment next to the "context" import; the import of "context" does not trigger any gosec rule so simply keep the import as "context" without the nolint annotation to clean up the file.migrations/migrator.go (1)
378-384: Pre-existing dead code: secondif err != nilis unreachable.After the first
if err != nil { return err }(line 378),erris guaranteed to benilat line 381, making the second guard block permanently unreachable. The second block also silently swallows the error (returnsnil), which differs from the first block's behaviour. Consider removing it.♻️ Proposed cleanup
_, err = conn.Exec(ctx, ` update `+m.getVersionsTable()+` set actual_counter = coalesce(actual_counter, 0) + $1 where version_id = $2 `, increment, lastVersion+1) if err != nil { return err } - if err != nil { - logging.FromContext(ctx).Debugf("failed to update actual counter: %v", err) - return nil - }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@migrations/migrator.go` around lines 378 - 384, The second `if err != nil` block after the first `if err != nil { return err }` is unreachable and silently swallows errors; remove that second guard and consolidate error handling for the `err` variable — either log the error using logging.FromContext(ctx).Debugf("failed to update actual counter: %v", err) and then return err, or simply keep the existing `return err` behavior from the first check; ensure no duplicate `if err != nil` remains around the "failed to update actual counter" log statement.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@temporal/logger.go`:
- Around line 29-31: The Warn method currently escalates warnings by calling
l.logger.Errorf; change it to call the underlying logger's Warn (not
Warnf/Errorf) so warnings are logged at the correct level—update the
implementation in function logger.Warn (and the call that uses keyvalsToMap) to
invoke Warn on the embedded logging.Logger; if logging.Logger does not yet
declare a Warn method, either add Warn(msg string, keyvals ...interface{}) to
that interface or document/implement a temporary mapping that forwards Warn to
an appropriate existing method without using Errorf.
---
Nitpick comments:
In `@migrations/migrator.go`:
- Around line 378-384: The second `if err != nil` block after the first `if err
!= nil { return err }` is unreachable and silently swallows errors; remove that
second guard and consolidate error handling for the `err` variable — either log
the error using logging.FromContext(ctx).Debugf("failed to update actual
counter: %v", err) and then return err, or simply keep the existing `return err`
behavior from the first check; ensure no duplicate `if err != nil` remains
around the "failed to update actual counter" log statement.
In `@queue/listener_test.go`:
- Line 4: Remove the unnecessary gosec suppression on the standard library
context import in queue/listener_test.go by deleting the "//nolint: gosec"
comment next to the "context" import; the import of "context" does not trigger
any gosec rule so simply keep the import as "context" without the nolint
annotation to clean up the file.
gfyrag
approved these changes
Feb 20, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
release go-libs v4 since #546 is a breaking change