Skip to content

fix(providers): register Atomic Chat as local OpenAI-compatible runtime - #550

Open
yanalialiuk wants to merge 10 commits into
Gitlawb:mainfrom
yanalialiuk:fix/atomic-chat-local-provider
Open

fix(providers): register Atomic Chat as local OpenAI-compatible runtime#550
yanalialiuk wants to merge 10 commits into
Gitlawb:mainfrom
yanalialiuk:fix/atomic-chat-local-provider

Conversation

@yanalialiuk

@yanalialiuk yanalialiuk commented Jul 6, 2026

Copy link
Copy Markdown

Summary

  • Register Atomic Chat as a local provider (Local: true, no API key) instead of a remote OpenAI-compat endpoint at api.atomic.chat.
  • Default base URL: http://127.0.0.1:1337/v1 (matches Atomic Chat's local OpenAI-compatible server).
  • Include Atomic Chat in zero providers detect / first-run local runtime probing alongside Ollama and LM Studio.

Why

Atomic Chat is a desktop local LLM runtime with an OpenAI-compatible API on port 1337. The catalog previously pointed at a cloud URL and required ATOMIC_CHAT_API_KEY, which blocked the keyless local workflow Zero already supports for Ollama/LM Studio.

Test plan

  • go test ./internal/providercatalog/... ./internal/provideronboarding/...
  • Start Atomic Chat with a loaded model
  • Run zero providers detect — expect Atomic Chat on 127.0.0.1:1337
  • zero providers add atomic-chat --set-active and chat in the TUI

Made with Cursor

Summary by CodeRabbit

  • New Features
    • Added Atomic Chat as a supported local provider, including an alias for “atomic chat”.
    • Local runtime detection now probes Atomic Chat alongside existing local runtimes using the default OpenAI-compatible endpoint.
    • Atomic Chat local runtime does not require an API key (hosted Atomic Chat still does).
  • Documentation
    • Updated setup guides and provider detection/help text to include Atomic Chat, requiring the local API be reachable (default http://127.0.0.1:1337/v1).
  • Tests
    • Updated expectations to cover the new local provider, ordering/alias resolution, and credential behavior.

Parent issue: #592

@coderabbitai

coderabbitai Bot commented Jul 6, 2026

Copy link
Copy Markdown

Review Change Stack

Note

Reviews paused

It looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the reviews.auto_review.auto_pause_after_reviewed_commits setting.

Use the following commands to manage reviews:

  • @coderabbitai resume to resume automatic reviews.
  • @coderabbitai review to trigger a single review.

Use the checkboxes below for quick actions:

  • ▶️ Resume reviews
  • 🔍 Trigger review

Walkthrough

Atomic Chat is reclassified from a remote authenticated provider to a local keyless runtime at port 1337. Catalog models, credential checks, runtime detection tests, CLI descriptions, and English and Chinese README setup instructions are updated.

Changes

Atomic Chat local provider

Layer / File(s) Summary
Provider catalog and credential migration
internal/providercatalog/catalog.go, internal/providercatalog/catalog_test.go, internal/providermodelcatalog/catalog.go, internal/config/credentials_test.go
Atomic Chat now uses a local OpenAI-compatible endpoint, requires no authentication, resolves the local alias, exposes local-model, and retains separate hosted credential coverage.
Local runtime detection coverage
internal/provideronboarding/localruntime_test.go
Local runtime tests verify Atomic Chat detection at port 1337 without an API key and exclude the hosted preset.
CLI and setup guidance
internal/cli/*.go, README.md, README_ZH.md
Detection help, comments, provider listings, and first-run instructions now include Atomic Chat and its local API endpoint.

Estimated code review effort: 2 (Simple) | ~10 minutes

Possibly related issues

  • Gitlawb/zero#592 — Covers Atomic Chat local, keyless runtime support across catalogs, detection, model discovery, and documentation.

Possibly related PRs

  • Gitlawb/zero#141 — Introduces the provider-catalog framework extended by this change.
  • Gitlawb/zero#238 — Adds the provider-detection command and wiring updated here.

Suggested reviewers: gnanam1990, anandh8x, vasanthdev2004

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title clearly matches the main change: registering Atomic Chat as a local OpenAI-compatible runtime.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests

Comment @coderabbitai help to get the list of available commands.

coderabbitai[bot]
coderabbitai Bot previously approved these changes Jul 6, 2026

@jatmn jatmn left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

I found a few issues that need to be addressed before this is ready.

Findings

  • [P1] Rebase on main and resolve the merge conflicts first
    GitHub mergeability
    GitHub currently reports this branch as CONFLICTING with mergeStateStatus: DIRTY, so the PR cannot be merged as-is. Please rebase on the latest main branch and resolve the conflicts before addressing the review findings below, so the final fixes apply to the code that will actually be merged.

  • [P2] Link an approved parent issue before continuing this community PR
    CONTRIBUTING.md:45
    The contribution policy requires community pull requests to be tied to an existing issue that has already been reviewed and approved by the core team, and the PR body does not link any approved parent issue. Please link the approved issue for the Atomic Chat provider change, or open one and wait for the issue-approved label before continuing this implementation work.

  • [P3] Update the providers help text for Atomic Chat detection
    internal/cli/command_center.go:474
    The README now tells users that zero providers detect can find Atomic Chat, and the catalog change makes LocalRuntimeCandidates probe it from the Local descriptor, but the user-facing providers help still says detection only probes “Ollama, LM Studio”. Please update that help text so zero providers --help does not contradict the new supported local runtime.

@Vasanthdev2004 Vasanthdev2004 left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

I read the provider-catalog and README changes. Moving the atomic-chat entry from a remote, key-required endpoint to a local, keyless one is fine mechanically and the catalog/onboarding tests are updated consistently. Two things to sort out before merge:

  1. The README now lists Atomic Chat alongside Ollama and LM Studio under "local models, run ..." and links to the hosted site. Ollama and LM Studio are servers the user runs themselves; the wording "run ... Atomic Chat" reads as if it's the same kind of self-hosted local server, but the link points to a hosted product. I couldn't tell from the diff what 127.0.0.1:1337 is meant to be — a local proxy the user runs, a desktop app, or a misregistered remote. Please clarify in the README what that local endpoint actually is and what the user has to run to get it, so people aren't told to "run" a hosted service locally.

  2. This PR is conflicting with main. Please rebase onto latest main. Heads up that main also has an unrelated test-build break right now (PR #589 is the one-line fix, queued for merge); once #589 lands and you rebase, Smoke should go green.

Requesting changes for the doc clarification + rebase.

@yanalialiuk

Copy link
Copy Markdown
Author

Addressed review requested items:

Re-requesting a fresh review once checks finish.

@yanalialiuk

Copy link
Copy Markdown
Author

Addressed review requested items:

Re-requesting a fresh review once checks finish.

@jatmn jatmn left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Thanks for the update. I rechecked the changed paths and found issues that still need to be addressed.

Findings

  • [P1] Remove the duplicate curated-model map entries
    internal/providermodelcatalog/catalog.go:159
    The current head adds opencode-go-anthropic-compatible to the curatedModels map multiple times in the same literal. Go rejects duplicate keys in map literals at compile time, so this branch will not build or run tests until those extra entries are removed and the map has a single entry for that provider.

  • [P2] Restore the OpenCode Go Anthropic-compatible provider descriptor
    internal/providercatalog/catalog.go:151
    The branch sync deleted the opencode-go-anthropic-compatible descriptor from the provider catalog and also removed it from the catalog-order tests, while the model catalog, allowlist, provider factory tests, and provider-model filtering code still reference that catalog ID. After the duplicate-key compile error is fixed, this would still regress an existing provider by making it unavailable through the catalog. Please restore the descriptor and the expected Anthropic-compatible catalog ordering while keeping the Atomic Chat move separate.

@yanalialiuk
yanalialiuk force-pushed the fix/atomic-chat-local-provider branch from 5e99853 to d0516c7 Compare July 8, 2026 19:26

@jatmn jatmn left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Thanks for the update. I rechecked the changed paths and found issues that still need to be addressed.

Findings

  • [P2] Get the linked issue approved before continuing this community PR
    CONTRIBUTING.md:45
    The PR now links #592, but that issue currently has no issue-approved label. The contribution policy requires community PRs to be tied to an existing issue that has already been reviewed and approved by the core team before implementation continues. Please wait for the linked Atomic Chat issue to receive issue-approved or get maintainer confirmation that this PR is exempt from that policy.

  • [P3] Keep the Chinese README in sync with the Atomic Chat setup docs
    README_ZH.md:25
    The English README now advertises Atomic Chat in the provider list and explains that local-model users can run the Atomic Chat desktop app with its local API on 127.0.0.1:1337, but the parallel Chinese README still only lists Ollama/LM Studio and tells local-model users to run only those two runtimes. Please update README_ZH.md as well so localized setup guidance does not omit the new local provider.

@Vasanthdev2004 Vasanthdev2004 left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

I reviewed the Atomic Chat registration: it moves atomic-chat from the remote, key-required endpoint to the same local OpenAI-compatible pattern as Ollama and LM Studio (127.0.0.1:1337, no key), and LocalRuntimeCandidates picks it up automatically. I checked the catalog and provider-construction code for partner-attribution headers — the descriptor sets none and only user-set profile headers are forwarded, so this is the clean forwarding pattern, not attribution or phone-home. Build, vet, and the provider catalog/model/onboarding tests all pass, and it merges cleanly against current main.

One thing before I approve: the parent issue #592 is still open with no maintainer sign-off, so I'd want an owner call there first. The bundled Go toolchain bump is also redundant now that main is on 1.26.5, but harmless.

@Vasanthdev2004

Copy link
Copy Markdown
Collaborator

@gnanam1990 #550 re-registers Atomic Chat as a local OpenAI-compatible runtime (127.0.0.1:1337, no key) like Ollama/LM Studio. Clean on attribution headers (none injected, only user-set forwarded). I held off approving only because the parent issue #592 is still open with no maintainer sign-off — flagging that as an owner call.

@kevincodex1

Copy link
Copy Markdown
Member

@yanalialiuk please rebase to main and fix conflicts

@yanalialiuk
yanalialiuk force-pushed the fix/atomic-chat-local-provider branch from 603c106 to d2332ea Compare July 22, 2026 10:53
coderabbitai[bot]
coderabbitai Bot previously approved these changes Jul 22, 2026
coderabbitai[bot]
coderabbitai Bot previously approved these changes Jul 22, 2026
@yanalialiuk

Copy link
Copy Markdown
Author

@yanalialiuk please rebase to main and fix conflicts

Done 🙌

kevincodex1
kevincodex1 previously approved these changes Jul 22, 2026

@kevincodex1 kevincodex1 left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

LGTM

@kevincodex1

Copy link
Copy Markdown
Member

please have a look guys @Vasanthdev2004 @jatmn

Vasanthdev2004
Vasanthdev2004 previously approved these changes Jul 22, 2026

@Vasanthdev2004 Vasanthdev2004 left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Approving on 143c0fa. The rework is clean and every earlier concern is resolved.

atomic-chat registers as a local, keyless OpenAI-compatible runtime (127.0.0.1:1337/v1), the same pattern as Ollama and LM Studio, and LocalRuntimeCandidates picks it up automatically. I re-checked for attribution or phone-home headers: the descriptor sets none, only user-set profile headers are forwarded, so this is the clean forwarding pattern. README and README_ZH are now synced, and the earlier redundant Go toolchain bump is gone.

Built and ran internal/providercatalog and internal/provideronboarding locally: gofmt and build clean, tests pass, and CI is green on all three OSes.

I approved the parent issue #592 to clear the contribution gate. Thanks yanalialiuk, clean addition. LGTM.

@jatmn jatmn left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Thanks for the update. I rechecked the changed paths and found issues that still need to be addressed.

Findings

  • [P1] Configure Atomic Chat with a model returned by its local server
    internal/providercatalog/catalog.go:118
    The new descriptor defaults to the literal local-model, but the advertised zero providers detect adoption command drops the actual IDs it just read from /v1/models and invokes zero providers add atomic-chat --set-active without --model. The add path persists that placeholder and forwards it unchanged in the next completion request, whereas Atomic Chat requires the ID of a loaded model. Consequently, a user who follows the documented detect/add flow gets a reachable provider that immediately fails with an unknown-model response. Carry a detected model through the adoption flow (or require an explicit model) and cover a server whose returned ID is not local-model.

  • [P2] Preserve credential validation for existing Atomic Chat profiles
    internal/providercatalog/catalog.go:118
    This repurposes the existing atomic-chat catalog ID from the authenticated https://api.atomic.chat/v1 preset to a keyless local one, but stored profiles retain their explicit remote base URL and ATOMIC_CHAT_API_KEY setting. Credential readiness now consults the repurposed descriptor and treats those profiles as no-auth, so an unset key is reported as ready and only fails later when the client calls the remote endpoint unauthenticated. Migrate/retire the old profile identity, or keep authentication requirements conditional on its saved endpoint, with regression coverage for a preexisting remote profile.

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

Actionable comments posted: 2

🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@internal/providermodelcatalog/catalog.go`:
- Around line 205-207: Remove the "atomic-chat-local" entry from curatedModels
so Models() falls through to registryModels(provider) and discovers the server’s
actual models via GET /v1/models. Do not retain the local-model placeholder
unless it is an explicitly supported server contract with an accurate
description.

In `@internal/provideronboarding/localruntime_test.go`:
- Around line 42-50: Strengthen the atomic-chat-local assertions in the test by
comparing atomicChat.BaseURL exactly with "http://127.0.0.1:1337/v1" instead of
checking for the port substring, and assert that atomicChat.DefaultModel equals
"local-model". Preserve the existing RequiresKey assertion and candidate lookup.
🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

  • Push a commit to this branch (recommended)
  • Create a new PR with the fixes

ℹ️ Review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

Run ID: 8af04f1f-c49f-4190-bdf2-ed7d835a2f21

📥 Commits

Reviewing files that changed from the base of the PR and between 143c0fa and 9fe7377.

📒 Files selected for processing (5)
  • internal/config/credentials_test.go
  • internal/providercatalog/catalog.go
  • internal/providercatalog/catalog_test.go
  • internal/providermodelcatalog/catalog.go
  • internal/provideronboarding/localruntime_test.go

Comment thread internal/providermodelcatalog/catalog.go Outdated
Comment thread internal/provideronboarding/localruntime_test.go
coderabbitai[bot]
coderabbitai Bot previously approved these changes Jul 22, 2026

@jatmn jatmn left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

I found issues that need to be addressed before this is ready.

Findings

  • [P1] Shell-escape the discovered model before printing the adopt command
    internal/provideronboarding/localruntime.go:105
    The selected ID comes directly from the local runtime's /v1/models response, then SetupCommandWithModel ultimately renders it with strconv.Quote. That produces double quotes, so a response containing {"id":"$(command)"} is emitted as --model "$(command)"; pasting the advertised zero providers detect command into a POSIX shell executes the substitution before Zero receives the model argument. This new model-pinning path needs POSIX-safe quoting (or a non-shell command representation) and regression coverage for command substitution and backticks.

  • [P1] Do not offer Atomic Chat setup paths that save the known-invalid placeholder model
    internal/providercatalog/catalog.go:118
    local-model is explicitly only a placeholder for Atomic Chat, yet the catalog output advertises zero providers setup atomic-chat-local --set-active. That command, and direct providers add, omit --model; providerProfileForAdd then persists Descriptor.DefaultModel. The resulting profile sends local-model to an Atomic instance that serves its actual loaded ID instead, so its first completion fails. providers detect also falls back to this same broken command after a successful response with no parseable IDs. Require/select a real served model, or suppress these setup actions rather than persisting the placeholder.

@yanalialiuk

Copy link
Copy Markdown
Author

hey @jatmn,

P2 — reverted atomic-chat to the remote authenticated preset it was on main (api.atomic.chat/v1 + ATOMIC_CHAT_API_KEY, RequiresAuth: true). The local runtime is a separate atomic-chat-local ID now, same split as ollama-cloud/ollama, so stored remote profiles keep their key requirement. Regression test in credentials_test.go.

P1AdoptModel() carries the probed model into the adopt command instead of persisting the local-model placeholder. TestSetupActionPinsProbedModel covers a server returning unsloth/Qwen3-Coder-30B-A3B-Instruct-GGUF; also covered: no models served, and a served id matching the catalog default.

gofmt/build clean, tests green locally. CI on these commits is waiting on workflow approval

@AlexFromAtomic

Copy link
Copy Markdown

hey @jatmn, both P1s fixed:

  • shell-escape: commandArg now POSIX single-quotes args instead of strconv.Quote, so a /v1/models id can't expand when the adopt command is pasted. Added regression tests ($(...), backticks, ;, spaces).
  • placeholder model: providerProfileForAdd now errors when atomic-chat-local is added without --model instead of saving local-model. Kept scoped to atomic-chat-local, lmstudio/ollama unchanged.

Verified against a running Atomic Chat: detect -> add --model -> zero exec works, bare add errors now, a $(...) id renders single-quoted. build/vet/gofmt/tests (incl. -race) green.

@jatmn jatmn left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

I found issues that need to be addressed before this is ready.

Findings

  • [P1] Keep generated adopt commands safe on Windows
    internal/provideronboarding/advice.go:189
    SetupAction now puts the model ID returned by the local /v1/models endpoint into a command rendered with POSIX single quotes. Windows cmd.exe treats those quotes as literal characters, so a reachable local service returning an ID such as x & calc & y makes zero providers detect print --model 'x & calc & y'; pasting it executes the &-separated commands instead of passing one model argument. This also breaks the normal detected names containing spaces on Windows. Keep the untrusted value safely represented for each supported shell/platform (or avoid emitting a pasteable shell command), rather than globally replacing the prior cross-platform command rendering with POSIX quoting.

  • [P1] Do not let Atomic Chat onboarding save the known-invalid placeholder model
    internal/cli/provider_setup.go:443
    The new guard rejects only an empty model, while local-model is explicitly the Atomic Chat placeholder that the surrounding comment says always fails at first completion. When live discovery times out, returns malformed data, or returns no IDs, both setup wizards fall back to DefaultModel and pass the nonempty local-model value through this guard, then persist an active Atomic Chat profile that receives an unknown-model error on its first request. The no-model detection and catalog setup actions are also still advertised even though this guard rejects them. Require a real discovered or user-entered model for every Atomic Chat onboarding path, and suppress or replace actions until one is available.

@Vasanthdev2004

Copy link
Copy Markdown
Collaborator

@jatmn heads-up, both of the holds from your last review are cleared now, so this is only waiting on your re-look:

The registration itself is unchanged in shape (local, keyless, 127.0.0.1:1337/v1, no attribution headers) and CI is green on all three OSes. Whenever you get a minute.

@jatmn jatmn left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

I found issues that need to be addressed before this is ready.

Findings

  • [P1] Do not use POSIX escaping for Windows adoption commands
    internal/provideronboarding/advice.go:189
    This PR newly puts the model ID returned by the untrusted local /v1/models endpoint into a copy-paste command, but commandArg only implements POSIX single-quote escaping. Zero supports Windows, where cmd.exe treats single quotes as ordinary characters: a model ID such as x&<command> becomes --model 'x&<command>' and executes the suffix when the advertised zero providers detect action is pasted. Render a command safe for the active Windows shell (including embedded quotes), or do not expose untrusted IDs as a shell command.

  • [P1] Do not let setup persist Atomic Chat's placeholder model
    internal/cli/provider_setup.go:443
    The new guard rejects only an empty model. Both interactive setup paths fall back to providermodelcatalog.Models after a failed or empty live /v1/models discovery; adding this descriptor makes that fallback select its DefaultModel, local-model, and forward it as a non-empty value. The wizard can therefore save and activate Atomic Chat with the exact placeholder this PR says is never served, causing its first completion to fail. Treat the placeholder as invalid too, and require a successfully discovered or manually supplied real model in every setup path.

  • [P2] Do not advertise an adopt command that the add command rejects
    internal/provideronboarding/localruntime.go:103
    A reachable Atomic Chat endpoint with an empty, malformed, or oversized /v1/models response has no parsed IDs, so AdoptModel returns empty and zero providers detect prints an adopt action without --model. The new providerProfileForAdd check deterministically rejects that exact action. TestSetupActionOmitsModelWhenProbeFoundNone explicitly preserves this no-model command shape, so a runtime that is up but has no currently discoverable model gets a non-working onboarding command. Omit or replace the action until a valid model ID is available.

@euxaristia

Copy link
Copy Markdown
Contributor

@coderabbitai full review

@coderabbitai

coderabbitai Bot commented Aug 22, 2026

Copy link
Copy Markdown
⚠️ Action not completed

Review rate limited.


Your included review limit is currently reached under our Fair Usage Limits Policy. This review may still proceed through usage-based billing if eligible. Your next included review will be available in 7 minutes.

@Vasanthdev2004

Copy link
Copy Markdown
Collaborator

Checking in. The Jul 25 review left a P1 about POSIX escaping in the Windows adoption commands, and the branch has not moved since. If you still want this in, a rebase onto current main and that fix are what it needs; I will review promptly. If I do not hear back within two weeks I will close it to keep the board honest, and it can be reopened any time.

yanalialiuk and others added 8 commits September 8, 2026 10:38
Point atomic-chat at the default local API (http://127.0.0.1:1337/v1),
mark it Local with no API key, and auto-detect it alongside Ollama and
LM Studio. Keep opencode-go-anthropic-compatible in the provider catalog
and curated model list after rebasing onto latest main.

Co-authored-by: Cursor <cursoragent@cursor.com>
The English README lists Atomic Chat in the provider list and tells
local-model users to run its desktop app on http://127.0.0.1:1337/v1,
but the Chinese README still mentioned only Ollama and LM Studio.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Repurposing the existing atomic-chat ID from the hosted, key-gated preset
to a keyless local one silently downgraded credential checks for profiles
already saved against the remote endpoint: MissingCredentialEnv resolves
RequiresAuth from the catalog descriptor, so those profiles were reported
as ready and only failed later with an unauthenticated call to
api.atomic.chat.

Keep the hosted atomic-chat descriptor as it was and register the local
runtime under a separate atomic-chat-local ID, mirroring the existing
ollama-cloud/ollama split. Stored profiles keep their identity and no
migration is needed.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…ult model

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Atomic Chat's model catalog is a Hugging Face scrape refreshed every 12h
plus arbitrary owner/repo lookups, so the served model ID is whichever
model the user loaded and no hardcoded list stays accurate.

The single local-model entry short-circuited Models() without adding
anything: the descriptor is openai-compatible, so modelMatchesProvider
never matches a registry entry and the fallback already yields the
descriptor's DefaultModel. Output is unchanged; the difference is that
local-model is no longer presented as a curated contract.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
probeLocalRuntime already reads the runtime's /v1/models, but SetupAction
dropped the result and emitted 'zero providers add <id> --set-active'
with no --model. The add path then persisted the catalog DefaultModel,
which for a local runtime is the placeholder 'local-model', and the first
completion failed with an unknown-model response.

SetupAction now pins the probed id via SetupCommandWithModel. The catalog
default wins when the server actually serves it, so an existing Ollama
setup does not switch models; otherwise the first advertised id is used.
An empty probe result leaves the command on the catalog default rather
than inventing an id.

Covers a server whose returned id is not 'local-model'.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…placeholder model

commandArg now POSIX single-quotes rendered args instead of strconv.Quote, so a model id from an untrusted /v1/models response cannot expand when the adopt command is pasted into a shell.

providerProfileForAdd now errors when atomic-chat-local is added without --model instead of persisting the local-model placeholder that fails the first completion.

Adds regression tests for command substitution, backticks, and the require-model guard.
Rebase on current main and keep the hosted Atomic Chat preset intact.
Only emit setup arguments that are portable across POSIX, cmd.exe and
PowerShell; show interactive guidance for other model IDs. Reject the
Atomic Chat placeholder before both CLI and TUI persistence, and stop
advertising adoption when discovery finds no usable model.

Validation: full Go test suite, focused race suite, vet, release build,
smoke, formatting, advisory lint and vulncheck pass with isolated config
and inherited Anthropic overrides removed. Onboarding, CLI and TUI test
packages cross-compile for Windows and Linux. Regression tests fail on
the rebased pre-fix code and pass with these changes.

Refs Gitlawb#550 and Gitlawb#592.
@AlexFromAtomic
AlexFromAtomic force-pushed the fix/atomic-chat-local-provider branch from 00c54d0 to a2b4945 Compare September 8, 2026 07:51

@jatmn jatmn left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

The earlier model-selection and shell-safety issues are addressed. I found one low-risk correctness edge case; I would treat this as a non-blocking P3.

Findings

[P3] Keep option-looking model IDs usable in adoption commands

internal/provideronboarding/advice.go:46-47

If a local /v1/models response advertises a custom ID such as -loaded-model, AdoptModel() retains it and the new renderer emits:

zero providers add atomic-chat-local --name "Atomic Chat Local" --model -loaded-model --set-active

Zero rejects this command with exit code 2 and --model requires a value. parseProviderAddArgs() passes the separate operand to nextFlagValue(), which rejects nonnumeric values beginning with -. Supplying --model=-loaded-model instead succeeds: the profile saves, reloads, and sends that same model ID in a completion request.

Root cause: the generated argument must satisfy both the shell's syntax and Zero's option grammar. setupCommandArg() allows the characters in this ID, but setupCommand() always places the model after a separate --model token. Shell quoting alone cannot resolve this mismatch: after the shell removes quotes, Zero still receives an option-looking operand. This PR introduces that model-pinning path for every detected local runtime, including existing LM Studio configurations; the base implementation did not emit a model operand.

Suggested correction: handle this at the changed command-generation boundary. Preserve the selected ID in a representation the existing add parser accepts, such as its inline-value form, or return the existing interactive-setup guidance for values that cannot be represented. If using the inline form, account for validation of the model value separately from the flag separator: passing a combined --model=... token unchanged through the current allowlist would reject =. Keep the shell-safety checks intact. This does not require relaxing the CLI's general option checks, changing provider defaults, selecting a different model, or refactoring other command renderers.

Regression coverage: exercise the generated action through the actual add parser, rather than only asserting the rendered string or a shell round trip. Cover an option-looking ID plus ordinary and space-containing IDs. For this edge, verify either that parsing preserves the exact model or that no command is advertised and interactive guidance is shown. Retain the existing unsafe-shell-value and Atomic-placeholder rejection tests.

Evidence boundary: the failure and successful save/reload/completion control were reproduced with the built CLI against a mock OpenAI-compatible server. I have not reproduced this identifier on an actual Atomic Chat installation or established that its standard model IDs use this shape. LM Studio supports custom API identifiers, and its SDK load options accept a string identifier without a leading-hyphen restriction. This supports keeping the finding as a narrow custom-ID correctness issue, with a working inline-value workaround.

Use the existing --model=value grammar when a discovered model begins
with a hyphen. Validate the model separately from the fixed separator so
shell safety remains unchanged and '=' is not added to the allowlist.

Exercise generated detection actions through shell tokenization, the actual
add parser, and profile construction for Atomic Chat, LM Studio and Ollama.
Cover leading hyphens, flag-looking IDs, ordinary IDs, spaces, and unsafe
inline values. Before the fix, the new regression failed with:
  generated adoption command rejected by add parser: --model requires a value

Validation: full Go suite, focused race tests, formatting, vet, release
build, smoke, lint-static and vulncheck passed. Affected test packages
cross-compiled for Windows and Linux.

Refs Gitlawb#550.

@jatmn jatmn left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

The earlier shell-safety, option-looking model ID, hosted-credential, and placeholder-onboarding issues are addressed. The current review has one P2 issue to address before merge and one non-blocking P3 correctness issue in the shared local-model selection path. The detailed guidance below explains those two findings and how to close them without expanding the PR.

Merge readiness

  • At the captured review state, head 344bbb85614f14170eb0e53934dda0ceeefc4794 is based on main at f30f550e6037c53f8521ba2d59c984ae6c574917, with no merge conflicts. CI and PR Auto Review report action_required, and there are no current-head check runs. The successful CodeRabbit status does not establish that CI passed. These workflows need maintainer action, and the branch's approval requirements remain in effect.
  • Parent issue #592 is approved. Please update the PR description/test plan to reflect the final implementation: hosted atomic-chat is preserved, the local preset is atomic-chat-local, and its adoption command needs a usable model. The old bare providers add atomic-chat example exercises the hosted preset.

Findings

[P2] Exclude known non-chat models from automatic adoption

internal/provideronboarding/localruntime.go:144-146

Trigger and failure path. Consider a reachable local server returning:

{"data":[{"id":"text-embedding-local"},{"id":"qwen3-coder-30b"}]}

When neither ID equals the catalog default, AdoptModel() chooses text-embedding-local because it is the first nonempty ID that passes the Atomic-placeholder check. SetupAction() includes that choice in an add command with --set-active. The CLI accepts and persists it; a subsequent process loads the saved profile, and the provider forwards that same ID to /chat/completions. An embedding-only model cannot fulfill that chat request, even though a usable chat model was present later in the list.

Root cause. The new adoption selector treats an advertised model as eligible for chat based only on being nonempty and not Atomic's placeholder. Those checks establish neither chat suitability nor the project's existing eligibility rule. Local chat discovery already excludes known noncoding IDs through IsKnownNonCodingModelID; TestMergeLocalModelsKeepsLiveOnlyEntries explicitly includes text-embedding-local and verifies its exclusion. The new path consumes the raw model list directly and omits that decision before committing a model choice.

The model renderer and add parser correctly preserve the chosen ID. Fixing quoting again, changing the parser, or rejecting arbitrary model IDs globally would address the wrong boundary. The failure begins when automatic adoption chooses a model that existing chat discovery already identifies as unsuitable.

Attribution and evidence limits. This PR newly feeds a server-advertised model into automatic adoption and the existing permissive explicit-model path. Where the catalog default is absent, base already fails on its unavailable default; that fixture alone does not establish newly broken availability. It establishes that the new selector chooses a known non-chat model despite an eligible alternative.

A separate mock-server fixture, [text-embedding-local, llama3.1:latest], demonstrates successful base completion with llama3.1 versus head saving and sending text-embedding-local, followed by a completion failure. That case also involves the default-alias mismatch in the next finding. The mock verifies Zero's selection, persistence, and wire behavior; its rejected embedding response is a simulated server contract, not an observation from an actual Atomic Chat installation.

Smallest correction. Apply the existing known-noncoding exclusion before considering an ID for automatic default preference or first-model fallback. Then choose an eligible advertised model using the existing preference order. If a nonempty response contains only rejected models, provide actionable guidance instead of activating one of them. In particular, ensure that returning an empty selection does not simply flow into an add command that saves the rejected/default model again. The existing commandless Action.Detail surface can express the guidance; the implementation mechanism is your choice.

Keep this correction at automatic adoption and its immediate action fallback. The raw model list may remain available for diagnostics. This does not require probing every model's capabilities, introducing a new model registry, changing the existing noncoding heuristic, or restricting explicitly supplied user model IDs. It also does not require redesigning pre-existing empty/malformed-response handling for other providers.

Regression cases. Cover a known non-chat ID before a chat ID with no matching default; verify that the chat ID is selected. Cover a nonempty list containing only known non-chat IDs; verify that no failing automatic adoption is advertised. Exercise the resulting action through the actual add parser, rather than asserting only a substring in the rendered command. Keep Atomic's existing empty/placeholder rejection tests.

[P3] Recognize Ollama's default tag when preserving its default model

internal/provideronboarding/localruntime.go:149-150

Non-blocking: I would not hold the PR solely for this finding. The independent example below selects a different chat model; it does not demonstrate an outage.

Trigger and observed behavior. Ollama's catalog default is llama3.1. Its API may advertise that model as llama3.1:latest: an omitted tag means latest, and the OpenAI model-list adapter preserves the returned model ID. For this response order:

{"data":[{"id":"qwen3:8b"},{"id":"llama3.1:latest"}]}

base emits no model operand and retains llama3.1. Head compares each ID literally with llama3.1, misses the available default, and emits --model qwen3:8b. A mock-server comparison through save, fresh-process reload, and completion confirms that change in the model sent.

Root cause. The selector uses byte-for-byte equality where Ollama has a documented equivalent spelling. TestAdoptModelPrefersCatalogDefaultWhenServed explicitly intends to preserve the available default so an existing Ollama setup does not silently switch models, but its fixture advertises the untagged string llama3.1. It therefore never exercises the normal tagged spelling that defeats this comparison. Preserving the default is already part of this PR's behavior, rather than a new reviewer preference.

Smallest correction. Recognize the catalog default's equivalent :latest spelling for Ollama before falling back to another eligible model. Preserve the selected advertised ID in the output command; use equivalence for comparison, rather than rewriting all model IDs. Keep the rule specific to Ollama. No global tag stripping, case folding, alias resolution, changed catalog default, or new preferred-model policy is needed.

Regression cases. Put llama3.1:latest after another eligible chat model and verify that it wins. Retain the existing exact-match test and an absent-default case that uses the first eligible model. Include a different explicit tag that must not be treated as :latest, and a non-Ollama case to ensure provider-specific equivalence does not change other runtimes. Preserve the existing command-safety and CLI-parser tests.

The findings have separate minimum outcomes. Filtering known non-chat models does not preserve Ollama's default in the two-chat-model example. Recognizing Ollama's alias does not stop adoption of an embedding model when no default is present.

Guidance for closing this round without expanding the PR

The repeated rounds have involved several boundaries in one onboarding flow. Adding the Atomic preset made model discovery important; carrying the discovered ID into a command then introduced shell and option-parser requirements; rejecting a placeholder required covering both CLI and TUI save paths. The shared selector also affects existing local runtimes, where model eligibility and provider-specific naming matter. Each boundary can work individually while the full flow produces a bad saved profile.

I should also have surfaced the shared selection cases earlier. The previous fixes remain valid. The current requested work is the P2 selection correction and, if you choose to include it, the non-blocking P3 default comparison. The guidance here consolidates the reasoning and validation for these findings; it is not an additional set of findings or a request to redesign provider onboarding.

For the follow-up, trace the changed behavior as one sequence:

model-list response
  -> eligibility for automatic chat adoption
  -> preference for an available default
  -> shell-safe command representation
  -> CLI argument parsing
  -> saved profile and fresh-process reload
  -> model ID sent in the completion request

Keep eligibility, preference, and representation distinct. An ID can be valid for chat but unsuitable for a portable shell command; the existing interactive guidance handles that case. Do not silently choose a different model merely because it is easier to quote. Conversely, a safely rendered ID is not evidence that the selected model is appropriate for chat.

A focused completion checklist is:

Boundary Expected outcome
Known non-chat entry followed by an eligible model Automatic adoption selects the eligible model.
Nonempty response containing only known non-chat entries Actionable guidance; no automatic activation of a rejected model.
Exact served catalog default Existing preference is preserved.
Ollama default advertised with :latest If addressing P3, the equivalent default wins and its advertised spelling is retained.
Default genuinely absent First eligible model is used; an unrelated model is not relabeled as the default.
Eligible ID containing spaces or a leading hyphen Existing rendering/parser behavior preserves the exact ID.
Selected ID requiring unsupported shell quoting Existing interactive guidance is retained; no substitution of another model merely to print a command.
Atomic discovery returns no usable ID or only its placeholder Existing guidance and CLI/TUI pre-persist rejection remain intact.
Hosted atomic-chat profile Its remote identity, endpoint, and credential requirements remain intact.
Saved valid local profile A fresh process preserves the selected model and sends it to the intended local endpoint.

Most of these protections already have tests in this PR. Retain them and add the narrow selection cases above. Use at least one representative generated action through the real add parser and saved-profile reload into a completion request; a mock server is sufficient to assert the exact request model. There is no need to duplicate a full integration test for every row. Show that the new regression tests fail on the unfixed selector for the intended reason and pass with the correction.

Please keep the follow-up focused on the selection decision and any immediately necessary commandless guidance. Preserve the separate hosted/local identities, keyless local behavior, explicit user model selection, shell allowlist, inline handling for option-looking IDs, and the existing Atomic save guards. No dependency changes, generalized capability probing, provider-registry rewrite, global model restrictions, or unrelated test cleanup are requested.

In the updated PR description, identify which finding each change addresses, record the focused validation, and state whether the non-blocking P3 was fixed or deferred. That makes the intended scope and evidence clear for the next review.

Validation and limits

Provider catalog, model catalog, onboarding, config, CLI, focused TUI setup, and adjacent discovery/provider tests passed, as did formatting, affected-package vet, release build, and smoke. Fresh-process completion checks preserved ordinary, space-containing, and leading-hyphen model IDs without authentication headers.

The broader TUI suite fails TestAltScreenTranscriptScrollKeepsFooterFixed; the same base test binary reproduces the identical failure under the same working path. That failure is not attributed to this PR and is not a request for unrelated cleanup.

The selection and completion probes used mock servers. They establish the exact IDs selected, saved, and sent, but do not establish integration behavior on an actual Atomic Chat or Ollama installation. Native Windows shell execution was not tested. Current-head CI still needs the maintainer action described above.

…ults

P2: apply the existing known-noncoding exclusion before default preference
or first-model fallback. When a nonempty list has no eligible model, offer
commandless guidance instead of persisting the rejected/default model.

P3: recognize an Ollama untagged default's :latest spelling, preserving the
advertised ID. Prefer exact matches and leave other tags/providers alone.

Add selector tests plus a mock-server integration test through discovery,
the generated action, the real add parser, profile persistence, a fresh
process, and the chat completion request. Before this fix, that test failed:
  fresh process sent model "text-embedding-local", want eligible model "-loaded chat model"
The Ollama alias case also failed before the fix:
  AdoptModel() = "qwen3:8b", want "llama3.1:latest"

Explicit model choices, shell checks, hosted/local provider identities and
Atomic placeholder guards retain their existing behavior.

Validation: full Go suite, focused race tests, formatting, vet, release
build, smoke, lint-static and vulncheck passed. Affected test packages
cross-compiled for Windows and Linux. Integration evidence uses a mock
server; native Windows shell and actual runtime behavior are not claimed.

Refs Gitlawb#550 and Gitlawb#592.

@jatmn jatmn left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants