Skip to content

Point Ollama login recovery to the current sign-in page#1941

Merged
steipete merged 4 commits into
steipete:mainfrom
joeVenner:fix/ollama-case-insensitive-labels
Jul 6, 2026
Merged

Point Ollama login recovery to the current sign-in page#1941
steipete merged 4 commits into
steipete:mainfrom
joeVenner:fix/ollama-case-insensitive-labels

Conversation

@joeVenner

@joeVenner joeVenner commented Jul 6, 2026

Copy link
Copy Markdown
Contributor

Summary

  • Point Ollama signed-out and missing-session recovery at https://ollama.com/signin instead of the protected settings page.
  • Give expired session cookies the same direct recovery URL.
  • Centralize the URL so all three session-auth errors stay consistent.
  • Update troubleshooting docs and add a maintainer changelog entry with @joeVenner credit.

Live proof

The contributor observed this signed-out response on 2026-07-06:

GET https://ollama.com/settings
HTTP/2 303
location: /signin

Maintainer re-verification on 2026-07-06 opened both https://ollama.com/settings and https://ollama.com/signin; each reached Ollama's live sign-in service and rendered the Sign in page. No browser cookie or Keychain access was used.

Implementation

OllamaUsageError.notLoggedIn, .invalidCredentials, and .noSessionCookie now share one canonical sign-in URL. The focused test locks all three error descriptions, while parser coverage confirms signed-out page classification still reaches the same error path.

Validation

  • swift test --filter OllamaUsageFetcherTests — 19 tests passed
  • swift test --filter OllamaUsageParserTests — 9 tests passed
  • make check — passed with 0 format or lint violations
  • make test — all 49 current groups passed
  • Final aggregate autoreview — no accepted/actionable findings
  • Live redirect target re-verified on 2026-07-06

Risk

Very low: error recovery copy, focused tests, docs, and changelog only. Dependencies and credential handling are unchanged.

@clawsweeper

clawsweeper Bot commented Jul 6, 2026

Copy link
Copy Markdown

Codex review: needs changes before merge. Reviewed July 6, 2026, 7:20 PM ET / 23:20 UTC.

Summary
The PR changes Ollama session-auth error descriptions and troubleshooting docs to point at https://ollama.com/signin, adds focused Swift coverage, and adds a CHANGELOG entry.

Reproducibility: yes. Current main still contains stale Ollama recovery copy, and a read-only live HEAD request shows the settings URL redirects to /signin.

Review metrics: 2 noteworthy metrics.

  • Changed files: 4 files: 2 Swift, 1 docs, 1 changelog. The behavior change is narrow, but one touched file is release-owned and remains the blocker.
  • Blocking file: 1 release-owned file. The current head still adds a CHANGELOG.md entry even though the code, test, and docs changes are otherwise focused.

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:

  • Remove the added CHANGELOG.md line only.

Risk before merge

  • [P1] Merging as-is would put this unreleased contributor-facing note into the changelog source used by the release process and Sparkle/GitHub release notes.

Maintainer options:

  1. Decide the mitigation before merge
    Merge the Ollama source, test, and docs recovery-guidance changes after dropping the added CHANGELOG.md line and keeping release-note context in the PR body or commits.
  2. Pause or close
    Do not merge this PR until maintainers decide whether the risk is worth taking.

Next step before merge

  • [P2] A repair lane can remove the single release-owned changelog line without changing the proven Ollama source, test, or docs fix.

Security
Cleared: No concrete security or supply-chain concern was found; the diff changes Ollama recovery strings, tests, docs, and a changelog line only.

Review findings

  • [P2] Remove the release-owned changelog entry — CHANGELOG.md:13
Review details

Best possible solution:

Merge the Ollama source, test, and docs recovery-guidance changes after dropping the added CHANGELOG.md line and keeping release-note context in the PR body or commits.

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

Yes. Current main still contains stale Ollama recovery copy, and a read-only live HEAD request shows the settings URL redirects to /signin.

Is this the best way to solve the issue?

Yes for the intended Ollama fix, but not for the branch exactly as submitted. Updating the recovery strings, docs, and focused test is the narrow path; the release-owned changelog edit should be dropped before merge.

Full review comments:

  • [P2] Remove the release-owned changelog entry — CHANGELOG.md:13
    CHANGELOG.md feeds release/appcast notes during the release flow, and normal PR release-note context should stay in the PR body or commits. The current head still adds an Ollama bullet, which repeats the prior ClawSweeper blocker; remove this line before merge.
    Confidence: 0.92

Overall correctness: patch is incorrect
Overall confidence: 0.92

AGENTS.md: found and applied where relevant.

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

Label changes

Label justifications:

  • P2: This is a bounded Ollama provider UX/docs correction with limited blast radius and one narrow merge blocker.
  • rating: 🦐 gold shrimp: Overall readiness is 🦐 gold shrimp; proof is 🦞 diamond lobster and patch quality is 🦐 gold shrimp.
  • status: ⏳ waiting on author: ClawSweeper has contributor-facing work open and is waiting for author action. Sufficient (live_output): The PR body includes redacted live HTTP redirect output and maintainer re-verification that the settings and sign-in URLs reach Ollama's live sign-in service.
  • proof: sufficient: Contributor real behavior proof is sufficient. The PR body includes redacted live HTTP redirect output and maintainer re-verification that the settings and sign-in URLs reach Ollama's live sign-in service.
Evidence reviewed

Acceptance criteria:

  • [P1] git diff --check main...HEAD.
  • [P1] swift test --disable-sandbox --filter OllamaUsageFetcherTests.
  • [P1] make check.

What I checked:

  • AGENTS.md read: Repository policy was read fully; its release-process pointer, focused SwiftPM test guidance, and no-Keychain-probe guidance shaped the review. (AGENTS.md:1, 093ed829a554)
  • Current main still has stale error recovery text: Current main still tells signed-out Ollama users to use ollama.com/settings or generic ollama.com, so the PR is not obsolete. (Sources/CodexBarCore/Providers/Ollama/OllamaUsageFetcher.swift:46, 093ed829a554)
  • Current docs still point recovery at settings: Current main's Ollama troubleshooting still directs missing or expired session recovery to the settings page. (docs/ollama.md:51, 093ed829a554)
  • Live redirect supports the new target: A read-only HEAD request to the current settings URL returned HTTP 303 with location: /signin; the sign-in URL then redirects into the WorkOS hosted authorization flow.
  • PR implements the intended source/test/docs fix: The PR head centralizes https://ollama.com/signin in OllamaUsageError, adds coverage for all three session-auth descriptions, and updates the Ollama troubleshooting page. (Sources/CodexBarCore/Providers/Ollama/OllamaUsageFetcher.swift:33, 7ff8373dc489)
  • Release flow owns changelog-derived release notes: The release guide says releases bump CHANGELOG and extract the current changelog section into GitHub/Sparkle release notes, which makes ordinary PR edits to CHANGELOG merge-sensitive. (docs/RELEASING.md:16, 093ed829a554)

