Skip to content

Verify Ollama API keys on an authenticated endpoint#1940

Open
joeVenner wants to merge 5 commits into
steipete:mainfrom
joeVenner:fix/ollama-hourly-window
Open

Verify Ollama API keys on an authenticated endpoint#1940
joeVenner wants to merge 5 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 the authenticated /api/web_search endpoint without performing a search
  • fetch the public model catalog only after authentication succeeds

Proof

Deterministic test run on this branch (injected HTTP transport — no network, no credentials):

$ swift test --disable-sandbox --filter OllamaUsageFetcherRetryMappingTests
✔ Test "authorized validation continues to model catalog" passed
✔ Test "api fetch sends bearer token and rejects unauthorized key" passed
✔ Test run with 8 tests in 1 suite passed.

The validator POSTs an empty query to https://ollama.com/api/web_search with the Bearer key and only proceeds to /api/tags when the response indicates the key is accepted (200/400/422); a 401/403 throws apiUnauthorized before the catalog fetch.

Note for the maintainer

The validator treats 400/422 from /api/web_search as "key accepted," on the assumption that those codes only arise from a malformed empty query after auth succeeds. If the endpoint validates the query body before the Bearer token, a 400 would be returned to any caller and the validation would be vacuous. I could not verify Ollama's auth-check ordering without a live key, so flagging it for your judgment. This is also why the bot tagged merge-risk: compatibility.

Testing

  • swift test --disable-sandbox --filter OllamaUsageFetcherRetryMappingTests (8 passed)
  • swift build clean

@clawsweeper

clawsweeper Bot commented Jul 6, 2026

Copy link
Copy Markdown

Codex review: needs maintainer review before merge. Reviewed July 6, 2026, 8:30 PM ET / 00:30 UTC.

Summary
The branch adds an Ollama /api/web_search validation POST before fetching /api/tags, with focused retry-mapping tests and provider documentation updates.

Reproducibility: yes. from source: current main sends the bearer token to public /api/tags and treats HTTP 200 as API-key success. I did not run a live provider probe because repository policy discourages unrequested provider probes with real services.

Review metrics: 2 noteworthy metrics.

  • Provider network flow: 1 validation POST added before /api/tags. Every Ollama API-key refresh would now depend on the authenticated endpoint before the existing model catalog fetch runs.
  • PR scope: 3 files changed, +78/-3. The patch is focused but touches provider network behavior, focused tests, and provider documentation.

Merge readiness
Overall: 🦐 gold shrimp
Proof: 🦞 diamond lobster
Patch quality: 🦐 gold shrimp
Result: needs maintainer review before merge.

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

Rank-up moves:

  • [P2] Confirm /api/web_search auth ordering and entitlement semantics, or choose a closer authenticated Ollama API endpoint before merge.

Risk before merge

  • [P1] The new hard preflight can reject existing Ollama API-key users before the previous catalog fetch if /api/web_search authorization, entitlement, or availability differs from general Ollama Cloud API-key access.
  • [P1] The validator treats 400/422 from a required-query endpoint as key-accepted, so maintainers should confirm Ollama checks Bearer auth before request-body validation.

Maintainer options:

  1. Verify the validator contract (recommended)
    Confirm before merge that /api/web_search rejection means the key is invalid for CodexBar-supported Ollama API-key use, not only missing web-search entitlement or malformed request handling.
  2. Switch endpoints if scope differs
    If web-search semantics are not equivalent, revise the PR to validate against a closer authenticated Ollama API endpoint and keep the retry-mapping coverage.
  3. Defer the auth change
    Pause or close this branch if maintainers need provider documentation or valid-key live proof before changing the refresh path for existing users.

Next step before merge

  • [P2] Maintainers need to accept or redirect the Ollama validation endpoint; no narrow automation repair is safe until that provider contract choice is made.

Maintainer decision needed

  • Question: Is /api/web_search with 200/400/422 acceptance a compatibility-safe validator for every Ollama API key CodexBar supports?
  • Rationale: The patch fixes the public-catalog validation gap, but it changes existing API-key refresh behavior based on endpoint authorization and auth-before-body ordering that local tests cannot prove.
  • Likely owner: steipete — steipete introduced the Ollama API-key path and is the clearest owner for accepting or redirecting this provider compatibility tradeoff.
  • Options:
    • Verify endpoint contract (recommended): Merge after maintainer verification that valid supported keys pass /api/web_search and invalid keys cannot pass through request-validation ordering.
    • Switch to a closer probe: Update the PR to validate against an authenticated Ollama endpoint whose scope matches CodexBar's API-key mode more directly.
    • Pause until documented: Hold the PR until Ollama documentation or maintainer live proof establishes which endpoint is safe for credential validation.

Security
Cleared: The diff sends the existing Ollama bearer token to another HTTPS endpoint on ollama.com and does not change dependencies, scripts, permissions, storage, or secret persistence.

Review details

Best possible solution:

Use an authenticated Ollama validator whose auth scope and status semantics maintainers accept as equivalent to CodexBar API-key mode, keeping the focused tests and docs.

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

Yes from source: current main sends the bearer token to public /api/tags and treats HTTP 200 as API-key success. I did not run a live provider probe because repository policy discourages unrequested provider probes with real services.

Is this the best way to solve the issue?

Unclear. Moving validation off the public catalog is the right narrow shape, but /api/web_search plus 400/422 acceptance needs maintainer confirmation before it is the safest credential validator.

AGENTS.md: found and applied where relevant.

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

Label changes

Label justifications:

  • P2: The PR fixes bounded Ollama API-key validation behavior without evidence of data loss, security bypass, crash loop, or core runtime outage.
  • merge-risk: 🚨 compatibility: The new /api/web_search preflight can fail existing API-key refreshes if that endpoint's auth scope or availability differs from the previous catalog path.
  • rating: 🦐 gold shrimp: Overall readiness is 🦐 gold shrimp; proof is 🦞 diamond lobster and patch quality is 🦐 gold shrimp.
  • status: 👀 ready for maintainer look: ClawSweeper has no concrete contributor-facing blocker left for this PR. Sufficient (terminal): Sufficient terminal proof is present for the after-fix invalid-key path; valid-key endpoint compatibility remains a maintainer decision rather than a contributor proof gap.
  • proof: sufficient: Contributor real behavior proof is sufficient. Sufficient terminal proof is present for the after-fix invalid-key path; valid-key endpoint compatibility remains a maintainer decision rather than a contributor proof gap.
Evidence reviewed

What I checked:

Likely related people:

  • steipete: Commit 9857e1d added Ollama API-key support across the fetcher, focused tests, and docs touched by this PR. (role: introduced API-key behavior and likely follow-up owner; confidence: high; commits: 9857e1de0bae; files: Sources/CodexBarCore/Providers/Ollama/OllamaUsageFetcher.swift, Tests/CodexBarTests/OllamaUsageFetcherRetryMappingTests.swift, docs/ollama.md)
  • joeVenner: Current main includes this contributor's merged Ollama auth/error follow-ups in the same provider area, beyond this open PR. (role: recent adjacent Ollama contributor; confidence: medium; commits: 093ed829a554, c5066c88c999; files: Sources/CodexBarCore/Providers/Ollama/OllamaUsageFetcher.swift, Tests/CodexBarTests/OllamaUsageFetcherRetryMappingTests.swift, docs/ollama.md)
  • Ratul Sarna: Recent Ollama history includes retry-mapping and auth-like parse failure work in the same provider and test area. (role: adjacent retry/auth contributor; confidence: medium; commits: 565ddca85a66, 32ffc9fd9e4b, b58c62e0f875; files: Sources/CodexBarCore/Providers/Ollama/OllamaUsageFetcher.swift, Tests/CodexBarTests/OllamaUsageFetcherRetryMappingTests.swift)
  • CryptoSageSnr: Commit 27c1bb9 introduced the original Ollama provider implementation and docs before API-key support was added later. (role: original provider introducer; confidence: medium; commits: 27c1bb95770b; files: Sources/CodexBarCore/Providers/Ollama/OllamaUsageFetcher.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 (9 earlier review cycles; latest 8 shown)
  • reviewed 2026-07-06T14:14:39.104Z sha 23a493a :: needs real behavior proof before merge. :: none
  • reviewed 2026-07-06T16:57:12.214Z sha 8373dd3 :: needs maintainer review before merge. :: none
  • reviewed 2026-07-06T19:04:03.944Z sha c3ba4b2 :: needs maintainer review before merge. :: none
  • 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

@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.

The committed proof image was a 1x1 placeholder, not real evidence.
Executable test output in the PR description replaces it.
@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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

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: 🦐 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.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant