[fix]: [AH-2686]: nuget error handling#91
Open
abhinavcode wants to merge 5 commits into
Open
Conversation
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
There was a problem hiding this comment.
Your free trial has ended. If you'd like to continue receiving code reviews, you can add a payment method here.
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdded extensive logging (Debug/Info/Warn/Error) across NuGet local and proxy implementations and changed Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
registry/app/pkg/nuget/local.go (1)
286-288:⚠️ Potential issue | 🟠 Major
err(nil) is wrapped instead oferr3— actual error is silently dropped.
errwas last assigned bybuildMetadataon line 268; because theif err != nilguard on line 269 returns early,erris alwaysnilby the time execution reaches line 288. Wrappingerrloses theCheckIfVersionExistsfailure entirely;err3should be used.🐛 Proposed fix
if err3 != nil { return headers, "", fmt.Errorf( "failed to check package version existence for id: %s , version: %s "+ - "with registry: %d with error: %w", info.Image, info.Version, info.RegistryID, err) + "with registry: %d with error: %w", info.Image, info.Version, info.RegistryID, err3) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@registry/app/pkg/nuget/local.go` around lines 286 - 288, The error returned in the fmt.Errorf call incorrectly wraps err (which is nil) instead of the actual error variable err3 from the CheckIfVersionExists call; in the function that calls buildMetadata and then CheckIfVersionExists (referencing buildMetadata, CheckIfVersionExists, info.Image, info.Version, info.RegistryID, err3), update the error construction to wrap err3 (not err) so the CheckIfVersionExists failure is preserved in the returned error message.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@registry/app/pkg/nuget/local.go`:
- Around line 290-292: Update the error message string in the return statement
that calls usererror.BadRequestf (the one returning headers, "",
usererror.BadRequestf(...)) to correct the grammar: change "doesn't exists" to
"doesn't exist" so the message reads "can't push symbol package as package
doesn't exist for id: %s, version: %s with registry: %d". Ensure you only modify
the literal text inside the BadRequestf format string and keep the format verbs
and arguments (info.Image, info.Version, info.RegistryID) unchanged.
- Around line 270-272: The call to usererror.BadRequestf is using the %w verb
(in the return that references headers, "", usererror.BadRequestf(...
info.RegistryID, err)), but usererror.BadRequestf uses fmt.Sprintf which doesn't
support %w; change the format verb from %w to %s so the error is formatted
correctly (i.e., "failed to build metadata for registry: %d with error: %s")
referencing the same variables (info.RegistryID and err). If you need to
preserve wrapping semantics use fmt.Errorf to wrap and then pass the wrapped
message string to usererror.BadRequestf instead.
---
Outside diff comments:
In `@registry/app/pkg/nuget/local.go`:
- Around line 286-288: The error returned in the fmt.Errorf call incorrectly
wraps err (which is nil) instead of the actual error variable err3 from the
CheckIfVersionExists call; in the function that calls buildMetadata and then
CheckIfVersionExists (referencing buildMetadata, CheckIfVersionExists,
info.Image, info.Version, info.RegistryID, err3), update the error construction
to wrap err3 (not err) so the CheckIfVersionExists failure is preserved in the
returned error message.
Comment on lines
+290
to
+292
| return headers, "", usererror.BadRequestf( | ||
| "can't push symbol package as package doesn't exists for id: %s , version: %s "+ | ||
| "with registry: %d with error: %w", info.Image, info.Version, info.RegistryID, err) | ||
| "with registry: %d", info.Image, info.Version, info.RegistryID) |
There was a problem hiding this comment.
Grammatical typo: "doesn't exists" → "doesn't exist".
- return headers, "", usererror.BadRequestf(
- "can't push symbol package as package doesn't exists for id: %s , version: %s "+
- "with registry: %d", info.Image, info.Version, info.RegistryID)
+ return headers, "", usererror.BadRequestf(
+ "can't push symbol package as package doesn't exist for id: %s , version: %s "+
+ "with registry: %d", info.Image, info.Version, info.RegistryID)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| return headers, "", usererror.BadRequestf( | |
| "can't push symbol package as package doesn't exists for id: %s , version: %s "+ | |
| "with registry: %d with error: %w", info.Image, info.Version, info.RegistryID, err) | |
| "with registry: %d", info.Image, info.Version, info.RegistryID) | |
| return headers, "", usererror.BadRequestf( | |
| "can't push symbol package as package doesn't exist for id: %s , version: %s "+ | |
| "with registry: %d", info.Image, info.Version, info.RegistryID) |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@registry/app/pkg/nuget/local.go` around lines 290 - 292, Update the error
message string in the return statement that calls usererror.BadRequestf (the one
returning headers, "", usererror.BadRequestf(...)) to correct the grammar:
change "doesn't exists" to "doesn't exist" so the message reads "can't push
symbol package as package doesn't exist for id: %s, version: %s with registry:
%d". Ensure you only modify the literal text inside the BadRequestf format
string and keep the format verbs and arguments (info.Image, info.Version,
info.RegistryID) unchanged.
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
registry/app/pkg/nuget/local.go (1)
335-337: Grammatical typo: "doesn't exists" → "doesn't exist".Line 336 still contains the grammar error from the previous review.
Proposed fix
return headers, "", usererror.BadRequestf( - "can't push symbol package as package doesn't exists for id: %s , version: %s "+ + "can't push symbol package as package doesn't exist for id: %s , version: %s "+ "with registry: %d", info.Image, info.Version, info.RegistryID)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@registry/app/pkg/nuget/local.go` around lines 335 - 337, Update the grammar in the error message returned by the NuGet push path: in the BadRequestf call that uses info.Image, info.Version and info.RegistryID (the return that currently does "can't push symbol package as package doesn't exists"), change "doesn't exists" to "doesn't exist" so the message reads e.g. "can't push symbol package as package doesn't exist for id: %s, version: %s with registry: %d".
🧹 Nitpick comments (1)
registry/app/pkg/nuget/local.go (1)
288-365: Consider whetherUploadPackagelog messages may leak sensitive data.The log messages include
info.RootIdentifier(Line 296-298),info.Image,info.Version, andinfo.RegIdentifieracross multiple log statements. Ensure these identifiers don't contain customer-sensitive or PII data that shouldn't appear in logs. For a package registry this is likely fine, but worth a quick confirmation given the volume of new logging.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@registry/app/pkg/nuget/local.go` around lines 288 - 365, The logs in UploadPackage call sites (around use of c.fileManager.UploadFileNoDBUpdate, DownloadFileByDigest, buildMetadata, validateAndNormaliseVersion, CheckIfVersionExists, and UpdateFileManagerAndCreateArtifact) currently print info.RootIdentifier, info.Image, info.Version, and info.RegIdentifier which may leak sensitive identifiers; update these log statements to either remove sensitive fields or redact/mask them (e.g., show hashed/truncated values or a boolean like "present" instead of the raw value), and make any full-identifier logging conditional on a configurable debug/PII-safe flag so you can enable detailed logs only when safe. Ensure changes cover all occurrences in this function including success and error paths and preserve contextual fields like registry id or error objects for debugging.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@registry/app/pkg/nuget/local.go`:
- Around line 326-338: The error block after calling
c.localBase.CheckIfVersionExists(ctx, info) is wrapping the wrong variable
(`err`) instead of the actual error (`err3`); update the fmt.Errorf in that
branch to wrap and include `err3` (the error returned by CheckIfVersionExists)
so the returned error message correctly reflects the failure from
CheckIfVersionExists when fileBundleType == SymbolsFile.
In `@registry/app/pkg/nuget/proxy.go`:
- Around line 352-365: The parse error handling returns the outer-scope err
(which is nil) instead of the actual parse error variable err2; update the error
returns in the parse error branches for parseRegistrationIndexPageResponse and
parseRegistrationIndexResponse to return err2 (not err) so callers receive the
real parsing error—look for the error checks that call
log.Ctx(ctx).Error().Err(err2)... and change the subsequent return statements
that currently return &nugettype.RegistrationIndexPageResponse{}, err and
&nugettype.RegistrationIndexResponse{}, err to return the same empty response
objects with err2.
---
Duplicate comments:
In `@registry/app/pkg/nuget/local.go`:
- Around line 335-337: Update the grammar in the error message returned by the
NuGet push path: in the BadRequestf call that uses info.Image, info.Version and
info.RegistryID (the return that currently does "can't push symbol package as
package doesn't exists"), change "doesn't exists" to "doesn't exist" so the
message reads e.g. "can't push symbol package as package doesn't exist for id:
%s, version: %s with registry: %d".
---
Nitpick comments:
In `@registry/app/pkg/nuget/local.go`:
- Around line 288-365: The logs in UploadPackage call sites (around use of
c.fileManager.UploadFileNoDBUpdate, DownloadFileByDigest, buildMetadata,
validateAndNormaliseVersion, CheckIfVersionExists, and
UpdateFileManagerAndCreateArtifact) currently print info.RootIdentifier,
info.Image, info.Version, and info.RegIdentifier which may leak sensitive
identifiers; update these log statements to either remove sensitive fields or
redact/mask them (e.g., show hashed/truncated values or a boolean like "present"
instead of the raw value), and make any full-identifier logging conditional on a
configurable debug/PII-safe flag so you can enable detailed logs only when safe.
Ensure changes cover all occurrences in this function including success and
error paths and preserve contextual fields like registry id or error objects for
debugging.
Comment on lines
326
to
338
| if fileBundleType == SymbolsFile { | ||
| versionExists, err3 := c.localBase.CheckIfVersionExists(ctx, info) | ||
| if err3 != nil { | ||
| log.Ctx(ctx).Error().Err(err3).Msgf("failed to check package version existence for id: %s, version: %s with registry: %d", info.Image, info.Version, info.RegistryID) | ||
| return headers, "", fmt.Errorf( | ||
| "failed to check package version existence for id: %s , version: %s "+ | ||
| "with registry: %d with error: %w", info.Image, info.Version, info.RegistryID, err) | ||
| } else if !versionExists { | ||
| return headers, "", fmt.Errorf( | ||
| log.Ctx(ctx).Warn().Msgf("can't push symbol package as package doesn't exist for id: %s, version: %s with registry: %d", info.Image, info.Version, info.RegistryID) | ||
| return headers, "", usererror.BadRequestf( | ||
| "can't push symbol package as package doesn't exists for id: %s , version: %s "+ | ||
| "with registry: %d with error: %w", info.Image, info.Version, info.RegistryID, err) | ||
| "with registry: %d", info.Image, info.Version, info.RegistryID) | ||
| } |
There was a problem hiding this comment.
Bug: wrapping err (nil) instead of err3 in the version-existence check error path.
At Line 332, the fmt.Errorf wraps err but should wrap err3. At this point in the code, err holds the result of c.buildMetadata(r) (Line 309) which was nil (otherwise execution wouldn't reach here). The actual error from CheckIfVersionExists is in err3.
🐛 Proposed fix
if err3 != nil {
log.Ctx(ctx).Error().Err(err3).Msgf("failed to check package version existence for id: %s, version: %s with registry: %d", info.Image, info.Version, info.RegistryID)
return headers, "", fmt.Errorf(
"failed to check package version existence for id: %s , version: %s "+
- "with registry: %d with error: %w", info.Image, info.Version, info.RegistryID, err)
+ "with registry: %d with error: %w", info.Image, info.Version, info.RegistryID, err3)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| if fileBundleType == SymbolsFile { | |
| versionExists, err3 := c.localBase.CheckIfVersionExists(ctx, info) | |
| if err3 != nil { | |
| log.Ctx(ctx).Error().Err(err3).Msgf("failed to check package version existence for id: %s, version: %s with registry: %d", info.Image, info.Version, info.RegistryID) | |
| return headers, "", fmt.Errorf( | |
| "failed to check package version existence for id: %s , version: %s "+ | |
| "with registry: %d with error: %w", info.Image, info.Version, info.RegistryID, err) | |
| } else if !versionExists { | |
| return headers, "", fmt.Errorf( | |
| log.Ctx(ctx).Warn().Msgf("can't push symbol package as package doesn't exist for id: %s, version: %s with registry: %d", info.Image, info.Version, info.RegistryID) | |
| return headers, "", usererror.BadRequestf( | |
| "can't push symbol package as package doesn't exists for id: %s , version: %s "+ | |
| "with registry: %d with error: %w", info.Image, info.Version, info.RegistryID, err) | |
| "with registry: %d", info.Image, info.Version, info.RegistryID) | |
| } | |
| if fileBundleType == SymbolsFile { | |
| versionExists, err3 := c.localBase.CheckIfVersionExists(ctx, info) | |
| if err3 != nil { | |
| log.Ctx(ctx).Error().Err(err3).Msgf("failed to check package version existence for id: %s, version: %s with registry: %d", info.Image, info.Version, info.RegistryID) | |
| return headers, "", fmt.Errorf( | |
| "failed to check package version existence for id: %s , version: %s "+ | |
| "with registry: %d with error: %w", info.Image, info.Version, info.RegistryID, err3) | |
| } else if !versionExists { | |
| log.Ctx(ctx).Warn().Msgf("can't push symbol package as package doesn't exist for id: %s, version: %s with registry: %d", info.Image, info.Version, info.RegistryID) | |
| return headers, "", usererror.BadRequestf( | |
| "can't push symbol package as package doesn't exists for id: %s , version: %s "+ | |
| "with registry: %d", info.Image, info.Version, info.RegistryID) | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@registry/app/pkg/nuget/local.go` around lines 326 - 338, The error block
after calling c.localBase.CheckIfVersionExists(ctx, info) is wrapping the wrong
variable (`err`) instead of the actual error (`err3`); update the fmt.Errorf in
that branch to wrap and include `err3` (the error returned by
CheckIfVersionExists) so the returned error message correctly reflects the
failure from CheckIfVersionExists when fileBundleType == SymbolsFile.
Comment on lines
352
to
365
| metadata, err2 := parseRegistrationIndexPageResponse(fileReader) | ||
| if err2 != nil { | ||
| //todo: add handling for registration leaf | ||
| log.Ctx(ctx).Error().Err(err2).Msgf("failed to parse registration index page response for registry: %s, image: %s", info.RegIdentifier, info.Image) | ||
| return &nugettype.RegistrationIndexPageResponse{}, err | ||
| } | ||
| updateRegistrationIndexPageResponse(metadata, packageURL, info.Image) | ||
| log.Ctx(ctx).Info().Msgf("successfully retrieved package metadata page from proxy for registry: %s, image: %s", info.RegIdentifier, info.Image) | ||
| return metadata, nil | ||
| } | ||
| metadata, err2 := parseRegistrationIndexResponse(fileReader) | ||
| if err2 != nil { | ||
| log.Ctx(ctx).Error().Err(err2).Msgf("failed to parse registration index response for registry: %s, image: %s", info.RegIdentifier, info.Image) | ||
| return &nugettype.RegistrationIndexResponse{}, err | ||
| } |
There was a problem hiding this comment.
Bug: returning err (nil) instead of err2 swallows parse errors.
On Line 355 and Line 364, the code returns err from the outer scope (which is nil at this point since helper.GetPackageMetadata succeeded) instead of err2 from the failed parse call. This means both parseRegistrationIndexPageResponse and parseRegistrationIndexResponse failures are silently swallowed — the caller receives a nil error with an empty response object.
This is a pre-existing bug, but since the new logging correctly uses err2, it's a good time to fix the returns as well.
🐛 Proposed fix
if info.ProxyEndpoint != "" {
metadata, err2 := parseRegistrationIndexPageResponse(fileReader)
if err2 != nil {
log.Ctx(ctx).Error().Err(err2).Msgf("failed to parse registration index page response for registry: %s, image: %s", info.RegIdentifier, info.Image)
- return &nugettype.RegistrationIndexPageResponse{}, err
+ return &nugettype.RegistrationIndexPageResponse{}, err2
}
updateRegistrationIndexPageResponse(metadata, packageURL, info.Image)
log.Ctx(ctx).Info().Msgf("successfully retrieved package metadata page from proxy for registry: %s, image: %s", info.RegIdentifier, info.Image)
return metadata, nil
}
metadata, err2 := parseRegistrationIndexResponse(fileReader)
if err2 != nil {
log.Ctx(ctx).Error().Err(err2).Msgf("failed to parse registration index response for registry: %s, image: %s", info.RegIdentifier, info.Image)
- return &nugettype.RegistrationIndexResponse{}, err
+ return &nugettype.RegistrationIndexResponse{}, err2
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| metadata, err2 := parseRegistrationIndexPageResponse(fileReader) | |
| if err2 != nil { | |
| //todo: add handling for registration leaf | |
| log.Ctx(ctx).Error().Err(err2).Msgf("failed to parse registration index page response for registry: %s, image: %s", info.RegIdentifier, info.Image) | |
| return &nugettype.RegistrationIndexPageResponse{}, err | |
| } | |
| updateRegistrationIndexPageResponse(metadata, packageURL, info.Image) | |
| log.Ctx(ctx).Info().Msgf("successfully retrieved package metadata page from proxy for registry: %s, image: %s", info.RegIdentifier, info.Image) | |
| return metadata, nil | |
| } | |
| metadata, err2 := parseRegistrationIndexResponse(fileReader) | |
| if err2 != nil { | |
| log.Ctx(ctx).Error().Err(err2).Msgf("failed to parse registration index response for registry: %s, image: %s", info.RegIdentifier, info.Image) | |
| return &nugettype.RegistrationIndexResponse{}, err | |
| } | |
| metadata, err2 := parseRegistrationIndexPageResponse(fileReader) | |
| if err2 != nil { | |
| log.Ctx(ctx).Error().Err(err2).Msgf("failed to parse registration index page response for registry: %s, image: %s", info.RegIdentifier, info.Image) | |
| return &nugettype.RegistrationIndexPageResponse{}, err2 | |
| } | |
| updateRegistrationIndexPageResponse(metadata, packageURL, info.Image) | |
| log.Ctx(ctx).Info().Msgf("successfully retrieved package metadata page from proxy for registry: %s, image: %s", info.RegIdentifier, info.Image) | |
| return metadata, nil | |
| } | |
| metadata, err2 := parseRegistrationIndexResponse(fileReader) | |
| if err2 != nil { | |
| log.Ctx(ctx).Error().Err(err2).Msgf("failed to parse registration index response for registry: %s, image: %s", info.RegIdentifier, info.Image) | |
| return &nugettype.RegistrationIndexResponse{}, err2 | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@registry/app/pkg/nuget/proxy.go` around lines 352 - 365, The parse error
handling returns the outer-scope err (which is nil) instead of the actual parse
error variable err2; update the error returns in the parse error branches for
parseRegistrationIndexPageResponse and parseRegistrationIndexResponse to return
err2 (not err) so callers receive the real parsing error—look for the error
checks that call log.Ctx(ctx).Error().Err(err2)... and change the subsequent
return statements that currently return
&nugettype.RegistrationIndexPageResponse{}, err and
&nugettype.RegistrationIndexResponse{}, err to return the same empty response
objects with err2.
There was a problem hiding this comment.
♻️ Duplicate comments (2)
registry/app/pkg/nuget/local.go (2)
334-336:⚠️ Potential issue | 🟡 MinorTypo still present: "doesn't exists" → "doesn't exist".
📝 Proposed fix
return headers, "", usererror.BadRequestf( - "can't push symbol package as package doesn't exists for id: %s , version: %s "+ + "can't push symbol package as package doesn't exist for id: %s , version: %s "+ "with registry: %d", info.Image, info.Version, info.RegistryID)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@registry/app/pkg/nuget/local.go` around lines 334 - 336, Fix the typo in the returned BadRequest message: change "doesn't exists" to "doesn't exist" in the return statement that constructs the error using info.Image, info.Version and info.RegistryID (the return that calls usererror.BadRequestf in local.go).
325-331:⚠️ Potential issue | 🔴 CriticalBug still present:
fmt.Errorfat Line 331 wrapserr(nil) instead oferr3.
errat this point is the return value ofc.buildMetadata(r)at Line 308, which is guaranteed to benil(a non-nil value returns early at Line 309). The actual CheckIfVersionExists error is inerr3. This causes the propagated error to silently drop the root cause.🐛 Proposed fix
if err3 != nil { log.Ctx(ctx).Error().Err(err3).Msgf("failed to check package version existence for id: %s, version: %s with registry: %d", info.Image, info.Version, info.RegistryID) return headers, "", fmt.Errorf( "failed to check package version existence for id: %s , version: %s "+ - "with registry: %d with error: %w", info.Image, info.Version, info.RegistryID, err) + "with registry: %d with error: %w", info.Image, info.Version, info.RegistryID, err3)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@registry/app/pkg/nuget/local.go` around lines 325 - 331, The error returned when CheckIfVersionExists fails is being wrapped with the wrong variable: change the fmt.Errorf in the error return inside the SymbolsFile branch to wrap err3 (the error from c.localBase.CheckIfVersionExists) instead of err so the root cause is preserved; locate the block handling fileBundleType == SymbolsFile, the call to c.localBase.CheckIfVersionExists(ctx, info) and the subsequent fmt.Errorf return and replace the wrapped error variable with err3.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@registry/app/pkg/nuget/local.go`:
- Around line 334-336: Fix the typo in the returned BadRequest message: change
"doesn't exists" to "doesn't exist" in the return statement that constructs the
error using info.Image, info.Version and info.RegistryID (the return that calls
usererror.BadRequestf in local.go).
- Around line 325-331: The error returned when CheckIfVersionExists fails is
being wrapped with the wrong variable: change the fmt.Errorf in the error return
inside the SymbolsFile branch to wrap err3 (the error from
c.localBase.CheckIfVersionExists) instead of err so the root cause is preserved;
locate the block handling fileBundleType == SymbolsFile, the call to
c.localBase.CheckIfVersionExists(ctx, info) and the subsequent fmt.Errorf return
and replace the wrapped error variable with err3.
There was a problem hiding this comment.
♻️ Duplicate comments (3)
registry/app/pkg/nuget/local.go (2)
362-364:⚠️ Potential issue | 🟡 MinorGrammatical typo: "doesn't exists" → "doesn't exist".
✏️ Proposed fix
- "can't push symbol package as package doesn't exists for id: %s , version: %s "+ + "can't push symbol package as package doesn't exist for id: %s , version: %s "+🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@registry/app/pkg/nuget/local.go` around lines 362 - 364, The BadRequestf error message in the usererror.BadRequestf call uses the incorrect phrase "doesn't exists"; update the string passed to usererror.BadRequestf (the call with info.Image, info.Version, info.RegistryID) to read "doesn't exist" instead of "doesn't exists" and preserve the rest of the message and formatting.
353-365:⚠️ Potential issue | 🔴 CriticalBug: still wrapping
err(nil) instead oferr3in the version-existence error path.
errat line 359 is the result ofc.buildMetadata(r)(line 336), which isnilat this point — otherwise execution would not have reached line 354. The real failure is inerr3fromc.localBase.CheckIfVersionExists. Callers receive an error message with no wrapped cause (%wof nil).🐛 Proposed fix
if err3 != nil { log.Ctx(ctx).Error().Err(err3).Msgf(...) return headers, "", fmt.Errorf( "failed to check package version existence for id: %s , version: %s "+ - "with registry: %d with error: %w", info.Image, info.Version, info.RegistryID, err) + "with registry: %d with error: %w", info.Image, info.Version, info.RegistryID, err3)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@registry/app/pkg/nuget/local.go` around lines 353 - 365, The error return in the SymbolsFile path wraps the wrong variable: currently fmt.Errorf uses err (from c.buildMetadata) which is nil; replace that with err3 (the error from c.localBase.CheckIfVersionExists) so the returned error properly includes the underlying cause. Locate the if-block checking fileBundleType == SymbolsFile and the call to c.localBase.CheckIfVersionExists, and update the fmt.Errorf call to wrap err3 (and adjust any variable names in the return) so the logged error and returned error reflect the actual failure from CheckIfVersionExists.registry/app/pkg/nuget/proxy.go (1)
408-427:⚠️ Potential issue | 🔴 CriticalBug: still returning
err(nil) instead oferr2in both parse-error branches.
errat lines 413 and 426 is the outer-scope error fromhelper.GetPackageMetadata, which isnilat this point (a non-nil value would have triggered the early return at lines 398–403). Both branches silently swallow the parse failure and return an empty response with a nil error, giving callers a false-success signal.🐛 Proposed fix
metadata, err2 := parseRegistrationIndexPageResponse(fileReader) if err2 != nil { log.Ctx(ctx).Error().Err(err2).Msgf(...) - return &nugettype.RegistrationIndexPageResponse{}, err + return &nugettype.RegistrationIndexPageResponse{}, err2 } ... } metadata, err2 := parseRegistrationIndexResponse(fileReader) if err2 != nil { log.Ctx(ctx).Error().Err(err2).Msgf(...) - return &nugettype.RegistrationIndexResponse{}, err + return &nugettype.RegistrationIndexResponse{}, err2 }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@registry/app/pkg/nuget/proxy.go` around lines 408 - 427, The parse-error branches in the function call parseRegistrationIndexPageResponse(fileReader) and parseRegistrationIndexResponse(fileReader) incorrectly return the outer-scope err (which is nil) instead of the parse error variable err2; update both branches to return the parse error (err2) so callers get the failure instead of a false-success. Locate the blocks that log errors with log.Ctx(ctx).Error().Err(err2).Msgf(...) (refer to parseRegistrationIndexPageResponse and parseRegistrationIndexResponse and the variables metadata, fileReader, info.RegIdentifier, info.Image) and change the return statements to return an appropriate nil/empty response and err2 (e.g., return nil, err2 or return &nugettype.RegistrationIndexPageResponse{}, err2 consistent with surrounding code).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@registry/app/pkg/nuget/local.go`:
- Around line 362-364: The BadRequestf error message in the
usererror.BadRequestf call uses the incorrect phrase "doesn't exists"; update
the string passed to usererror.BadRequestf (the call with info.Image,
info.Version, info.RegistryID) to read "doesn't exist" instead of "doesn't
exists" and preserve the rest of the message and formatting.
- Around line 353-365: The error return in the SymbolsFile path wraps the wrong
variable: currently fmt.Errorf uses err (from c.buildMetadata) which is nil;
replace that with err3 (the error from c.localBase.CheckIfVersionExists) so the
returned error properly includes the underlying cause. Locate the if-block
checking fileBundleType == SymbolsFile and the call to
c.localBase.CheckIfVersionExists, and update the fmt.Errorf call to wrap err3
(and adjust any variable names in the return) so the logged error and returned
error reflect the actual failure from CheckIfVersionExists.
In `@registry/app/pkg/nuget/proxy.go`:
- Around line 408-427: The parse-error branches in the function call
parseRegistrationIndexPageResponse(fileReader) and
parseRegistrationIndexResponse(fileReader) incorrectly return the outer-scope
err (which is nil) instead of the parse error variable err2; update both
branches to return the parse error (err2) so callers get the failure instead of
a false-success. Locate the blocks that log errors with
log.Ctx(ctx).Error().Err(err2).Msgf(...) (refer to
parseRegistrationIndexPageResponse and parseRegistrationIndexResponse and the
variables metadata, fileReader, info.RegIdentifier, info.Image) and change the
return statements to return an appropriate nil/empty response and err2 (e.g.,
return nil, err2 or return &nugettype.RegistrationIndexPageResponse{}, err2
consistent with surrounding code).
There was a problem hiding this comment.
♻️ Duplicate comments (2)
registry/app/pkg/nuget/local.go (2)
375-383:⚠️ Potential issue | 🔴 CriticalBug:
erris nil here — should beerr3.At line 383,
fmt.Errorfwrapserr, buterrwas last assigned at line 356 (c.buildMetadata) and is guaranteednilat this point (thenilcheck on line 357 would have returned otherwise). The actual error fromCheckIfVersionExistsiserr3.🐛 Proposed fix
return headers, "", fmt.Errorf( "failed to check package version existence for id: %s , version: %s "+ - "with registry: %d with error: %w", info.Image, info.Version, info.RegistryID, err) + "with registry: %d with error: %w", info.Image, info.Version, info.RegistryID, err3)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@registry/app/pkg/nuget/local.go` around lines 375 - 383, The error returned from c.localBase.CheckIfVersionExists is stored in err3 but the fmt.Errorf call and return wrap the wrong variable (err) in the fileBundleType == SymbolsFile branch; update the fmt.Errorf and returned wrapped error to use err3 (i.e., change the final parameter from err to err3) so the actual CheckIfVersionExists error is propagated (locate the block around fileBundleType == SymbolsFile where c.localBase.CheckIfVersionExists(ctx, info) is called).
388-390:⚠️ Potential issue | 🟡 MinorGrammatical typo: "doesn't exists" → "doesn't exist".
📝 Proposed fix
return headers, "", usererror.BadRequestf( - "can't push symbol package as package doesn't exists for id: %s , version: %s "+ + "can't push symbol package as package doesn't exist for id: %s , version: %s "+ "with registry: %d", info.Image, info.Version, info.RegistryID)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@registry/app/pkg/nuget/local.go` around lines 388 - 390, The error message string in the NuGet symbol-push path uses incorrect grammar ("doesn't exists"); update the message in the return that constructs usererror.BadRequestf (the call returning headers, "" , usererror.BadRequestf(...)) to use "doesn't exist" instead of "doesn't exists" while preserving the rest of the formatted placeholders (info.Image, info.Version, info.RegistryID).
🧹 Nitpick comments (3)
registry/app/pkg/nuget/local.go (3)
125-127: Consider usingDebuginstead ofInfofor routine success paths.Success logs at
Infolevel in high-throughput paths (list, search, count, download, etc.) can generate significant log volume.Debugis typically more appropriate for "operation completed" messages, reservingInfofor noteworthy state changes. This applies throughout the file (Lines 125, 151, 170, 189, 209, 241, 265, 295, 325, 418, 511, 538).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@registry/app/pkg/nuget/local.go` around lines 125 - 127, Several routine success log calls use log.Ctx(ctx).Info() and should be lowered to Debug to reduce high-throughput log noise: replace occurrences of log.Ctx(ctx).Info().Msgf(...) with log.Ctx(ctx).Debug().Msgf(...) for the listed success paths in this file (the Info calls emitting "successfully listed %d package versions..." and the other similar success messages), keeping the same Msgf message and parameters and preserving context and structured fields; apply the same change for the other Info calls noted in this file so only noteworthy state changes remain at Info level.
358-361: User-facing error drops the root cause — consider including it via%s.The
%wissue from the previous review was fixed, but the error detail was dropped entirely. ThebuildMetadataerrors contain user-actionable messages (e.g., "invalid package id", "failed to parse .nuspec file") that would help users diagnose upload failures.💡 Suggested improvement
- return headers, "", usererror.BadRequestf("failed to build metadata for registry: %d", - info.RegistryID) + return headers, "", usererror.BadRequestf("failed to build metadata for registry: %d: %s", + info.RegistryID, err)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@registry/app/pkg/nuget/local.go` around lines 358 - 361, The user-facing BadRequest currently drops the underlying error message after buildMetadata fails; update the return so the BadRequestf includes the error detail (e.g., use usererror.BadRequestf("failed to build metadata for registry %d: %s", info.RegistryID, err) or similar using err.Error()/%v) while keeping the existing log.Ctx(ctx).Error().Err(err).Msgf call; locate the failing block around buildMetadata in local.go (the log.Ctx(...).Error() and the return of headers, "", usererror.BadRequestf(...)) and append the root-cause error text to the formatted user error.
360-361:usererror.BadRequestfexposes internalRegistryIDto clients.The
BadRequestfmessages are user-facing HTTP responses. Including the internal databaseRegistryID(numeric) leaks implementation details. Consider using the user-facinginfo.RegIdentifierinstead.Also applies to: 370-371, 388-390
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@registry/app/pkg/nuget/local.go` around lines 360 - 361, The BadRequestf calls currently expose the internal numeric RegistryID (usererror.BadRequestf(..., info.RegistryID)); change these user-facing error messages to use the public identifier info.RegIdentifier instead. Locate each occurrence of usererror.BadRequestf that references info.RegistryID (the ones near the metadata-building return paths in local.go) and replace the RegistryID interpolation with info.RegIdentifier so the response shows the user-facing registry identifier rather than the internal numeric ID.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@registry/app/pkg/nuget/local.go`:
- Around line 375-383: The error returned from c.localBase.CheckIfVersionExists
is stored in err3 but the fmt.Errorf call and return wrap the wrong variable
(err) in the fileBundleType == SymbolsFile branch; update the fmt.Errorf and
returned wrapped error to use err3 (i.e., change the final parameter from err to
err3) so the actual CheckIfVersionExists error is propagated (locate the block
around fileBundleType == SymbolsFile where c.localBase.CheckIfVersionExists(ctx,
info) is called).
- Around line 388-390: The error message string in the NuGet symbol-push path
uses incorrect grammar ("doesn't exists"); update the message in the return that
constructs usererror.BadRequestf (the call returning headers, "" ,
usererror.BadRequestf(...)) to use "doesn't exist" instead of "doesn't exists"
while preserving the rest of the formatted placeholders (info.Image,
info.Version, info.RegistryID).
---
Nitpick comments:
In `@registry/app/pkg/nuget/local.go`:
- Around line 125-127: Several routine success log calls use log.Ctx(ctx).Info()
and should be lowered to Debug to reduce high-throughput log noise: replace
occurrences of log.Ctx(ctx).Info().Msgf(...) with log.Ctx(ctx).Debug().Msgf(...)
for the listed success paths in this file (the Info calls emitting "successfully
listed %d package versions..." and the other similar success messages), keeping
the same Msgf message and parameters and preserving context and structured
fields; apply the same change for the other Info calls noted in this file so
only noteworthy state changes remain at Info level.
- Around line 358-361: The user-facing BadRequest currently drops the underlying
error message after buildMetadata fails; update the return so the BadRequestf
includes the error detail (e.g., use usererror.BadRequestf("failed to build
metadata for registry %d: %s", info.RegistryID, err) or similar using
err.Error()/%v) while keeping the existing log.Ctx(ctx).Error().Err(err).Msgf
call; locate the failing block around buildMetadata in local.go (the
log.Ctx(...).Error() and the return of headers, "", usererror.BadRequestf(...))
and append the root-cause error text to the formatted user error.
- Around line 360-361: The BadRequestf calls currently expose the internal
numeric RegistryID (usererror.BadRequestf(..., info.RegistryID)); change these
user-facing error messages to use the public identifier info.RegIdentifier
instead. Locate each occurrence of usererror.BadRequestf that references
info.RegistryID (the ones near the metadata-building return paths in local.go)
and replace the RegistryID interpolation with info.RegIdentifier so the response
shows the user-facing registry identifier rather than the internal numeric ID.
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.
Summary by CodeRabbit
Bug Fixes
Chores