Likely related people:

  • steipete: Blame points the current-main Ollama recovery strings/docs to the v0.40.0 release commit, and the release process/changelog area is predominantly in this history. (role: recent area contributor and release-flow owner; confidence: high; commits: a3454640cf53, 9857e1de0bae, 7ff8373dc489; files: Sources/CodexBarCore/Providers/Ollama/OllamaUsageFetcher.swift, docs/ollama.md, CHANGELOG.md)
  • ratulsarna: Git history shows repeated work on Ollama cookie fallback, auth retry, diagnostics, and parse-failure handling in the same provider/test area. (role: Ollama cookie and auth hardening contributor; confidence: high; commits: 32ffc9fd9e4b, b58c62e0f875, c7bdbc670cf7; files: Sources/CodexBarCore/Providers/Ollama/OllamaUsageFetcher.swift, Tests/CodexBarTests/OllamaUsageFetcherTests.swift, docs/ollama.md)
  • CryptoSageSnr: Commit history shows the original Ollama provider support added the affected fetcher, tests, and docs surface. (role: introduced Ollama provider; confidence: medium; commits: 27c1bb95770b; files: Sources/CodexBarCore/Providers/Ollama/OllamaUsageFetcher.swift, Tests/CodexBarTests/OllamaUsageFetcherTests.swift, docs/ollama.md)
  • joeVenner: Current main includes adjacent merged Ollama error/docs work from this contributor, separate from their authorship of this PR. (role: recent adjacent contributor; confidence: medium; commits: 093ed829a554; 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 (5 earlier review cycles)
  • reviewed 2026-07-06T14:02:45.094Z sha 1e3bc4b :: needs real behavior proof before merge. :: none
  • reviewed 2026-07-06T14:08:08.343Z sha 1e3bc4b :: needs real behavior proof before merge. :: none
  • reviewed 2026-07-06T16:59:58.762Z sha 4a4469d :: needs changes before merge. :: [P3] Remove the release-owned changelog entry
  • reviewed 2026-07-06T21:09:19.022Z sha 59886a7 :: needs changes before merge. :: [P2] Remove the release-owned changelog entry
  • reviewed 2026-07-06T21:43:14.658Z sha 6cc673a :: needs changes before merge. :: [P2] Remove the release-owned changelog entry

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

@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 joeVenner force-pushed the fix/ollama-case-insensitive-labels branch from 1e3bc4b to 4a4469d Compare July 6, 2026 16:46
@joeVenner joeVenner changed the title Handle Ollama usage labels case insensitively Point Ollama login recovery to the current sign-in page 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.

@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. 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 joeVenner force-pushed the fix/ollama-case-insensitive-labels branch from 4a4469d to 59886a7 Compare July 6, 2026 21:03
@clawsweeper clawsweeper Bot added status: ⏳ waiting on author ClawSweeper has contributor-facing work open and is waiting for author action. and removed status: 👀 ready for maintainer look ClawSweeper has no concrete contributor-facing blocker left for this PR. labels Jul 6, 2026
@joeVenner joeVenner force-pushed the fix/ollama-case-insensitive-labels branch from 59886a7 to 6cc673a Compare July 6, 2026 21:33
@steipete steipete force-pushed the fix/ollama-case-insensitive-labels branch from 6cc673a to 7ff8373 Compare July 6, 2026 23:16
@steipete

steipete commented Jul 6, 2026

Copy link
Copy Markdown
Owner

Maintainer proof for exact head 7ff8373dc4898d84823d7ed62ff126405f05a13f:

  • Live re-verification on 2026-07-06: both https://ollama.com/settings and https://ollama.com/signin reached Ollama's sign-in service and rendered the Sign in page; no browser cookie or Keychain access was used.
  • Deep review extended the contributor fix to the expired-cookie path and centralized one recovery URL across notLoggedIn, invalidCredentials, and noSessionCookie.
  • Public Model Identifier Gate: PASS.
  • Scope: 4 files, +15/-5; maintainer changelog entry credits @joeVenner; dependency manifests/locks unchanged.
  • Focused proof: OllamaUsageFetcherTests 19/19; OllamaUsageParserTests 9/9.
  • Full local proof: make check passed; make test passed all 49 current groups; aggregate autoreview reported no accepted/actionable findings.
  • Exact-head CI: change detection, lint, GitGuardian, Linux arm64, and Linux x64 are green. Four hosted macOS shards remain capacity-queued; landing uses the established capacity-only policy backed by the exact-head local 49-group pass and live redirect proof above.

Ready to merge this exact SHA.

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

Labels

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: ⏳ waiting on author ClawSweeper has contributor-facing work open and is waiting for author action.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants