feat(ai): overhaul the AI subsystem - #79
Conversation
Anthropic had never once produced a commit message: its tool loop sent
`messages: []` on the first iteration, which the Messages API rejects with a
400, and `use_tools` defaults to true so that was the default path. The
shipped default model was a retired id that appeared in no picker, so a fresh
install rendered the Model row with nothing selected at all. The model pill
row had no `flex_wrap()`, so the default window could not render the default
provider's own model list. `get_file_content` correctly rejected `../.env` and
then happily read `.env`. All four are fixed here, along with the structural
problems underneath them.
Correctness
- Seed an opening user turn for every provider, guarded by a test asserting
no provider sends an empty conversation on iteration 1.
- Replace string provider dispatch with an `AiProvider` enum owning endpoint,
auth shape and default model; unknown ids in settings.json are reported
through `load_warnings` instead of failing at click time.
- Make every Gemini response field optional and iterate all parts, so a
safety block, a `MAX_TOKENS` stop, a leading thought part and parallel
function calls stop collapsing into one opaque parse error.
- Bail on a `tool_calls` finish with an empty array rather than burning
iterations to reach a generic message.
- Cap response bodies at 8 MB, move the Gemini key from the query string to
`x-goog-api-key`, and use `from_utf8_lossy` in the diff walker so non-UTF-8
lines stop vanishing from what the model reasons about.
Threading and lifecycle
- Run the whole provider dispatch, tool execution and JSON parsing on the
background executor, reporting tool progress over an mpsc channel that a
small foreground task drains. `execute_tool` spawns `git` and walks
directories synchronously; none of that belongs on the render thread.
- Give every request a 60s deadline and hold its `Task`, so a stalled provider
no longer leaves the spinner running until the app is restarted, and a
cancel button can drop it.
- Carry a `GenerationId { sequence, repo_path }` on every event and route by
repo path rather than `active_tab`, so a message generated for one tab
cannot land in another's commit box.
- Stamp the cooldown on completion rather than dispatch, and report it as
info without clearing an in-flight generation's spinner.
- Skip unchanged keychain writes, debounce the rest by 400ms, and resolve all
providers' keys in one pass.
- Retry 429/5xx honouring `Retry-After`, set an Anthropic cache breakpoint
after the system prompt, lower the diff cap to 40 KB, and give tool output a
per-generation budget.
Security
- Deny `.git/`, a credentials denylist, git-ignored paths and non-UTF-8
content before `get_file_content` reads anything, and re-check the
canonicalised path so an in-repo symlink cannot reach `.git/config`.
Per-provider credentials
- One keychain slot per provider (`ai/provider/{id}`), migrated from the
single `ai/default` slot, with per-provider model pins so switching provider
no longer destroys the previous provider's key or its model choice.
Features
- Live model catalogue with stale-while-revalidate caching, a bundled
fallback, and a generation guard; the picker labels which source it is
showing so "three weeks old" is distinguishable from "shipped with the app".
- OpenRouter, via the existing OpenAI-compatible path widened with an endpoint
struct; the tool-support 404 gets an actionable message.
- `base_url_override` for the OpenAI-compatible family, validated, with an
inline warning naming the host the key will be sent to.
UI
- Provider accordion replacing six flat cards, each row owning its key field,
connection status and model pin, with a connection test that can tell a
working key from a typo.
- `flex_wrap()` on `pill_group` and `overflow_hidden` on `setting_card`.
- The AI button states its own reason when it cannot be used, stays reachable
when the fix is one click away, and shows the shortcut that was registered
all along.
- Tool progress routed to the panel chip, cancel, regenerate with a style
override, and undo of an AI overwrite.
- Errors stop auto-dismissing at 3s and can carry an action; per-level toast
durations replace the single hardcoded one.
- Delete the Save button: text inputs now flush on Enter and on blur, so the
same value no longer persists or vanishes depending on how it arrived.
- `tab_index` across the settings page, which previously had none.
Removals
- `last_result`/`last_error` had no callers; `uuid` was one call site whose id
was never transmitted; `http_client` was never referenced by path.
- `fuzzy_score` is now one shared implementation rather than three.
Not included: streaming tokens into the summary field. It needs per-provider
SSE parsing including tool-call deltas across three wire formats, with no way
to exercise it offline; the progress trace covers the feedback gap meanwhile.
The recommended model ids come from the plan's research and have not been
verified against a live API. A wrong id is now visible and correctable in the
picker rather than silently unselectable, but each still wants one real
request to confirm.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: ff71bae62d
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| Request::builder(), | ||
| ), | ||
| AiProvider::OpenAi => ( | ||
| "https://api.openai.com/v1/models".to_string(), |
There was a problem hiding this comment.
When an OpenAI-compatible provider is configured with a custom gateway, automatic catalogue refreshes and “Test connection” still send the provider's stored key to this hardcoded official endpoint. Opening the AI settings can therefore disclose a gateway credential to OpenAI and report a failed connection even though generation correctly targets the override; derive the /models endpoint from base_url_override as well.
Useful? React with 👍 / 👎.
| .find(|tab| tab.effective_repo_path(cx) == id.repo_path) | ||
| .map(|tab| tab.commit_panel.clone()) |
There was a problem hiding this comment.
If the user enters, leaves, or switches an inspected worktree while generation is running, effective_repo_path no longer equals the path captured in GenerationId. Subsequent progress/completion events then find no panel, so the generated message is discarded and the original commit panel can remain stuck in its generating state; capture the originating panel/tab identity rather than looking it up through mutable inspection state.
Useful? React with 👍 / 👎.
| if let Some(panel) = commit_panel_for(this, id, cx) { | ||
| panel.update(cx, |cp, cx| cp.fail_ai_generation(cx)); |
There was a problem hiding this comment.
When the user explicitly cancels a generation, this routes the cancellation through fail_ai_generation, leaving the panel displaying the red “AI failed — retry” control even though no failure occurred. Cancellation should clear the generating state without marking the operation as failed.
Useful? React with 👍 / 👎.
| let bare = host.split(':').next().unwrap_or(host); | ||
| matches!(bare, "localhost" | "127.0.0.1" | "[::1]" | "::1") |
There was a problem hiding this comment.
For the valid local endpoint http://[::1]:11434/v1, splitting at the first colon produces "[", so is_loopback_host rejects it as insecure even though ::1 is explicitly listed as allowed. Parse the bracketed authority before removing the port so IPv6-only Ollama or other local gateways can be configured.
Useful? React with 👍 / 👎.
1 participant
Implements the AI subsystem overhaul plan in full, bar one item noted below.
The four bugs that were reachable out of the box
messages: []on iteration 1, which the Messages API rejects with a 400.use_toolsdefaults totrue, so that was the default path — this provider had never produced a commit message for anyone.gemini-2.0-flash, which appeared in no picker, andpill_groupmarks a pill selected on exact string equality. Three separate places disagreed about what the default was.pill_groupwas the one pill row in the file withoutflex_wrap(). Flex children default tomin-width: auto, so the row grew past its card and was sliced at the window edge. At the 880px default the default provider's own model list did not fit.get_file_content("../.env")was correctly rejected andget_file_content(".env")was accepted, uploadingDATABASE_URL=postgres://user:pass@…to the provider and echoing it back into the conversation for the remaining iterations.What changed
Correctness.
build_request_bodyis a pure function per provider, guarded by a test asserting none of them sends an empty conversation on the first iteration. String provider dispatch became anAiProviderenum owning endpoint, auth shape and default model, with unknown ids insettings.jsonreported throughload_warningsrather than surfacing after a click. Every Gemini response field is now optional and all parts are iterated, so a safety block, aMAX_TOKENSstop, a leading thought part and parallel function calls stop collapsing into one opaque parse error. Response bodies are capped at 8 MB, the Gemini key moved from the query string tox-goog-api-key, and the diff walker usesfrom_utf8_lossyso non-UTF-8 lines stop silently vanishing from what the model reasons about.Threading. The whole provider dispatch — HTTP, JSON parsing,
gitspawns,read_dirwalks — moved onto the background executor, with tool progress arriving over anmpscchannel that a small foreground task drains. Every request has a 60s deadline and itsTaskis held, so a stalled provider no longer leaves the spinner running until the app is restarted. Every event carries aGenerationId { sequence, repo_path }and routes by repo path rather thanactive_tab, so a message generated for one tab cannot land in another's commit box. The cooldown is stamped on completion rather than dispatch, and reported as info without clearing an in-flight generation's spinner.Security.
get_file_contentdenies.git/, a credentials denylist, git-ignored paths and non-UTF-8 content before reading anything, and re-checks the canonicalised path so an in-repo symlink cannot reach.git/config.Per-provider credentials. One keychain slot per provider, migrated from the single shared slot, with per-provider model pins — switching provider no longer destroys the previous provider's key or its model choice, and "connected" is no longer asserted for a provider the app has no credential for.
Features. Live model catalogue with stale-while-revalidate caching, a bundled fallback and a generation guard, labelled so "three weeks old" is distinguishable from "shipped with the app". OpenRouter, via the existing OpenAI-compatible path widened with an endpoint struct.
base_url_overridefor that family, validated, warning inline which host the key will reach.UI. A provider accordion replacing six flat cards, each row owning its key field, connection status and model pin, with a connection test that can tell a working key from a typo. The AI button states its own reason when it cannot be used, stays reachable when the fix is one click away, and finally shows the shortcut that was registered all along. Tool progress is routed to the panel chip, with cancel, regenerate-with-style, and undo of an AI overwrite. Errors stop auto-dismissing at 3s and can carry an action. The Save button is gone — text inputs flush on Enter and on blur, so the same value no longer persists or vanishes depending on how it arrived.
tab_indexnow exists on the settings page, which previously had none across 4,215 lines.Removals.
last_result/last_errorhad no callers;uuidwas one call site whose id was never transmitted;http_clientwas never referenced by path;fuzzy_scoreis now one shared implementation rather than three.Migration
CURRENT_SETTINGS_VERSION2 → 3. The v2→v3 step remaps known-retired model ids to successors, andmigrate_legacy_secretspromotes the sharedai/defaultkey into the active provider's slot.ai/defaultis deliberately left in place so a downgrade still finds its key; it can be deleted a release later. Every newAiSettingsfield is#[serde(default)], so v2 files load unchanged.Verification
cargo fmt --all,cargo clippy --workspace --all-targets -- -D warnings, andcargo test --workspaceall pass — 1665 tests, up from ~1180. New coverage is pure and display-free per theCLAUDE.mdconvention: request-body shape per provider, endpoint and attribution-header resolution,base_url_overridevalidation, Gemini parsing against the four shapes that used to produce one error, catalogue parsing against a trimmed real OpenRouter fixture (negative-price BYOK row, nulltop_provider.context_length, records missingreasoning/benchmarks), filter/freshness/pinned-model classification, the tool denylist and output budget, and the shared AI guard.Two things to know before merging
Streaming is not included. Everything else in that plan item — the overwrite guard, undo, read-only editors during generation — is. Streaming needs per-provider SSE parsing including tool-call deltas across three different wire formats, with no way to exercise it offline; the
ToolCallStarted→ chip re-route covers the feedback gap in the meantime. Happy to take it on separately.The default model ids are unverified. They come from the plan's research, which flags them⚠️ and says to confirm against a live API; I could not make real requests. The structural fix means a wrong id is now visible and correctable — the picker classifies it as
Missingand offers the closest match — rather than silently rendering an unselected row, but each default still wants one real request to confirm before release.🤖 Generated with Claude Code