Conversation
Features: - JWT authentication with automatic token refresh - Entry CRUD operations (Create, Read, Update, Delete, Merge) - Attachment upload and download - Type-safe query builder for AR qualifications - Built-in request serialization (prevents BMC Error 9093) - Token bucket rate limiting - Context-aware with cancellation support - Zero external runtime dependencies (stdlib only) Token refresh uses lazy evaluation with double-check locking for thread-safe credential management and automatic re-authentication.
|
Caution Review failedThe pull request is closed. Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. WalkthroughAdds a new go-remedy client library: typed models and interfaces, a thread-safe Client with JWT auth and auto-refresh, per-client request queue and optional rate limiter, entry CRUD and attachment streaming/upload, a query-builder, centralized API error types, CI/lint/configuration, and comprehensive unit tests. Changes
Sequence Diagram(s)mermaid mermaid Estimated code review effort🎯 4 (Complex) | ⏱️ ~60–75 minutes
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
📜 Recent review detailsConfiguration used: Organization UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (5)
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 |
|
Review the following changes in direct dependencies. Learn more about Socket for GitHub.
|
Summary of ChangesHello @tphakala, 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 delivers the initial implementation of Highlights
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. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
![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 introduces the initial implementation of go-remedy, a Go client library for the BMC Remedy AR System REST API. The implementation is comprehensive, covering authentication with auto-refresh, CRUD operations, attachments, rate limiting, and request serialization. The code is well-structured with good use of interfaces for testability and internal packages for implementation details. The test suite is extensive and covers many edge cases, especially for the complex token refresh and concurrency logic.
I've found a few issues, including a potential goroutine leak, an incorrect JSON tag, and a minor state inconsistency during logout. I've also suggested an improvement to an example in the README to promote better error handling practices. Overall, this is a very strong initial implementation.
| req, cancel, err := s.client.newRequest(ctx, http.MethodPost, path, pr) | ||
| if err != nil { | ||
| return fmt.Errorf("creating upload request: %w", err) | ||
| } |
There was a problem hiding this comment.
There is a potential goroutine leak in the Upload function. If s.client.newRequest returns an error, the function returns, but the goroutine started earlier to handle the multipart writing is not terminated. It may block forever on writing to the pipe pw, as nothing will be reading from the other end pr. To fix this, you should close the pipe writer with an error to unblock the goroutine.
| req, cancel, err := s.client.newRequest(ctx, http.MethodPost, path, pr) | |
| if err != nil { | |
| return fmt.Errorf("creating upload request: %w", err) | |
| } | |
| req, cancel, err := s.client.newRequest(ctx, http.MethodPost, path, pr) | |
| if err != nil { | |
| _ = pw.CloseWithError(err) // Unblock the writer goroutine | |
| return fmt.Errorf("creating upload request: %w", err) | |
| } |
README.md
Outdated
| file, _ := os.Open("document.pdf") | ||
| defer file.Close() | ||
|
|
||
| err := client.Attachments().Upload(ctx, "Form", "EntryID", "FieldName", | ||
| "document.pdf", file) |
There was a problem hiding this comment.
The example code for uploading an attachment ignores the error returned by os.Open. This is not a good practice to demonstrate in documentation, as it can lead to a nil pointer panic if the file fails to open. Additionally, the error from the Upload call is declared but not handled in the snippet.
It would be better to show proper error handling for both operations, for example:
// Upload attachment
file, err := os.Open("document.pdf")
if err != nil {
log.Fatal(err)
}
defer file.Close()
if err := client.Attachments().Upload(ctx, "Form", "EntryID", "FieldName", "document.pdf", file); err != nil {
log.Fatal(err)
}
auth.go
Outdated
| resp, err := c.do(req) | ||
| if err != nil { | ||
| // Clear token even if request fails | ||
| c.setToken("") |
There was a problem hiding this comment.
In the Logout function, when clearing the token upon request failure, only the token string is cleared via c.setToken(""). The tokenExpiry time is left unchanged. For better state consistency, it's advisable to clear both the token and its expiry time.
| c.setToken("") | |
| c.setTokenWithExpiry("", time.Time{}) |
auth.go
Outdated
| }() | ||
|
|
||
| // Clear token regardless of response | ||
| c.setToken("") |
There was a problem hiding this comment.
In the Logout function, after a successful logout, only the token string is cleared via c.setToken(""). The tokenExpiry time is left unchanged. For better state consistency, it's advisable to clear both the token and its expiry time.
| c.setToken("") | |
| c.setTokenWithExpiry("", time.Time{}) |
- attachment.go: Fix goroutine leak by closing pipe writer on error - auth.go: Clear both token and expiry on logout for state consistency - README.md: Add proper error handling to upload example
![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 (5)
attachment.go (2)
64-83: Consider callingwriter.Close()on all exit paths.If
io.Copyfails (line 77-80), the function returns without callingwriter.Close(). While the error will be propagated and the pipe will be closed, explicitly closing the multipart writer ensures consistent cleanup. This is a minor improvement for defensive coding.go func() { defer func() { + _ = writer.Close() // Ensure multipart writer is closed _ = pw.Close() // Always close pipe }() part, err := writer.CreateFormFile("entry", filename) if err != nil { errCh <- fmt.Errorf("creating form file: %w", err) return } if _, err := io.Copy(part, data); err != nil { errCh <- fmt.Errorf("copying data: %w", err) return } - errCh <- writer.Close() + errCh <- nil }()
115-128: Non-blocking channel receive may miss write errors.The
selectwithdefaulton line 124 means if the HTTP request fails before the goroutine finishes writing, the write error is silently dropped. While the HTTP error is still returned, this could hide the root cause (e.g., a read error from the input data). Consider documenting this behavior or using a short timeout instead.token_refresh_test.go (1)
145-171: Test uses a 3-second sleep.The
time.Sleep(3 * time.Second)on line 167 makes this test slow. While acceptable for testing time-based behavior, consider if the lifetime/threshold values could be reduced further to speed up the test suite.query.go (1)
71-80: Raw always uses AND conjunction.The Raw method always joins with AND, which may be unexpected if users want OR logic with their raw qualification. Consider documenting this limitation more explicitly or adding a parameter to control the conjunction.
Example documentation addition:
-// Raw adds a raw qualification string with AND conjunction. +// Raw adds a raw qualification string joined with AND. +// If you need OR conjunction, start with Or() before calling Raw(). // Use this for complex expressions that can't be built with And/Or..golangci.yaml (1)
55-56: Justify disabling theunusedlinter.The
unusedlinter is explicitly disabled without explanation. This linter identifies unused constants, variables, functions, and types, which is valuable for code cleanliness. If there's a specific reason for disabling it (e.g., known false positives), document it with a comment.Consider re-enabling unless there's a strong reason:
disable: - - unused + # - unused # Disabled because: [reason]
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
📒 Files selected for processing (24)
.github/workflows/ci.yml(1 hunks).gitignore(1 hunks).golangci.yaml(1 hunks)LICENSE(1 hunks)README.md(1 hunks)attachment.go(1 hunks)attachment_test.go(1 hunks)auth.go(1 hunks)client.go(1 hunks)client_test.go(1 hunks)entry.go(1 hunks)entry_test.go(1 hunks)errors.go(1 hunks)go.mod(1 hunks)interfaces.go(1 hunks)internal/queue/queue.go(1 hunks)internal/queue/queue_test.go(1 hunks)internal/ratelimit/ratelimit.go(1 hunks)internal/ratelimit/ratelimit_test.go(1 hunks)options.go(1 hunks)query.go(1 hunks)query_test.go(1 hunks)token_refresh_test.go(1 hunks)types.go(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (13)
client_test.go (4)
client.go (1)
New(94-113)options.go (8)
WithHTTPClient(18-22)QueryOption(66-66)WithFields(80-84)WithQualification(87-91)WithSort(94-99)WithLimit(102-106)WithOffset(109-113)WithExpand(116-120)errors.go (4)
APIError(35-50)ErrUnauthorized(12-12)ErrForbidden(15-15)ErrNotFound(18-18)types.go (2)
SortAsc(33-33)SortDesc(35-35)
internal/queue/queue_test.go (3)
client.go (1)
New(94-113)internal/queue/queue.go (2)
New(27-32)ErrQueueClosed(16-16)options.go (1)
WithTimeout(26-30)
query_test.go (1)
query.go (1)
NewQuery(35-37)
interfaces.go (2)
options.go (1)
QueryOption(66-66)types.go (3)
Entry(4-7)EntryList(10-13)DeleteOption(39-39)
errors.go (3)
client.go (1)
New(94-113)internal/queue/queue.go (1)
New(27-32)internal/ratelimit/ratelimit.go (1)
New(23-30)
client.go (5)
interfaces.go (3)
HTTPDoer(11-13)EntryServicer(17-35)AttachmentServicer(39-45)internal/ratelimit/ratelimit.go (2)
Limiter(12-18)New(23-30)internal/queue/queue.go (2)
Queue(20-24)New(27-32)options.go (2)
Option(13-13)WithTimeout(26-30)errors.go (2)
ErrNoCredentials(24-24)APIError(35-50)
token_refresh_test.go (6)
client.go (1)
New(94-113)internal/queue/queue.go (1)
New(27-32)internal/ratelimit/ratelimit.go (1)
New(23-30)options.go (4)
WithHTTPClient(18-22)WithTokenLifetime(42-46)WithRefreshThreshold(50-54)WithAutoRefresh(59-63)types.go (1)
Entry(4-7)errors.go (2)
ErrNoCredentials(24-24)APIError(35-50)
attachment.go (1)
client.go (1)
Client(67-91)
auth.go (3)
client.go (2)
New(94-113)Client(67-91)internal/queue/queue.go (1)
New(27-32)internal/ratelimit/ratelimit.go (1)
New(23-30)
entry_test.go (5)
client.go (2)
Client(67-91)New(94-113)options.go (2)
WithFields(80-84)WithQualification(87-91)types.go (4)
Entry(4-7)EntryList(10-13)DeleteOptionForce(45-45)DeleteOption(39-39)query.go (2)
Query(18-21)NewQuery(35-37)errors.go (3)
ErrNotFound(18-18)ErrEmptyFormName(27-27)ErrEmptyEntryID(30-30)
entry.go (4)
client.go (1)
Client(67-91)options.go (1)
QueryOption(66-66)types.go (3)
Entry(4-7)EntryList(10-13)DeleteOption(39-39)errors.go (2)
ErrEmptyFormName(27-27)ErrEmptyEntryID(30-30)
options.go (4)
client.go (2)
Client(67-91)New(94-113)interfaces.go (1)
HTTPDoer(11-13)internal/ratelimit/ratelimit.go (1)
New(23-30)types.go (2)
SortOrder(29-29)SortDesc(35-35)
attachment_test.go (2)
client.go (1)
New(94-113)errors.go (1)
APIError(35-50)
🔇 Additional comments (78)
LICENSE (1)
1-21: LGTM!Standard MIT License with proper copyright notice.
.gitignore (1)
1-29: LGTM!Comprehensive
.gitignorecovering build artifacts, test outputs, IDE files, and generated content.README.md (1)
1-296: LGTM!Well-structured documentation with comprehensive coverage of all library features, including authentication flows, entry operations, query builder syntax, attachment handling, and error handling patterns. The testing section appropriately highlights the interface-based design for mockability.
attachment_test.go (3)
15-42: Well-designed test utility.The
closeTrackingReadercorrectly simulates real HTTP response body behavior by failing reads after close, which helps catch bugs where code attempts to read from a closed body.
44-74: LGTM!Test properly verifies that API error details are preserved and returned rather than generic HTTP status messages. Good use of
closeTrackingReaderto catch potential read-after-close bugs.
100-121: LGTM!Upload success test correctly drains the request body to prevent blocking the pipe writer goroutine. The comment on line 105-106 is helpful for understanding this requirement.
attachment.go (2)
17-51: LGTM!The
Getmethod correctly:
- Parses the API error before closing the response body (lines 39-43)
- Returns an
attachmentReaderwrapper that cancels the request context when closed- Properly cleans up resources on error paths
135-146: LGTM!Clean wrapper pattern that ensures the request context is cancelled when the caller closes the reader, preventing resource leaks for long-lived connections.
query_test.go (3)
10-69: LGTM!Comprehensive table-driven tests covering various value types (strings, integers, booleans, nil) with correct AR System qualification syntax.
112-131: Excellent security-focused tests.Good coverage of injection prevention through field name escaping (lines 112-121) and operator validation (lines 123-131). The single quote escaping test verifies that malicious field names like
Status' OR '1'='1are safely escaped.
147-165: LGTM!Good coverage of AR System field naming conventions, including spaces, colons, parentheses, dashes, and underscores.
token_refresh_test.go (4)
19-37: LGTM!Clear test verifying that credentials are stored after successful login for auto-refresh functionality.
222-270: LGTM!Excellent test for verifying that concurrent requests trigger only a single token refresh. The use of
atomic.Int32for counting login calls and proper synchronization withWaitGroupensures accurate testing of the double-check locking pattern.
428-469: LGTM!Good test for context cancellation propagation during token refresh. Properly verifies that cancelled contexts are respected in the refresh flow.
254-264:sync.WaitGroup.Go()requires Go 1.25.The
wg.Go(func() { ... })pattern introduced in Go 1.25 provides a new method for managing goroutines within a WaitGroup. This aligns with thego 1.25directive in go.mod, but ensure this is intentional, as users on earlier Go versions will encounter compilation errors.go.mod (1)
3-3: Go 1.25 is a valid released version. Go 1.25 was released on August 12, 2025 and is currently stable. No action needed.auth.go (6)
1-19: LGTM on imports and token size guard.The 64KB token size limit is reasonable headroom for JWTs while preventing memory exhaustion attacks. The sentinel error pattern with
ErrTokenTooLargefollows Go conventions.
21-46: LGTM on Login flow.Clean delegation from
LogintoLoginWithAuth. Credentials are stored only after successful authentication, and the queue is properly released via defer.
48-63: LGTM on queue and rate limiter coordination.Correctly acquires queue first, then rate limiter, with proper cleanup if rate limiting fails.
65-110: LGTM on loginInternal implementation.The implementation correctly:
- Uses
io.LimitReaderwithmaxTokenSize+1to detect oversized tokens- Defers both cancel function and response body close
- Wraps errors with context
- Trims whitespace from the token before storing
112-148: LGTM on Logout implementation.Defensive token clearance on both success and failure paths ensures the client doesn't retain stale credentials. Accepting both
200 OKand204 No Contentis appropriate for logout endpoints.
150-154: LGTM on IsAuthenticated.The comment correctly clarifies the method's behavior - it checks token presence, not validity. Token expiry handling is managed separately by the auto-refresh mechanism.
entry_test.go (6)
14-37: LGTM on setupAuthenticatedClient helper.Clean test helper that simulates login on first call and delegates subsequent calls to the provided function. Uses
t.Helper()correctly for proper test failure attribution.
39-94: LGTM on Get tests.Good coverage including the authorization header check, query options, and proper
APIError.Is()assertion for 404 responses.
96-175: LGTM on List, Create, and Update tests.Thorough validation of HTTP methods, paths, headers, and request/response bodies.
200-217: Excellent security test for query parameter injection.Testing that malicious option values like
"FORCE&evil=1"are properly URL-encoded to"FORCE%26evil%3D1"is a valuable security verification.
258-274: Good path traversal prevention test.Verifying that slashes in form names are encoded as
%2Fprevents potential path traversal attacks when form names contain forward slashes.
276-355: LGTM on validation error tests.Complete coverage of input validation for empty form names and entry IDs across all operations. Using unauthenticated clients confirms validation happens before authentication checks.
errors.go (3)
9-31: LGTM on sentinel errors.Well-organized sentinel errors with clear documentation. The separation between HTTP-related errors (Unauthorized, Forbidden, NotFound) and application-level errors (NotAuthenticated, NoCredentials, EmptyFormName, EmptyEntryID) is appropriate.
63-76: LGTM on APIError.Is() implementation.The
Is()method correctly implements theerrors.Isinterface, allowing callers to checkerrors.Is(apiErr, ErrNotFound)which maps to status code 404. The use oferrors.Is(target, ErrX)in the switch handles wrapped targets correctly.
52-61: LGTM on Error() formatting.Clear and consistent error message format that includes all diagnostic information (type, number, text, and optional appended text).
client_test.go (7)
15-42: LGTM on test helpers.
mockHTTPClientandnewMockResponseare clean utilities. The panic innewMockResponseon marshal error is acceptable for test code since it indicates a test bug.
44-57: LGTM on New constructor tests.Validates client initialization and trailing slash normalization in base URL.
59-146: LGTM on authentication flow tests.Good coverage of login success/failure, logout with proper auth header, and idempotent logout when not authenticated.
148-222: LGTM on APIError tests.Table-driven tests thoroughly cover error string formatting and
Is()method behavior for all mapped status codes.
224-283: LGTM on query parameter tests.Comprehensive coverage of all query options with proper value formatting.
306-326: LGTM on input validation edge cases.Good defensive testing for negative and zero values in limit/offset parameters.
328-349: LGTM on oversized token test.Testing rejection of a 2MB token response validates the memory exhaustion protection implemented in
loginInternal.types.go (3)
3-27: LGTM on core data types.Clean struct definitions with appropriate JSON tags. Using
omitzeroonLinksfields prevents empty arrays in JSON output.
28-48: LGTM on enumeration types.Type-safe string wrappers for sort orders and delete options with well-documented constants.
50-57: LGTM on apiErrorResponse.Correctly unexported internal type for parsing BMC Remedy API error responses with appropriate JSON field mappings.
interfaces.go (4)
9-13: LGTM!The HTTPDoer interface is a clean abstraction for testing, allowing easy mocking of HTTP clients.
15-35: LGTM!The EntryServicer interface provides a complete CRUD API with proper context support and idiomatic option patterns.
37-45: LGTM!The AttachmentServicer interface uses appropriate streaming types (io.ReadCloser, io.Reader) for memory-efficient attachment handling.
47-64: LGTM!The RemedyClient interface provides a complete and testable API surface with proper context support and flexible authentication options.
query.go (5)
9-37: LGTM!The Query builder design is clean with helpful documentation. The example usage clearly demonstrates the fluent API pattern.
39-69: LGTM!The distinction between safe (validated) and unsafe (unvalidated) methods provides flexibility. Capturing only the first error is a reasonable design choice for fluent APIs.
82-94: LGTM!Build and BuildSafe provide appropriate options for checked and unchecked query building.
151-160: LGTM!The exported operator constants provide a user-friendly API with IDE support and prevent typos.
128-149: String value formatting uses incorrect escape mechanism for AR System qualifications.AR System uses quote doubling for escaped delimiters within expressions (e.g., field names with apostrophes are written as 'Doug''s Requests'). String values must be enclosed in double quotes, which means special characters within values should follow the same quote-doubling pattern. However, line 132 uses
fmt.Sprintf("%q", val), which produces Go-style backslash escaping instead of quote doubling. A value containing quotes liketest"valuewould be formatted as"test\"value"by %q, but AR System likely expects"test""value".options.go (3)
12-63: LGTM!The functional options pattern is well-implemented with clear documentation including default values. This provides a clean, extensible API for client configuration.
65-120: LGTM!The QueryOption pattern provides a flexible and type-safe way to configure entry queries with all common parameters supported.
122-160: Fields parameter format is correct; sort parameter documentation is sparse in Remedy API.The
fieldsparameter format usingvalues(field1,field2,...)is confirmed in BMC Remedy REST API documentation. However, the sort parameter documentation is notably sparse. The.descsuffix convention appears reasonable, but I recommend verifying the exact sort format with your Remedy instance or BMC documentation if descending sorts are critical to your use case.internal/ratelimit/ratelimit_test.go (1)
1-158: LGTM!Comprehensive test suite with excellent coverage of:
- Core rate limiting behavior (Allow, Wait)
- Context handling (cancellation, timeout)
- Edge cases (token capping, immediate return)
- Concurrency safety (race conditions)
The tests demonstrate thorough validation of the rate limiter implementation.
entry.go (6)
15-47: LGTM!The Get method properly validates inputs, manages queue resources with defer, and handles errors with context wrapping.
49-78: LGTM!The List method follows the same robust pattern as Get with proper validation, resource management, and error handling.
80-104: LGTM!The Create method correctly wraps field values in the expected JSON structure and follows the established pattern for resource management.
106-132: LGTM!The Update method uses the correct HTTP method (PUT) for updating existing entries and properly handles the no-content response.
134-164: LGTM!The Delete method correctly handles optional delete options, using only the first option which is appropriate for a singular operation parameter.
166-191: LGTM!The Merge method implements the create-or-update semantics correctly with a POST to the merge endpoint. The use of apiBasePath constant (defined elsewhere) maintains consistency with other path builders.
internal/queue/queue_test.go (1)
1-155: LGTM!Excellent test coverage of the queue synchronization primitive:
- Serialization guarantees (max concurrent = 1)
- Context handling with proper error checking
- Lifecycle management (close, idempotency)
- Safety guarantees (panic on invalid release, safe double-close)
The tests provide high confidence in the queue's correctness and thread-safety.
internal/ratelimit/ratelimit.go (3)
34-46: LGTM!The
Allow()method correctly acquires the mutex, refills tokens, and atomically checks/consumes a token.
50-66: LGTM!The atomic acquire-or-wait pattern correctly prevents TOCTOU races. Context cancellation is properly handled.
68-98: LGTM!The
tryAcquireOrGetWaitTime()andrefill()implementations are correct. The mutex contract forrefill()is properly documented.client.go (11)
51-91: LGTM!Good defensive design with private credential fields to prevent accidental logging via
%+v. The TODO comments for Go 1.26'sruntime/secretare forward-looking. The mutex separation (tokenMu, credentialsMu, refreshMu) correctly partitions concerns.
93-113: LGTM!Constructor correctly applies options before initializing services, ensuring configuration is finalized first.
115-128: LGTM!Interface-returning accessors enable easier testing.
Close()correctly only closes the queue, not thehttpClient, which may be shared.
130-161: LGTM!Token accessors correctly use
RLockfor reads andLockfor writes, ensuring thread-safe access.
163-190: LGTM!Credential management is thread-safe. Exporting
ClearCredentialsallows users to explicitly purge sensitive data from memory.
209-233: LGTM!The double-check locking pattern is correctly implemented: fast-path read check, then serialize with
refreshMu, then recheck before actually refreshing. This prevents thundering herd on token refresh.
249-269: LGTM!Resource management is correct: queue is released if rate limiting fails. Token validation before queue acquisition avoids holding the queue during potentially slow token refresh.
271-314: LGTM!Request creation correctly manages the cancel function: it's cancelled on error in
newRequestand returned for caller management otherwise. JSON headers are properly set.
316-357: LGTM!Response handling correctly defers cleanup (cancel function and body close), handles 204 No Content, and routes errors to
parseAPIError.
359-390: LGTM!Error parsing gracefully handles unparseable responses and empty error arrays with sensible fallbacks.
392-400: LGTM!Path helpers correctly use
url.PathEscapefor form names and entry IDs, preventing injection and encoding issues.internal/queue/queue.go (3)
1-32: LGTM!Clear documentation explaining the motivation (BMC Error 9093). The 1-slot semaphore pattern correctly enforces single-request serialization.
34-60: LGTM!The pre-checks before the main
selectcorrectly ensure priority: context errors first, then closed-queue detection, before waiting. This prevents the non-deterministicselectfrom acquiring a semaphore on an already-closed queue.
62-80: LGTM!
Release()panics on misuse, which is appropriate for a programming error (Release without Acquire).Close()is correctly idempotent viasync.Once.
- ratelimit: Validate requestsPerSecond > 0 to prevent division by zero - attachment: Clean up multipart writer on error paths
1 participant
Summary
Test plan
go test ./...)golangci-lint run)Summary by CodeRabbit
New Features
Enhancements
Documentation
Chores
Tests
✏️ Tip: You can customize this high-level summary in your review settings.