chore: code review fixes + README reorganization#57
Merged
Conversation
The LMStudio, SGLang, VLLM, Custom, and LlamaCpp providers used to raise
ArgumentError when base_url validation failed or required options were
missing. That broke the documented `chat/2 :: {:ok, _} | {:error, _}`
contract and forced callers to rescue exceptions instead of pattern
matching.
Normalize the error path: bad URLs and missing config now flow back as
`{:error, {:invalid_config, reason}}` (or `%Nous.Errors.ProviderError{}`
for the llamacpp missing-model case). The high-level `Nous.run/2`,
`Nous.generate_text/3`, and `Nous.Agent.run/3` paths are unaffected
since they already returned result tuples.
Breaking change documented in CHANGELOG.
Every provider was rebuilding the same content-type header and the same "add Bearer auth if api_key is non-empty" conditional inline. The HTTP module already exposed `bearer_auth_header/1` and `api_key_header/2` helpers (and handles the nil/empty/"not-needed" cases correctly), but nobody was using them. Centralize the duplication on three new HTTP helpers — `json_headers/0`, `organization_header/1`, `openai_project_header/1` — and rewire every provider's `build_headers` to compose them. The OpenAI / OpenAICompatible / Custom org+project chain becomes a flat list concatenation; the four local providers (LMStudio, SGLang, vLLM, Mistral) collapse to one line. Pure refactor — no header values changed. Verified against the existing provider Bypass tests (LMStudio/SGLang/vLLM/HTTP) plus new unit coverage for the three new helpers.
…args
Two unrelated reliability fixes that share a theme: prefer explicit
errors over silent fallthrough.
**Vertex AI: prefer Goth over the env-var fallback.**
`resolve_token/1` used to call the macro-injected `api_key/1` (which
walks opts → env var → app config) BEFORE checking for a configured
Goth instance. If a user had Goth set up but also a stale
`VERTEX_AI_ACCESS_TOKEN` env var lying around, a Goth misconfig would
silently fall through to the env var and produce confusing 401s. Now,
when `:goth` is named, we use it exclusively for that request — Goth
failures surface as `{:error, %{reason: :goth_error, ...}}`. The env
var / app config remains as the last-resort fallback for non-Goth users.
**StringTools: type-guard arg extraction.**
`replace_text`, `split_text`, `count_occurrences`, and `contains`
chained `Map.get(args, "k1") || Map.get(args, "k2") || ""` to support
aliased argument names. When the LLM handed back a non-string value
(e.g. `"pattern" => 123`), it flowed straight into `String.replace/3`
and crashed the tool call with `FunctionClauseError`. Extracted to a
single `fetch_arg/3` helper that falls back to the default when the
value isn't a binary, eliminating the nil-pun chains and the crash.
Documented in CHANGELOG.
The security test for `Nous.PromptTemplate.from_template/1` rejection
asserted that a hostile `<%= File.write!("/tmp/nous_pwn_test", ...) %>`
template never produced the side-effect file. The hardcoded `/tmp/`
path was async-unsafe — and worse, would have left a sentinel under
`/tmp/` on any future test machine if the rejection ever regressed.
Switch to ExUnit's `@tag :tmp_dir`, which gives a per-test unique
directory that's cleaned automatically. The test stays parallel-safe
and the canary path is now scoped to the test's own temp dir.
Note: provider @SPEC coverage was inventoried as part of this PR.
Public callbacks (`chat/2`, `chat_stream/2`, `request/3`,
`request_stream/3`, `count_tokens/1`, etc.) are already typed by the
`@callback` declarations in `Nous.Provider`, and the macro-injected
helpers (`api_key/1`, `base_url/1`, `count_tokens/1`) carry @SPEC in
the macro body. `mix dialyzer` runs clean with 0 errors and
`mix credo --strict` reports no issues. No spec changes needed.
Cut README from 1454 to 791 lines by extracting Vertex AI setup and
HTTP backend internals to dedicated guides, splitting contributor docs
into CONTRIBUTING.md, and reordering for a new-user flow: positioning
block, AI-agent callout, tool-calling Quick Start, features overview
linking to deep dives, then condensed deep dives.
- Add docs/guides/vertex_ai_setup.md (verbatim extract, with back-link)
- Add docs/guides/http_backends.md (verbatim extract, with back-link)
- Add CONTRIBUTING.md (moves dev/test/lint sections out of README; fixes
stale "OTP 26+/Elixir 1.15+" prereqs to OTP 27+/Elixir 1.18+; adds
Security subsection pointing at AGENTS.md critical-rules)
- Repurpose docs/getting-started.md: fix {:nous, "~> 0.9.0"} to ~> 0.16.0,
drop broken examples/tutorials and examples/quickstart links, strip
duplicate Install/Quick Setup, add multi-tool agent / error handling /
persistence / observability sections, keep error-handling and
GenServer conversation patterns
- Fix docs/README.md link typos: tool-development.md, best-practices.md,
migration.md to use underscores; also fix broken examples/tutorials and
examples/quickstart references
- Preserve H2 anchors: Quick Start, Supported Providers, Features,
Examples, Architecture, Contributing, License (the [Status](#features)
badge still resolves)
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Five stacked commits from an extensive code review and README reorganization:
93aa7e7provider error contracts (breaking). LMStudio, SGLang, VLLM, Custom, and LlamaCpp providers now return{:error, {:invalid_config, reason}}(or%Nous.Errors.ProviderError{}for LlamaCpp's missing-model case) instead of raisingArgumentError. Restores the documentedchat/2 :: {:ok, _} | {:error, _}contract. High-levelNous.run/2,Nous.generate_text/3, andNous.Agent.run/3are unaffected — they already returned result tuples.4dc0b3bshared HTTP header helpers. Extractedjson_headers/0,organization_header/1,openai_project_header/1into
Nous.Providers.HTTPand rewired all 11 providers. The four local providers (LMStudio, SGLang, vLLM, Mistral) collapse to asingle-line
build_headers. Pure refactor — no header values changed.0029d88Vertex Goth precedence + StringTools type guards.Nous.Providers.VertexAI.resolve_token/1now uses Gothexclusively when configured, surfacing Goth failures as
{:error, %{reason: :goth_error, ...}}instead of silently fallingthrough to a stale
VERTEX_AI_ACCESS_TOKEN.Nous.Tools.StringToolsextracts args through a typed helper, eliminating nil-punchains that crashed on non-string LLM input.
1a4cfd6EEx-injection canary uses@tag :tmp_dir. Replaces the hardcoded/tmp/nous_pwn_testpath with anasync-safe per-test temp dir. Spec coverage was inventoried as part of this PR; provider callbacks are already typed via
Nous.Providerbehaviour, dialyzer runs clean.105486dREADME + docs reorganization. README dropped from 1454 → 791 lines. Extracted Vertex AI setup (140 lines)to
docs/guides/vertex_ai_setup.md, HTTP backends (65 lines) todocs/guides/http_backends.md, contributor docs (140 lines) tonew
CONTRIBUTING.md. Repurposeddocs/getting-started.md(was bit-rotted with~> 0.9.0and broken example links). Addedpositioning section, AGENTS.md callout, troubleshooting link.
Breaking changes are documented in
CHANGELOG.mdunder[Unreleased].Test plan
mix compile --warnings-as-errorscleanmix test— 1734 tests, 0 failures (101 excluded)mix dialyzer— 0 errorsmix credo --strict— 0 issuescustom_test.exs, invalid-URL assertions in lmstudio/sglang/vllm tests,string_tools_test.exs)key before merging — PR 2 is the highest header-regression risk)
docs/guides/*.mdandexamples/*.exslink