feat(llamaswap): add llama-swaps (TLS) DSN scheme #4

Merged

steve merged 1 commits from feat/llama-swaps-tls into main 2026-06-27 22:57:00 +00:00

Conversation 10 Commits 1 Files Changed 6

+73 -29

steve commented 2026-06-27 21:59:16 +00:00

Owner

Copy Link

Copy Source

Follow-up to #3. llama-swap was reachable by DSN only over http (llama-swap://), which pushed TLS-fronted instances onto the openai:// scheme — losing the management methods and image generation that live on the concrete *llamaswap.Provider.

This adds a llama-swaps:// scheme that builds an https:// base URL, alongside llama-swap:// (http, local-first). Mirrors the redis/rediss convention.

LLM_LS=llama-swap://token@box.local:8080     # http  (local-first)
LLM_LS=llama-swaps://token@swap.example.com  # https (TLS-fronted)

• Both schemes share one factory in builtin.go; llama-swaps is scheme-only (no default built-in provider name).
• Why a second scheme rather than auto-detect: a DSN carries no reliable http-vs-https signal, so the choice stays explicit.
• Tests: TestLlamaSwapsScheme asserts the https base URL; existing llama-swap http test unchanged.
• Docs updated in the same commit: ADR-0015, README DSN section, CLAUDE.md scheme list, progress.md.

Gates green (build, vet, test -race, mod tidy). No new dependencies.

🤖 Generated with Claude Code

Follow-up to #3. llama-swap was reachable by DSN only over **http** (`llama-swap://`), which pushed TLS-fronted instances onto the `openai://` scheme — losing the management methods and image generation that live on the concrete `*llamaswap.Provider`. This adds a **`llama-swaps://`** scheme that builds an `https://` base URL, alongside `llama-swap://` (http, local-first). Mirrors the `redis`/`rediss` convention. ``` LLM_LS=llama-swap://token@box.local:8080 # http (local-first) LLM_LS=llama-swaps://token@swap.example.com # https (TLS-fronted) ``` - Both schemes share one factory in `builtin.go`; `llama-swaps` is scheme-only (no default built-in provider name). - Why a second scheme rather than auto-detect: a DSN carries no reliable http-vs-https signal, so the choice stays explicit. - Tests: `TestLlamaSwapsScheme` asserts the https base URL; existing `llama-swap` http test unchanged. - Docs updated in the same commit: ADR-0015, README DSN section, CLAUDE.md scheme list, progress.md. Gates green (`build`, `vet`, `test -race`, `mod tidy`). No new dependencies. 🤖 Generated with [Claude Code](https://claude.com/claude-code)

steve added 1 commit 2026-06-27 21:59:16 +00:00

feat(llamaswap): add llama-swaps (TLS) DSN scheme ... de2b2f0f28

llama-swap was http-only by DSN, pushing TLS-fronted instances onto the openai://
scheme (which loses the management/image methods). Add a "llama-swaps" scheme
that builds an https base URL, alongside "llama-swap" (http, local-first) —
mirroring redis/rediss. Both share one factory; llama-swaps is scheme-only (no
default built-in). The choice stays explicit because a DSN has no reliable
http-vs-https signal.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>

gitea-actions bot commented 2026-06-27 21:59:18 +00:00

Copy Link

Copy Source

🪰 Gadfly review — m5/qwen3.6:35b-mlx (m5)

Verdict: No material issues found — 3 reviewers: security, correctness, error-handling

🔒 Security — No material issues found

VERDICT: No material issues found

Findings:

I traced the full input path for the llama-swaps:// DSN scheme and confirmed through reading:

1. No new injection vectors introduced. The llamaSwapScheme(urlScheme) factory in builtin.go:89-97 constructs base URLs as urlScheme+"://"+dsn.Host. This mirrors the existing llama-swap (http) path exactly. Both schemes share identical code paths through ParseDSN() → WithBaseURL() → doJSON() → http.NewRequestWithContext().

2. Existing host validation gap is pre-existing, not introduced by this PR. ParseDSN() at env.go:46-64 performs zero validation on the Host field — no character whitelist, no length limit, no internal IP blocking. An attacker who controls LLM_LS could set llama-swaps://internal-service.local/path for SSRF. But this is true for all providers (openai, ollama, anthropic, etc.) which all use DSN.Host identically without validation. The PR does not make this worse.

