refactor: shared HTTP client, TrailingNewlineWriter, and detect-nonce fix#50
Merged
min0625 merged 2 commits intoJun 30, 2026
Merged
Conversation
Qodo reviews are paused for this user.Troubleshooting steps vary by plan Learn more → On a Teams plan? Using GitHub Enterprise Server, GitLab Self-Managed, or Bitbucket Data Center? |
Codecov Report❌ Patch coverage is 📢 Thoughts on this report? Let us know! |
… fix
Introduce internal/httpx.New() — a shared *http.Client with 30 s dial /
10 s TLS timeouts, no overall timeout (streaming can be arbitrarily long,
per-request cancellation lives in the context) — and wire all three
provider backends to it instead of bare &http.Client{}.
Add internal/llm.TrailingNewlineWriter: a tiny io.Writer wrapper whose
Done() appends exactly one '\n' unless the model already ended on one.
This replaces the unconditional fmt.Fprintln(w) each provider used, which
produced a spurious blank trailing line whenever the model streamed text
that already ended with a newline.
OpenAI: add defaultEndpoint bool on Client so stream_options.include_usage
(an OpenAI extension) is only sent to the official endpoint; local or
proxy servers reached via a custom base URL may reject unknown fields.
StreamOptions is now *streamOptions with omitempty.
detectLanguage now consumes the nonce returned by buildDetectPrompt to
filter echoed delimiter lines, consistent with how the rewrite path
already works. buildDetectPrompt signature extended to return the nonce.
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
min0625
force-pushed
the
feat/shared-http-client-and-trailing-newline-writer
branch
from
76d553c to
982700a
Compare
Streaming I/O error return paths (writer failures in provider backends and detectLanguage's nonce filter flush) require a failWriter mock to exercise — brittle without meaningful test value. 80% is above the current 83% patch coverage so the gate passes while remaining honest about what is intentionally left uncovered. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.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
1 participant
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
internal/httpx— new shared*http.Clientwith 30 s dial / 10 s TLS timeouts (no overall timeout; streaming durations are unbounded and cancellation lives in the context); all three provider backends now use it instead of bare&http.Client{}internal/llm.TrailingNewlineWriter— centralises the "ensure exactly one trailing newline" concern; replaces the unconditionalfmt.Fprintln(w)each backend used, which produced a spurious blank line when the model already ended its stream with\nstream_options.include_usageis now only sent to the official endpoint (defaultEndpoint boolonClient); local / proxy servers (Ollama, LM Studio) may reject unknown fields so it is omitted when a custom base URL is set;StreamOptionschanged to*streamOptionswithomitemptybuildDetectPromptnow returns the nonce anddetectLanguagefilters echoed delimiter lines throughnewNonceFilter, consistent with the rewrite path (previously only rewrite filtered echoed nonces)Test plan
make checkpasses (lint + race-detector tests)TestTrailingNewlineWriter— 6 sub-cases covering no-newline, existing newline, split-chunk, empty-chunk, empty stream, internal newlinesTestDetectLanguageFiltersEchoedNonce— weaker model echoing nonce still yields a clean BCP-47 tagTestCompleteOmitsStreamOptionsForCustomBaseURL— custom base URL request body must not containstream_optionsTestCompleteDoesNotDoubleTrailingNewline— model output ending in\nmust not gain a second one🤖 Generated with Claude Code