Originally created by: adityaharishch
First of six PRs adding a centralised, user-selectable AI provider: the user picks which AI runs their pipeline (their own Claude / Codex / Cursor / Copilot CLI subscription, or the on-device MLX model) once during setup, switchable afterwards from Settings.
This PR is the foundation only - nothing reads the new fields yet. The resolver, the backends, and the wizard step follow in later PRs.
LlmProvider { Claude, Codex, Cursor, Copilot, Local } in meridian-core/src/llm_provider.rs, modelled on the existing canonical_task::Provider. Wire forms deliberately match the coding-agent summariser's Source::as_str(), so the two map without a translation table. Default is Local - on-device is the product's pitch, and it is the only backend guaranteed to be present.
Three settings fields: llm_provider, llm_provider_model, llm_local_chat_model_ready. #[serde(default)] was already on the struct, so every existing settings.json upgrades with no migration.
Validation in update_settings - the only writer. An unrecognised provider would otherwise save cleanly and then quietly resolve back to on-device, so the user would pick Claude and keep getting the local model.
ui/lib/llm-providers.ts - the single source of truth for the UI, deliberately mirroring integrations.ts's TRACKERS. One list, shared by the wizard and Settings later.
llm_provider is stored as a String, not the enum. load_runtime_settings() (settings.rs:185) falls back to RuntimeSettings::default() on any deserialise error, so a single unparseable field silently resets every setting the daemon reads. Had this been typed as the enum, a value written by a newer build would cost an older daemon not just the provider but the user's work hours, poll interval and log level.
It is parsed with LlmProvider::from_wire, which returns None for anything unknown - the caller falls back to the default. An unknown provider costs you the provider, and nothing else. an_unknown_llm_provider_does_not_reset_every_other_setting pins this, and fails loudly if anyone retypes the field.
llm_prefer_local - declared in Rust, defaulted, mirrored in TS, and toggleable with a Save button in Advanced settings... and read by nobody. grep across src/ services/ tray/ meridian-core/ finds only the declaration and the writer. Users can flip this switch today and it does nothing. llm_provider == "local" is what it was trying to say, expressed once and actually honoured.llm_model_preference - TS-only, with a comment claiming it "mirrors RuntimeSettings.llm_model_preference". That Rust field never existed.LlmProvider round-trip: as_str() <-> from_wire() <-> serde, all five variants; unknown string -> None, not a panic.llm_provider loads as local, existing keys survive.MERIDIAN_SETTINGS_PATH, a process-global env var, and cargo runs tests in parallel. With only one such test it never raced; adding two exposed it. They now share a lock.cargo test --workspace (427 passed), cargo clippy -- -D warnings, bun test in ui/ (181 passed), and npm run build are all green.
Originally posted by: coderabbitai[bot]
✨ Finishing Touches
🧪 Generate unit tests (beta)
- [ ] Create PR with unit tests - [ ] Commit unit tests in branch `feat/llm-provider-enum`Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.
❤️ Share
- [X](https://twitter.com/intent/tweet?text=I%20just%20used%20%40coderabbitai%20for%20my%20code%20review%2C%20and%20it%27s%20fantastic%21%20It%27s%20free%20for%20OSS%20and%20offers%20a%20free%20trial%20for%20the%20proprietary%20code.%20Check%20it%20out%3A&url=https%3A//coderabbit.ai) - [Mastodon](https://mastodon.social/share?text=I%20just%20used%20%40coderabbitai%20for%20my%20code%20review%2C%20and%20it%27s%20fantastic%21%20It%27s%20free%20for%20OSS%20and%20offers%20a%20free%20trial%20for%20the%20proprietary%20code.%20Check%20it%20out%3A%20https%3A%2F%2Fcoderabbit.ai) - [Reddit](https://www.reddit.com/submit?title=Great%20tool%20for%20code%20review%20-%20CodeRabbit&text=I%20just%20used%20CodeRabbit%20for%20my%20code%20review%2C%20and%20it%27s%20fantastic%21%20It%27s%20free%20for%20OSS%20and%20offers%20a%20free%20trial%20for%20proprietary%20code.%20Check%20it%20out%3A%20https%3A//coderabbit.ai) - [LinkedIn](https://www.linkedin.com/sharing/share-offsite/?url=https%3A%2F%2Fcoderabbit.ai&mini=true&title=Great%20tool%20for%20code%20review%20-%20CodeRabbit&summary=I%20just%20used%20CodeRabbit%20for%20my%20code%20review%2C%20and%20it%27s%20fantastic%21%20It%27s%20free%20for%20OSS%20and%20offers%20a%20free%20trial%20for%20proprietary%20code)Comment
@coderabbitai helpto get the list of available commands.Originally posted by: adityaharishch
Review: correctness-focused pass (large PR — 12,247 additions across multiple bundled workstreams; reviewed by area)
LLM provider core (
llm_provider.rs,settings.rs,src/llm/*)llm_providerreally is stored as an unvalidatedStringinRuntimeSettings(settings.rs:52),LlmProvider::from_wirereally does degrade gracefully toNone→default on an unrecognized value rather than nuking the whole settings load, andupdate_settings(tray/src-tauri/src/commands/settings.rs:96-108) really does hard-reject an unrecognized provider server-side before writing.llm_prefer_local/llm_model_preferenceare fully removed on both sides (one stale mention survives only in a comment atAdvancedSection.tsx:119— harmless, worth a follow-up cleanup).llm::completeis already called from three live production paths in this PR —src/pm_worklog/generate.rs,src/worklog_pipeline/hour.rs::build_report, andsrc/worklog_pipeline/workstream.rs. The resolver's fallback chain (retry→fallback-to-local, rate-limit backoff) is sound and well-tested, and no panics were found indetect.rs/claude.rs/resolver.rs(proper timeouts,kill_on_drop, no unwraps on external command output) — but reviewers should evaluate this PR knowing the LLM calls are active now, not deferred.Worklog pipeline / day-task generation (
generate.rs,post_comment.rs, migrations 058-060)src/pm_worklog/generate.rsapprove_inner, ~lines 290-307):post_comment(...)succeeds, thenmark_posted(...)persistsstate='posted'. If the process crashes ormark_posteditself errors, the row is stuck atstate='approved'(the error path only setslast_error, it doesn't revert state). The idempotency check at the top ofapprove()(line 197) only short-circuits onstate == "posted", so a retry re-entersapprove_inner, correctly skips re-creating the ticket (guarded bycreated_task_key), but callspost_commentagain unconditionally — andpost_comment.rsdeliberately carries no dedup marker in the comment body (unlikeformat_worklog_comment's⏱/meridian-worklogmarker elsewhere), so there's no way to detect the duplicate after the fact. This breaks the "idempotent + retry-safe" claim in the module docs. Suggest persistingposted_comment_idatomically with the post attempt, or embedding a stable per-(day_local, task_id)marker in the comment body.mark_created(day_task_worklogs.rs~line 240) has noWHERE state = 'approved'guard, unlikemark_approved/mark_posted— not exploitable today (single caller), but a latent trap for a future caller.fetch_open_candidates(generate.rs~419-445) doesLIMIT 30with noORDER BY— SQLite doesn't guarantee row order here, so on boards with >30 open tickets the candidate set fed to the matcher can vary nondeterministically between calls.upsert_draft'sON CONFLICT ... DO UPDATE ... WHERE state = 'drafted'correctly follows the repo's no-DELETE-then-INSERT rule;parse_answernever panics on malformed LLM JSON.Ticket status picker, per provider (
ticket_update/*.rs)jira.rs:684-699,pick_transition_for_choice):transition_status_options()filters requiring bothto.idandto.name;pick_transition_for_choice()separately filters the raw transitions on onlyto.id.is_some(), then.zip()s the two lists. If any transition hasto.idbut missingto.name, the two filtered sequences misalign and.zip()pairs the wrong transition id with the wrong target status —set_statuscould silently POST a transition to a different status than the one requested. Given Jira is the priority-1 provider, recommend building(transition_id, StatusOption)pairs in one pass instead of filtering two lists independently and zipping.azure_devops.rs:364-393,try_patch_state): any non-2xx response (401/403/404/429/500) is treated identically to a legitimate workflow-reject and returned asOk(Some(text))→redirected(...), logged viatracing::info!on what should be an error path. An expired PAT or rate-limit surfaces to the user as "your board can't move it to that status" instead of a real error, and hides credential/config problems from logs/alerting.issueUpdate(stateId)with real errors bubbling asErr).DRM detector / capture (
drm_detector.rs,screenpipe.rs)resolve_url_via_applescriptalways queries the front window of the named app, butcapture_once_ocriterates all visible windows. A background Netflix window that isn't frontmost can get captured anyway (the privacy/legal risk this feature exists to prevent), while a legitimate non-front window can get wrongly skipped if the front window happens to be a streaming URL. Worth gating on window identity, or at minimum documenting this as a known gap.resolve_url_via_applescript→run_osascript→std::process::Command::output()(drm_detector.rs:184-199) is a synchronous blocking call inside the async capture loop, invoked for every frame lacking abrowser_urlon Safari/Chromium-family browsers, not just when streaming is suspected.any_streaming_content_visible()similarly does up to 9pgrepcalls plus AppleScript, once per tick, unconditionally whenpause_on_streamingis on. Recommendspawn_blockingand/or caching across ticks rather than shelling out every tick in a continuously-running background daemon.Commandoutput handled via.ok()?/.unwrap_or(false)throughout).observability/mod.rs/uninstall.rschanges look intentional, no leftover WIP.Given the scope here, I'd suggest splitting this into the six PRs the description originally promised rather than merging as one — at minimum the two hard bugs above (Jira zip-misalignment, worklog duplicate-comment-on-retry) should block merge until fixed.
Ticket changed by: adityaharishch