Octopus Germany: Fix Access Validation Infinite Loop#29433
Draft
r0oland wants to merge 3 commits intoevcc-io:masterfrom
Draft
Octopus Germany: Fix Access Validation Infinite Loop#29433r0oland wants to merge 3 commits intoevcc-io:masterfrom
r0oland wants to merge 3 commits intoevcc-io:masterfrom
Conversation
Contributor
There was a problem hiding this comment.
Hey - I've found 1 issue, and left some high level feedback:
- Relying on
strings.Containsagainst error messages (especially the generic"Invalid data") is brittle; if possible, prefer checking concrete error types or well-defined error codes fromActiveAgreementso authentication failures can be identified more robustly. - Instead of using the
authErrside flag, consider returning a wrapped sentinel error (e.g.,ErrAuthFailed) from the backoff closure and then branching onerrors.Is(err, ErrAuthFailed)afterRetryso the control flow is clearer and less stateful.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- Relying on `strings.Contains` against error messages (especially the generic `"Invalid data"`) is brittle; if possible, prefer checking concrete error types or well-defined error codes from `ActiveAgreement` so authentication failures can be identified more robustly.
- Instead of using the `authErr` side flag, consider returning a wrapped sentinel error (e.g., `ErrAuthFailed`) from the backoff closure and then branching on `errors.Is(err, ErrAuthFailed)` after `Retry` so the control flow is clearer and less stateful.
## Individual Comments
### Comment 1
<location path="tariff/octopusde.go" line_range="98-100" />
<code_context>
if err := backoff.Retry(func() error {
agr, err := t.gqlClient.ActiveAgreement()
if err != nil {
+ // Check for authentication errors and mark them as permanent
+ errMsg := err.Error()
+ if strings.Contains(errMsg, "authentication failed") || strings.Contains(errMsg, "Invalid data") {
+ authErr = true
+ return backoff.Permanent(err)
</code_context>
<issue_to_address>
**issue (bug_risk):** String-based auth error detection is brittle and may misclassify errors.
Relying on `err.Error()` substrings like "authentication failed" or a generic "Invalid data" makes this logic fragile and easy to break if messages change or collide with other errors. Where possible, use a typed error (e.g. `var ErrAuth`), a client error code, or another stable discriminator instead of message text to avoid misclassifying auth failures.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Co-authored-by: Copilot <copilot@github.com>
Member
|
Hi @r0oland. Please pick descriptive PR title. |
andig
reviewed
Apr 27, 2026
|
|
||
| // Exit immediately on authentication errors instead of retrying | ||
| if errors.Is(err, ErrAuthFailed) { | ||
| t.log.ERROR.Printf("authentication failed - configuration error: %v", err) |
Member
There was a problem hiding this comment.
what does "configuration error" mean here?
andig
reviewed
Apr 27, 2026
| // isAuthError checks if an error indicates an authentication failure. | ||
| // It inspects error messages from the GraphQL client which typically contain | ||
| // "authentication failed" when credentials are invalid or missing. | ||
| func isAuthError(err error) bool { |
Member
There was a problem hiding this comment.
Suggested change
| func isAuthError(err error) bool { | |
| func (t *OctopusDe) isAuthError(err error) bool { |
andig
reviewed
Apr 27, 2026
Comment on lines
+145
to
+146
| errMsg := err.Error() | ||
| return strings.Contains(errMsg, "authentication failed") |
Member
There was a problem hiding this comment.
Suggested change
| errMsg := err.Error() | |
| return strings.Contains(errMsg, "authentication failed") | |
| return strings.Contains(err.Error(), "authentication failed") |
andig
reviewed
Apr 27, 2026
| }, bo()); err != nil { | ||
| once.Do(func() { done <- err }) | ||
|
|
||
| // Exit immediately on authentication errors instead of retrying |
Member
There was a problem hiding this comment.
this is already after the retry. Just test for permanent error here and return if permanent?
andig
reviewed
Apr 27, 2026
Co-authored-by: andig <cpuidle@gmail.com>
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
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.
fix issue #29041