feat(generate): openai, anthropic and ollama provider adapters (WS1) - #2775
Open
devarismeroxa wants to merge 3 commits into
Open
feat(generate): openai, anthropic and ollama provider adapters (WS1)#2775devarismeroxa wants to merge 3 commits into
devarismeroxa wants to merge 3 commits into
Conversation
The three Provider implementations behind the seam merged in #2772. Zero new dependencies, per design doc §1: - openai wraps the ALREADY-VENDORED sashabaranov/go-openai — the same client the `openai` built-in processor uses. Hand-rolling a second OpenAI HTTP client would be the YAGNI violation, not avoiding one. - anthropic thin net/http JSON client against the Messages API. - ollama thin net/http JSON client against a local server's /api/generate, the same two-method Doer shape the `ollama` built-in processor already uses, so the tree keeps a single hand-rolled-client pattern rather than forking a second. Decisions worth stating: Token counts are only ever what the provider REPORTED. A 0 means it reported nothing; nothing is estimated. An estimated count would be indistinguishable from a real one in --json output that users may bill against. Every call is bounded by DefaultTimeout (60s). Generation is interactive — a human is watching a spinner — so an unbounded wait on a wedged provider is worse than a clear failure they can retry. The retry loop applies this per attempt, not to the whole budget. Anthropic responses concatenate every text block rather than taking the first. A response split across blocks is valid, and taking only the first would silently truncate the generated config — which would then fail validation for a reason that has nothing to do with the config. Ollama sets stream:false. Nothing renders tokens as they arrive, so streaming would only mean parsing a JSONL body for no benefit. Reachable() probes with HEAD and accepts any status < 500, deliberately NOT checking whether a model is loaded. Pulling a model is the user's job, and failing that belongs in the completion call's error where it can say so — not in a silent exclusion from auto-detection that leaves them with "no provider configured" on a machine that plainly has one. Provider errors carry the provider name and the HTTP status, never a stack trace: a user seeing this needs to know which provider failed and why, and a trace from inside an HTTP client tells them nothing actionable. Error bodies are bounded to 512 bytes and flattened to one line so an HTML error page cannot dump itself into a terminal. Tests are httptest-based (no network, no keys) and assert what actually went on the wire — headers, body, path — rather than trusting struct tags. Every mutation verified: - anthropic taking only the first text block -> ConcatenatesTextBlocks - dropping the mandatory anthropic-version header -> HappyPath - allowing an empty ollama response -> EmptyResponseIsAnError - estimating tokens instead of reporting -> HappyPath - treating non-2xx as success -> HTTPErrorsAreProviderErrors - enabling ollama streaming -> HappyPath The non-2xx mutation initially SURVIVED. The test asserted the error code and provider name, both of which still held: with status checking removed the error body decodes to empty content and yields the same provider_error, so the assertion passed for the wrong reason while the user would be told "empty response" instead of "HTTP 401". The test now asserts the status appears in the message, and the mutation is killed. Two httptest handlers initially blocked on r.Context().Done(), which deadlocks against httptest.Server.Close — the cleanup waits for the handler, the handler waits for a cancellation the cleanup would have caused. The package hung for the full 10-minute timeout. Both waits are now bounded. Risk tier: 3. Leaf package, no call sites yet, no data path, no new deps. Roadmap: v0.20 WS1 (`conduit generate`), design doc §1. Next: the generation loop (§3). Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_015GQFzakPShAYj8CcwajYDD
The repo's depguard list requires github.com/goccy/go-json over encoding/json. No behaviour change — same struct tags, same decoding. I pushed before reading the lint output on the previous commit; this is that output being addressed rather than a second attempt at the same thing.
The retry loop feeds a failed candidate's findings back into the next prompt,
and that text is partly derived from the MODEL'S OWN PRIOR OUTPUT — finding
messages quote the config paths, connector IDs, and values the candidate
contained. The design doc calls this out as two distinct hazards and makes
bounding it a testable requirement rather than a soft guideline:
(a) prompt growth — a candidate with dozens of findings balloons every
retry's payload;
(b) a channel by which model-originated text, including any injected
instruction the first candidate echoed, re-enters the next prompt.
So feedback is constructed from TYPED FIELDS (Code, ConfigPath, Suggestion) and
hard-bounded, never concatenated report prose. The finding's free-form Message
— which is exactly where candidate-derived text lives — never reaches the
prompt.
Bounds: at most 10 findings per retry, each value clipped to 120 characters and
flattened to a single line. Flattening matters as much as clipping: a value
containing "\n\nIgnore previous instructions" would otherwise render as its own
line in the retry prompt, visually indistinguishable from the instructions this
package wrote.
Truncation is stated in the prompt rather than hidden. A model told "fix these
10" when there are 40 will confidently return something still broken.
Only error-severity findings are fed back. Spending a retry "fixing" a lint
warning wastes a call and risks introducing a real error.
WithConnectorSuggestions is the retry loop's consumer of fuzzymatch (§7):
"connector `postgre` does not exist; did you mean `postgres`?" turns a
hallucination into a self-correction inside the retry budget instead of a
terminal failure. Suggestions come only from the known catalog, so nothing
model-originated is ever echoed back as if it were a valid name.
Tests, every mutation verified:
- echoing the finding Message -> NeverEchoesRawProse (the injected-instruction
test)
- removing the findings cap -> IsCapped
- dropping newline flattening -> ClipsAndFlattensValues + NeverEchoesRawProse
- feeding back warnings -> OnlyErrorSeverity
- skipping the sort -> Deterministic
- inventing a suggestion when nothing matches -> ConnectorSuggestions
- hiding truncation from the prompt -> IsCapped
Two of those did not kill on the first attempt and the tests were the problem,
not the mutations. The determinism test re-rendered one already-ordered report,
which passes with or without a sort — it proved stability, not ordering. It now
feeds deliberately unsorted findings across two files and asserts the output
order. The truncation mutation initially failed to apply at all (shell
escaping), so its "0 killed" was meaningless; re-run correctly, it kills.
Risk tier: 3. Leaf package, no call sites yet, no data path.
Roadmap: v0.20 WS1, design doc §3. Next: the loop itself wiring
provider.Complete -> parse -> validate.RunBytes -> semantic check -> preview.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_015GQFzakPShAYj8CcwajYDD
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.
The three
Providerimplementations behind the seam merged in #2772. Zero new dependencies, per design doc §1.openaisashabaranov/go-openai— the same client theopenaibuilt-in processor uses. Hand-rolling a second OpenAI HTTP client would be the YAGNI violation, not avoiding oneanthropicnet/httpJSON client against the Messages APIollamanet/httpJSON client against a local server, using the same two-methodDoershape theollamabuilt-in processor already has — so the tree keeps one hand-rolled-client pattern instead of forking a secondDecisions worth stating
Token counts are only ever what the provider reported. A
0means it reported nothing. Nothing is estimated — an estimated count is indistinguishable from a real one in--jsonoutput users may bill against.Every call is bounded (60s default). Generation is interactive — a human is watching a spinner — so an unbounded wait on a wedged provider is worse than a clear failure they can retry. Applied per attempt, not to the whole retry budget.
Anthropic concatenates every text block rather than taking the first. A split response is valid, and taking only the first would silently truncate the config — which would then fail validation for a reason unrelated to the config.
Reachable()deliberately does not check whether a model is loaded. Pulling a model is the user's job, and failing that belongs in the completion call's error where it can say so — not in a silent exclusion from auto-detection that leaves them with "no provider configured" on a machine that plainly has one.Errors carry the provider name and HTTP status, never a stack trace. A trace from inside an HTTP client tells a user nothing actionable. Error bodies are bounded to 512 bytes and flattened to one line, so an HTML error page can't dump itself into a terminal.
Adversarial self-review
provider_error. The assertion passed for the wrong reason, while the user would be told "empty response" instead of "HTTP 401". The test now asserts the status appears in the message; the mutation is killed.r.Context().Done()deadlocks againsthttptest.Server.Close: the cleanup waits for the handler, the handler waits for a cancellation the cleanup would have caused. Both waits are now bounded.depguardrequiresgoccy/go-jsonoverencoding/json. Fixed in a follow-up commit, and called out as such rather than folded in silently.Tests
httptest-based — no network, no API keys — and they assert what actually went on the wire (headers, body, path), not the struct tags.ConcatenatesTextBlocksanthropic-versionheaderHappyPathEmptyResponseIsAnErrorHappyPathHTTPErrorsAreProviderErrorsHappyPathAlso covered: context cancellation aborts promptly, the per-call timeout bounds a wedged provider, and
--modelprecedence over the adapter default.Risk tier
3. Leaf package, no call sites yet, no data path, no new dependencies.
Roadmap
v0.20 WS1 (
conduit generate), design doc §1. Next: the generation loop (§3) — grounded prompt → candidate →validate.RunBytesgate → semantic-intent check → preview.🤖 Generated with Claude Code
https://claude.ai/code/session_015GQFzakPShAYj8CcwajYDD