3. Token leakage to upstream server via HTTPS. The llama-swaps scheme explicitly sends the bearer token over TLS (https://), which is the correct and intended behavior. Contrast with llama-swap (http://) where the token would be sent in cleartext — a pre-existing design consideration noted in comments, not changed by this PR.

4. Scheme name collision unlikely. ProviderLlamaSwapTLS = "llama-swaps" is registered as a new key in r.schemes. No other registration uses this string (confirmed via grep). The scheme-only registration (no no-DSN built-in) follows the same pattern as documented.

Verified by:

• Read builtin.go:89-100 — confirmed llamaSwapScheme factory logic for both http/https entries
• Read env.go:46-64 — confirmed ParseDSN() performs no host validation
• Read provider/llamaswap/llamaswap.go:64,85-97 — confirmed WithBaseURL and provider construction
• Grep'd for RegisterScheme, schemes[ProviderLlamaSwapTLS] — confirmed unique registration
• Read builtin.go:20-29 — confirmed constant definition

Conclusion: The PR is a straightforward scheme addition with no new security properties. It does not introduce injection, SSRF, credential leakage, or any other vulnerability beyond what already existed in the llama-swap:// scheme (and all other schemes). Both schemes share identical code paths through unvalidated input — but that's pre-existing and shared across all provider factories, not introduced by this change.

🎯 Correctness — No material issues found

No material issues found.

Findings:

• README.md:172 — misleading duplicate env var name ⚠️ (verified by reading README.md:170-173): Both example DSN lines use LLM_LS, which in practice would conflict — the second would overwrite the first. The TLS example should use a distinct var name like LLM_LST to match the test and make sense as parallel examples. This is a documentation correctness issue: readers copying both lines into the same env will get unexpected behavior. Suggested fix: change line 172's prefix from LLM_LS= to LLM_LST= (or another distinct name).

🧯 Error handling & edge cases — No material issues found

No material issues found

• Factory error contract is clean: llamaSwapScheme consistently returns (llm.Provider, nil), mirroring the existing llama-swap scheme and matching the pre-existing contract that llamaswap.New() never errors on construction (errors surface at request time). No new unhappy paths or swallowed failures were introduced. (builtin.go:89-97)
• Closure safety verified: The extracted llamaSwapOpts(extra ...Option) closure safely handles variadic expansion and the optional httpClient append. The backing slice is fresh per call, eliminating any risk of shared-slice aliasing bugs across concurrent factory invocations. (builtin.go:83-88)
• Edge cases handled identically to baseline: Empty dsn.Host or missing token produce "https://" + empty host via string concat. This matches the pre-existing behavior for llama-swap:// (which produces "http://" + empty host) and defers validation to request time, avoiding early panics. (env.go:36, provider/llamaswap/llamaswap.go:65)
• No missing cleanup: The factory path performs zero I/O or resource allocation that requires defer or rollback. Registry map insertion is atomic and safe.

_{Automated adversarial review by Gadfly. Advisory only — does not block merge. · ⏱️ reviewed in 11m 45s}

<!-- gadfly-review:ollama:m5/qwen3.6:35b-mlx --> ### 🪰 Gadfly review — `m5/qwen3.6:35b-mlx` (m5) **Verdict: No material issues found** — 3 reviewers: security, correctness, error-handling <details><summary><b>🔒 Security</b> — No material issues found</summary> # VERDICT: No material issues found **Findings:** I traced the full input path for the `llama-swaps://` DSN scheme and confirmed through reading: 1. **No new injection vectors introduced.** The `llamaSwapScheme(urlScheme)` factory in `builtin.go:89-97` constructs base URLs as `urlScheme+"://"+dsn.Host`. This mirrors the existing `llama-swap` (`http`) path exactly. Both schemes share identical code paths through `ParseDSN()` → `WithBaseURL()` → `doJSON()` → `http.NewRequestWithContext()`. 2. **Existing host validation gap is pre-existing, not introduced by this PR.** `ParseDSN()` at `env.go:46-64` performs zero validation on the `Host` field — no character whitelist, no length limit, no internal IP blocking. An attacker who controls `LLM_LS` could set `llama-swaps://internal-service.local/path` for SSRF. But this is true for all providers (openai, ollama, anthropic, etc.) which all use `DSN.Host` identically without validation. The PR does not make this worse. 3. **Token leakage to upstream server via HTTPS.** The `llama-swaps` scheme explicitly sends the bearer token over TLS (`https://`), which is the correct and intended behavior. Contrast with `llama-swap` (`http://`) where the token would be sent in cleartext — a pre-existing design consideration noted in comments, not changed by this PR. 4. **Scheme name collision unlikely.** `ProviderLlamaSwapTLS = "llama-swaps"` is registered as a new key in `r.schemes`. No other registration uses this string (confirmed via grep). The scheme-only registration (no no-DSN built-in) follows the same pattern as documented. **Verified by:** - Read `builtin.go:89-100` — confirmed `llamaSwapScheme` factory logic for both http/https entries - Read `env.go:46-64` — confirmed `ParseDSN()` performs no host validation - Read `provider/llamaswap/llamaswap.go:64,85-97` — confirmed `WithBaseURL` and provider construction - Grep'd for `RegisterScheme`, `schemes[ProviderLlamaSwapTLS]` — confirmed unique registration - Read `builtin.go:20-29` — confirmed constant definition **Conclusion:** The PR is a straightforward scheme addition with no new security properties. It does not introduce injection, SSRF, credential leakage, or any other vulnerability beyond what already existed in the `llama-swap://` scheme (and all other schemes). Both schemes share identical code paths through unvalidated input — but that's pre-existing and shared across all provider factories, not introduced by this change. </details> <details><summary><b>🎯 Correctness</b> — No material issues found</summary> No material issues found. **Findings:** - **README.md:172 — misleading duplicate env var name** ⚠️ (verified by reading `README.md:170-173`): Both example DSN lines use `LLM_LS`, which in practice would conflict — the second would overwrite the first. The TLS example should use a distinct var name like `LLM_LST` to match the test and make sense as parallel examples. This is a documentation correctness issue: readers copying both lines into the same env will get unexpected behavior. Suggested fix: change line 172's prefix from `LLM_LS=` to `LLM_LST=` (or another distinct name). </details> <details><summary><b>🧯 Error handling & edge cases</b> — No material issues found</summary> No material issues found - **Factory error contract is clean**: `llamaSwapScheme` consistently returns `(llm.Provider, nil)`, mirroring the existing `llama-swap` scheme and matching the pre-existing contract that `llamaswap.New()` never errors on construction (errors surface at request time). No new unhappy paths or swallowed failures were introduced. (`builtin.go:89-97`) - **Closure safety verified**: The extracted `llamaSwapOpts(extra ...Option)` closure safely handles variadic expansion and the optional `httpClient` append. The backing slice is fresh per call, eliminating any risk of shared-slice aliasing bugs across concurrent factory invocations. (`builtin.go:83-88`) - **Edge cases handled identically to baseline**: Empty `dsn.Host` or missing token produce `"https://"` + empty host via string concat. This matches the pre-existing behavior for `llama-swap://` (which produces `"http://"` + empty host) and defers validation to request time, avoiding early panics. (`env.go:36`, `provider/llamaswap/llamaswap.go:65`) - **No missing cleanup**: The factory path performs zero I/O or resource allocation that requires `defer` or rollback. Registry map insertion is atomic and safe. </details> _{Automated adversarial review by Gadfly. Advisory only — does not block merge. · ⏱️ reviewed in 11m 45s}

gitea-actions bot commented 2026-06-27 21:59:18 +00:00

Copy Link

Copy Source

🪰 Gadfly review — glm-5.2:cloud (ollama-cloud)

Verdict: No material issues found — 3 reviewers: security, correctness, error-handling

🔒 Security — No material issues found

Verdict: No material issues found

• The llama-swaps factory reuses the same dsn.Host/dsn.Token handling as every other built-in scheme (e.g. openai uses dsn.BaseURL() = "https://"+dsn.Host), so it introduces no new input-validation or injection surface. The base URL is composed as urlScheme+"://"+dsn.Host and consumed via http.NewRequestWithContext(ctx, method, p.baseURL+path, ...) in provider/llamaswap/llamaswap.go:200; dsn.Host is operator-supplied env config (not untrusted request input), so this is not an SSRF vector.
• TLS is enforced purely by the literal "https" prefix passed into llamaSwapScheme("https") (builtin.go:100); there is no path by which a llama-swaps:// DSN can degrade to a plaintext http:// base URL. The bearer token is forwarded identically to the existing llama-swap path (WithToken(dsn.Token) → Authorization: Bearer … at llamaswap.go:207-209), so credentials are not leaked over an unencrypted transport when the TLS scheme is selected.
• ProviderLlamaSwapTLS is intentionally scheme-only (no r.providers[ProviderLlamaSwapTLS] registration), so a bare llama-swaps provider name won't resolve to a no-URL built-in that could silently hit a default host — this is the safer choice and is consistent with the PR's stated design. Verified by reading builtin.go:98-100.
• No new dependencies, deserialization, or secret-logging paths are added.

Nothing in the security lens is materially wrong with this change.

🎯 Correctness — No material issues found

VERDICT: No material issues found

• Verified builtin.go:89-100: the shared llamaSwapScheme(urlScheme) factory builds urlScheme+"://"+dsn.Host correctly for both schemes. For llama-swaps://tok@swap.example.com, ParseDSN yields Host="swap.example.com" (verified in env.go:46-64), so the base URL is https://swap.example.com — matching the test assertion and the documented TLS behavior.
• Verified WithBaseURL (provider/llamaswap/llamaswap.go:64-66) trims only trailing slashes and preserves any /path, so dsn.Host path handling is identical to the existing llama-swap http path. No divergence introduced by the https twin.
• Verified ProviderLlamaSwapTLS = "llama-swaps" is registered only as a scheme factory (r.schemes), not as a r.providers entry (builtin.go:98-100), so it correctly has no no-DSN built-in — consistent with the PR intent and ADR-0015 wording.
• The DSN.BaseURL() https-always helper (env.go:41) is intentionally bypassed for both llama-swap schemes via the explicit urlScheme parameter, so there is no double-prefix or scheme mismatch risk.
• Lazy resolution path (registry.go:254-283) handles llama-swaps identically to other schemes via providerFromDSN → r.schemes[dsn.Scheme]; no special-casing needed and none missing.

Nothing in the correctness lens is materially wrong.

🧯 Error handling & edge cases — No material issues found

VERDICT: No material issues found

• The change is narrowly scoped: it adds a llama-swaps scheme factory that mirrors the existing llama-swap one, differing only by the URL scheme string ("https" vs "http"). I verified builtin.go:89-100 — the factory closure correctly threads urlScheme into WithBaseURL(urlScheme+"://"+dsn.Host), and both schemes route to llamaswap.New, which never fails at construction (confirmed at provider/llamaswap/llamaswap.go:82-91), so the factory's error return is always nil here, consistent with the existing llama-swap path.
• No-URL / empty-host edge case: llama-swaps is scheme-only (no r.providers[ProviderLlamaSwapTLS] registration — verified via grep, only r.schemes[ProviderLlamaSwapTLS] exists at builtin.go:100). The provider is only ever built from a DSN, and ParseDSN (env.go:46-63) already rejects an empty host with ErrInvalidDSN before the factory is reached, so the factory never sees dsn.Host == "" to produce a malformed https:// base URL. If someone constructs via llamaswap.New directly with no base URL, the existing request-time guard at llamaswap.go:103-105 / 189-191 returns a clear error — unchanged behavior, no new unhandled path introduced.
• No new ignored/swallowed errors: the factory returns the llamaswap.New(...) result directly (which is non-nil and error-free here), and providerFromDSN (env.go:104-120) already wraps any factory error via the scheme %q: %w path. The diff adds no defer, cleanup, or rollback concern (factory returns a stateless provider).
• Boundary/empty-collection concerns: none introduced; the only collection touched is r.schemes, a simple map assignment, with no iteration or length-dependent logic.

Outside my lens: the package doc comment at provider/llamaswap/llamaswap.go:20-22 still describes only the single llama-swap scheme and says "a TLS-fronted instance can use the openai:// scheme for chat instead" — now stale given llama-swaps exists — but that's a docs-consistency issue, not an error-handling one.

_{Automated adversarial review by Gadfly. Advisory only — does not block merge. · ⏱️ reviewed in 49s}

<!-- gadfly-review:ollama:glm-5.2:cloud --> ### 🪰 Gadfly review — `glm-5.2:cloud` (ollama-cloud) **Verdict: No material issues found** — 3 reviewers: security, correctness, error-handling <details><summary><b>🔒 Security</b> — No material issues found</summary> **Verdict: No material issues found** - The `llama-swaps` factory reuses the same `dsn.Host`/`dsn.Token` handling as every other built-in scheme (e.g. `openai` uses `dsn.BaseURL()` = `"https://"+dsn.Host`), so it introduces no new input-validation or injection surface. The base URL is composed as `urlScheme+"://"+dsn.Host` and consumed via `http.NewRequestWithContext(ctx, method, p.baseURL+path, ...)` in `provider/llamaswap/llamaswap.go:200`; `dsn.Host` is operator-supplied env config (not untrusted request input), so this is not an SSRF vector. - TLS is enforced purely by the literal `"https"` prefix passed into `llamaSwapScheme("https")` (`builtin.go:100`); there is no path by which a `llama-swaps://` DSN can degrade to a plaintext `http://` base URL. The bearer token is forwarded identically to the existing `llama-swap` path (`WithToken(dsn.Token)` → `Authorization: Bearer …` at `llamaswap.go:207-209`), so credentials are not leaked over an unencrypted transport when the TLS scheme is selected. - `ProviderLlamaSwapTLS` is intentionally scheme-only (no `r.providers[ProviderLlamaSwapTLS]` registration), so a bare `llama-swaps` provider name won't resolve to a no-URL built-in that could silently hit a default host — this is the safer choice and is consistent with the PR's stated design. Verified by reading `builtin.go:98-100`. - No new dependencies, deserialization, or secret-logging paths are added. Nothing in the security lens is materially wrong with this change. </details> <details><summary><b>🎯 Correctness</b> — No material issues found</summary> VERDICT: No material issues found - Verified `builtin.go:89-100`: the shared `llamaSwapScheme(urlScheme)` factory builds `urlScheme+"://"+dsn.Host` correctly for both schemes. For `llama-swaps://tok@swap.example.com`, `ParseDSN` yields `Host="swap.example.com"` (verified in `env.go:46-64`), so the base URL is `https://swap.example.com` — matching the test assertion and the documented TLS behavior. - Verified `WithBaseURL` (`provider/llamaswap/llamaswap.go:64-66`) trims only trailing slashes and preserves any `/path`, so `dsn.Host` path handling is identical to the existing `llama-swap` http path. No divergence introduced by the https twin. - Verified `ProviderLlamaSwapTLS = "llama-swaps"` is registered only as a scheme factory (`r.schemes`), not as a `r.providers` entry (`builtin.go:98-100`), so it correctly has no no-DSN built-in — consistent with the PR intent and ADR-0015 wording. - The `DSN.BaseURL()` https-always helper (`env.go:41`) is intentionally bypassed for both llama-swap schemes via the explicit `urlScheme` parameter, so there is no double-prefix or scheme mismatch risk. - Lazy resolution path (`registry.go:254-283`) handles `llama-swaps` identically to other schemes via `providerFromDSN` → `r.schemes[dsn.Scheme]`; no special-casing needed and none missing. Nothing in the correctness lens is materially wrong. </details> <details><summary><b>🧯 Error handling & edge cases</b> — No material issues found</summary> ## VERDICT: No material issues found - The change is narrowly scoped: it adds a `llama-swaps` scheme factory that mirrors the existing `llama-swap` one, differing only by the URL scheme string (`"https"` vs `"http"`). I verified `builtin.go:89-100` — the factory closure correctly threads `urlScheme` into `WithBaseURL(urlScheme+"://"+dsn.Host)`, and both schemes route to `llamaswap.New`, which never fails at construction (confirmed at `provider/llamaswap/llamaswap.go:82-91`), so the factory's `error` return is always `nil` here, consistent with the existing `llama-swap` path. - No-URL / empty-host edge case: `llama-swaps` is scheme-only (no `r.providers[ProviderLlamaSwapTLS]` registration — verified via grep, only `r.schemes[ProviderLlamaSwapTLS]` exists at `builtin.go:100`). The provider is only ever built from a DSN, and `ParseDSN` (`env.go:46-63`) already rejects an empty host with `ErrInvalidDSN` before the factory is reached, so the factory never sees `dsn.Host == ""` to produce a malformed `https://` base URL. If someone constructs via `llamaswap.New` directly with no base URL, the existing request-time guard at `llamaswap.go:103-105` / `189-191` returns a clear error — unchanged behavior, no new unhandled path introduced. - No new ignored/swallowed errors: the factory returns the `llamaswap.New(...)` result directly (which is non-nil and error-free here), and `providerFromDSN` (`env.go:104-120`) already wraps any factory error via the `scheme %q: %w` path. The diff adds no `defer`, cleanup, or rollback concern (factory returns a stateless provider). - Boundary/empty-collection concerns: none introduced; the only collection touched is `r.schemes`, a simple map assignment, with no iteration or length-dependent logic. Outside my lens: the package doc comment at `provider/llamaswap/llamaswap.go:20-22` still describes only the single `llama-swap` scheme and says "a TLS-fronted instance can use the openai:// scheme for chat instead" — now stale given `llama-swaps` exists — but that's a docs-consistency issue, not an error-handling one. </details> _{Automated adversarial review by Gadfly. Advisory only — does not block merge. · ⏱️ reviewed in 49s}

gitea-actions bot commented 2026-06-27 21:59:18 +00:00

Copy Link

Copy Source

🪰 Gadfly review — glm-5.1:cloud (ollama-cloud)

Verdict: No material issues found — 3 reviewers: security, correctness, error-handling

🔒 Security — No material issues found

VERDICT: No material issues found

The change is narrow and security-sound:

• URL scheme is a closed constant: llamaSwapScheme("http") and llamaSwapScheme("https") hard-code the scheme string; dsn.Host is appended after :// with no interpolation of user-controlled scheme data, so there is no scheme-injection or SSRF risk.
• TLS uses Go's secure defaults: llama-swaps builds an https:// base URL with the default http.Client{} (no InsecureSkipVerify, no custom TLSClientConfig), so certificate verification follows the standard, secure path — consistent with every other scheme in builtin.go.
• Bearer token handling unchanged: dsn.Token flows to WithToken, which sets the Authorization: Bearer header (line 208 of llamaswap.go). Under llama-swaps:// this is sent over TLS; under llama-swap:// it remains plaintext HTTP, but that is pre-existing behavior the PR improves by offering the TLS twin.
• No secret leakage: neither the new test nor the factory logs or stringifies the token; BaseURL() returns the host URL only.
• No accidental fallback: llama-swaps registers only a scheme factory (no default built-in provider), so it cannot be used without an explicit LLM_* DSN — no risk of silently falling back to an unconfigured provider.

🎯 Correctness — No material issues found

VERDICT: No material issues found

The change is a narrow, well-structured addition of a llama-swaps DSN scheme that mirrors the llama-swap http scheme with an https base URL. I verified the following:

• Scheme factory correctness: llamaSwapScheme("https") produces urlScheme+"://"+dsn.Host → "https://swap.example.com" for the test DSN, confirmed by the TestLlamaSwapsScheme test. The llama-swap scheme still correctly produces "http://" URLs. No off-by-one or scheme-construction bug.

• No accidental default provider: llama-swaps is intentionally registered only as a scheme (r.schemes), not as a default built-in (r.providers). This matches the documented design ("scheme-only, no default built-in provider name") and the ADR. A user who references llama-swaps without a DSN will get a provider-not-found, same as any unregistered name — not a silent wrong-behavior bug.

• DSN parsing: ParseDSN("llama-swaps://tok@swap.example.com") correctly splits on :// to yield {Scheme: "llama-swaps", Token: "tok", Host: "swap.example.com"}, which providerFromDSN then routes to the "llama-swaps" scheme factory. Verified by reading env.go.

• No stale references: The package doc comment in llamaswap/llamaswap.go:20-22 still mentions only llama-swap:// and suggests using openai:// for TLS instances. This is now slightly stale (since llama-swaps:// exists as an alternative), but this is a documentation clarity issue, not a correctness bug — the old advice still works.

No logic bugs, incorrect conversions, or semantic errors found.

🧯 Error handling & edge cases — No material issues found

VERDICT: No material issues found

• Error propagation: llamaSwapScheme returns the (llm.Provider, error) tuple from llamaswap.New, which per its docs never errors at construction time — missing base URL surfaces at request time via Provider.Model's p.baseURL == "" guard (verified at provider/llamaswap/llamaswap.go:102-104). The nil error return from the factory is therefore correct, not swallowed.
• Empty/malformed DSN host: ParseDSN (at env.go:60) already rejects host == "" with ErrInvalidDSN, so llama-swaps:// or llama-swaps://@ correctly fails before reaching the scheme factory.
• No built-in provider for llama-swaps: This is intentional (scheme-only, documented in ADR-0015 and the ProviderLlamaSwapTLS comment). Without a DSN, Provider("llama-swaps") returns (nil, false), which Parse will surface as ErrUnknownProvider — consistent with how unregistered providers behave.
• Closure capture: urlScheme is a parameter to llamaSwapScheme, not a loop variable, so no stale-capture bug.
• No nil dereferences, no missing defers, no swallowed errors, and no boundary issues found in the change.

_{Automated adversarial review by Gadfly. Advisory only — does not block merge. · ⏱️ reviewed in 1m 4s}

<!-- gadfly-review:ollama:glm-5.1:cloud --> ### 🪰 Gadfly review — `glm-5.1:cloud` (ollama-cloud) **Verdict: No material issues found** — 3 reviewers: security, correctness, error-handling <details><summary><b>🔒 Security</b> — No material issues found</summary> **VERDICT: No material issues found** The change is narrow and security-sound: - **URL scheme is a closed constant**: `llamaSwapScheme("http")` and `llamaSwapScheme("https")` hard-code the scheme string; `dsn.Host` is appended after `://` with no interpolation of user-controlled scheme data, so there is no scheme-injection or SSRF risk. - **TLS uses Go's secure defaults**: `llama-swaps` builds an `https://` base URL with the default `http.Client{}` (no `InsecureSkipVerify`, no custom `TLSClientConfig`), so certificate verification follows the standard, secure path — consistent with every other scheme in `builtin.go`. - **Bearer token handling unchanged**: `dsn.Token` flows to `WithToken`, which sets the `Authorization: Bearer` header (line 208 of `llamaswap.go`). Under `llama-swaps://` this is sent over TLS; under `llama-swap://` it remains plaintext HTTP, but that is pre-existing behavior the PR *improves* by offering the TLS twin. - **No secret leakage**: neither the new test nor the factory logs or stringifies the token; `BaseURL()` returns the host URL only. - **No accidental fallback**: `llama-swaps` registers only a scheme factory (no default built-in provider), so it cannot be used without an explicit `LLM_*` DSN — no risk of silently falling back to an unconfigured provider. </details> <details><summary><b>🎯 Correctness</b> — No material issues found</summary> **VERDICT: No material issues found** The change is a narrow, well-structured addition of a `llama-swaps` DSN scheme that mirrors the `llama-swap` http scheme with an https base URL. I verified the following: - **Scheme factory correctness**: `llamaSwapScheme("https")` produces `urlScheme+"://"+dsn.Host` → `"https://swap.example.com"` for the test DSN, confirmed by the `TestLlamaSwapsScheme` test. The `llama-swap` scheme still correctly produces `"http://"` URLs. No off-by-one or scheme-construction bug. - **No accidental default provider**: `llama-swaps` is intentionally registered only as a scheme (`r.schemes`), not as a default built-in (`r.providers`). This matches the documented design ("scheme-only, no default built-in provider name") and the ADR. A user who references `llama-swaps` without a DSN will get a provider-not-found, same as any unregistered name — not a silent wrong-behavior bug. - **DSN parsing**: `ParseDSN("llama-swaps://tok@swap.example.com")` correctly splits on `://` to yield `{Scheme: "llama-swaps", Token: "tok", Host: "swap.example.com"}`, which `providerFromDSN` then routes to the `"llama-swaps"` scheme factory. Verified by reading `env.go`. - **No stale references**: The package doc comment in `llamaswap/llamaswap.go:20-22` still mentions only `llama-swap://` and suggests using `openai://` for TLS instances. This is now slightly stale (since `llama-swaps://` exists as an alternative), but this is a documentation clarity issue, not a correctness bug — the old advice still works. No logic bugs, incorrect conversions, or semantic errors found. </details> <details><summary><b>🧯 Error handling & edge cases</b> — No material issues found</summary> **VERDICT: No material issues found** - **Error propagation**: `llamaSwapScheme` returns the `(llm.Provider, error)` tuple from `llamaswap.New`, which per its docs never errors at construction time — missing base URL surfaces at request time via `Provider.Model`'s `p.baseURL == ""` guard (verified at `provider/llamaswap/llamaswap.go:102-104`). The `nil` error return from the factory is therefore correct, not swallowed. - **Empty/malformed DSN host**: `ParseDSN` (at `env.go:60`) already rejects `host == ""` with `ErrInvalidDSN`, so `llama-swaps://` or `llama-swaps://@` correctly fails before reaching the scheme factory. - **No built-in provider for `llama-swaps`**: This is intentional (scheme-only, documented in ADR-0015 and the `ProviderLlamaSwapTLS` comment). Without a DSN, `Provider("llama-swaps")` returns `(nil, false)`, which `Parse` will surface as `ErrUnknownProvider` — consistent with how unregistered providers behave. - **Closure capture**: `urlScheme` is a parameter to `llamaSwapScheme`, not a loop variable, so no stale-capture bug. - No nil dereferences, no missing defers, no swallowed errors, and no boundary issues found in the change. </details> _{Automated adversarial review by Gadfly. Advisory only — does not block merge. · ⏱️ reviewed in 1m 4s}

gitea-actions bot commented 2026-06-27 21:59:18 +00:00

Copy Link

Copy Source

🪰 Gadfly review — minimax-m3:cloud (ollama-cloud)

Verdict: No material issues found — 3 reviewers: security, correctness, error-handling

🔒 Security — No material issues found

Now I have a complete picture. Let me reason through the security lens for this PR.

What changes from a security perspective:

• A new DSN scheme llama-swaps constructs https://<host> and passes it through WithBaseURL/WithToken to llamaswap.New.
• The token-bearing path is unchanged: token still flows from the DSN userinfo into Authorization: Bearer <token> (lines 207–209 in llamaswap.go).
• The HTTP client is still the default &http.Client{} (no TLS pinning, no custom Transport).
• No new HTTP transport, no new credential handling, no new input parsing beyond what ParseDSN already does.

Trace of the only new tainted input: the DSN host is concatenated into a URL via urlScheme + "://" + dsn.Host. ParseDSN only trims trailing /; it does not validate that the host contains no whitespace, control chars, scheme, or ?#/. But that's pre-existing behaviour shared with every other scheme (foreman, ollama, openai, anthropic, google, gemini) — llama-swaps is no worse than its siblings. (Strictly out-of-scope for this PR; pre-existing for all schemes, would belong to a "input validation" cleanup PR across the whole registry.)

Other security checks:

• Authn/authz: unchanged — same bearer-token semantics, no new auth path. The DSN userinfo (token@) is documented as the credential.
• Injection (SQL/command/path/template): no new SQL, no new commands, no new path templating. The llama-swap package's own path-building (/api/models/unload) is unchanged.
• SSRF: the DSN host is caller-controlled by design (operator's LLM_* env var). No change in attack surface vs. the existing llama-swap:// scheme. The factory passes the host straight to the existing HTTP client; nothing new is fetched, redirected-to, or parsed in a new way.
• Unsafe deserialization: json.NewDecoder(...).Decode(out) on responses is unchanged; body bounded by maxResponseBytes = 64 << 20 already.
• Secret/credential leakage: token stays in p.token, only sent as Authorization: Bearer ... over the wire — no new logging, no new surface.
• Insecure defaults: nothing newly insecure. The default HTTP client has no InsecureSkipVerify override. The llama-swaps factory goes through the standard Go http.Client, which uses the system trust store for TLS validation by default.
• TLS handling: actually a small improvement — llama-swaps is the explicit way to use TLS with a llama-swap instance, so an operator who needs TLS now has a first-class scheme rather than being nudged at openai://.

Verified by reading:

• builtin.go lines 22–28, 77–100 — scheme registration and factory closure construction.
• env.go lines 27–63, 104–119 — DSN parsing and providerFromDSN dispatch.
• provider/llamaswap/llamaswap.go lines 49–91, 188–227 — Provider struct, WithBaseURL/WithToken/New, and doJSON (auth header, response cap, no insecure TLS overrides).
• registry.go line 51 — SchemeFactory signature matches what llamaSwapScheme returns.
• No occurrences of InsecureSkipVerify / TLSClientConfig / MinVersion anywhere in the package.

Verdict: No material issues found in the security lens.

The change is a tiny wiring addition (one factory closure parameterised by urlScheme, one extra entry in r.schemes) that reuses the same token-handling, HTTP client, JSON decoding, and Authorization header logic that the existing llama-swap:// scheme already exercises. No new authentication, parsing, transport, or deserialization paths are introduced; the only behavioural delta is the URL prefix, which is the entire point of the feature.

🎯 Correctness — No material issues found

VERDICT: No material issues found

Verified by reading builtin.go:77-100, env.go:27-64, provider/llamaswap/llamaswap.go:82-97, and builtin_llamaswap_test.go:54-74:

• DSN parsing (env.go:46-64) for llama-swaps://tok@swap.example.com yields Scheme=llama-swaps, Token=tok, Host=swap.example.com. The factory (builtin.go:89-97) builds https://swap.example.com via urlScheme + "://" + dsn.Host, which is exactly what TestLlamaSwapsScheme asserts. ✓
• Closure capture: llamaSwapScheme is a pure factory-of-factory; urlScheme is a string captured by value, so llamaSwapScheme("http") and llamaSwapScheme("https") produce independent factories with no aliasing risk.
• New never errors (provider/llamaswap/llamaswap.go:85-91: "Construction never fails"), so the (llm.Provider, error) return is functionally (provider, nil), matching every other scheme factory in the file — no asymmetry introduced.
• Path preservation: dsn.Host may carry /path (the DSN grammar is scheme://[token@]host[/path]); both llama-swap and llama-swaps factories pass dsn.Host through unchanged, so a hypothetical llama-swaps://tok@host/proxy would correctly yield https://host/proxy. No regression vs. the existing http variant.
• No-DSN built-in (builtin.go:98): only ProviderLlamaSwap is registered, not ProviderLlamaSwapTLS. This matches the ADR-0015 contract update ("llama-swaps is a scheme only") and the PR description.
• token="" handling: identical to the existing llama-swap factory — the openai-compat chat provider substitutes "no-key" for an empty token (see provider/llamaswap/llamaswap.go:114-118), so behavior is consistent across both schemes.

The change is a small, symmetric extension of an existing factory pattern with no logic regression. Nothing in the correctness lens is materially wrong.

🧯 Error handling & edge cases — No material issues found

VERDICT: No material issues found

I reviewed builtin.go, the new test, env.go (DSN parsing), registry.go (lazy/eager provider resolution), and provider/llamaswap/llamaswap.go (the underlying provider) to verify how the new llama-swaps factory interacts with the existing DSN and provider plumbing.

Through the error-handling & edge-cases lens, the diff is clean:

• Factory closures (builtin.go:89-97) only concatenate a known constant ("http" / "https") with dsn.Host and pass to llamaswap.New. llamaswap.New is documented (provider/llamaswap/llamaswap.go:82-91) to never fail, so the error return is always nil — consistent with the SchemeFactory contract and not a swallowed-error smell.
• Host validation is already handled upstream: ParseDSN (env.go:60-62) rejects an empty host with ErrInvalidDSN before the factory is called, so the factory never sees "".
• Trailing slash / path handling is fine: ParseDSN trims a trailing / (env.go:59), and WithBaseURL trims again (provider/llamaswap/llamaswap.go:64-66). Port is part of dsn.Host, so llama-swaps://tok@host:8443 yields https://host:8443.
• Empty token (llama-swaps://host) leaves dsn.Token == "", and WithToken("") simply means no Authorization header — same path as the existing llama-swap scheme.
• No-DSN built-in is intentionally only registered for ProviderLlamaSwap, not ProviderLlamaSwapTLS (builtin.go:98 vs :100). That's deliberate per the ADR/doc updates and avoids registering an unusable no-URL built-in.
• Lazy resolution path (registry.go:266-269) goes through providerFromDSN, which uses the same r.schemes lookup and the same factory — no separate edge case introduced.
• No new cleanup/rollback/defer paths were touched; no panics on bad input added; nothing was silently ignored.

Nothing material in my lane. (Aside: the DSN parser is case-sensitive on the scheme, so LLAMA-SWAPS://… would not match — but that is pre-existing behavior of ParseDSN, not introduced by this PR, and is outside the strict scope of error/edge handling.)

_{Automated adversarial review by Gadfly. Advisory only — does not block merge. · ⏱️ reviewed in 48s}

<!-- gadfly-review:ollama:minimax-m3:cloud --> ### 🪰 Gadfly review — `minimax-m3:cloud` (ollama-cloud) **Verdict: No material issues found** — 3 reviewers: security, correctness, error-handling <details><summary><b>🔒 Security</b> — No material issues found</summary> Now I have a complete picture. Let me reason through the security lens for this PR. **What changes from a security perspective:** - A new DSN scheme `llama-swaps` constructs `https://<host>` and passes it through `WithBaseURL`/`WithToken` to `llamaswap.New`. - The token-bearing path is unchanged: token still flows from the DSN userinfo into `Authorization: Bearer <token>` (lines 207–209 in `llamaswap.go`). - The HTTP client is still the default `&http.Client{}` (no TLS pinning, no custom `Transport`). - No new HTTP transport, no new credential handling, no new input parsing beyond what `ParseDSN` already does. **Trace of the only new tainted input:** the DSN `host` is concatenated into a URL via `urlScheme + "://" + dsn.Host`. `ParseDSN` only trims trailing `/`; it does **not** validate that the host contains no whitespace, control chars, scheme, or `?#/`. But that's pre-existing behaviour shared with every other scheme (`foreman`, `ollama`, `openai`, `anthropic`, `google`, `gemini`) — `llama-swaps` is no worse than its siblings. (Strictly out-of-scope for this PR; pre-existing for all schemes, would belong to a "input validation" cleanup PR across the whole registry.) **Other security checks:** - Authn/authz: unchanged — same bearer-token semantics, no new auth path. The DSN userinfo (`token@`) is documented as the credential. - Injection (SQL/command/path/template): no new SQL, no new commands, no new path templating. The `llama-swap` package's own path-building (`/api/models/unload`) is unchanged. - SSRF: the DSN host is caller-controlled by design (operator's `LLM_*` env var). No change in attack surface vs. the existing `llama-swap://` scheme. The factory passes the host straight to the existing HTTP client; nothing new is fetched, redirected-to, or parsed in a new way. - Unsafe deserialization: `json.NewDecoder(...).Decode(out)` on responses is unchanged; body bounded by `maxResponseBytes = 64 << 20` already. - Secret/credential leakage: token stays in `p.token`, only sent as `Authorization: Bearer ...` over the wire — no new logging, no new surface. - Insecure defaults: nothing newly insecure. The default HTTP client has no `InsecureSkipVerify` override. The `llama-swaps` factory goes *through* the standard Go `http.Client`, which uses the system trust store for TLS validation by default. - TLS handling: actually a small *improvement* — `llama-swaps` is the explicit way to use TLS with a llama-swap instance, so an operator who needs TLS now has a first-class scheme rather than being nudged at `openai://`. **Verified by reading:** - `builtin.go` lines 22–28, 77–100 — scheme registration and factory closure construction. - `env.go` lines 27–63, 104–119 — DSN parsing and `providerFromDSN` dispatch. - `provider/llamaswap/llamaswap.go` lines 49–91, 188–227 — `Provider` struct, `WithBaseURL`/`WithToken`/`New`, and `doJSON` (auth header, response cap, no insecure TLS overrides). - `registry.go` line 51 — `SchemeFactory` signature matches what `llamaSwapScheme` returns. - No occurrences of `InsecureSkipVerify` / `TLSClientConfig` / `MinVersion` anywhere in the package. **Verdict: No material issues found** in the security lens. The change is a tiny wiring addition (one factory closure parameterised by `urlScheme`, one extra entry in `r.schemes`) that reuses the same token-handling, HTTP client, JSON decoding, and `Authorization` header logic that the existing `llama-swap://` scheme already exercises. No new authentication, parsing, transport, or deserialization paths are introduced; the only behavioural delta is the URL prefix, which is the entire point of the feature. </details> <details><summary><b>🎯 Correctness</b> — No material issues found</summary> **VERDICT: No material issues found** Verified by reading `builtin.go:77-100`, `env.go:27-64`, `provider/llamaswap/llamaswap.go:82-97`, and `builtin_llamaswap_test.go:54-74`: - **DSN parsing (`env.go:46-64`)** for `llama-swaps://tok@swap.example.com` yields `Scheme=llama-swaps`, `Token=tok`, `Host=swap.example.com`. The factory (`builtin.go:89-97`) builds `https://swap.example.com` via `urlScheme + "://" + dsn.Host`, which is exactly what `TestLlamaSwapsScheme` asserts. ✓ - **Closure capture**: `llamaSwapScheme` is a pure factory-of-factory; `urlScheme` is a string captured by value, so `llamaSwapScheme("http")` and `llamaSwapScheme("https")` produce independent factories with no aliasing risk. - **`New` never errors** (`provider/llamaswap/llamaswap.go:85-91`: "Construction never fails"), so the `(llm.Provider, error)` return is functionally `(provider, nil)`, matching every other scheme factory in the file — no asymmetry introduced. - **Path preservation**: `dsn.Host` may carry `/path` (the DSN grammar is `scheme://[token@]host[/path]`); both `llama-swap` and `llama-swaps` factories pass `dsn.Host` through unchanged, so a hypothetical `llama-swaps://tok@host/proxy` would correctly yield `https://host/proxy`. No regression vs. the existing http variant. - **No-DSN built-in** (`builtin.go:98`): only `ProviderLlamaSwap` is registered, not `ProviderLlamaSwapTLS`. This matches the ADR-0015 contract update ("`llama-swaps` is a scheme only") and the PR description. - **`token=""` handling**: identical to the existing `llama-swap` factory — the openai-compat chat provider substitutes `"no-key"` for an empty token (see `provider/llamaswap/llamaswap.go:114-118`), so behavior is consistent across both schemes. The change is a small, symmetric extension of an existing factory pattern with no logic regression. Nothing in the correctness lens is materially wrong. </details> <details><summary><b>🧯 Error handling & edge cases</b> — No material issues found</summary> **VERDICT: No material issues found** I reviewed `builtin.go`, the new test, `env.go` (DSN parsing), `registry.go` (lazy/eager provider resolution), and `provider/llamaswap/llamaswap.go` (the underlying provider) to verify how the new `llama-swaps` factory interacts with the existing DSN and provider plumbing. Through the error-handling & edge-cases lens, the diff is clean: - **Factory closures** (`builtin.go:89-97`) only concatenate a known constant (`"http"` / `"https"`) with `dsn.Host` and pass to `llamaswap.New`. `llamaswap.New` is documented (provider/llamaswap/llamaswap.go:82-91) to never fail, so the `error` return is always `nil` — consistent with the `SchemeFactory` contract and not a swallowed-error smell. - **Host validation** is already handled upstream: `ParseDSN` (env.go:60-62) rejects an empty host with `ErrInvalidDSN` before the factory is called, so the factory never sees `""`. - **Trailing slash / path handling** is fine: `ParseDSN` trims a trailing `/` (env.go:59), and `WithBaseURL` trims again (provider/llamaswap/llamaswap.go:64-66). Port is part of `dsn.Host`, so `llama-swaps://tok@host:8443` yields `https://host:8443`. - **Empty token** (`llama-swaps://host`) leaves `dsn.Token == ""`, and `WithToken("")` simply means no `Authorization` header — same path as the existing `llama-swap` scheme. - **No-DSN built-in** is intentionally only registered for `ProviderLlamaSwap`, not `ProviderLlamaSwapTLS` (builtin.go:98 vs :100). That's deliberate per the ADR/doc updates and avoids registering an unusable no-URL built-in. - **Lazy resolution** path (`registry.go:266-269`) goes through `providerFromDSN`, which uses the same `r.schemes` lookup and the same factory — no separate edge case introduced. - **No new cleanup/rollback/defer paths** were touched; no panics on bad input added; nothing was silently ignored. Nothing material in my lane. (Aside: the DSN parser is case-sensitive on the scheme, so `LLAMA-SWAPS://…` would not match — but that is pre-existing behavior of `ParseDSN`, not introduced by this PR, and is outside the strict scope of error/edge handling.) </details> _{Automated adversarial review by Gadfly. Advisory only — does not block merge. · ⏱️ reviewed in 48s}

gitea-actions bot commented 2026-06-27 22:00:07 +00:00

Copy Link

Copy Source

🪰 Gadfly review — kimi-k2.7-code:cloud (ollama-cloud)

Verdict: No material issues found — 3 reviewers: security, correctness, error-handling

🔒 Security — No material issues found

No material issues found

• The change adds an explicit llama-swaps:// scheme that builds an https:// base URL, alongside the pre-existing llama-swap:// (http, local-first). This is a security-positive addition: it gives TLS-fronted instances a first-class DSN instead of pushing them onto the generic openai:// scheme where llama-swap-specific management/image endpoints were unavailable.
• Credential handling is unchanged: the DSN token flows through llamaswap.WithToken into the existing Authorization: Bearer <token> path, and only when non-empty. No new logging or serialization of the token was introduced.
• URL construction uses the same hardcoded-scheme + dsn.Host string concatenation pattern that llama-swap:// already used ("http://"+dsn.Host). llama-swaps:// simply substitutes the hardcoded "https" scheme, so no new injection surface or SSRF vector is introduced by this PR.
• No custom TLS config or InsecureSkipVerify is added; the registry’s injected or default http.Client is used, so normal certificate validation applies for https.
• Scheme resolution remains exact-match via r.schemes[dsn.Scheme]; llama-swaps cannot be confused with llama-swap at lookup time.

🎯 Correctness — No material issues found

No material issues found.

I verified the implementation against the actual code:

• builtin.go:89-100: the new llama-swaps scheme uses the same factory closure as llama-swap, only swapping "http" for "https"; the constructed base URL is urlScheme + "://" + dsn.Host. For the TLS variant this correctly yields https://host[/path].
• env.go:41: the general DSN.BaseURL() is https:// always, but the llama-swap factory deliberately bypasses it and uses dsn.Host directly with an explicit scheme, so both http and https variants are possible and explicit. That matches the PR's stated design.
• provider/llamaswap/llamaswap.go:64-97: WithBaseURL trims a trailing slash and exposes it via BaseURL(); the chat client appends /v1 internally, so the registered base URL stays at https://host as tested.
• builtin_llamaswap_test.go:56-73: the new test asserts exactly https://swap.example.com, which I confirmed corresponds to the factory behavior.
• The no-DSN built-in remains registered only under ProviderLlamaSwap (llama-swap), not under the scheme-only TLS variant, consistent with the ADR update.

The change is semantically correct and internally consistent. No logic bug or domain-correctness issue found through the Correctness lens.

🧯 Error handling & edge cases — No material issues found

No material issues found

Reviewed through the error-handling & edge-cases lens. The change is clean:

• llamaSwapScheme returns a SchemeFactory that still delegates provider-construction errors from llamaswap.New (which never fails) and the DSN lookup path in providerFromDSN wraps them normally (builtin.go:89-97, env.go:115-118).
• r.schemes[ProviderLlamaSwapTLS] is registered alongside the http twin, so LoadEnv/providerFor will surface ErrUnknownProvider for the new scheme only if a user registers it externally and then unregisters it — same as any other scheme. No registration gap.
• The no-DSN built-in llama-swap still errors on use; llama-swaps is correctly scheme-only, so there is no accidental default provider with an empty/missing URL to mishandle.
• ParseDSN rejects missing hosts and missing ://, and the new test uses a valid DSN. No nil/null or empty-collection edge cases are introduced.
• The urlScheme + "://" + dsn.Host construction is bounded by prior validation; no integer/buffer concerns.

_{Automated adversarial review by Gadfly. Advisory only — does not block merge. · ⏱️ reviewed in 1m 20s}

<!-- gadfly-review:ollama:kimi-k2.7-code:cloud --> ### 🪰 Gadfly review — `kimi-k2.7-code:cloud` (ollama-cloud) **Verdict: No material issues found** — 3 reviewers: security, correctness, error-handling <details><summary><b>🔒 Security</b> — No material issues found</summary> No material issues found - The change adds an explicit `llama-swaps://` scheme that builds an `https://` base URL, alongside the pre-existing `llama-swap://` (http, local-first). This is a security-positive addition: it gives TLS-fronted instances a first-class DSN instead of pushing them onto the generic `openai://` scheme where llama-swap-specific management/image endpoints were unavailable. - Credential handling is unchanged: the DSN token flows through `llamaswap.WithToken` into the existing `Authorization: Bearer <token>` path, and only when non-empty. No new logging or serialization of the token was introduced. - URL construction uses the same hardcoded-scheme + `dsn.Host` string concatenation pattern that `llama-swap://` already used (`"http://"+dsn.Host`). `llama-swaps://` simply substitutes the hardcoded `"https"` scheme, so no new injection surface or SSRF vector is introduced by this PR. - No custom TLS config or `InsecureSkipVerify` is added; the registry’s injected or default `http.Client` is used, so normal certificate validation applies for `https`. - Scheme resolution remains exact-match via `r.schemes[dsn.Scheme]`; `llama-swaps` cannot be confused with `llama-swap` at lookup time. </details> <details><summary><b>🎯 Correctness</b> — No material issues found</summary> No material issues found. I verified the implementation against the actual code: - `builtin.go:89-100`: the new `llama-swaps` scheme uses the same factory closure as `llama-swap`, only swapping `"http"` for `"https"`; the constructed base URL is `urlScheme + "://" + dsn.Host`. For the TLS variant this correctly yields `https://host[/path]`. - `env.go:41`: the general `DSN.BaseURL()` is `https://` always, but the llama-swap factory deliberately bypasses it and uses `dsn.Host` directly with an explicit scheme, so both http and https variants are possible and explicit. That matches the PR's stated design. - `provider/llamaswap/llamaswap.go:64-97`: `WithBaseURL` trims a trailing slash and exposes it via `BaseURL()`; the chat client appends `/v1` internally, so the registered base URL stays at `https://host` as tested. - `builtin_llamaswap_test.go:56-73`: the new test asserts exactly `https://swap.example.com`, which I confirmed corresponds to the factory behavior. - The no-DSN built-in remains registered only under `ProviderLlamaSwap` (`llama-swap`), not under the scheme-only TLS variant, consistent with the ADR update. The change is semantically correct and internally consistent. No logic bug or domain-correctness issue found through the Correctness lens. </details> <details><summary><b>🧯 Error handling & edge cases</b> — No material issues found</summary> **No material issues found** Reviewed through the error-handling & edge-cases lens. The change is clean: - `llamaSwapScheme` returns a `SchemeFactory` that still delegates provider-construction errors from `llamaswap.New` (which never fails) and the DSN lookup path in `providerFromDSN` wraps them normally (`builtin.go:89-97`, `env.go:115-118`). - `r.schemes[ProviderLlamaSwapTLS]` is registered alongside the http twin, so `LoadEnv`/`providerFor` will surface `ErrUnknownProvider` for the new scheme only if a user registers it externally and then unregisters it — same as any other scheme. No registration gap. - The no-DSN built-in `llama-swap` still errors on use; `llama-swaps` is correctly scheme-only, so there is no accidental default provider with an empty/missing URL to mishandle. - `ParseDSN` rejects missing hosts and missing `://`, and the new test uses a valid DSN. No nil/null or empty-collection edge cases are introduced. - The `urlScheme + "://" + dsn.Host` construction is bounded by prior validation; no integer/buffer concerns. </details> _{Automated adversarial review by Gadfly. Advisory only — does not block merge. · ⏱️ reviewed in 1m 20s}

gitea-actions bot commented 2026-06-27 22:00:08 +00:00

Copy Link

Copy Source

🪰 Gadfly review — deepseek-v4-pro:cloud (ollama-cloud)

Verdict: No material issues found — 3 reviewers: security, correctness, error-handling

🔒 Security — No material issues found

No material issues found

The change is a straightforward addition of an HTTPS variant (llama-swaps://) alongside the existing HTTP scheme (llama-swap://), sharing the same factory, token handling, and URL construction. From a security lens:

• Token in transit: The llama-swaps scheme sends the bearer token over TLS (encrypted), which is a net improvement over the existing llama-swap HTTP scheme where the token travels in cleartext. The default http.Client uses Go's standard TLS certificate verification — no insecure skip-verify defaults are introduced.

• No new injection surface: The URL construction (urlScheme+"://"+dsn.Host at builtin.go:93) is identical to the pre-existing llama-swap pattern. The dsn.Host value comes from the same ParseDSN parser with the same (lack of) host sanitization — any host-injection concerns (e.g., embedding # to redirect the effective host) are pre-existing and not worsened by this PR. The DSN source is environment variables, which in practice requires process-level access.

• No built-in provider for the TLS scheme: ProviderLlamaSwapTLS is registered only as a scheme factory (r.schemes), not as a default provider (r.providers). This means llama-swaps cannot be used bare (without a DSN), eliminating any risk of a no-URL default silently hitting a wrong host over TLS.

• Token handling unchanged: The token flows through llamaswap.WithToken(dsn.Token) → p.token → Authorization: Bearer header, exactly as in the existing scheme. No logging, no leakage, no new code paths for the credential.

🎯 Correctness — No material issues found

No material issues found

The change is straightforward and correct:

• llamaSwapScheme("http") and llamaSwapScheme("https") each capture the right scheme string in a closure, and the factory correctly builds urlScheme+"://"+dsn.Host. Verified by reading builtin.go:89-100 and env.go:46-63 — dsn.Host is the raw host[:port][/path] with no scheme prefix, so prepending http:// or https:// is exactly right.

• The llama-swaps scheme is intentionally scheme-only (no r.providers[ProviderLlamaSwapTLS] entry), matching the ADR and PR description. Verified by reading builtin.go:98-100 — only r.schemes[ProviderLlamaSwapTLS] is set, not r.providers.

• The test TestLlamaSwapsScheme correctly asserts lp.BaseURL() == "https://swap.example.com", confirming the https construction. Verified by reading builtin_llamaswap_test.go:56-73.

• No path-duplication regression: the existing llama-swap scheme already had the same scheme+"://"+dsn.Host pattern (previously hardcoded "http://"+dsn.Host), so the refactor to a parameterized closure is behavior-preserving for the http case. Verified by comparing the old inline factory (removed in the diff) with the new llamaSwapScheme("http") — both produce "http://"+dsn.Host.

🧯 Error handling & edge cases — No material issues found

Verdict: No material issues found

The change is straightforward and follows existing patterns faithfully. After verifying the full code paths:

• ParseDSN already rejects empty hosts (env.go:60-62), so dsn.Host is guaranteed non-empty before the factory runs. No nil/empty injection into the URL builder.
• The factory always returns nil error, matching llamaswap.New's contract ("Construction never fails" — llamaswap.go:85-91). This is consistent with the existing llama-swap scheme factory and all other built-in factories.
• llama-swaps is scheme-only (no r.providers[ProviderLlamaSwapTLS] entry). Using it as a bare provider name in a spec falls through to lazy LLM_LLAMA_SWAPS env resolution, then to a clear ErrUnknownProvider — the same path every other unregistered name takes. No panic, no silent wrong behavior.
• Nil httpClient is handled by the llamaSwapOpts closure (guarded if httpClient != nil) and again by llamaswap.WithHTTPClient (guarded if c != nil), with llamaswap.New providing a default &http.Client{}. Same pattern as all other providers in registerBuiltins.
• Empty token flows through WithToken("") → p.token = "" → chatProvider() substitutes "no-key" and doJSON omits the Authorization header. This is pre-existing behavior, unchanged by the new scheme.
• Port and path in dsn.Host compose correctly: https://host:port and https://host/path both work, and the downstream chatProvider() and doJSON append /v1 and management paths on top without double-slash issues (base URL is TrimRight("/")'d by WithBaseURL).

No swallowed errors, missing cleanup, panic-on-bad-input, or boundary condition issues introduced by this diff.

_{Automated adversarial review by Gadfly. Advisory only — does not block merge. · ⏱️ reviewed in 1m 34s}

<!-- gadfly-review:ollama:deepseek-v4-pro:cloud --> ### 🪰 Gadfly review — `deepseek-v4-pro:cloud` (ollama-cloud) **Verdict: No material issues found** — 3 reviewers: security, correctness, error-handling <details><summary><b>🔒 Security</b> — No material issues found</summary> **No material issues found** The change is a straightforward addition of an HTTPS variant (`llama-swaps://`) alongside the existing HTTP scheme (`llama-swap://`), sharing the same factory, token handling, and URL construction. From a security lens: - **Token in transit**: The `llama-swaps` scheme sends the bearer token over TLS (encrypted), which is a net improvement over the existing `llama-swap` HTTP scheme where the token travels in cleartext. The default `http.Client` uses Go's standard TLS certificate verification — no insecure skip-verify defaults are introduced. - **No new injection surface**: The URL construction (`urlScheme+"://"+dsn.Host` at `builtin.go:93`) is identical to the pre-existing `llama-swap` pattern. The `dsn.Host` value comes from the same `ParseDSN` parser with the same (lack of) host sanitization — any host-injection concerns (e.g., embedding `#` to redirect the effective host) are pre-existing and not worsened by this PR. The DSN source is environment variables, which in practice requires process-level access. - **No built-in provider for the TLS scheme**: `ProviderLlamaSwapTLS` is registered only as a scheme factory (`r.schemes`), not as a default provider (`r.providers`). This means `llama-swaps` cannot be used bare (without a DSN), eliminating any risk of a no-URL default silently hitting a wrong host over TLS. - **Token handling unchanged**: The token flows through `llamaswap.WithToken(dsn.Token)` → `p.token` → `Authorization: Bearer` header, exactly as in the existing scheme. No logging, no leakage, no new code paths for the credential. </details> <details><summary><b>🎯 Correctness</b> — No material issues found</summary> **No material issues found** The change is straightforward and correct: - `llamaSwapScheme("http")` and `llamaSwapScheme("https")` each capture the right scheme string in a closure, and the factory correctly builds `urlScheme+"://"+dsn.Host`. Verified by reading `builtin.go:89-100` and `env.go:46-63` — `dsn.Host` is the raw `host[:port][/path]` with no scheme prefix, so prepending `http://` or `https://` is exactly right. - The `llama-swaps` scheme is intentionally scheme-only (no `r.providers[ProviderLlamaSwapTLS]` entry), matching the ADR and PR description. Verified by reading `builtin.go:98-100` — only `r.schemes[ProviderLlamaSwapTLS]` is set, not `r.providers`. - The test `TestLlamaSwapsScheme` correctly asserts `lp.BaseURL() == "https://swap.example.com"`, confirming the https construction. Verified by reading `builtin_llamaswap_test.go:56-73`. - No path-duplication regression: the existing `llama-swap` scheme already had the same `scheme+"://"+dsn.Host` pattern (previously hardcoded `"http://"+dsn.Host`), so the refactor to a parameterized closure is behavior-preserving for the http case. Verified by comparing the old inline factory (removed in the diff) with the new `llamaSwapScheme("http")` — both produce `"http://"+dsn.Host`. </details> <details><summary><b>🧯 Error handling & edge cases</b> — No material issues found</summary> ## Verdict: No material issues found The change is straightforward and follows existing patterns faithfully. After verifying the full code paths: - **`ParseDSN` already rejects empty hosts** (`env.go:60-62`), so `dsn.Host` is guaranteed non-empty before the factory runs. No nil/empty injection into the URL builder. - **The factory always returns `nil` error**, matching `llamaswap.New`'s contract ("Construction never fails" — `llamaswap.go:85-91`). This is consistent with the existing `llama-swap` scheme factory and all other built-in factories. - **`llama-swaps` is scheme-only** (no `r.providers[ProviderLlamaSwapTLS]` entry). Using it as a bare provider name in a spec falls through to lazy `LLM_LLAMA_SWAPS` env resolution, then to a clear `ErrUnknownProvider` — the same path every other unregistered name takes. No panic, no silent wrong behavior. - **Nil `httpClient`** is handled by the `llamaSwapOpts` closure (guarded `if httpClient != nil`) and again by `llamaswap.WithHTTPClient` (guarded `if c != nil`), with `llamaswap.New` providing a default `&http.Client{}`. Same pattern as all other providers in `registerBuiltins`. - **Empty token** flows through `WithToken("")` → `p.token = ""` → `chatProvider()` substitutes `"no-key"` and `doJSON` omits the `Authorization` header. This is pre-existing behavior, unchanged by the new scheme. - **Port and path in `dsn.Host`** compose correctly: `https://host:port` and `https://host/path` both work, and the downstream `chatProvider()` and `doJSON` append `/v1` and management paths on top without double-slash issues (base URL is `TrimRight("/")`'d by `WithBaseURL`). No swallowed errors, missing cleanup, panic-on-bad-input, or boundary condition issues introduced by this diff. </details> _{Automated adversarial review by Gadfly. Advisory only — does not block merge. · ⏱️ reviewed in 1m 34s}

gitea-actions bot commented 2026-06-27 22:00:23 +00:00

Copy Link

Copy Source

🪰 Gadfly review — nemotron-3-super:cloud (ollama-cloud)

Verdict: No material issues found — 3 reviewers: security, correctness, error-handling

🔒 Security — No material issues found

No material issues found

🎯 Correctness — No material issues found

No material issues found.

🧯 Error handling & edge cases — No material issues found

No material issues found

The changes introduce a new DSN scheme llama-swaps for TLS-fronted llama-swap instances, mirroring the existing llama-swap scheme but using https instead of http. After verifying the changes:

1. Error handling is consistent with the existing llama-swap scheme:

   • The new scheme uses the same factory function pattern, reusing existing validation in ParseDSN (which returns errors for missing host or invalid format)
   • Token handling remains unchanged (empty token allowed for no auth)
   • Host parsing preserves ports and trims trailing slashes identically
   • No new error cases introduced; edge cases handled by existing parsing logic

2. Edge cases verified:

   • Empty token: handled by existing llamaswap.WithToken("")
   • Missing host: caught by ParseDSN returning error
   • Invalid scheme: caught by scheme lookup in providerFromDSN
   • IPv6 addresses: existing limitation unchanged (not introduced by this change)
   • Path in DSN: ignored for base URL (consistent with existing behavior)
   • Case sensitivity: schemes must match registered lowercase names (existing behavior)

3. Testing:

   • New test TestLlamaSwapsScheme verifies https base URL construction
   • Existing TestLlamaSwapScheme unchanged and passes
   • No new error paths introduced that require additional error handling tests

The implementation correctly mirrors the redis/rediss scheme pattern with explicit http/https selection via scheme name, maintaining consistency with existing error handling approaches. No material issues found in error handling or edge cases for this lens.

_{Automated adversarial review by Gadfly. Advisory only — does not block merge. · ⏱️ reviewed in 5m 23s}

<!-- gadfly-review:ollama:nemotron-3-super:cloud --> ### 🪰 Gadfly review — `nemotron-3-super:cloud` (ollama-cloud) **Verdict: No material issues found** — 3 reviewers: security, correctness, error-handling <details><summary><b>🔒 Security</b> — No material issues found</summary> No material issues found </details> <details><summary><b>🎯 Correctness</b> — No material issues found</summary> No material issues found. </details> <details><summary><b>🧯 Error handling & edge cases</b> — No material issues found</summary> No material issues found The changes introduce a new DSN scheme `llama-swaps` for TLS-fronted llama-swap instances, mirroring the existing `llama-swap` scheme but using https instead of http. After verifying the changes: 1. Error handling is consistent with the existing `llama-swap` scheme: - The new scheme uses the same factory function pattern, reusing existing validation in `ParseDSN` (which returns errors for missing host or invalid format) - Token handling remains unchanged (empty token allowed for no auth) - Host parsing preserves ports and trims trailing slashes identically - No new error cases introduced; edge cases handled by existing parsing logic 2. Edge cases verified: - Empty token: handled by existing `llamaswap.WithToken("")` - Missing host: caught by `ParseDSN` returning error - Invalid scheme: caught by scheme lookup in `providerFromDSN` - IPv6 addresses: existing limitation unchanged (not introduced by this change) - Path in DSN: ignored for base URL (consistent with existing behavior) - Case sensitivity: schemes must match registered lowercase names (existing behavior) 3. Testing: - New test `TestLlamaSwapsScheme` verifies https base URL construction - Existing `TestLlamaSwapScheme` unchanged and passes - No new error paths introduced that require additional error handling tests The implementation correctly mirrors the redis/rediss scheme pattern with explicit http/https selection via scheme name, maintaining consistency with existing error handling approaches. No material issues found in error handling or edge cases for this lens. </details> _{Automated adversarial review by Gadfly. Advisory only — does not block merge. · ⏱️ reviewed in 5m 23s}

gitea-actions bot commented 2026-06-27 22:01:28 +00:00

Copy Link

Copy Source

🪰 Gadfly review — gpt-oss:120b-cloud (ollama-cloud)

Verdict: Minor issues — 3 reviewers: security, correctness, error-handling

🔒 Security — No material issues found

Verdict: No material issues found

• The new llama-swaps:// scheme only affects how the base URL is constructed (https:// instead of http://).
• All input parsing remains unchanged and uses the existing ParseDSN implementation, which validates scheme, token, and host, and strips trailing slashes.
• The scheme factories (llamaSwapScheme) receive the DSN struct and set the base URL using the supplied scheme string; no untrusted data is interpolated into command lines, file paths, or SQL statements.
• No secrets, tokens, or host values are logged or exposed; they are passed directly to the llamaswap provider, which already handles bearer‑token authentication over TLS (or HTTP for local‑first).
• No changes introduce SSRF or insecure defaults: TLS is enforced for the new scheme, and the original llama-swap scheme remains explicitly HTTP‑only for local use.

Thus, from a security perspective the modifications do not create authentication/authorization gaps, injection vectors, credential leakage, or insecure defaults.

🎯 Correctness — No material issues found

Verdict: No material issues found

🧯 Error handling & edge cases — Minor issues

Verdict: Minor issues

• builtin.go:93 – Missing validation for empty token – the scheme factories for both llama-swap and llama-swaps forward dsn.Token directly to llamaswap.WithToken without checking whether the token string is empty. An empty token could lead to unauthenticated requests and subsequent API errors at runtime.

_{Automated adversarial review by Gadfly. Advisory only — does not block merge. · ⏱️ reviewed in 1m 29s}

<!-- gadfly-review:ollama:gpt-oss:120b-cloud --> ### 🪰 Gadfly review — `gpt-oss:120b-cloud` (ollama-cloud) **Verdict: Minor issues** — 3 reviewers: security, correctness, error-handling <details><summary><b>🔒 Security</b> — No material issues found</summary> **Verdict: No material issues found** - The new `llama-swaps://` scheme only affects how the base URL is constructed (`https://` instead of `http://`). - All input parsing remains unchanged and uses the existing `ParseDSN` implementation, which validates scheme, token, and host, and strips trailing slashes. - The scheme factories (`llamaSwapScheme`) receive the DSN struct and set the base URL using the supplied scheme string; no untrusted data is interpolated into command lines, file paths, or SQL statements. - No secrets, tokens, or host values are logged or exposed; they are passed directly to the `llamaswap` provider, which already handles bearer‑token authentication over TLS (or HTTP for local‑first). - No changes introduce SSRF or insecure defaults: TLS is enforced for the new scheme, and the original `llama-swap` scheme remains explicitly HTTP‑only for local use. Thus, from a security perspective the modifications do not create authentication/authorization gaps, injection vectors, credential leakage, or insecure defaults. </details> <details><summary><b>🎯 Correctness</b> — No material issues found</summary> **Verdict:** No material issues found </details> <details><summary><b>🧯 Error handling & edge cases</b> — Minor issues</summary> **Verdict:** Minor issues - **builtin.go:93 – Missing validation for empty token** – the scheme factories for both `llama-swap` and `llama-swaps` forward `dsn.Token` directly to `llamaswap.WithToken` without checking whether the token string is empty. An empty token could lead to unauthenticated requests and subsequent API errors at runtime. </details> _{Automated adversarial review by Gadfly. Advisory only — does not block merge. · ⏱️ reviewed in 1m 29s}

gitea-actions bot commented 2026-06-27 22:01:43 +00:00

Copy Link

Copy Source

🪰 Gadfly review — qwen3-coder:480b-cloud (ollama-cloud)

Verdict: No material issues found — 3 reviewers: security, correctness, error-handling

🔒 Security — No material issues found

No material issues found.

The changes introduce a new llama-swaps DSN scheme for TLS-fronted llama-swap instances, correctly building an https:// base URL. The implementation is secure and follows the intended design:

1. The new scheme properly uses HTTPS for TLS-fronted instances, addressing the security concern of ensuring encrypted communication.
2. The separation of llama-swap (HTTP) and llama-swaps (HTTPS) schemes makes the security model explicit and prevents accidental insecure connections.
3. Tests verify that the llama-swaps scheme correctly builds an HTTPS base URL.
4. Documentation clearly explains the security implications and proper usage of both schemes.

No security vulnerabilities or issues were identified in the implementation.

🎯 Correctness — No material issues found

No material issues found.

🧯 Error handling & edge cases — No material issues found

No material issues found.

The changes introduce a new DSN scheme llama-swaps for TLS-fronted llama-swap instances, which correctly builds an https:// base URL. The implementation:

1. Defines ProviderLlamaSwapTLS = "llama-swaps" constant
2. Refactors the llama-swap scheme factory to accept a URL scheme parameter
3. Registers both llama-swap (http) and llama-swaps (https) schemes
4. Includes a test that verifies the https base URL is correctly built
5. Updates documentation consistently

All error handling paths appear correct:

• The scheme factory returns (llm.Provider, error) but always returns nil for error - this is consistent with other scheme factories in the file
• The test properly checks for registration and type assertion
• No new error cases are introduced that need handling

The change is well-contained and properly tested. No issues in error handling or edge cases found.

_{Automated adversarial review by Gadfly. Advisory only — does not block merge. · ⏱️ reviewed in 52s}

<!-- gadfly-review:ollama:qwen3-coder:480b-cloud --> ### 🪰 Gadfly review — `qwen3-coder:480b-cloud` (ollama-cloud) **Verdict: No material issues found** — 3 reviewers: security, correctness, error-handling <details><summary><b>🔒 Security</b> — No material issues found</summary> No material issues found. The changes introduce a new `llama-swaps` DSN scheme for TLS-fronted llama-swap instances, correctly building an `https://` base URL. The implementation is secure and follows the intended design: 1. The new scheme properly uses HTTPS for TLS-fronted instances, addressing the security concern of ensuring encrypted communication. 2. The separation of `llama-swap` (HTTP) and `llama-swaps` (HTTPS) schemes makes the security model explicit and prevents accidental insecure connections. 3. Tests verify that the `llama-swaps` scheme correctly builds an HTTPS base URL. 4. Documentation clearly explains the security implications and proper usage of both schemes. No security vulnerabilities or issues were identified in the implementation. </details> <details><summary><b>🎯 Correctness</b> — No material issues found</summary> No material issues found. </details> <details><summary><b>🧯 Error handling & edge cases</b> — No material issues found</summary> No material issues found. The changes introduce a new DSN scheme `llama-swaps` for TLS-fronted llama-swap instances, which correctly builds an `https://` base URL. The implementation: 1. Defines `ProviderLlamaSwapTLS = "llama-swaps"` constant 2. Refactors the llama-swap scheme factory to accept a URL scheme parameter 3. Registers both `llama-swap` (http) and `llama-swaps` (https) schemes 4. Includes a test that verifies the https base URL is correctly built 5. Updates documentation consistently All error handling paths appear correct: - The scheme factory returns `(llm.Provider, error)` but always returns `nil` for error - this is consistent with other scheme factories in the file - The test properly checks for registration and type assertion - No new error cases are introduced that need handling The change is well-contained and properly tested. No issues in error handling or edge cases found. </details> _{Automated adversarial review by Gadfly. Advisory only — does not block merge. · ⏱️ reviewed in 52s}

gitea-actions bot commented 2026-06-27 22:02:36 +00:00

Copy Link

Copy Source

🪰 Gadfly review — gemma4:cloud (ollama-cloud)

Verdict: No material issues found — 3 reviewers: security, correctness, error-handling

🔒 Security — No material issues found

Verdict: No material issues found

The change introduces a new DSN scheme llama-swaps to support HTTPS (TLS) for llama-swap providers, mirroring the redis/rediss convention. I have verified the following:

• Input Validation: The llamaSwapScheme factory uses the DSN struct parsed by ParseDSN. The DSN.Host is used directly in the URL construction (urlScheme + "://" + dsn.Host). Since this is used for internal provider configuration via environment variables, it is not exposed to untrusted user input in a way that would lead to injection.
• Transport Security: The intent is specifically to move TLS-fronted instances from the generic openai:// scheme to a concrete llama-swaps:// scheme to regain provider-specific management methods. The implementation correctly maps llama-swaps to https:// and llama-swap to http://.
• Secret Handling: The dsn.Token is passed to llamaswap.WithToken, which is the standard way tokens are handled across other providers in this codebase (e.g., openai, anthropic).
• Correctness: I verified builtin.go and registry.go to ensure the scheme factory is correctly registered and invoked. The logic is consistent with how other schemes (like ollama or openai) are implemented.

No security gaps, injection vectors, or credential leakages were identified.

🎯 Correctness — No material issues found

Verdict: No material issues found

🧯 Error handling & edge cases — No material issues found

No material issues found.

I reviewed the change through the lens of error handling and edge cases. The implementation of the llama-swaps scheme is a straightforward extension of the existing llama-swap logic.

• Input Handling: The use of urlScheme + "://" + dsn.Host for base URL construction is consistent with the previous llama-swap implementation. Since the DSN struct is populated by the registry's parser, dsn.Host is guaranteed to be present if the scheme is recognized.
• Error Paths: The SchemeFactory returns (llm.Provider, error). In this case, llamaswap.New is called, which returns a provider without an error (consistent with other built-in factories in the same file like openai or anthropic).
• Edge Cases:
  • Empty token: Handled by llamaswap.WithToken(dsn.Token).
  • Missing host: The registry's DSN parser ensures that for a URI-based scheme, the host is extracted.
  • TLS/HTTPS: By explicitly using https for llama-swaps, the provider will correctly use the HTTPS protocol.

Verified by reading builtin.go and reviewing the provided test TestLlamaSwapsScheme in builtin_llamaswap_test.go.

_{Automated adversarial review by Gadfly. Advisory only — does not block merge. · ⏱️ reviewed in 30s}

<!-- gadfly-review:ollama:gemma4:cloud --> ### 🪰 Gadfly review — `gemma4:cloud` (ollama-cloud) **Verdict: No material issues found** — 3 reviewers: security, correctness, error-handling <details><summary><b>🔒 Security</b> — No material issues found</summary> Verdict: No material issues found The change introduces a new DSN scheme `llama-swaps` to support HTTPS (TLS) for `llama-swap` providers, mirroring the `redis`/`rediss` convention. I have verified the following: - **Input Validation**: The `llamaSwapScheme` factory uses the `DSN` struct parsed by `ParseDSN`. The `DSN.Host` is used directly in the URL construction (`urlScheme + "://" + dsn.Host`). Since this is used for internal provider configuration via environment variables, it is not exposed to untrusted user input in a way that would lead to injection. - **Transport Security**: The intent is specifically to move TLS-fronted instances from the generic `openai://` scheme to a concrete `llama-swaps://` scheme to regain provider-specific management methods. The implementation correctly maps `llama-swaps` to `https://` and `llama-swap` to `http://`. - **Secret Handling**: The `dsn.Token` is passed to `llamaswap.WithToken`, which is the standard way tokens are handled across other providers in this codebase (e.g., `openai`, `anthropic`). - **Correctness**: I verified `builtin.go` and `registry.go` to ensure the scheme factory is correctly registered and invoked. The logic is consistent with how other schemes (like `ollama` or `openai`) are implemented. No security gaps, injection vectors, or credential leakages were identified. </details> <details><summary><b>🎯 Correctness</b> — No material issues found</summary> Verdict: No material issues found </details> <details><summary><b>🧯 Error handling & edge cases</b> — No material issues found</summary> No material issues found. I reviewed the change through the lens of error handling and edge cases. The implementation of the `llama-swaps` scheme is a straightforward extension of the existing `llama-swap` logic. - **Input Handling**: The use of `urlScheme + "://" + dsn.Host` for base URL construction is consistent with the previous `llama-swap` implementation. Since the `DSN` struct is populated by the registry's parser, `dsn.Host` is guaranteed to be present if the scheme is recognized. - **Error Paths**: The `SchemeFactory` returns `(llm.Provider, error)`. In this case, `llamaswap.New` is called, which returns a provider without an error (consistent with other built-in factories in the same file like `openai` or `anthropic`). - **Edge Cases**: - Empty token: Handled by `llamaswap.WithToken(dsn.Token)`. - Missing host: The registry's DSN parser ensures that for a URI-based scheme, the host is extracted. - TLS/HTTPS: By explicitly using `https` for `llama-swaps`, the provider will correctly use the HTTPS protocol. Verified by reading `builtin.go` and reviewing the provided test `TestLlamaSwapsScheme` in `builtin_llamaswap_test.go`. </details> _{Automated adversarial review by Gadfly. Advisory only — does not block merge. · ⏱️ reviewed in 30s}

steve merged commit aa25b2c334 into main 2026-06-27 22:57:00 +00:00

Sign in to join this conversation.

Reviewers

No Reviewers

Labels

No items

No Label

Milestone

No items

No Milestone

Projects

Clear projects

No project

Assignees

Clear assignees

steve

No Assignees

2 Participants

Notifications

Subscribe

Due Date

No due date set.

Dependencies

No dependencies set.

Reference: steve/majordomo#4