Skip to content

Include html respone if kosli command fails and the debug flag is set server/#4839#744

Merged
ToreMerkely merged 1 commit intomainfrom
4839-response-debug
Mar 30, 2026
Merged

Include html respone if kosli command fails and the debug flag is set server/#4839#744
ToreMerkely merged 1 commit intomainfrom
4839-response-debug

Conversation

@ToreMerkely
Copy link
Copy Markdown
Contributor

No description provided.

@ToreMerkely ToreMerkely enabled auto-merge (squash) March 30, 2026 08:39
@claude
Copy link
Copy Markdown
Contributor

claude bot commented Mar 30, 2026

Claude finished @ToreMerkely's task in 2m 30s —— View job


PR Review: Include HTML response if kosli command fails and the debug flag is set

  • Run static analysis checks (build, vet, mod tidy)
  • Review code changes
  • Post review feedback

Static Analysis

All checks pass: go build, go vet, go mod tidy — no issues.

Review Summary

Clean, well-scoped change. When a non-2xx response body can't be parsed as JSON (e.g. an HTML 403 from a proxy), the raw body is now logged at debug level. This is helpful for diagnosing proxy or gateway issues that return HTML error pages.

What's good:

  • The fix is minimal and precisely targeted — one Debug log line in the right error path
  • Good test coverage: new /proxy-block/ fake endpoint with a realistic HTML 403 body
  • The debug field addition to the test struct is a nice improvement — it makes existing tests more flexible without changing their behavior (Go zero-value false preserves current behavior for all existing cases)
  • The expectedLog assertion using require.Contains is appropriately scoped

Minor observations:

  • The returned error is still the raw json.Unmarshal error (e.g. invalid character '<' looking for beginning of value), which isn't the most user-friendly message for someone not running with --debug. That's a pre-existing issue and out of scope for this PR, but worth noting as a potential follow-up.
  • Left one nit as an inline comment — string(body) is fine but technically unnecessary since %s handles []byte directly.

Verdict: Looks good to merge. 👍

Comment thread internal/requests/requests.go
@ToreMerkely ToreMerkely merged commit ed0badf into main Mar 30, 2026
17 checks passed
@ToreMerkely ToreMerkely deleted the 4839-response-debug branch March 30, 2026 08:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants