Handle parse-only agent blocks in client migrator #253
Conversation
ReneWerner87
requested review from
efectn,
gaby and
sixcolors
and removed request for
a team
Summary of ChangesHello @ReneWerner87, 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 refines the client migration tool by introducing support for Highlights
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
|
There was a problem hiding this comment.
Pull request overview
This PR adds support for migrating AcquireAgent blocks that only call Parse() without subsequent data retrieval operations (like Bytes(), String(), or Struct()). Previously, such blocks would not be migrated; now they are converted to simple client method calls that discard the response with _.
- Adds a
defaultcase handler in the switch statement to migrate parse-only agent blocks - Includes comprehensive test coverage for the new migration path
- Ensures the client import is properly added when migrations are applied
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
| cmd/internal/migrations/v3/client_usage.go | Implements the default case in rewriteAcquireAgentBlocksWithAlias to handle parse-only agent blocks by generating client calls with discarded responses |
| cmd/internal/migrations/v3/client_usage_test.go | Adds Test_MigrateClientUsage_AcquireAgentOnlyParse to verify the migration of agent blocks that only parse requests without data retrieval |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
![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 correctly extends the client usage migrator to handle AcquireAgent blocks that only perform a Parse() call without retrieving the response body. The logic for this new case is sound and follows the existing patterns in the function. A new test case has been added that effectively covers this scenario.
However, I've identified a potential issue with how the err variable is declared in the generated code. The migration uses :=, which can lead to compilation errors if err is already in scope. I've left a comment with more details. While this seems to be an existing issue in the migrator, it would be beneficial to address it to make the migration more robust.
|
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 body-aware request handling and threaded, non-conflicting error-identifier propagation through client-usage migration rewrites; injects conditional error-handling scaffolding, many private error-scoping helpers, and expands tests validating AcquireAgent → client.Request/Send rewrites (body, errs→err, name collisions). Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ 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: 0
🧹 Nitpick comments (1)
cmd/internal/migrations/v3/client_usage.go (1)
403-423: Minor performance consideration: Regex compilation inside loop.
identifierDeclaredInLinescompiles new regex patterns on each invocation (lines 408-409). For a migration CLI tool, this is likely acceptable. If performance becomes a concern with large files, consider caching compiled patterns or using string-based matching.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
cmd/internal/migrations/v3/client_usage.go(9 hunks)cmd/internal/migrations/v3/client_usage_test.go(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- cmd/internal/migrations/v3/client_usage_test.go
⏰ 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: Build (1.25.x, macos-13)
- GitHub Check: Build (1.25.x, macos-latest)
- GitHub Check: Build (1.25.x, windows-latest)
🔇 Additional comments (7)
cmd/internal/migrations/v3/client_usage.go (7)
25-26: LGTM!The regex patterns correctly detect short declarations (
err :=) and var declarations (var err) for determining whethererris already in scope.
356-378: LGTM!The
errAssignmentOperatorfunction correctly determines whether to use:=or=based on whethererris already in scope and whether new variables need to be declared. This addresses the previously flagged issue about redeclaringerr.
334-344: LGTM!The default branch correctly handles the parse-only agent blocks scenario. Since no response variable is captured (
_), theerrAssignmentOperatoris called without new variables, correctly determining whethererrneeds:=or=.
609-641: LGTM!The logic correctly determines
newVarbased on the original assignment operator (m[4]). If the original code used:=, the variables were being declared fresh, sovarNameis passed toerrAssignmentOperator. This ensures the generated code uses the appropriate assignment operator.
661-665: LGTM!The function signature update and
errAssignusage in the response line correctly generate either:=or=based on the computed assignment operator.
380-393: LGTM!The
errDeclaredhelper correctly iterates through lines and checks for both short and var-styleerrdeclarations.
242-263: LGTM!The
errAssignmentOperatoris correctly invoked with"resp"as the new variable being introduced, and the computederrAssignis properly used in the generated response line.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
cmd/internal/migrations/v3/client_usage.go (1)
425-438: Consider moving regex compilation to package level.The pattern
\berr\bis compiled on every call toreplaceErrIdentifier. For better performance, compile it once at package level.+var errIdentifierPattern = regexp.MustCompile(`\berr\b`) + func replaceErrIdentifier(lines []string, errName string) []string { if errName == defaultErrName { return lines } replaced := make([]string, len(lines)) - pattern := regexp.MustCompile(`\berr\b`) for i, line := range lines { - replaced[i] = pattern.ReplaceAllString(line, errName) + replaced[i] = errIdentifierPattern.ReplaceAllString(line, errName) } return replaced }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
cmd/internal/migrations/v3/client_usage.go(12 hunks)cmd/internal/migrations/v3/client_usage_test.go(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
cmd/internal/migrations/v3/client_usage_test.go (1)
cmd/internal/migrations/v3/client_usage.go (1)
MigrateClientUsage(74-112)
⏰ 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: Build (1.25.x, macos-13)
- GitHub Check: Build (1.25.x, macos-latest)
- GitHub Check: Build (1.25.x, ubuntu-latest)
- GitHub Check: Build (1.25.x, windows-latest)
🔇 Additional comments (10)
cmd/internal/migrations/v3/client_usage.go (7)
69-72: LGTM - Constants for error name handling.Good addition of constants to centralize the default error variable name and call type identifier.
382-404: Good approach for avoiding error variable conflicts.The logic correctly handles three scenarios: reusing an existing
err, safely declaring a newerrwhen no conflicts exist, and falling back to alternative names when needed.
448-468: LGTM - Dynamic identifier detection.The regex patterns are necessarily dynamic since they depend on the variable name being checked. For a migration tool that runs once per file, this overhead is acceptable.
358-380: Good solution for the assignment operator selection.This properly addresses the past review concern about
err redeclared in this block. The function correctly determines when to use:=(new variables introduced) vs=(all variables already in scope).
336-346: LGTM - Handles parse-only AcquireAgent blocks.This correctly migrates blocks that only call
Parse()without subsequent response handling. The blank identifier_for the unused response and proper error propagation look correct.
240-293: Error variable handling improved, but status/body redeclaration risk remains.The
errNameanderrAssignlogic is correct. However, the unconditionalvardeclarations at lines 273-277 could still cause compilation errors ifstatusVarorbodyVarare already declared in scope (as seen in testTest_MigrateClientUsage_RewritesAcquireAgentStructwhereretCode/retBodyare pre-declared). Consider applying similar scope-checking logic to these variables.
707-744: LGTM - Updated signature properly threads error handling.The function correctly uses the provided
errNameanderrAssignparameters throughout the generated code blocks.cmd/internal/migrations/v3/client_usage_test.go (3)
932-991: LGTM - Good coverage for parse-only migration.This test validates the new default case where
AcquireAgent().Parse()is used without subsequentBytes()/String()/Struct()calls. The expected output correctly shows_, err := client.Post(...)pattern.
993-1052: LGTM - Tests existing error variable reuse.Good test coverage for the case where
erris already declared. The expected output correctly shows_, err = client.Post(...)with=instead of:=.
1054-1117: LGTM - Tests collision avoidance.This test validates the
chooseErrNamefunction when a futurevar errdeclaration exists. The expected output correctly usesclientErras the alternative name.
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request enhances the client usage migrator by adding support for AcquireAgent blocks that only parse requests, and by making the error variable handling more robust to avoid naming collisions. The changes are well-structured and include comprehensive test cases for the new functionality.
I've found a potential issue in the futureErrConflictExists function that might fail to detect some variable conflicts, and a typo in one of the new test cases. My review includes suggestions to address these points.
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
cmd/internal/migrations/v3/client_usage.go(16 hunks)cmd/internal/migrations/v3/client_usage_test.go(2 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
cmd/internal/migrations/v3/client_usage_test.go (1)
cmd/internal/migrations/v3/client_usage.go (1)
MigrateClientUsage(76-114)
⏰ 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: Build (1.25.x, macos-13)
- GitHub Check: Build (1.25.x, macos-latest)
- GitHub Check: Build (1.25.x, windows-latest)
- GitHub Check: Build (1.25.x, ubuntu-latest)
🔇 Additional comments (24)
cmd/internal/migrations/v3/client_usage.go (19)
32-36: requestBodyPattern looks correct and matches both SetBody and SetBodyString.The new
requestBodyPatterncleanly captures calls toreq.SetBody(...)andreq.SetBodyString(...)with the same capture groups as the other request patterns, which is consistent with how you later use it when populatingbodyExpr. No issues here.
38-53: New generic agent patterns are consistent and non-overlapping.The added patterns (
headerSetSimplePattern,queryStringPattern,timeoutPattern,jsonBodyPattern,bodyPattern,basicAuthPattern,tlsConfigPattern,debugPattern,reusePattern,agentBytesCallPattern,agentStringCallPattern,agentStructCallPattern,futureErrConflictPattern) use the same capture style as the rest of the file and match the expected call shapes. In particular,futureErrConflictPattern = ^(?:var\s+err\b|err\s*:=)resolves the earlier false-positive concern around"var error"/"var errMsg"while focusing on realerrdeclarations.
71-74: Centralizing callTypeString and defaultErrName is a nice cleanup.Using
callTypeStringanddefaultErrNameconstants keeps the later code paths (buildSimpleAgentReplacement,chooseErrName) clearer and avoids magic strings.
172-199: Body extraction from AcquireAgent request is wired correctly.The new
bodyExprhandling withrequestBodyPatternintegrates cleanly into the existing scan loop: only lines tied to the samereqVarare used, andbodyExpris trimmed before being passed through tobuildConfig. This matches how URI and headers are handled and should cover bothSetBodyandSetBodyStringcases.
246-247: Config now correctly carries headers and body into client.Config.
configLine := buildConfig(headers, bodyExpr)threads both collected headers and optional body expression into the client call. This is what enables the new Parse+Body test, and the interface (headers map[string]string,body string) is straightforward.
272-277: Struct-case AcquireAgent rewrite correctly maps Parse+Struct to client + JSON.The generated sequence:
resp, <errName> <errAssign> client.<Method>(uri, config)if <errName> != nil { <parseBody> }- Declaration of status/body vars.
if <errName> == nil { ...; <errName> = resp.JSON(target) }- Final
if <errName> != nil { <structBody> }preserves the original control flow while avoiding nil dereferences and reusing the parse/struct error-handling bodies (with
replaceErrIdentifierto adjust the identifier as needed). This is a solid transformation.
285-296: Struct-case post-call error-path reuse is correct and identifier-safe.Reusing the original struct error block via:
if <errName> == nil { ... }to guard JSON decoding.- Final
if <errName> != nil { replaceErrIdentifier(structBody[..], errName)... }.ensures you keep the original response-handling semantics (status codes, JSON error response, etc.), and
replaceErrIdentifieronly touches standaloneerrtokens, leaving any other identifiers intact.
302-317: Bytes-case AcquireAgent rewrite mirrors struct-case semantics appropriately.The bytes path:
- Uses
errAssignmentOperatorwith"resp"and the same<errName>logic.- Declares
statusVarandbodyVarexplicitly.- Only reads from
respwhen<errName> == nil.This matches the intended “Parse+Bytes” behavior while keeping error-handling centralized on the chosen error variable.
323-338: String-case AcquireAgent rewrite is consistent with bytes/struct branches.The string variant uses:
bodyVarasstring, not[]byte.resp.String()instead ofresp.Body().and otherwise follows the same flow as the bytes case, which is exactly what we want.
389-411: Err-name selection strategy is robust and avoids most real collisions.
chooseErrName:
- Prefers
defaultErrNamewhen it’s already declared (to reuse the existing error variable).- Otherwise, uses
defaultErrNameif no futurevar err/err :=is detected in the remaining lines.- When a future conflict exists, cycles through
{"clientErr","agentErr","parseErr",...}and thenerr1,err2, ... until it finds a name not declared beforeidxor later inoriginal[idx:].This approach nicely handles:
- Shared error variables across multiple rewrites.
- Late declarations of
errthat might clash with generated code.- Avoiding collisions with locals introduced after the rewrite insertion point.
Given the complexity trade-offs without a real AST, this is a reasonable and pragmatic design.
413-430: futureErrConflictExists correctly filters out control statements and comments.By trimming, skipping empty/comment lines, and ignoring lines starting with
if/for/switch,futureErrConflictExistsavoids misclassifying inlineif err := ...patterns and focuses on “naked”var err/err :=statements. Combined withfutureErrConflictPattern, this should minimize false positives while still catching real future conflicts.
432-445: replaceErrIdentifier is safe within the rewritten blocks.The helper:
- Early-returns if
errNameisdefaultErrName, avoiding unnecessary work.- Uses
\berr\bso it won’t toucherrs,errMsg, etc.- Operates only on the slice of lines explicitly passed from the parse/struct bodies (excluding the closing brace).
Given Go’s scoping for
if err := ...; err != nil { ... }, theerrinside these blocks always refers to the inner error, so renaming allerrtokens in those slices is semantically correct.
447-475: identifierDeclared / identifierDeclaredInLines are well-scoped and avoid “err-like” names.The combined behavior:
- Checks both
existing(already-emitted code) andoriginalbeforeidx.- Uses two regexes:
shortPatternfor<name> :=(with delimiters).varPatternfor^var <name>....These won’t accidentally match
errMsg,error, etc., and they correctly ignore comments/blank lines. This is exactly what you want for the err-name and assignment heuristics.
495-520: buildConfig(headers, body) correctly assembles client.Config, including body.The function:
- Returns
""when there’s no header and no body, preserving the “no config” case.- Sorts header keys and emits
Header: map[string]string{...}with stable ordering.- Adds
Body: <bodyExpr>verbatim when non-empty.- Wraps everything as
, client.Config{...}(matching existing callers’ expectations).This lines up with how
buildSimpleConfigworks and is exactly what the new Parse+Body and AcquireAgent tests expect.
592-593: Choosing a shared errName for simple agent blocks improves consistency.Calling
errName := chooseErrName(out, lines, i)at the start ofrewriteSimpleAgentBlocksWithAliaslets all the transformed calls in that block share a coherent error variable, while still avoiding conflicts with pre-existing or futureerrdeclarations. This makes multi-call functions easier to read post-migration.
673-702: Simple-agent call branches correctly coordinate err assignment with potential new agent vars.For each of the
agentBytesCallPattern,agentStringCallPattern, andagentStructCallPatternmatches:
newVaris set tovarNameonly when the original call uses:=, signaling that the agent itself may be newly introduced there.errAssignmentOperatorthen decides between:=and=based on whethererrNameis already declared and whethervarNameis a genuinely new identifier at this point.- The resulting
errAssignis threaded intobuildSimpleAgentReplacement, which uses<varName>, <errName> <errAssign> client....This should address the earlier concern about spurious
errredeclarations in simple-agent flows while still handling cases where the agent variable is reused.
725-762: buildSimpleAgentReplacement integrates errName cleanly and keeps behavior.The updated helper:
- Emits
<varName>, <errName> <errAssign> client.<Method>(url, config).- Initializes
status/bodyeither asvardeclarations or by resetting existing variables whenassignOp == "=".- Uses
if <errName> == nil { ... }to guard response reads.- For struct calls, adds
errName = <varName>.JSON(target)inside the success branch.This generalizes nicely across bytes/string/struct cases and respects both “redeclare” and “reuse existing” scenarios for the status/body variables.
247-252: Err-name selection and assignment operator for struct-case AcquireAgent look sound.
errName := chooseErrName(out, lines, i)pluserrAssign := errAssignmentOperator(errName, out, lines, i, "resp")mean:
- You reuse an existing
err-like identifier when safe.- You fall back to
=instead of:=when that identifier is already declared and there are no new vars on the LHS.- For this branch,
"resp"is considered as a potential new variable, which helps avoid unnecessary name changes or shadowing.In Go, a multi-variable short declaration like
resp, err := client.Post(...)is valid whenerris already declared in the same block, as long asrespis a new identifier. The error "no new variables on left side of :=" only occurs when all identifiers on the LHS are already declared. This approach should eliminate most of theerr-redeclaration pitfalls while keeping the generated code idiomatic.
365-387: Current implementation already handles err-variable safety correctly.
errAssignmentOperatoris sound as written. The underlying concern about futureerrdeclarations is already addressed:chooseErrNameusesfutureErrConflictExiststo detect downstreamvar errorerr :=declarations and selects an alternative error variable name if needed. Once a safe error variable is chosen,errAssignmentOperatorcorrectly decides between=and:=based on whether that variable is already declared and whether new variables are being introduced. No refactor is necessary.cmd/internal/migrations/v3/client_usage_test.go (5)
538-607: Parse+Bytes+Body test correctly exercises header+body propagation.
Test_MigrateClientUsage_RewritesParseBytesFlowWithBody:
- Starts from an AcquireAgent + Parse + Bytes flow that sets
Content-Typeand body on the request.- Expects a
client.Postcall withclient.Config{Header: map[string]string{"Content-Type": "application/json"}, Body: "{\"demo\":true}"}.- Verifies the standard
resp, err/ status/body extraction /if err != nil { panic(err) }pattern.This is a solid regression test for the new
requestBodyPattern+buildConfigbehavior.
1005-1064: AcquireAgent-only Parse test validates the new parse-only migration path.
Test_MigrateClientUsage_AcquireAgentOnlyParse:
- Starts from an AcquireAgent that only calls
Parse()and logs on error.- Expects
_, err := client.Post(..., client.Config{Header: ...})and a simpleif err != nil { log.Errorf(...) }.This verifies that the new default branch in
rewriteAcquireAgentBlocksWithAliashandles “parse-only” blocks without requiring a subsequentBytes/Structcall.
1066-1125: Existing-err Parse-only test confirms=path and no redeclaration.
Test_MigrateClientUsage_AcquireAgentOnlyParseWithExistingErr:
- Declares
var err errorbefore the AcquireAgent block.- Expects the migration to emit
_, err = client.Post(...)(assignment, not short declaration) and reuse thaterrin the subsequentif err != nilcheck.This nicely confirms that
errAssignmentOperatordetects an in-scope error variable and avoids:=when there are no new non-blank LHS variables.
1127-1190: Collision test for futurevar errcorrectly exercises alternate errName logic.
Test_MigrateClientUsage_AcquireAgentOnlyParseAvoidsErrCollisions:
- Has an AcquireAgent-only
Parseblock with logging, and then later declaresvar err error.- Expects the migration to use
clientErr(noterr) for the client call and logging (_, clientErr := client.Post(...); if clientErr != nil { log.Errorf(..., clientErr) }).This directly validates the
futureErrConflictExists+chooseErrNamelogic and ensures the generated error identifier doesn’t collide with a futurevar errin the same function.
1192-1255: “Err-like” name test ensures identifierDeclared doesn’t overmatch.
Test_MigrateClientUsage_AcquireAgentOnlyParseIgnoresErrLikeNames:
- Defines
var errMsg stringafter the Parse-only block.- Expects the migration to still use the default
erridentifier (_, err := client.Post(...)), and to leaveerrMsgalone.This confirms that
identifierDeclaredandidentifierDeclaredInLinesrespect full identifier names and don’t treaterrMsgas anerrcollision, addressing the earlier false-positive concern from prior reviews.
|
/gemini review |
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
cmd/internal/migrations/v3/client_usage.go(16 hunks)cmd/internal/migrations/v3/client_usage_test.go(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- cmd/internal/migrations/v3/client_usage.go
🧰 Additional context used
🧬 Code graph analysis (1)
cmd/internal/migrations/v3/client_usage_test.go (1)
cmd/internal/migrations/v3/client_usage.go (1)
MigrateClientUsage(77-115)
⏰ 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: Build (1.25.x, ubuntu-latest)
- GitHub Check: Build (1.25.x, windows-latest)
- GitHub Check: Build (1.25.x, macos-13)
- GitHub Check: Build (1.25.x, macos-latest)
🔇 Additional comments (5)
cmd/internal/migrations/v3/client_usage_test.go (5)
538-609: Parse+Bytes with body migration test looks solidThe sample/expected pair for
Test_MigrateClientUsage_RewritesParseBytesFlowWithBody(Line [538]-[609]) correctly exercises the newclient.Postpath, including propagation of method,Content-Typeheader, and body, and reuses the established error-handling pattern from the other Parse+Bytes tests. No issues from my side here.
1005-1064: Good coverage of parse-only AcquireAgent with logging
Test_MigrateClientUsage_AcquireAgentOnlyParse(Line [1005]-[1064]) nicely verifies the new path where aParse-onlyAcquireAgentblock is rewritten into aclient.Postcall, preserving the original logging behavior and ensuring theclientimport is added while keeping thelogimport. This matches the intended semantics.
1066-1125: Reusing existingerrvariable is appropriateIn
Test_MigrateClientUsage_AcquireAgentOnlyParseWithExistingErr(Line [1066]-[1125]), the expected code reuses the predeclaredvar err errorand assigns theclient.Posterror into it. That’s consistent with the test’s intent (“WithExistingErr”) and will generally integrate better with real-world handlers that already use a sharederr. Looks good.
1127-1190: Nice edge‑case test for avoidingerrname collisions
Test_MigrateClientUsage_AcquireAgentOnlyParseAvoidsErrCollisions(Line [1127]-[1190]) correctly checks that the migrator introduces a distinctclientErrwhen a separatevar err erroris declared later in the function, preventing name clashes. This is a valuable regression test for name-selection logic.
1192-1255: Correct handling oferr-like but non-erridentifiers
Test_MigrateClientUsage_AcquireAgentOnlyParseIgnoresErrLikeNames(Line [1192]-[1255]) ensures that identifiers such aserrMsgdon’t interfere with choosingerras the error variable for the migratedclient.Postcall. The expected output keepserrMsguntouched and useserrfor the client call, which is the right behavior.
There was a problem hiding this comment.
Code Review
This pull request extends the client usage migrator to handle AcquireAgent blocks that only parse requests without subsequent result extraction calls (e.g., .Bytes()). It also introduces sophisticated logic to avoid error variable name collisions by choosing non-conflicting names like clientErr and correctly using = or := for assignment. The changes are well-tested with several new test cases covering various scenarios.
However, I've found a couple of significant issues:
- A critical flaw in the
rewriteClientErrorHandlingfunction causes it to perform a blind replacement oferrswitherr. This can break user code that uses anerrsvariable for other purposes, leading to uncompilable code. A new test case unfortunately asserts this incorrect behavior. - The logic for determining the assignment operator (
:=vs=) inrewriteSimpleAgentBlocksWithAliasis overly complex and incorrect when dealing with shadowed variables, which can also lead to compilation errors.
I've left detailed comments with suggestions on how to address these issues. Once these are fixed, this will be a great improvement to the migration tool.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
cmd/internal/migrations/v3/client_usage.go (1)
498-513: Heuristic indeclaredInVarBlockLinemay be fragile.The function checks if a line starts with the identifier name and lacks assignment operators. However, this approach might not correctly handle all declaration formats, especially:
- Multi-line var blocks with alignment
- Declarations with complex types
- Lines with comments
Consider adding test coverage for edge cases like:
var ( err error // comment errs []error )
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
cmd/internal/migrations/v3/client_usage.go(17 hunks)cmd/internal/migrations/v3/client_usage_test.go(2 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
cmd/internal/migrations/v3/client_usage_test.go (1)
cmd/internal/migrations/v3/client_usage.go (1)
MigrateClientUsage(77-115)
🔇 Additional comments (14)
cmd/internal/migrations/v3/client_usage.go (8)
32-32: Pattern definitions look good.The new regex patterns correctly use word boundaries (
\b) to avoid false positives when matching error variable declarations, addressing concerns from previous reviews.Also applies to: 40-54
72-75: LGTM!The constants are well-defined and improve code clarity.
177-177: Body extraction and error-name selection properly integrated.The body expression is correctly extracted from the request setup and passed through to the config builder, while the error name is chosen to avoid conflicts.
Also applies to: 197-200, 247-249
366-388: Assignment operator logic correctly handles existing error variables.The function properly determines when to use
=vs:=based on scope analysis, addressing the redeclaration concerns from previous reviews. Whenerris already in scope and no new variables are being introduced, it correctly uses=to avoid compilation errors.
390-412: Error name selection logic is thorough.The function systematically checks for conflicts with both existing and future declarations, falling back to a list of candidate names. This should prevent most collision scenarios.
414-443: Future conflict detection handles most cases correctly.The function appropriately skips block-scoped declarations (if/for/switch) since they won't conflict with function-level error variables. The regex patterns with word boundaries help avoid false positives.
533-558: Body parameter correctly integrated into config builder.The function now properly includes the body in the generated
client.Configwhen provided, supporting the parse-only migration path with request bodies.
994-1009: Guard clause prevents mutating unrelated error slices.The check for legacy error patterns (
hasLegacyErrs) before rewriting ensures that customerrsslices unrelated to the v2 client API are preserved. This addresses concerns from previous reviews about breaking valid user code.cmd/internal/migrations/v3/client_usage_test.go (6)
538-609: Test correctly validates body migration for AcquireAgent+Parse flow.The test verifies that request bodies set via
SetBodyStringare properly extracted and included in theclient.Config, ensuring the parse-with-body migration path works as intended.
1005-1064: Parse-only migration test validates core functionality.The test confirms that AcquireAgent blocks containing only
Parse()(without subsequentBytes()/String()/Struct()calls) are correctly migrated toclient.Postwith the response assigned to_since it's unused.
1066-1125: Test validates assignment operator selection with existing error variable.The test correctly verifies that when
erris already declared, the migration uses=instead of:=for the assignment, preventing "no new variables on left side of :=" compilation errors.
1127-1190: Test confirms collision avoidance with future error declarations.The test validates that the migration detects future
errdeclarations and chooses an alternative error variable name (clientErr) to prevent redeclaration errors, demonstrating proper forward-looking conflict detection.
1192-1255: Test validates precise error name conflict detection.The test confirms that variables with
erras a prefix (likeerrMsg) don't trigger false positives in conflict detection, thanks to the word-boundary checks in the regex patterns.
1257-1311: Test correctly preserves custom error slices.The test validates that custom
errs []errorslices (not using legacy v2 client patterns) are preserved during migration, with the migrated code introducing a separateerrvariable. This addresses concerns from previous reviews and ensures custom error handling logic remains intact.
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
cmd/internal/migrations/v3/client_usage.go (1)
234-285: AcquireAgent struct/bytes/string + parse‑only rewrites: structure is sound but leaves unusederrslocalsThe expanded AcquireAgent handling correctly:
- Preserves comments and non‑
err/errsvar declarations betweenParseand the Struct/Bytes/String call.- Introduces a computed
errNameanderrAssign, usingerrAssignmentOperatorso:=vs=is chosen based on prior declarations and new response vars.- Guards response field access and JSON decoding behind
if <errName> == nilto avoid nil dereferences.- Adds a default branch that rewrites parse‑only blocks into a single
client.<Method>call followed by anif <errName> != nil { ... }containing the original Parse error body.However, for struct‑based flows (e.g.,
Test_MigrateClientUsage_RewritesAcquireAgentStruct), the migration leaveserrs []errordeclared in the function’s var block while all uses oferrsare removed in favor of the single<errName>variable. This results in migrated code with an unused localerrs, which is a compile‑time error in Go.You’ll need to either:
- Also rewrite the corresponding
var errs []errordeclaration to a singleerr(or<errName>) declaration when you perform the struct rewrite, or- Ensure
rewriteClientErrorHandlingstill runs for these cases and rewrites the declaration and remainingerrsusages, without regressing the new “errs slice” preservation tests.Right now, after the struct rewrite, there are no remaining
len(errs)/"errs": errspatterns, sohasLegacyErrsstays false andrewriteClientErrorHandlingnever touches the now‑orphanederrsdeclaration.Also applies to: 287-352, 355-433, 434-443
cmd/internal/migrations/v3/client_usage_test.go (1)
256-342: Tests correctly exercise AcquireAgent struct flows but encode an unusederrslocalThe
RewritesAcquireAgentStructandRewritesAcquireAgentStructWithVarsBetweenParseAndCalltests do a good job of:
- Asserting that AcquireAgent+Struct flows are rewritten to a single
client.Postcall with guarded status/body extraction and JSON decoding.- Verifying that interleaved
vardeclarations betweenParseandStructare preserved appropriately, and that error identifiers (errvsclientErr) are chosen to avoid collisions with latervar err error.However, the expected code in
RewritesAcquireAgentStructstill declareserrs []errorin the function’s var block while all its usages have been removed in favor of a singleerrvalue. That mirrors the production behavior noted inclient_usage.goand would cause migrated user code not to compile (errsdeclared but not used).Once the underlying migrator is updated to drop or rewrite that declaration, these expectations should be adjusted to match the new, compile‑correct output.
Also applies to: 344-426
🧹 Nitpick comments (1)
cmd/internal/migrations/v3/client_usage.go (1)
534-566:replaceErrIdentifierandhasBlankAssignmentare reasonable heuristics
replaceErrIdentifiercleanly rewriteserrand (optionally)errsusages inside migrated blocks to the chosenerrName, withreplaceErrsselectively enabled where a slice of errors is intentionally collapsed.hasBlankAssignmentscans the original file for_ = <name>to avoid injecting duplicate blank assignments when synthesizing_ = statusVar/_ = bodyVarsafety lines.The trade‑off (potentially skipping a needed blank assignment if one only appears in a comment, and sometimes adding redundant
_ =for variables that end up used later) is acceptable for a migration tool and is covered by tests.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
cmd/internal/migrations/v3/client_usage.go(15 hunks)cmd/internal/migrations/v3/client_usage_internal_test.go(1 hunks)cmd/internal/migrations/v3/client_usage_test.go(6 hunks)
⏰ 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: Build (1.25.x, macos-13)
- GitHub Check: Build (1.25.x, macos-latest)
- GitHub Check: Build (1.25.x, ubuntu-latest)
- GitHub Check: Build (1.25.x, windows-latest)
🔇 Additional comments (8)
cmd/internal/migrations/v3/client_usage_internal_test.go (1)
68-81: AcquireAgent→client.Post struct rewrite expectation looks consistentThe new expected block matches the struct-case rewrite in
rewriteAcquireAgentBlocksWithAlias: early return on transport error, guarded population ofretCode/retBody, thenresp.JSON(&t)and a finalerr != nilbranch returning a 500 JSON. Behavior is consistent with the single‑error model used elsewhere and is a good fit for the production rewrite.cmd/internal/migrations/v3/client_usage.go (5)
31-36: Regex additions for body and agent methods align with intended patternsThe new
requestBodyPatternand the expanded agent‑method patterns (headerSetSimplePattern,bodyPattern,agent*CallPatterns, etc.) are consistent with the existing style and correctly capture theSetBody/SetBodyStringand other method invocations needed by the migrator. They giverewriteAcquireAgentBlocksWithAliasandrewriteSimpleAgentBlocksWithAliasthe necessary hooks without over‑matching obvious non‑agent code.Also applies to: 38-54
173-201: Body-aware AcquireAgent config looks correctCapturing
SetBody/SetBodyStringintobodyExprand threading it intobuildConfig(headers, bodyExpr)produces the expectedclient.Config{Header: ..., Body: ...}literals (as exercised by the newRewritesParseBytesFlowWithBodytest). The matching logic ensures only the current request variable is considered, so unrelatedSetBody*calls won’t be pulled into the config.Also applies to: 279-285
479-532: Err-name selection and future-conflict detection behave well for new testsThe
chooseErrName+futureErrConflictExistscombo correctly supports the new test cases:
- Defaulting to
"err"when neithererrnorerrsexists in the current scope and no futurevar err/err :=/var errs/errs :=appears (e.g.,AcquireAgentOnlyParse,AcquireAgentOnlyParseRespectsErrsSlice).- Falling back to alternate names like
clientErrwhen a futurevar err errorwould otherwise collide (AcquireAgentOnlyParseAvoidsErrCollisions).- Treating
var errMsg stringand other “err‑like” identifiers as non‑conflicting (AcquireAgentOnlyParseIgnoresErrLikeNames).The additional checks for var‑block entries via
declaredInVarBlockLineare a good touch to catchvar (...)declarations without over‑matching.
641-666:buildConfig’s newbodysupport matches expected client.Config literalsExtending
buildConfigto accept abodyexpression and appendBody: <expr>into theclient.Config{...}literal works as intended, including:
- Sorting headers for deterministic output.
- Emitting
Body: <expr>only when a body was actually captured (e.g., fromSetBodyString), which is validated byRewritesParseBytesFlowWithBody.This keeps the simple-agent and AcquireAgent code paths consistent in how they construct configs.
738-833: Simple-agent rewrites now use a fixed:=for response/err, avoiding shadowing bugs
rewriteSimpleAgentBlocksWithAliasnow:
- Picks an
errNamewithchooseErrName.- Always passes
":="forerrAssignintobuildSimpleAgentReplacement.And
buildSimpleAgentReplacementemits:<indent><varName>, <errName> := client.Method(...)This avoids the earlier risk of using
=when the response variable shadows the original agent variable (causing type mismatches), and matches how Go permits reusingerralongside a newly introduced<varName>. It’s a good simplification and aligns with previous review feedback on this path.Also applies to: 856-893
cmd/internal/migrations/v3/client_usage_test.go (2)
622-695: New tests around parse-only AcquireAgent and errs-slice behavior are well targetedThe added tests:
AcquireAgentOnlyParse,AcquireAgentOnlyParseWithExistingErr,AcquireAgentOnlyParseAvoidsErrCollisions, andAcquireAgentOnlyParseIgnoresErrLikeNamescollectively validate:
- Basic parse‑only migration to
client.Post.- Correct reuse of an existing
errvariable (_= client.Postwith=).- Selection of alternate names like
clientErrto avoid futurevar errcollisions.- Ignoring
err‑like identifiers such aserrMsgwhen choosing the error variable.AcquireAgentOnlyParseRespectsErrsSliceensures that a legitimate[]erroraccumulator (var errs []error+errs = append(errs, err)) remains a slice and is not rewritten to a singleerr, addressing the earlier semantic concern around multi‑error collectors.These tests give very good coverage of the new error‑naming and slice‑preservation logic around parse‑only blocks, and will help prevent regressions in future tweaks.
Also applies to: 1093-1399
622-695: Body-handling tests validate AcquireAgent parse+Bytes migration
RewritesParseBytesFlowWithBody(and the existingRewritesParseBytesFlow) nicely confirm that:
- AcquireAgent flows using
req.SetBodyString(...)anda.Bytes()migrate to aclient.Postcall with aclient.Config{Header: ..., Body: ...}argument.- Status and body are populated only when
err == nil, and error paths still propagate viapanic(err).This aligns with the new
requestBodyPattern+buildConfigbehavior and confirms the body-aware changes are wired through correctly.Also applies to: 537-617
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (4)
cmd/internal/migrations/v3/client_usage.go (4)
649-664:declaredInVarBlockLineonly detects grouped var block declarations.This function checks for lines starting with the identifier name (e.g.,
err errorinsidevar (...)), not standalonevar err errordeclarations. This is intentional since standalone declarations are handled byvarPatterninidentifierDeclaredInLines. The logic is correct but the function name could be slightly misleading.Consider adding a brief doc comment clarifying this handles grouped
var (...)block entries:+// declaredInVarBlockLine checks if a line within a grouped var block +// declares the given identifier (e.g., "err error" for name="err"). func declaredInVarBlockLine(name, trimmed string) bool {
1008-1008: Hardcodederridentifier in replacement builders may cause conflicts.Functions like
buildBytesWithBodyReplacementand others in this section hardcodeerrin the generated output. Unlike therewriteAcquireAgentBlocksWithAliaspath which useschooseErrName, these don't adapt to existingerrdeclarations in scope.This is pre-existing behavior, but for consistency with the new error-naming logic, consider threading
errNamethrough these replacement functions in a follow-up.
562-578:replaceErrIdentifiercreates new regex on every call.The function compiles
errPatternanderrsPatternregexes on each invocation. Since this may be called multiple times during migration, consider moving these to package-level compiled patterns for efficiency.+var ( + errWordPattern = regexp.MustCompile(`\berr\b`) + errsWordPattern = regexp.MustCompile(`\berrs\b`) +) + func replaceErrIdentifier(lines []string, errName string, replaceErrs bool) []string { replaced := make([]string, len(lines)) - errPattern := regexp.MustCompile(`\berr\b`) - errsPattern := regexp.MustCompile(`\berrs\b`) for i, line := range lines { - updated := errPattern.ReplaceAllString(line, errName) + updated := errWordPattern.ReplaceAllString(line, errName) if replaceErrs { - replaced[i] = errsPattern.ReplaceAllString(updated, errName) + replaced[i] = errsWordPattern.ReplaceAllString(updated, errName) continue } replaced[i] = updated } return replaced }
585-585: Regex compiled inside loop inhasBlankAssignment.Similar to
replaceErrIdentifier, this function compiles a regex on each call. For a small number of calls this is acceptable, but if performance becomes a concern, consider caching or restructuring.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
cmd/internal/migrations/v3/client_usage.go(13 hunks)
⏰ 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: Build (1.25.x, macos-13)
- GitHub Check: Build (1.25.x, macos-latest)
- GitHub Check: Build (1.25.x, windows-latest)
- GitHub Check: Build (1.25.x, ubuntu-latest)
🔇 Additional comments (8)
cmd/internal/migrations/v3/client_usage.go (8)
52-53: Regex patterns for future conflict detection look correct.The use of
\bword boundaries infutureErrConflictPatternandfutureErrsConflictPatternproperly constrains matching to exacterranderrsidentifiers, avoiding false positives with similar names likeerrMsg.
72-75: Constants are well-defined.
callTypeStringanddefaultErrNameprovide clear named constants that improve readability throughout the file.
254-266: Skippingvar errandvar errsduring preserved-line collection is correct.This prevents these declarations from being duplicated or misplaced in the output, as they'll be handled by the error-naming logic.
309-315: Body handling with SetRawBody looks correct.The logic correctly wraps string body expressions with
[]byte()whenbodyIsStringis true, ensuring type compatibility withSetRawBody.
461-471: Parse-only AcquireAgent migration path is well-implemented.This default case correctly handles AcquireAgent blocks that only call
Parse()without subsequentBytes/String/Structcalls. The response is discarded with_, and error handling is properly injected using the chosen error name.
507-529:chooseErrNamelogic is sound.The function correctly:
- Reuses
errif already declared in scope- Uses
errif no future conflicts exist- Falls back to candidate names (
clientErr,agentErr, etc.) when conflicts are detectedThis addresses the previously flagged concern about
errredeclaration.
874-878:buildSimpleAgentReplacementcorrectly uses:=for new response declarations.The response variable shadows the original agent variable with a different type (
*client.Responsevs*fiber.Agent), so using:=is correct here since it's a new declaration in the output.
1120-1135: Conditional error handling rewrite is well-designed.The
hasLegacyErrsgate correctly limits theerrs→errtransformation to files with actual legacy Fiber multi-error patterns (if len(errs) > 0,"errs": errs, etc.), preserving unrelated[]errorslices.
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request significantly improves the client usage migrator by adding support for 'parse-only' agent blocks and making the error handling more robust by avoiding variable name collisions. The changes are extensive and well-tested, covering many edge cases. I've identified a couple of areas for improvement to prevent potential data loss during migration and to make the legacy pattern detection even more comprehensive. Overall, this is a very solid contribution.
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
cmd/internal/migrations/v3/client_usage.go(13 hunks)cmd/internal/migrations/v3/client_usage_test.go(6 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
cmd/internal/migrations/v3/client_usage_test.go (1)
cmd/internal/migrations/v3/client_usage.go (1)
MigrateClientUsage(77-115)
⏰ 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: Build (1.25.x, macos-latest)
- GitHub Check: Build (1.25.x, macos-13)
- GitHub Check: Build (1.25.x, windows-latest)
- GitHub Check: Build (1.25.x, ubuntu-latest)
🔇 Additional comments (6)
cmd/internal/migrations/v3/client_usage.go (3)
173-501: AcquireAgent rewrite + parse-only path and body handling look consistent and well-scoped.The extended
rewriteAcquireAgentBlocksWithAliaslogic correctly:
- Tracks
bodyExpr/bodyIsStringviarequestBodyPatternand emitsSetRawBody([]byte(...))when needed.- Preserves comments and non-
err/errsvar declarations betweenParse()and the terminalStruct/Bytes/Stringcall, then re-inserts them while marking original lines as skipped.- Reconstructs the flow for
Struct/Bytes/Stringcases by:
- Choosing an error identifier with
chooseErrName.- Using
errAssignmentOperatorto pick:=vs=safely whenerrNameorrespare already in scope.- Preventing nil dereferences by declaring status/body vars and populating them only under
if errName == nil { ... }.- Re-emitting the original error branches with
replaceErrIdentifierso they key off the newerrNameinstead of the old parse/errs variables.- Handles the parse‑only branch in the
defaultcase by emitting_, <errName> <op> req.Send()followed by the original handler body guarded underif <errName> != nil { ... }, which aligns with the new tests forAcquireAgentOnlyParse*.This restructuring aligns with the new tests (e.g., parse‑only, with existing
err, and collision‑avoidance cases) and avoids double‑emitting old agent/parse lines thanks to the updatedi = ...jumps andskipLinesusage.
800-977: Simple-agent rewrite now uses expliciterrNameand always short-declares the response/err pair.The changes in
rewriteSimpleAgentBlocksWithAliasandbuildSimpleAgentReplacementlook good:
errName := chooseErrName(out, lines, i)mirrors the AcquireAgent path, so simple-agent rewrites benefit from the same collision avoidance and future-conflict logic.- Passing
errAssign := ":="explicitly intobuildSimpleAgentReplacementguaranteesvarName, errNameare short-declared (no attempt to reuse an existing agent variable with=), which matches the originalagent := fiber.Get(...)shape and addresses the prior concern about over‑smart assignment-operator selection.buildSimpleAgentReplacementcorrectly parameterizes:
- The call type (
bytesvsstringvsstruct) viacallTypeandcallTypeString.- The error variable via
errNamein both theif errName == nilblock and the JSON decode path forstructcalls.Given this is a one-shot migration tool, the small cost of recalculating config and generating these patterns is acceptable, and the behavior now aligns with the updated tests.
1186-1199: GatedrewriteClientErrorHandlingavoids clobbering arbitraryerrsslices.The
hasLegacyErrsguard ensures you only rewrite to single-error semantics when the classic multi-error patterns are present (if len(errs) > 0,len(errs)usage,"errs": errs), which fixes the earlier issue where unrelated[]erroraccumulators were being renamed and broken.This gating is especially important now that AcquireAgent-only-parse flows can legitimately use an
errs []erroraccumulator (e.g.,AcquireAgentOnlyParseRespectsErrsSlice), and the implementation here matches that intent.cmd/internal/migrations/v3/client_usage_test.go (3)
349-436: Good coverage for struct flow with vars betweenParseand the call, including futureerrcollisions.
Test_MigrateClientUsage_RewritesAcquireAgentStructWithVarsBetweenParseAndCallnicely exercises:
- Local vars declared between
a.Parse()anda.Struct(...)(status/body/errs/target), ensuring the migrator preserves their declarations and moves them to the right place.- A later
var err errorin the same function, verifyingchooseErrName/futureErrConflictExistspickclientErrand that the rewritten flow usesclientErrconsistently in both the directreturn clientErrpath and the JSON/response error branch.This directly validates the more complex scoping and collision-avoidance logic added in
rewriteAcquireAgentBlocksWithAlias.
634-711: Parse-bytes-with-body test validates request-body capture and raw-body emission.
Test_MigrateClientUsage_RewritesParseBytesFlowWithBodycorrectly asserts that:
- A
POST+SetBodyString+SetRequestURI+Parse()+Bytes()flow is rewritten to:
client.New()/a.R()withSetMethod("POST"),SetURL(...), andSetHeader("Content-Type", "application/json").SetRawBody([]byte("{\"demo\":true}"))carrying over the string body.- A single
resp, err := req.Send()plus the standard status/body extraction and error handling.This test gives solid coverage for the new
requestBodyPatternandbodyExpr/bodyIsStringhandling in the AcquireAgent path.
1112-1442: AcquireAgent-only-parse test matrix comprehensively exercises err naming and slice preservation.The new tests around parse-only AcquireAgent usage are well-chosen:
AcquireAgentOnlyParsechecks the basic transformation froma.Parse()with logging intoreq.Send()witherrand preserved log call.AcquireAgentOnlyParseWithExistingErrverifies that an existingvar err errorleads to_ , err = req.Send()(assignment, not declaration).AcquireAgentOnlyParseAvoidsErrCollisionsconfirms that a latervar err errorforces an alternate name (clientErr), and that all uses in the log branch are rewritten toclientErr.AcquireAgentOnlyParseIgnoresErrLikeNamesensures identifiers likeerrMsgdo not influence the chosen error name or assignment operator.AcquireAgentOnlyParseRespectsErrsSliceensures a user-managedvar errs []erroraccumulator plusappend(errs, err)andfmt.Println(errs)is preserved, with the migrator only injecting_ , err := req.Send()andif err != nil { errs = append(errs, err) }.Together these tests provide strong regression coverage for the new
chooseErrName,futureErrConflictExists, andrewriteClientErrorHandlinggating behavior in the parse-only path.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
cmd/internal/migrations/v3/client_usage.go (1)
1203-1216: Redundant condition check.Lines 1213-1216 are redundant since
baseLegacyis already checked at line 1208. If!baseLegacy, the function returns at line 1210, sohasLegacyErrswill always be true when reaching line 1214.Consider simplifying:
func rewriteClientErrorHandling(content string) string { // Only rewrite when we see the legacy multi-error usage patterns. This avoids // mutating unrelated identifiers such as custom "errs" slices. baseLegacy := clientErrIfPattern.MatchString(content) || clientErrLenPattern.MatchString(content) || clientErrComparePattern.MatchString(content) || clientErrMapPattern.MatchString(content) - declaredErrs := clientErrsDeclPattern.MatchString(content) if !baseLegacy { // Declaration-only cases keep errs slices intact. return content } - - hasLegacyErrs := baseLegacy || declaredErrs - if !hasLegacyErrs { - return content - }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
cmd/internal/migrations/v3/client_usage.go(13 hunks)cmd/internal/migrations/v3/client_usage_test.go(6 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
cmd/internal/migrations/v3/client_usage_test.go (1)
cmd/internal/migrations/v3/client_usage.go (1)
MigrateClientUsage(76-114)
⏰ 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: Build (1.25.x, windows-latest)
- GitHub Check: Build (1.25.x, macos-13)
- GitHub Check: Build (1.25.x, macos-latest)
- GitHub Check: Build (1.25.x, ubuntu-latest)
🔇 Additional comments (32)
cmd/internal/migrations/v3/client_usage.go (20)
71-74: LGTM!Constants are well-defined and provide good maintainability for the error name handling logic.
176-201: LGTM!Body handling logic correctly distinguishes between
SetBodyandSetBodyString, capturing the expression for proper translation toSetRawBodyin the migrated code.
235-311: LGTM!The preserved lines handling correctly addresses the previous concerns about code deletion. Parse-only blocks are now properly detected and handled, with comments and variable declarations being preserved appropriately.
313-348: LGTM!Error name selection and client setup logic properly handles collision detection and method literal conversion. The body expression wrapping for string bodies (
[]byte(bodyExpr)) is correct.
349-417: LGTM!The struct case handling correctly propagates error names, handles variable declarations, and includes backward compatibility for
errsslices when needed.
418-459: LGTM!Bytes case handling follows the same consistent pattern as the struct case with proper error propagation.
460-501: LGTM!String case handling correctly uses
stringtype for the body variable and follows the same consistent pattern.
502-519: LGTM!The default case properly handles parse-only AcquireAgent blocks as described in the PR objectives. The conditional placement of
addPreservedensures preserved lines appear in the correct location.
530-552: LGTM!The
errAssignmentOperatorfunction correctly determines whether to use:=or=based on whether the error variable is already in scope and whether new variables are being introduced.
554-576: LGTM!The
chooseErrNamefunction properly handles error variable name selection, avoiding collisions with both existing declarations and future declarations in the code.
609-625: LGTM!The
replaceErrIdentifierfunction correctly uses word boundaries to replace error identifiers without affecting similarly-named variables likeerrMsg.
627-662: LGTM!Helper functions for detecting blank assignments and identifier usage are well-implemented with proper word boundary matching.
664-676: LGTM!Simple and effective block boundary detection using brace depth tracking.
678-729: LGTM!The identifier declaration detection correctly handles control-statement scoped declarations by skipping
if/for/switch/else iflines for short declaration patterns while still checking var-block style declarations. This addresses the previous review concerns about false positives fromif err := ...patterns.
731-747: LGTM!The
declaredInVarBlockLinefunction now uses token-based matching (fields[0] != name) instead of prefix-based matching, which correctly addresses the previous review concerns about false positives with similarly-named variables likeerrMsg.
749-767: LGTM!The
methodLiteralfunction correctly converts Fiber method constants to quoted string literals for the new client API.
839-839: LGTM!Error name selection is properly integrated into the simple agent block rewrite path.
957-993: LGTM!The
buildSimpleAgentReplacementfunction correctly uses the chosen error name throughout the generated code.
1222-1237: LGTM!The line-by-line replacement correctly preserves
var errsdeclaration lines while replacing other uses oferrswitherr. This aligns with the gated approach to avoid breaking legitimate error slice accumulators.
578-607: Control-flow scoped declarations are correctly handled.The function properly detects function-scoped
var errandvar errsdeclarations while correctly skipping block-scoped declarations in control statements (if,for,switch). The regex patterns use word boundaries (\b) to precisely matchvar err,err :=,var errs, anderrs :=without false positives. Test coverage viaTest_MigrateClientUsage_AcquireAgentOnlyParseAvoidsErrCollisionsvalidates that the collision detection works correctly, preserving block-scoped errors in if statements while detecting function-scoped declarations that would conflict with the injected error variable.cmd/internal/migrations/v3/client_usage_test.go (12)
317-346: LGTM!The test correctly verifies that
errs []errordeclarations in var blocks are preserved while the error handling is migrated to the new singleerrpattern.
604-632: LGTM!The test correctly verifies the parse bytes flow migration with the new client API.
634-711: LGTM!New test correctly verifies that
SetBodyStringis properly migrated toSetRawBody([]byte(...)).
1086-1110: LGTM!The test correctly verifies the AcquireAgent block migration with the exact expected output format.
1112-1176: LGTM!This test verifies the core PR feature: migrating AcquireAgent blocks that only parse requests. The response is correctly discarded with
_since only the error is needed.
1178-1242: LGTM!The test correctly verifies that when
erris already declared, the migration uses assignment (=) instead of short declaration (:=) to avoid redeclaration errors.
1244-1312: LGTM!The test correctly verifies that collision avoidance works when
var err erroris declared after the AcquireAgent block. The migration choosesclientErrto avoid conflicts.
1314-1370: LGTM!The test correctly verifies that comments and code following parse-only AcquireAgent blocks are preserved in the migration.
1372-1426: LGTM!The test verifies that the token-based matching in
declaredInVarBlockLinecorrectly ignores variables likeerrMsgthat start with "err" but are not theerridentifier.
1428-1496: LGTM!The test verifies that
var errMsg stringdeclarations (with err-like prefix) don't incorrectly trigger collision avoidance for future declarations.
1498-1556: LGTM!This test correctly verifies that legitimate
errs []erroraccumulators are preserved while using a separateerrvariable for theSend()call. This addresses the previous critical review concern about type errors withappend.
349-436: Test expectation for map key naming needs verification against actual output.The test
Test_MigrateClientUsage_RewritesAcquireAgentStructWithVarsBetweenParseAndCallexpects the JSON map key to be renamed from"errs"to"clientErr"when the error variable is renamed toclientErrdue to collision avoidance. However, thereplaceErrIdentifierfunction uses word boundary patterns (\berrs\b) that only match identifiers, not string literals. The map key"errs"infiber.Map{"errs": errs}is a string literal and should not be transformed by the standard identifier replacement logic.The
rewriteClientErrorHandlingfunction'sclientErrMapPattern("errs"\s*:\s*errs) also won't match"errs": clientErrafter the main migration runs, so it cannot rename the key either.While the test currently passes, clarify whether:
- The JSON key should remain as
"err"(semantic API contract) regardless of the internal variable name, or- Both key and value should be renamed consistently when collision avoidance renames the error variable
Summary
Testing
Codex Task
Summary by CodeRabbit
New Features
Bug Fixes
Tests
✏️ Tip: You can customize this high-level summary in your review settings.