Conversation
WalkthroughThis PR introduces a hierarchical error classification system for remote connection failures. A new Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes 🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
Tip Issue Planner is now in beta. Read the docs and try it out! Share your feedback on Discord. 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 |
![kiloconnect[bot]](https://avatars.githubusercontent.com/in/2193792?s=60&v=4){kind=small}
| if err != nil { | ||
| blocklogger.Infof(ctx, "[conndebug] ERROR dial error: %v\n", err) | ||
| return nil, utilds.MakeCodedError(ConnErrCode_Dial, err) | ||
| subCode := DialSubCode_ProxyJump // This is a proxy jump connection error |
There was a problem hiding this comment.
WARNING: Hardcoded subcode loses error classification detail
This hardcodes DialSubCode_ProxyJump for all proxy jump errors, but doesn't classify the actual error type. A proxy jump connection can fail for many reasons (DNS, timeout, refused, etc.).
Consider calling ClassifyDialErrorSubCode(err) to get the actual error type, or use a composite approach that preserves both the proxy-jump context and the underlying error classification.
Code Review SummaryStatus: 1 Issue Found | Recommendation: Address before merge Overview
Issue Details (click to expand)WARNING
Files Reviewed (5 files)
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (2)
pkg/remote/sshclient.go (2)
178-182:DialSubCode_ContextCanceledgroups both cancellation and deadline exceeded.
context.DeadlineExceededis mapped toDialSubCode_ContextCanceled("context-canceled"). If downstream telemetry consumers need to distinguish user-initiated cancellation from timeouts, this subcode won't differentiate them. Consider whether a separateDialSubCode_ContextDeadline(or renaming the constant to something likeDialSubCode_ContextError) would be more accurate. This is a minor telemetry clarity concern sinceConnContextErrorin telemetry already captures this as a boolean, and the timeout string-pattern checks below would never fire for these errors anyway.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/remote/sshclient.go` around lines 178 - 182, The current mapping treats both context.Canceled and context.DeadlineExceeded as DialSubCode_ContextCanceled, which loses distinction between user cancellation and timeouts; update the handling in the error-to-subcode logic (the block that checks errors.Is(err, context.Canceled) || errors.Is(err, context.DeadlineExceeded)) to return a separate subcode for deadline exceeded (e.g., add DialSubCode_ContextDeadline) while keeping DialSubCode_ContextCanceled for context.Canceled, or alternatively rename to DialSubCode_ContextError if you prefer a single bucket; ensure the new constant is declared alongside other DialSubCode values and update any telemetry code that consumes DialSubCode_ContextCanceled if it must account for the new DialSubCode_ContextDeadline.
854-859: Proxy jump errors always getDialSubCode_ProxyJump, losing the underlying cause.When dialing through a proxy and it fails (e.g., DNS failure on the proxy hop, timeout, connection refused), the subcode is unconditionally set to
"proxy-jump". If you want to capture the root cause for telemetry, you could nest or prefix the underlying subcode. That said, this may be intentional to distinguish proxy-hop failures from direct-dial failures—just flagging in case finer granularity was desired.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/remote/sshclient.go` around lines 854 - 859, The current code unconditionally sets DialSubCode_ProxyJump for any error from currentClient.DialContext, losing the underlying cause; modify the error handling in the clientConn = currentClient.DialContext(ctx, "tcp", networkAddr) block to detect if err is a utilds-style subcoded error (e.g., type assert to whatever subcoded error type the project uses or call an extractor like utilds.ExtractSubCode(err)), extract the inner subcode, and produce a composite subcode (for example DialSubCode_ProxyJump + ":" + innerSubCode) or otherwise nest/prefix the inner subcode; log both the proxy-jump context and the inner subcode via blocklogger.Infof and pass the composite/nested subcode into utilds.MakeSubCodedError(ConnErrCode_Dial, compositeSubCode, err) so the original error and its finer-grained subcode are preserved (refer to clientConn, currentClient.DialContext, DialSubCode_ProxyJump, utilds.MakeSubCodedError, ConnErrCode_Dial, blocklogger.Infof).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@pkg/remote/sshclient.go`:
- Around line 178-182: The current mapping treats both context.Canceled and
context.DeadlineExceeded as DialSubCode_ContextCanceled, which loses distinction
between user cancellation and timeouts; update the handling in the
error-to-subcode logic (the block that checks errors.Is(err, context.Canceled)
|| errors.Is(err, context.DeadlineExceeded)) to return a separate subcode for
deadline exceeded (e.g., add DialSubCode_ContextDeadline) while keeping
DialSubCode_ContextCanceled for context.Canceled, or alternatively rename to
DialSubCode_ContextError if you prefer a single bucket; ensure the new constant
is declared alongside other DialSubCode values and update any telemetry code
that consumes DialSubCode_ContextCanceled if it must account for the new
DialSubCode_ContextDeadline.
- Around line 854-859: The current code unconditionally sets
DialSubCode_ProxyJump for any error from currentClient.DialContext, losing the
underlying cause; modify the error handling in the clientConn =
currentClient.DialContext(ctx, "tcp", networkAddr) block to detect if err is a
utilds-style subcoded error (e.g., type assert to whatever subcoded error type
the project uses or call an extractor like utilds.ExtractSubCode(err)), extract
the inner subcode, and produce a composite subcode (for example
DialSubCode_ProxyJump + ":" + innerSubCode) or otherwise nest/prefix the inner
subcode; log both the proxy-jump context and the inner subcode via
blocklogger.Infof and pass the composite/nested subcode into
utilds.MakeSubCodedError(ConnErrCode_Dial, compositeSubCode, err) so the
original error and its finer-grained subcode are preserved (refer to clientConn,
currentClient.DialContext, DialSubCode_ProxyJump, utilds.MakeSubCodedError,
ConnErrCode_Dial, blocklogger.Infof).
1 participant
No description provided.