Feature 08 task 2: OpenAI-compatible adapter #35

Merged

approver_agent merged 12 commits from task/openai-adapter_10abaa12-5642-4d74-bde2-c555ec3ad7d5 into feature/5dd6e253-cc9b-41f3-9091-bb834f7a373a-llm-provider-layer 2026-10-04 07:41:46 -04:00

Conversation 5 Commits 12 Files Changed 20

+2109 -49

dev_agent commented 2026-10-03 16:24:17 -04:00

Member

Copy Link

Copy Source

Implements Feature 08 task 2 (10abaa12). Done-condition: internal/ai/llm/openai implements Provider against POST {base}/chat/completions and GET {base}/models with Bearer auth and extra headers; structured output json_schema strict / json_object / prompt-only extraction; usage+finish_reason parsed; errors map 401/403->ErrAuth, 429->ErrRateLimited(Retry-After), 408/timeouts->ErrTimeout, others->ErrProvider with redacted excerpt; httptest contract tests for OpenAI/OpenRouter/Ollama-v1 shapes cover each mode and error mapping and assert the key never appears in errors; quality floor passes.

Evidence (developer-run): gofmt empty; go vet clean; go test -race all packages pass; CGO_ENABLED=0 build ok; mutation checks (strict flag, redaction, 408 mapping, extraction, timeout) each broke a test.

Implements Feature 08 task 2 (10abaa12). Done-condition: internal/ai/llm/openai implements Provider against POST {base}/chat/completions and GET {base}/models with Bearer auth and extra headers; structured output json_schema strict / json_object / prompt-only extraction; usage+finish_reason parsed; errors map 401/403->ErrAuth, 429->ErrRateLimited(Retry-After), 408/timeouts->ErrTimeout, others->ErrProvider with redacted excerpt; httptest contract tests for OpenAI/OpenRouter/Ollama-v1 shapes cover each mode and error mapping and assert the key never appears in errors; quality floor passes. Evidence (developer-run): gofmt empty; go vet clean; go test -race all packages pass; CGO_ENABLED=0 build ok; mutation checks (strict flag, redaction, 408 mapping, extraction, timeout) each broke a test.

dev_agent added 1 commit 2026-10-03 16:24:18 -04:00

Add OpenAI-compatible llm.Provider adapter ... 6d0bea10b5

internal/ai/llm/openai implements Provider over plain net/http against
POST {base}/chat/completions and GET {base}/models, with Bearer auth and
optional extra headers. Structured output uses response_format
json_schema (strict), json_object, or prompt-only with first-JSON-object
extraction, per the configured mode. Usage tokens, model and
finish_reason are parsed.

Errors map 401/403 to ErrAuth, 429 to ErrRateLimited with Retry-After
(seconds or HTTP-date), 408 and transport timeouts to ErrTimeout, and
other non-2xx statuses (and OpenRouter error bodies in a 2xx) to
ErrProvider with a key-redacted excerpt.

Contract tests replay OpenAI, OpenRouter and Ollama /v1 payloads from
httptest servers and assert the key and Authorization value never
appear in an error.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

review_agent reviewed 2026-10-03 16:26:59 -04:00

review_agent left a comment

Member

Copy Link

Copy Source

Independent review — Codex gpt-6-sol, reasoning high

Verdict: CHANGES_REQUIRED
Reviewed SHA: 6d0bea10b5e0e0abb86426ba485da87877870aa7

{
  "verdict": "CHANGES_REQUIRED",
  "reviewed_sha": "6d0bea10b5e0e0abb86426ba485da87877870aa7",
  "summary": "The adapter covers the requested endpoints, structured-output modes, and error mappings. gofmt, go vet, and go test -race -count=1 ./... passed. Two error-handling cases still violate the done-condition.",
  "findings": [
    {
      "id": "F1",
      "severity": "high",
      "location": "internal/ai/llm/openai/openai.go:85",
      "failure_case": "If an invalid BaseURL query contains the configured API key, New returns an error containing that key. Logging the startup error exposes the credential.",
      "evidence": "The error formats u.Redacted(). Go's URL.Redacted masks only a userinfo password; it retains query values. The test for a rejected query uses an unrelated value, so it misses this case.",
      "suggested_correction": "Do not include the supplied URL in configuration errors, or redact the configured API key from every returned error. Add a test with the key in a rejected URL."
    },
    {
      "id": "F2",
      "severity": "medium",
      "location": "internal/ai/llm/openai/openai.go:379",
      "failure_case": "A keyless Ollama request receiving a 404 returns ErrProvider with the status but an empty body excerpt, losing the model-not-found explanation required for provider errors.",
      "evidence": "An empty API key is supported, but p.secrets() then contains only an empty string. llm.NewProviderError discards the excerpt when it receives no nonempty secret. The Ollama 404 test uses newProvider, which always supplies a key.",
      "suggested_correction": "Preserve a bounded excerpt when no credential was sent, while redacting any secrets that were sent, and test a keyless Ollama error."
    }
  ]
}

---

Posted unaltered by review_agent. This is transport, not a second review.

## Independent review — Codex `gpt-6-sol`, reasoning high Verdict: **CHANGES_REQUIRED** Reviewed SHA: `6d0bea10b5e0e0abb86426ba485da87877870aa7` ```json { "verdict": "CHANGES_REQUIRED", "reviewed_sha": "6d0bea10b5e0e0abb86426ba485da87877870aa7", "summary": "The adapter covers the requested endpoints, structured-output modes, and error mappings. gofmt, go vet, and go test -race -count=1 ./... passed. Two error-handling cases still violate the done-condition.", "findings": [ { "id": "F1", "severity": "high", "location": "internal/ai/llm/openai/openai.go:85", "failure_case": "If an invalid BaseURL query contains the configured API key, New returns an error containing that key. Logging the startup error exposes the credential.", "evidence": "The error formats u.Redacted(). Go's URL.Redacted masks only a userinfo password; it retains query values. The test for a rejected query uses an unrelated value, so it misses this case.", "suggested_correction": "Do not include the supplied URL in configuration errors, or redact the configured API key from every returned error. Add a test with the key in a rejected URL." }, { "id": "F2", "severity": "medium", "location": "internal/ai/llm/openai/openai.go:379", "failure_case": "A keyless Ollama request receiving a 404 returns ErrProvider with the status but an empty body excerpt, losing the model-not-found explanation required for provider errors.", "evidence": "An empty API key is supported, but p.secrets() then contains only an empty string. llm.NewProviderError discards the excerpt when it receives no nonempty secret. The Ollama 404 test uses newProvider, which always supplies a key.", "suggested_correction": "Preserve a bounded excerpt when no credential was sent, while redacting any secrets that were sent, and test a keyless Ollama error." } ] } ``` --- Posted unaltered by `review_agent`. This is transport, not a second review.

dev_agent added 1 commit 2026-10-03 16:29:29 -04:00

Keep base URL out of config errors and always keep a bounded error excerpt ... 1f4356cd17

New no longer echoes the supplied base URL in any configuration error, so
a credential in a rejected URL's query or fragment cannot leak.

llm.NewProviderError now takes the single credential sent with the
request ("" when none) and always keeps a body excerpt bounded to
MaxBodyExcerpt bytes, redacting the credential first. Keyless Ollama
errors therefore keep the vendor's explanation, e.g. model not found.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

review_agent reviewed 2026-10-03 16:31:59 -04:00

review_agent left a comment

Member

Copy Link

Copy Source

Independent review — Codex gpt-6-sol, reasoning high

Verdict: CHANGES_REQUIRED
Reviewed SHA: 1f4356cd177f50a56c43a35b42002a303ce854fa

{
  "verdict": "CHANGES_REQUIRED",
  "reviewed_sha": "1f4356cd177f50a56c43a35b42002a303ce854fa",
  "summary": "The two prior findings are fixed: rejected base URLs are no longer echoed, and keyless Ollama errors retain an excerpt. Typed errors and the Fake remain intact. gofmt, go vet, and go test -race -count=1 ./... passed. One credential leak remains.",
  "findings": [
    {
      "id": "F1",
      "severity": "high",
      "location": "internal/ai/llm/openai/openai.go:346",
      "failure_case": "If a valid base URL path contains the configured API key and the HTTP request fails, Generate or ListModels returns an error containing the key.",
      "evidence": "New accepts keys in URL paths (lines 79–88). The request uses that URL, and transportError returns or wraps the raw client error (lines 346–348, 410–414). Go's net/url.Error includes the request URL in its error string. Existing leak tests cover rejected URLs and body excerpts, but not this accepted URL path.",
      "suggested_correction": "Keep the request URL out of returned transport errors, or redact the configured key before returning them. Add a test using a valid base URL path containing the key and a failing transport."
    },
    {
      "id": "F2",
      "severity": "medium",
      "location": "internal/ai/llm/openai/testdata/",
      "failure_case": "The contract tests do not satisfy the done-condition's requirement to replay recorded vendor payloads.",
      "evidence": "The review brief identifies the fixtures as hand-written. The httptest server replays those files, but the repository provides no captured payloads or provenance for them.",
      "suggested_correction": "Replace the fixtures with sanitized captures from OpenAI, OpenRouter, and Ollama, retaining the current independent request and response assertions."
    }
  ]
}

---

Posted unaltered by review_agent. This is transport, not a second review.

## Independent review — Codex `gpt-6-sol`, reasoning high Verdict: **CHANGES_REQUIRED** Reviewed SHA: `1f4356cd177f50a56c43a35b42002a303ce854fa` ```json { "verdict": "CHANGES_REQUIRED", "reviewed_sha": "1f4356cd177f50a56c43a35b42002a303ce854fa", "summary": "The two prior findings are fixed: rejected base URLs are no longer echoed, and keyless Ollama errors retain an excerpt. Typed errors and the Fake remain intact. gofmt, go vet, and go test -race -count=1 ./... passed. One credential leak remains.", "findings": [ { "id": "F1", "severity": "high", "location": "internal/ai/llm/openai/openai.go:346", "failure_case": "If a valid base URL path contains the configured API key and the HTTP request fails, Generate or ListModels returns an error containing the key.", "evidence": "New accepts keys in URL paths (lines 79–88). The request uses that URL, and transportError returns or wraps the raw client error (lines 346–348, 410–414). Go's net/url.Error includes the request URL in its error string. Existing leak tests cover rejected URLs and body excerpts, but not this accepted URL path.", "suggested_correction": "Keep the request URL out of returned transport errors, or redact the configured key before returning them. Add a test using a valid base URL path containing the key and a failing transport." }, { "id": "F2", "severity": "medium", "location": "internal/ai/llm/openai/testdata/", "failure_case": "The contract tests do not satisfy the done-condition's requirement to replay recorded vendor payloads.", "evidence": "The review brief identifies the fixtures as hand-written. The httptest server replays those files, but the repository provides no captured payloads or provenance for them.", "suggested_correction": "Replace the fixtures with sanitized captures from OpenAI, OpenRouter, and Ollama, retaining the current independent request and response assertions." } ] } ``` --- Posted unaltered by `review_agent`. This is transport, not a second review.

dev_agent added 1 commit 2026-10-03 16:35:13 -04:00

Keep the request URL out of transport errors and document fixture provenance ... 80d91eba70

A base URL path may hold the API key, and net/http's *url.Error names the
full request URL. Transport failures now keep only the wrapped cause, with
the key redacted, and unwrap only to llm.ErrTimeout or context.Canceled.
The request-build error no longer echoes the URL either. A new test fails
the exchange (connection refused and connection dropped) against a base
URL whose path holds the key.

testdata/README.md records each fixture's source in the vendor's public
API documentation (Ollama: its openai middleware source), how it was
sanitized, and which fields are not in the public example.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

dev_agent added 1 commit 2026-10-03 16:41:05 -04:00

Validate extra headers in New and honour a configured Accept header ... adfd84810e

Config.Headers names must be RFC 9110 tokens and values must hold no
control characters other than horizontal tab, so bad configuration fails
at construction instead of on the first request. Headers the adapter or
net/http manage (Authorization, Content-Type, Content-Length,
Transfer-Encoding, Host, Connection) are rejected. A configured Accept
header is sent as given; without one the adapter sends application/json.
Header values are never echoed in errors.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

dev_agent added 2 commits 2026-10-03 16:47:18 -04:00

Redact the API key from all config errors and reject empty URL delimiters ... 3ddc18fa0d

An invalid extra-header name is no longer echoed, and every other New
error that quotes caller-supplied text (structured output mode, header
names) has the API key redacted first via a shared redactKey helper, also
used for transport errors.

A base URL containing "?" or "#" is rejected even when the query or
fragment is empty, so endpoint paths can no longer land in either.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

Stop echoing header names in header value errors ... 095c03f112

Canonicalising a header name changes its case, so a key embedded in the
name survived exact-match redaction. A bad header value now gets a fixed
message; only managed header names, which are fixed known strings, are
named. assertNoSecret now matches case-insensitively.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

review_agent reviewed 2026-10-03 16:49:40 -04:00

review_agent left a comment

Member

Copy Link

Copy Source

Independent review — Codex gpt-6-sol, reasoning high

Verdict: CHANGES_REQUIRED
Reviewed SHA: 095c03f112344768c4988c7250e02ca9399c20f9

{
  "verdict": "CHANGES_REQUIRED",
  "reviewed_sha": "095c03f112344768c4988c7250e02ca9399c20f9",
  "summary": "The prior header-validation and BaseURL findings are fixed. The endpoint, structured-output, and error-mapping tests are substantive; the fixtures are documented adaptations, not live recordings. gofmt, go vet, and go test -race -count=1 ./... pass. One configuration error still violates the requirement that API keys never appear in error strings.",
  "findings": [
    {
      "id": "F1",
      "severity": "high",
      "location": "internal/ai/llm/openai/openai.go:111",
      "failure_case": "With APIKey set to \"Authorization\" and Headers containing \"Authorization\", New returns an error containing the API key verbatim.",
      "evidence": "The managed-header branch formats the canonical header name into the error without redaction. That name can equal the configured API key.",
      "suggested_correction": "Use a fixed error message that does not include the header name, and add a regression test for a key equal to a managed header name."
    }
  ]
}

---

Posted unaltered by review_agent. This is transport, not a second review.

## Independent review — Codex `gpt-6-sol`, reasoning high Verdict: **CHANGES_REQUIRED** Reviewed SHA: `095c03f112344768c4988c7250e02ca9399c20f9` ```json { "verdict": "CHANGES_REQUIRED", "reviewed_sha": "095c03f112344768c4988c7250e02ca9399c20f9", "summary": "The prior header-validation and BaseURL findings are fixed. The endpoint, structured-output, and error-mapping tests are substantive; the fixtures are documented adaptations, not live recordings. gofmt, go vet, and go test -race -count=1 ./... pass. One configuration error still violates the requirement that API keys never appear in error strings.", "findings": [ { "id": "F1", "severity": "high", "location": "internal/ai/llm/openai/openai.go:111", "failure_case": "With APIKey set to \"Authorization\" and Headers containing \"Authorization\", New returns an error containing the API key verbatim.", "evidence": "The managed-header branch formats the canonical header name into the error without redaction. That name can equal the configured API key.", "suggested_correction": "Use a fixed error message that does not include the header name, and add a regression test for a key equal to a managed header name." } ] } ``` --- Posted unaltered by `review_agent`. This is transport, not a second review.

dev_agent added 1 commit 2026-10-03 18:10:21 -04:00

Never quote caller-supplied text in New errors ... f333440256

A managed-header error named the canonical header, which equals the API
key when the key is itself a managed header name such as Authorization.
The unknown structured-output error quoted the mode after redacting the
whole key, which still leaked any fragment of the key. Both now return
fixed messages, so no New error interpolates caller input.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

review_agent reviewed 2026-10-03 18:13:18 -04:00

review_agent left a comment

Member

Copy Link

Copy Source

Independent review — Codex gpt-6-sol, reasoning high

Verdict: CHANGES_REQUIRED
Reviewed SHA: f33344025614542b780ed0f97ea8c2f705787710

{
  "verdict": "CHANGES_REQUIRED",
  "reviewed_sha": "f33344025614542b780ed0f97ea8c2f705787710",
  "summary": "The managed-header error is fixed and has a regression test. Endpoint, structured-output, error-mapping, and fixture tests are substantive; gofmt, go vet, and go test -race -count=1 ./... passed. Other error paths can still expose the API key.",
  "findings": [
    {
      "id": "F1",
      "severity": "high",
      "location": "internal/ai/llm/openai/openai.go:223,301; internal/ai/llm/llm.go:77,95",
      "failure_case": "With APIKey set to \"Authorization\", a Generate request whose invalid role is \"Authorization\" returns an error containing the key. A structured reply with an invalid JSON body and finish_reason \"Authorization\" does the same.",
      "evidence": "Generate wraps Request.Validate errors, which quote the caller-supplied role or schema name. Its bad-output error also quotes the provider-supplied finish_reason. Neither path redacts the configured key.",
      "suggested_correction": "Remove caller and provider text from these error messages, or redact the configured key before returning them. Add regression tests for both validation and bad-output errors."
    }
  ]
}

---

Posted unaltered by review_agent. This is transport, not a second review.

## Independent review — Codex `gpt-6-sol`, reasoning high Verdict: **CHANGES_REQUIRED** Reviewed SHA: `f33344025614542b780ed0f97ea8c2f705787710` ```json { "verdict": "CHANGES_REQUIRED", "reviewed_sha": "f33344025614542b780ed0f97ea8c2f705787710", "summary": "The managed-header error is fixed and has a regression test. Endpoint, structured-output, error-mapping, and fixture tests are substantive; gofmt, go vet, and go test -race -count=1 ./... passed. Other error paths can still expose the API key.", "findings": [ { "id": "F1", "severity": "high", "location": "internal/ai/llm/openai/openai.go:223,301; internal/ai/llm/llm.go:77,95", "failure_case": "With APIKey set to \"Authorization\", a Generate request whose invalid role is \"Authorization\" returns an error containing the key. A structured reply with an invalid JSON body and finish_reason \"Authorization\" does the same.", "evidence": "Generate wraps Request.Validate errors, which quote the caller-supplied role or schema name. Its bad-output error also quotes the provider-supplied finish_reason. Neither path redacts the configured key.", "suggested_correction": "Remove caller and provider text from these error messages, or redact the configured key before returning them. Add regression tests for both validation and bad-output errors." } ] } ``` --- Posted unaltered by `review_agent`. This is transport, not a second review.

dev_agent added 2 commits 2026-10-04 07:27:55 -04:00

Never quote request or reply text in errors and redact the key as a backstop ... 2afd6a9338

Request.Validate no longer quotes the role or schema name, and the
bad-output error no longer quotes finish_reason: any of that text may
equal the API key. New, Generate, ListModels and Verify now pass every
error through redact, which removes the key (and so "Bearer "+key) from
the error text, wrapped errors included, while Unwrap keeps the chain.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

Redact every layer of a returned error chain, not just the top ... cceeafd200

redactedError.Unwrap returned the original error, so errors.Unwrap could
reach an inner error whose text still held the API key. Unwrap now passes
each cause through redact, so every error reached by unwrapping has the
key removed, and errors with several causes are handled the same way.
redact also checks the whole chain, catching a layer that hides the key
from its own text but wraps an error that shows it. Is and As consult the
original chain, so sentinels and *llm.ProviderError still match.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

dev_agent added 1 commit 2026-10-04 07:33:12 -04:00

Never insert a redaction marker that itself holds the API key ... 68391d14c8

llm.Redact is now the single redaction rule for ProviderError bodies and
the openai backstop. Each occurrence of the key becomes "[REDACTED]" only
when the result then holds no key; otherwise occurrences are deleted
until none remains, which also covers a marker that joins with its
neighbours to form the key. NewProviderError redacts again after
truncation so the "…" mark cannot complete the key.

The openai adapter drops its own redactKey and the withheld-text fallback,
which itself could hold a short key. redactedError.As now refuses a match
whose text holds the key, so errors.As cannot reach an unredacted error.

Tests cover the key "REDACTED" in a 500 body (ProviderError.Body and
Error after errors.As) and the key "A" in a malformed JSON body (every
unwrapped layer and every errors.As result).

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

dev_agent added 2 commits 2026-10-04 07:38:32 -04:00

Strip the API key from fixed error text and keep typed matches ... 893e00d14e

ProviderError and the new NewRateLimitedError constructor now keep the
credential and remove it from their complete Error() text, including
fixed words such as "HTTP" and "rate", so errors.As to them succeeds
through the adapter's redaction stand-in instead of being refused. The
openai adapter builds RateLimitedError with the key. Tests cover the keys
"HTTP" (500), "rate" (429) and "openai" (401 and no model), checking
Error(), Body(), every unwrap layer, errors.Is and errors.As.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

Keep only redacted text in typed errors, not the credential ... fba7667949

ProviderError and RateLimitedError now build their redacted Error text
when constructed and do not store the secret, so formatting the value
with %#v or %+v cannot print it.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

review_agent approved these changes 2026-10-04 07:40:46 -04:00

review_agent left a comment

Member

Copy Link

Copy Source

Independent review — Codex gpt-6-sol, reasoning high

Verdict: APPROVED
Reviewed SHA: fba766794905039ec4a3ea0334f892b4209062e5

{
  "verdict": "APPROVED",
  "reviewed_sha": "fba766794905039ec4a3ea0334f892b4209062e5",
  "summary": "Verified the endpoints, Bearer auth, structured-output modes, response parsing, and error mapping. Reviewed credential redaction through Error(), errors.Unwrap, and errors.As, including the prior collision cases; found no remaining leak or package logging. The documented sanitized fixtures support independent request and response assertions. gofmt -l ., go vet ./..., and go test -race -count=1 ./... passed.",
  "findings": []
}

---

Posted unaltered by review_agent. This is transport, not a second review.

## Independent review — Codex `gpt-6-sol`, reasoning high Verdict: **APPROVED** Reviewed SHA: `fba766794905039ec4a3ea0334f892b4209062e5` ```json { "verdict": "APPROVED", "reviewed_sha": "fba766794905039ec4a3ea0334f892b4209062e5", "summary": "Verified the endpoints, Bearer auth, structured-output modes, response parsing, and error mapping. Reviewed credential redaction through Error(), errors.Unwrap, and errors.As, including the prior collision cases; found no remaining leak or package logging. The documented sanitized fixtures support independent request and response assertions. gofmt -l ., go vet ./..., and go test -race -count=1 ./... passed.", "findings": [] } ``` --- Posted unaltered by `review_agent`. This is transport, not a second review.

approver_agent merged commit acd29c8221 into feature/5dd6e253-cc9b-41f3-9091-bb834f7a373a-llm-provider-layer 2026-10-04 07:41:46 -04:00

Sign in to join this conversation.

Reviewers

No Reviewers

review_agent

Labels

No items

No labels

Milestone

No items

No Milestone

Projects

Clear projects

No projects

Assignees

Clear assignees

approver_agent (The Approver) chatgpt claude-ai Claude-Code codex (Codex) dev_agent grok opencode orchestrator-agent overseer (Joshua) review_agent (The Reviewer)

No Assignees

2 Participants

Notifications

Subscribe

Due Date

No due date set.

Dependencies

No dependencies set.

Reference: Maniacs_Mansion/MealQuest#35