[BUG] wrap underlying errors with %w#307
[BUG] wrap underlying errors with %w#307krrishrastogi05 wants to merge 1 commit intovolcano-sh:mainfrom
Conversation
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
|
Welcome @krrishrastogi05! It looks like this is your first PR to volcano-sh/agentcube 🎉 |
There was a problem hiding this comment.
Pull request overview
This PR improves error propagation in AgentCube by wrapping underlying errors with %w so callers can reliably use errors.Is/errors.As to inspect root causes.
Changes:
- Wrap Valkey
MGETfailure errors with%win the Valkey store implementation. - Wrap X.509 public key parsing errors with
%win PicoD auth key loading.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
| pkg/store/store_valkey.go | Wraps Valkey MGET errors with %w to preserve the original error for callers. |
| pkg/picod/auth.go | Wraps x509.ParsePKIXPublicKey errors with %w so key-load failures retain underlying parse details. |
![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 updates error formatting to use "%w" for proper error wrapping in "pkg/picod/auth.go" and "pkg/store/store_valkey.go". Feedback highlights a potential logic inconsistency in "pkg/store/store_valkey.go" where "AsStrSlice()" may fail if keys are missing, and identifies a typo in the variable name "stingSliceResults".
| stingSliceResults, err := vs.cli.Do(ctx, vs.cli.B().Mget().Key(sessionIDKeys...).Build()).AsStrSlice() | ||
| if err != nil { | ||
| return nil, fmt.Errorf("loadSandboxesBySessionIDs: Valkey MGet sandboxes failed: %v", err) | ||
| return nil, fmt.Errorf("loadSandboxesBySessionIDs: Valkey MGet sandboxes failed: %w", err) |
There was a problem hiding this comment.
While wrapping the error with %w is correct, there is a potential logic inconsistency in the surrounding code. The AsStrSlice() call on line 114 will return an error if any of the requested keys do not exist in Valkey (as MGET returns nil for missing keys, which AsStrSlice cannot convert to a string). This seems to conflict with the logic on lines 125-128, which attempts to skip missing entries. If missing keys are expected, consider using AsSlice() to handle individual elements. Additionally, note the typo in the variable name stingSliceResults on line 114.
|
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #307 +/- ##
==========================================
+ Coverage 47.57% 47.75% +0.18%
==========================================
Files 30 30
Lines 2819 2854 +35
==========================================
+ Hits 1341 1363 +22
- Misses 1338 1343 +5
- Partials 140 148 +8
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Signed-off-by: Krrish <krrishrastogi00@gmail.com>
krrishrastogi05
force-pushed
the
krrish/fix-error-wrapping
branch
from
b104649 to
2b77ed7
Compare
/kind bug
What this PR does / why we need it:
This PR wraps two underlying errors with
%winstead of formatting them with%v, preserving error context for callers that useerrors.Isorerrors.As.Special notes for your reviewer:
go test ./pkg/storepasses.go test ./pkg/picodwas attempted, but fails on this Windows setup due to missing Unix commands likesh/sleepand symlink privilege restrictions.Does this PR introduce a user-facing change?: