Skip to content

Verify Ollama API keys on an authenticated endpoint#1940

Merged
steipete merged 8 commits into
steipete:mainfrom
joeVenner:fix/ollama-hourly-window
Jul 9, 2026
Merged

Verify Ollama API keys on an authenticated endpoint#1940
steipete merged 8 commits into
steipete:mainfrom
joeVenner:fix/ollama-hourly-window

Conversation

@joeVenner

@joeVenner joeVenner commented Jul 6, 2026

Copy link
Copy Markdown
Contributor

Summary

  • stop treating the public /api/tags catalog as API-key verification
  • validate the key against authenticated /api/web_search without performing a search, accepting only the observed authenticated success/query-validation responses
  • trim the key, preserve cancellation, and keep 401/403 mapped to the actionable invalid/revoked error
  • fail closed on unproven statuses and reject insecure or cross-origin credential endpoints before transport
  • retain contributor credit and add a changelog entry thanking @joeVenner

Maintainer hardening

  • 200 and the live authenticated empty-query 400 response prove validation; the originally proposed 422 allowance was removed because it was not observed
  • the public catalog is fetched only after validation and its own 401/403 responses remain credential failures
  • custom transport seams derive validation on the catalog origin, require HTTPS or loopback HTTP, and reject an explicit cross-origin validation URL before attaching credentials
  • cancellation remains cancellation at both network stages instead of becoming a retryable provider error

Proof

Exact head: 16defab547d5551518e7a31a09bc43ab26ee10a9

  • Public Model Identifier Gate: PASS; this is model-free behavior plumbing.
  • Real endpoint matrix, with no secret output: missing key returned 401, deliberately invalid key returned 401, and the saved valid key reached the authenticated empty-query validation response (400).
  • Focused OllamaUsageFetcherRetryMappingTests: 17 passed.
  • make check: passed with zero formatting or lint violations.
  • make test: all 50 shards passed.
  • Structured branch review: clean after adding second-stage catalog authorization and endpoint-origin coverage.
  • Developer-ID signed debug package: CodexGitCommit=16defab5; deep/strict signature valid; Gatekeeper accepted. App binary SHA-256 c98b81e440361e4283ca400e8ca0d8e81fff8286e1bd2ec363b95b5843909ad1; bundled CLI SHA-256 22c7062b3c9fe0640aee76a61bf2e4ab92a63210608f1cb4feaf220c98114ecc.
  • Exact bundled CLI with the saved valid key exited 0 and rendered Ollama API-key state. A deliberately invalid key exited 1 with the invalid/revoked remediation.
  • Exact signed app, isolated to Ollama API mode, refreshed successfully and showed the verified-key/no-cloud-quota state as updated just now.
  • Screenshot stayed local; debug preferences were restored byte-identically and the production app remained running.
  • Dependencies unchanged; no dependency-freshness audit required.

Risk

Low and isolated to Ollama API-key validation. The validation request uses an empty query and performs no search. The accepted status set is deliberately narrow and fails closed if provider behavior changes.

@clawsweeper

clawsweeper Bot commented Jul 6, 2026

Copy link
Copy Markdown

Codex review: needs maintainer review before merge. Reviewed July 9, 2026, 11:32 AM ET / 15:32 UTC.

Summary
The branch validates Ollama API keys with a same-origin /api/web_search probe before fetching /api/tags, expands retry-mapping coverage, and updates Ollama docs and changelog text.

Reproducibility: yes. for the source-level bug boundary: current main treats the public /api/tags request as key verification, while the PR adds an authenticated validation stage. I did not run extra live provider probes because repository policy requires explicit authorization for real provider checks.

Review metrics: 2 noteworthy metrics.

  • Provider runtime requests: 1 added before catalog fetch. Ollama API-key refresh now depends on an authenticated validation endpoint before the existing model catalog request.
  • Changed files: 4 files affected. The PR spans runtime provider code, focused tests, docs, and release notes, so the review needs both behavior and compatibility coverage.

Merge readiness
Overall: 🐚 platinum hermit
Proof: 🦞 diamond lobster
Patch quality: 🐚 platinum hermit
Result: ready for maintainer review.

Overall follows the weaker of proof and patch quality, so missing proof can cap an otherwise strong patch.

Rank-up moves:

  • none.

Risk before merge

  • [P1] Existing Ollama API-key refreshes would gain a required /api/web_search validation request before the catalog request, so users can fail earlier if that endpoint is unavailable, rate-limited, or changes status semantics.
  • [P1] Accepting HTTP 400 from an empty-query /api/web_search request as authenticated success relies on observed provider behavior rather than a documented contract in the repository.
  • [P1] The branch intentionally fails closed on unrecognized validation statuses, which is safer for revoked keys but can turn provider-side validation drift into a user-visible API-key mode failure.

Maintainer options:

  1. Accept the endpoint dependency (recommended)
    Maintainers can merge after explicitly accepting /api/web_search empty-query validation as the Ollama API-key contract CodexBar will rely on.
  2. Harden before merge
    Require a validation endpoint or request shape that separately handles unauthorized, authenticated malformed-query, and rate-limit responses before release.
  3. Defer the validator
    Close or pause this PR if the undocumented endpoint behavior is not worth the compatibility risk for Ollama API-key mode.

Next step before merge

  • [P2] Human review is needed for the Ollama endpoint compatibility choice; there is no narrow automation repair until maintainers accept the validator or choose a different endpoint contract.

Maintainer decision needed

  • Question: Should CodexBar ship Ollama API-key mode with /api/web_search empty-query validation, treating HTTP 400 as authenticated success and other statuses such as 429 as fail-closed validation failures?
  • Rationale: Local tests and contributor proof show the branch rejects the current live invalid-key path, but automation cannot prove that Ollama's endpoint ordering and rate-limit behavior are stable enough for existing users.
  • Likely owner: steipete — steipete authored the latest branch hardening commits and is the clearest owner for accepting or redirecting the provider compatibility tradeoff.
  • Options:
    • Accept current validator (recommended): Merge with the current /api/web_search dependency and own the compatibility risk if Ollama changes its empty-query or rate-limit behavior.
    • Require stronger validation: Ask for a validation request or endpoint whose accepted responses cannot be confused with pre-auth request validation or unrelated endpoint quota failures.
    • Pause this hardening: Leave the public-catalog behavior unchanged until Ollama documents or exposes a stable API-key verification endpoint.

Security
Cleared: The diff adds no dependencies, scripts, workflows, or secret-storage changes; the bearer-token path is same-origin and HTTPS/loopback guarded, with remaining concerns treated as auth-provider compatibility risk.

Review details

Best possible solution:

Land this after maintainers explicitly accept the /api/web_search validation contract and failure modes, or switch to a documented authenticated probe whose responses distinguish valid, unauthorized, and quota/rate-limit states.

Do we have a high-confidence way to reproduce the issue?

Yes for the source-level bug boundary: current main treats the public /api/tags request as key verification, while the PR adds an authenticated validation stage. I did not run extra live provider probes because repository policy requires explicit authorization for real provider checks.

Is this the best way to solve the issue?

Unclear: validating before reading the public catalog is the right shape, but the current empty-query /api/web_search response contract and fail-closed status handling need maintainer acceptance before this is the safest shipped solution.

AGENTS.md: found and applied where relevant.

Codex review notes: model internal, reasoning high; reviewed against c2d0cd1a1ff7.

Label changes

Label changes:

  • add merge-risk: 🚨 auth-provider: The PR changes how CodexBar verifies Ollama bearer tokens before reporting API-key mode as valid.
  • add rating: 🐚 platinum hermit: Overall readiness is 🐚 platinum hermit; proof is 🦞 diamond lobster and patch quality is 🐚 platinum hermit.
  • remove rating: 🦐 gold shrimp: Current PR rating is rating: 🐚 platinum hermit, so this older rating label is no longer current.

Label justifications:

  • P2: This is a normal-priority Ollama API-key correctness improvement with limited blast radius to one provider mode.
  • merge-risk: 🚨 compatibility: Merging changes existing Ollama API-key refresh from one catalog request to validation plus catalog, adding a new endpoint dependency and fail-closed statuses.
  • merge-risk: 🚨 auth-provider: The PR changes how CodexBar verifies Ollama bearer tokens before reporting API-key mode as valid.
  • rating: 🐚 platinum hermit: Overall readiness is 🐚 platinum hermit; proof is 🦞 diamond lobster and patch quality is 🐚 platinum hermit.
  • status: 👀 ready for maintainer look: ClawSweeper has no concrete contributor-facing blocker left for this PR. Sufficient (live_output): The PR includes redacted after-fix live CLI output for an invalid Ollama key plus PR-body live endpoint and signed-app proof for valid-key behavior.
  • proof: sufficient: Contributor real behavior proof is sufficient. The PR includes redacted after-fix live CLI output for an invalid Ollama key plus PR-body live endpoint and signed-app proof for valid-key behavior.
Evidence reviewed

What I checked:

  • Repository policy read: AGENTS.md was read fully; its provider-testing guidance affected the review by avoiding extra live provider probes and treating the contributor's redacted CLI/live-output proof as the right evidence boundary. (AGENTS.md:1, c2d0cd1a1ff7)
  • Current main behavior: Current main builds one bearer GET to https://ollama.com/api/tags and has no separate authenticated key-validation request, so the central behavior is not already implemented on main. (Sources/CodexBarCore/Providers/Ollama/OllamaUsageFetcher.swift:711, c2d0cd1a1ff7)
  • PR implementation: The PR head adds validationURL, posts an empty JSON query with the bearer token, accepts HTTP 200 and 400 as validation success, maps 401/403 to unauthorized, and fails other validation statuses. (Sources/CodexBarCore/Providers/Ollama/OllamaUsageFetcher.swift:711, 16defab547d5)
  • Focused test coverage: The PR adds injected-transport tests for rejected validation, successful validation continuing to the catalog, catalog authorization failures, same-origin validation, insecure endpoint rejection, cancellation, and unproven status failure. (Tests/CodexBarTests/OllamaUsageFetcherRetryMappingTests.swift:87, 16defab547d5)
  • Contributor proof: The PR discussion includes after-fix CLI output from a live Ollama API-key path where a deliberately invalid key is rejected through CodexBarCLI usage --provider ollama --source api, and the PR body reports a redacted valid-key live endpoint matrix and signed-app refresh proof. (16defab547d5)
  • Compatibility review comments: Automated PR review comments on the current head flagged the accepted HTTP 400 response and fail-closed HTTP 429/rate-limit behavior as endpoint-contract risks, which I am routing as maintainer merge-risk rather than a definite line-level defect without provider-contract evidence. (Sources/CodexBarCore/Providers/Ollama/OllamaUsageFetcher.swift:782, 16defab547d5)

Likely related people:

  • steipete: steipete pushed the latest hardening commits on this PR branch and authored the earlier Ollama API-key support visible in current-main history. (role: recent area contributor and likely decision owner; confidence: high; commits: 9857e1de0bae, 6b3e8a026893, 58732f0bb335; files: Sources/CodexBarCore/Providers/Ollama/OllamaUsageFetcher.swift, Tests/CodexBarTests/OllamaUsageFetcherRetryMappingTests.swift, docs/ollama.md)
  • joeVenner: joeVenner authored the original validation branch and recently contributed merged adjacent Ollama WorkOS/session fixes touching the same provider surface. (role: recent Ollama contributor; confidence: medium; commits: 8f4cf6819d3f, 50bc69bf34c2, 28162d989835; files: Sources/CodexBarCore/Providers/Ollama/OllamaUsageFetcher.swift, Tests/CodexBarTests/OllamaUsageFetcherRetryMappingTests.swift, docs/ollama.md)
What the crustacean ranks mean
  • 🦀 challenger crab: rare, exceptional readiness with strong proof, clean implementation, and convincing validation.
  • 🦞 diamond lobster: very strong readiness with only minor maintainer review expected.
  • 🐚 platinum hermit: good normal PR, likely mergeable with ordinary maintainer review.
  • 🦐 gold shrimp: useful signal, but proof or patch confidence is still limited.
  • 🦪 silver shellfish: thin signal; proof, validation, or implementation needs work.
  • 🧂 unranked krab: not merge-ready because proof is missing/unusable or there are serious correctness or safety concerns.
  • 🌊 off-meta tidepool: rating does not apply to this item.

Shiny media proof means a screenshot, video, or linked artifact directly shows the changed behavior. Runtime, network, CSP, and security claims still need visible diagnostics.

How this review workflow works
  • ClawSweeper keeps one durable marker-backed review comment per issue or PR.
  • Re-runs edit this comment so the latest verdict, findings, and automation markers stay together instead of adding duplicate bot comments.
  • A fresh review can be triggered by eligible @clawsweeper re-review comments, exact-item GitHub events, scheduled/background review runs, or manual workflow dispatch.
  • PR/issue authors and users with repository write access can comment @clawsweeper re-review or @clawsweeper re-run on an open PR or issue to request a fresh review only.
  • Maintainers can also comment @clawsweeper review to request a fresh review only.
  • Fresh-review commands do not start repair, autofix, rebase, CI repair, or automerge.
  • Maintainer-only repair and merge flows require explicit commands such as @clawsweeper autofix, @clawsweeper automerge, @clawsweeper fix ci, or @clawsweeper address review.
  • Maintainers can comment @clawsweeper explain to ask for more context, or @clawsweeper stop to stop active automation.
Review history (12 earlier review cycles; latest 8 shown)
  • reviewed 2026-07-06T19:09:44.195Z sha c3ba4b2 :: needs maintainer review before merge. :: none
  • reviewed 2026-07-06T19:21:26.616Z sha c3ba4b2 :: needs maintainer review before merge. :: none
  • reviewed 2026-07-06T21:50:02.083Z sha f1a54dd :: needs maintainer review before merge. :: none
  • reviewed 2026-07-06T21:57:09.993Z sha f1a54dd :: needs maintainer review before merge. :: none
  • reviewed 2026-07-07T00:22:51.103Z sha 1ad231d :: needs maintainer review before merge. :: none
  • reviewed 2026-07-07T00:30:53.892Z sha 1ad231d :: needs maintainer review before merge. :: none
  • reviewed 2026-07-09T15:12:30.331Z sha a626d24 :: needs maintainer review before merge. :: none
  • reviewed 2026-07-09T15:27:00.504Z sha 16defab :: needs maintainer review before merge. :: none

@clawsweeper clawsweeper Bot added rating: 🦪 silver shellfish Thin PR readiness signal; proof, validation, or implementation needs work. status: 📣 needs proof The PR needs real behavior proof before ClawSweeper can clear the contributor ask. P2 Normal priority bug or improvement with limited blast radius. labels Jul 6, 2026
@joeVenner

Copy link
Copy Markdown
Contributor Author

@clawsweeper re-review

@clawsweeper

clawsweeper Bot commented Jul 6, 2026

Copy link
Copy Markdown

🦞🧹
ClawSweeper re-review requested.

I asked ClawSweeper to review this item again.
Action: item re-review queued (workflow sweep.yml, event repository_dispatch).
Result: the existing ClawSweeper review comment will be edited in place when the review finishes.

@joeVenner joeVenner force-pushed the fix/ollama-hourly-window branch from 23a493a to 8373dd3 Compare July 6, 2026 16:46
@joeVenner joeVenner changed the title Fix Ollama hourly usage cadence Verify Ollama API keys on an authenticated endpoint Jul 6, 2026
@joeVenner

Copy link
Copy Markdown
Contributor Author

@clawsweeper re-review

@clawsweeper

clawsweeper Bot commented Jul 6, 2026

Copy link
Copy Markdown

🦞🧹
ClawSweeper re-review requested.

I asked ClawSweeper to review this item again.
Action: item re-review queued (workflow sweep.yml, event repository_dispatch).
Result: the existing ClawSweeper review comment will be edited in place when the review finishes.

@joeVenner

Copy link
Copy Markdown
Contributor Author

After-fix CLI proof from the rewritten branch (2026-07-06):

$ swift build --disable-sandbox --product CodexBarCLI
Build of product CodexBarCLI complete!

$ OLLAMA_API_KEY=codexbar-proof-invalid .build/debug/CodexBarCLI usage --provider ollama --source api --no-color
Error: Ollama API key is invalid or expired.

This executes the real OllamaAPIUsageFetcher against live Ollama and rejects the deliberately invalid key. --source api avoids browser-cookie and Keychain paths. The error wording shown here is the current-main wording and is corrected independently in #1943.

@joeVenner

Copy link
Copy Markdown
Contributor Author

@clawsweeper re-review

@clawsweeper

clawsweeper Bot commented Jul 6, 2026

Copy link
Copy Markdown

🦞🧹
ClawSweeper re-review requested.

I asked ClawSweeper to review this item again.
Action: item re-review queued (workflow sweep.yml, event repository_dispatch).
Result: the existing ClawSweeper review comment will be edited in place when the review finishes.

@joeVenner

Copy link
Copy Markdown
Contributor Author

@clawsweeper re-review

@clawsweeper

clawsweeper Bot commented Jul 6, 2026

Copy link
Copy Markdown

🦞🧹
ClawSweeper re-review requested.

I asked ClawSweeper to review this item again.
Action: item re-review queued (workflow sweep.yml, event repository_dispatch).
Result: the existing ClawSweeper review comment will be edited in place when the review finishes.

@joeVenner

Copy link
Copy Markdown
Contributor Author

@clawsweeper re-review

@clawsweeper clawsweeper Bot added proof: sufficient Contributor real behavior proof is sufficient. rating: 🦐 gold shrimp Decent PR readiness signal, but merge confidence is limited. status: 👀 ready for maintainer look ClawSweeper has no concrete contributor-facing blocker left for this PR. merge-risk: 🚨 compatibility 🚨 Merging this PR could break existing users, config, migrations, defaults, or upgrades. and removed rating: 🦪 silver shellfish Thin PR readiness signal; proof, validation, or implementation needs work. status: 📣 needs proof The PR needs real behavior proof before ClawSweeper can clear the contributor ask. labels Jul 6, 2026
@joeVenner

Copy link
Copy Markdown
Contributor Author

@clawsweeper re-review

@clawsweeper

clawsweeper Bot commented Jul 6, 2026

Copy link
Copy Markdown

🦞🧹
ClawSweeper re-review requested.

I asked ClawSweeper to review this item again.
Action: item re-review queued (workflow sweep.yml, event repository_dispatch).
Result: the existing ClawSweeper review comment will be edited in place when the review finishes.

@joeVenner joeVenner force-pushed the fix/ollama-hourly-window branch from f1a54dd to 1ad231d Compare July 7, 2026 00:17
@joeVenner

Copy link
Copy Markdown
Contributor Author

@clawsweeper re-review

@clawsweeper

clawsweeper Bot commented Jul 7, 2026

Copy link
Copy Markdown

🦞🧹
ClawSweeper re-review requested.

I asked ClawSweeper to review this item again.
Action: item re-review queued (workflow sweep.yml, event repository_dispatch).
Result: the existing ClawSweeper review comment will be edited in place when the review finishes.

@steipete steipete force-pushed the fix/ollama-hourly-window branch from 1ad231d to a626d24 Compare July 9, 2026 15:06

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: a626d2460d

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

Comment on lines +782 to +784
switch response.statusCode {
case 200, 400:
return

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Badge Do not accept malformed validation responses as success

When /api/web_search returns 400 for this deliberately empty query, this branch treats that malformed-request response as proof the bearer token was accepted. Since the endpoint contract requires a non-empty query and 400 is a generic bad-request status, any deployment that validates the payload before auth (or reports malformed auth as 400) will still proceed to the public /api/tags catalog and show revoked/invalid keys as verified, which is the false-positive this change is meant to prevent.

Useful? React with 👍 / 👎.

joeVenner and others added 8 commits July 9, 2026 08:15
The committed proof image was a 1x1 placeholder, not real evidence.
Executable test output in the PR description replaces it.
Co-authored-by: JoeVenner <ylafrimi@gmail.com>
Co-authored-by: JoeVenner <ylafrimi@gmail.com>
Co-authored-by: JoeVenner <ylafrimi@gmail.com>
@steipete steipete force-pushed the fix/ollama-hourly-window branch from a626d24 to 16defab Compare July 9, 2026 15:23

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 16defab547

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

Comment on lines +787 to +788
default:
throw OllamaUsageError.networkError("HTTP \(response.statusCode)")

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Badge Handle web-search rate limits during validation

When a valid Ollama key has exhausted or hit the rate limit for /api/web_search, this switch treats the validation response (for example HTTP 429) as fatal and never reaches the public /api/tags catalog, so API-key mode reports Ollama as failed even though the bearer token is still valid for the account. Since this probe is only meant to prove the key before reading the catalog, handle post-auth quota/rate-limit responses separately or validate against an endpoint whose quota matches the feature being checked.

Useful? React with 👍 / 👎.

@clawsweeper clawsweeper Bot added rating: 🐚 platinum hermit Good normal PR readiness with ordinary maintainer review expected. merge-risk: 🚨 auth-provider 🚨 Merging this PR could break OAuth, tokens, provider routing, model choice, or credentials. and removed rating: 🦐 gold shrimp Decent PR readiness signal, but merge confidence is limited. labels Jul 9, 2026
@steipete

steipete commented Jul 9, 2026

Copy link
Copy Markdown
Owner

Exact-head proof for 16defab547d5551518e7a31a09bc43ab26ee10a9:

  • swift test --filter Ollama — 17 focused tests passed.
  • make check — passed.
  • make test — all 50 local shards passed.
  • autoreview --mode branch --base origin/main — clean; no accepted/actionable findings.
  • Signed debug package: Developer ID signature valid; codesign --verify --deep --strict and Gatekeeper assessment passed.
  • Bundled CLI live proof: saved valid key exited 0 and resolved Ollama (api) / Plan: Api Key; a deliberately invalid key exited 1 with the actionable invalid/revoked error.
  • Isolated signed app live proof: Ollama refreshed as a verified API key and correctly reported that Ollama does not expose cloud quota.
  • Exact-head CI: lint, both Linux builds, changes, and GitGuardian are green. Four hosted macOS shards remain capacity-queued; landing under the documented capacity-only policy because exact-head local tests, review, and authenticated packaged proof are complete.
  • Public Model Identifier Gate: PASS (model-free diff). Dependency freshness: no dependency changes.

@steipete steipete merged commit 3b039d1 into steipete:main Jul 9, 2026
9 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

merge-risk: 🚨 auth-provider 🚨 Merging this PR could break OAuth, tokens, provider routing, model choice, or credentials. merge-risk: 🚨 compatibility 🚨 Merging this PR could break existing users, config, migrations, defaults, or upgrades. P2 Normal priority bug or improvement with limited blast radius. proof: sufficient Contributor real behavior proof is sufficient. rating: 🐚 platinum hermit Good normal PR readiness with ordinary maintainer review expected. status: 👀 ready for maintainer look ClawSweeper has no concrete contributor-facing blocker left for this PR.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants