3 Commits

Include renames

Author	SHA1	Message	Date
Kevin Codex	16550569c1	feat(providers): OAuth login profiles and list-first /provider manager (#560) ... * feat(providers): OAuth login profiles and list-first /provider manager Make multiple providers a first-class workflow: every OAuth login is now a visible, switchable provider, and /provider manages the whole lifecycle. OAuth logins become provider profiles: - zero auth chatgpt / zero auth login <preset> and all four TUI login paths (wizard + first-run, browser + device-code) now upsert a catalog-backed profile via config.EnsureCatalogProvider after storing the token — without stealing the active provider. Previously the token was stored invisibly: the provider showed up nowhere and 'providers use' failed not-found. - Switching to an OAuth provider no longer inlines the bearer as an API key, which suppressed the runtime resolver (no token refresh, no Codex chatgpt-account-id header → guaranteed 401s). The credential gate now recognizes stored OAuth logins on keyless profiles. - providers list / current report truthful credential state: stored keys count as configured (APIKeySet honors apiKeyStored) and token logins show 'oauth login'; providers check accepts an OAuth login as a credential. List-first /provider manager (TUI): - /provider opens the saved-provider list: Enter activates via the shared switch path, 'a' adds (existing wizard), 'e' opens a field-level editor (endpoint, model, key, description, and safe rename — the stored key, activeProvider, and live ZERO_PROVIDER follow the new name), 'd' deletes with an inline confirm and stored-key cleanup. Esc walks back one level. - Credential states resolve asynchronously so keychain reads never block the render loop; 'stored key missing' surfaces broken keychain entries. - Implemented as manager steps on providerWizardState so every existing overlay gate, key route, and mouse hook covers it unchanged. CLI + config parity: - zero providers remove <name> (deletes the stored key, hands the active pointer off) and zero providers rename <old> <new>. - config.RemoveProvider / RenameProvider: read-modify-atomic-write; rename migrates the credential-store entry write-new-then-delete-old so an interruption can never lose the key. * fix(providers): address PR #560 review findings All six review findings validated as real; fixes with regression tests: - Persist the apiKeyStored marker when a key is captured onto an existing profile: mergeProfile deliberately ignores the marker (a project config must not claim the user's stored keys at resolve time), so the user-config WRITE path (UpsertProvider) now re-applies it — previously editing a key onto an env/no-key profile stranded the secret in the credential store while every ApplyStoredAPIKey gate skipped it. [jatmn P2] - Let active-pointer recovery select OAuth-login providers: firstUsableProvider now applies the same providerHasOAuthLogin check as setupRequired/usableSavedProviders, so a stale activeProvider plus a saved ChatGPT/xAI login recovers instead of forcing onboarding. [jatmn P2] - Delete stored keys from the store BESIDE the config being edited: providers remove and the TUI manager delete now use ProviderKeyStoreAt (matching setup/rename) instead of the default-path store. [jatmn P2] - Roll back the key migration when RenameProvider's config write fails, so the store and config.json can't desync (key under the new name, config still naming the old one). [CodeRabbit] - Clear provider descriptions for real: new SetProviderDescription writes the field verbatim (the upsert merge treats empty as unchanged); the manager's editor routes description changes through it. [jatmn P3] - Surface stored-key deletion failures in providers remove (stderr warning + keyError in the JSON payload) and the TUI delete status, instead of letting a lingering secret read as a clean removal. [CodeRabbit] * fix(providers): second review round — case-only rename, logout hint, test hermeticity - Skip the credential-store migration on case-only renames (groq -> Groq): the store normalizes names case-insensitively, so Set(new)+Delete(old) target ONE entry and the delete removed the key that was just "moved". The existing entry already serves the new name. [CodeRabbit] - Point the delete flow's cleanup hint at the stored login that actually exists: after a rename ({name:"codex", catalogID:"chatgpt"}) the token lives under the catalog id, and `zero auth logout codex` would delete nothing. oauthLoginName resolves the same FirstStored candidate the runtime uses. [jatmn P3] - Lock in APIKeyEnv semantics after a key edit: the env reference survives as an explicit override (a SET env var wins at resolve time; the stored key applies when it is absent). [CodeRabbit nitpick] - Make main's new firstUsableProvider tests hermetic: they read the REAL OAuth token store, so any developer with an actual xai/chatgpt login gets spurious failures (found because this machine has one). Also rebased onto main; the only conflict was comment wording in firstUsableProvider, where the audit commit (#481) independently landed the same OAuth-login fallback check this branch added. * fix(tui): read stored keys from the active config's credential store The TUI's write paths (SecureProviderProfile capture, rename migration, manager delete) all target the store beside m.userConfigPath, but the read paths still opened the DEFAULT-path store: profileWithCredential (feeding the /model and manager switch gate), the model picker's discovery credential lookup, and the manager's async credential-status probe. With a non-default config path, a provider saved with only apiKeyStored had its key in the config-adjacent store, so the gate rejected it as having no usable credential and the manager showed 'stored key missing' for a healthy profile. All TUI stored-key reads now resolve through providerKeyStoreForPath (config-adjacent, default-path fallback for ephemeral sessions), so the UI reads from the same store it writes to. Regression test covers both the credential probe and the switch path with a key seeded only beside the config. Also consolidates the OAuth test-store seeding on withAuthStore (now pins ZERO_OAUTH_STORAGE=file so an inherited keyring backend can't bypass the temp path) per review nitpick. * fix(providers): resolve self-review findings — atomic edits, key normalization, threading Findings from a multi-agent code review of this branch (8 finder angles, independently verified); all confirmed against the code, several with empirical repros. Correctness: - config.EditProvider: one atomic read-modify-write for the manager's Save (rename + fields + verbatim description + stored-key marker + key migration with write-failure rollback). Replaces the rename→upsert→ describe sequence that (a) turned a case-only rename (groq → Groq) into a DUPLICATE profile via UpsertProvider's case-sensitive merge, and (b) could fail halfway, leaving config.json renamed while savedProviders and ZERO_PROVIDER still held the old name (retry then failed 'not found'). A rename now also migrates a freshly captured replacement key (captured under the current name; the migration moves it). - savedProviders is mutated surgically on manager delete/edit instead of being replaced with the raw user-config list — the session list is seeded from the RESOLVED (project-config layered) and usability-FILTERED set, so wholesale replacement dropped project-contributed providers and resurrected filtered stubs for the rest of the session. - oauth.ProviderKey normalizes provider names to lower case at the single write/lookup choke point: `zero auth login xAI` stored provider:xAI while every candidate lookup was case-sensitive — a successful login invisible to the runtime. Map-based presence checks lowercase to match. - OAuth login persistence moved onto the Update goroutine (message handlers) — a background tea.Cmd (device-code polls land minutes later) doing an unlocked config read-modify-write raced UI-thread writes (lost updates). The write error is now surfaced and the in-memory mirror is skipped on failure, so a profile that never reached disk can't show as a working provider that vanishes on restart. UX / consistency: - Esc inside a manager-launched add flow walks back to the provider list from EVERY step (was: first step only; deeper steps destroyed the overlay and the user's place). In-flight OAuth logins are invalidated on the way out; footers say 'Esc back'. - switchProviderModel returns a structured outcome; the manager branches on it instead of strings.Contains(text, "Switched to") — UI copy is no longer a control-flow contract (a provider named 'Switched to prod' could close the manager on a refusal). - Manager delete's keychain subprocess + OAuth lookup run in a follow-up tea.Cmd (the confirm keypress no longer blocks the render loop); the credential probe takes ONE token-store snapshot instead of 2 reads per row, and resolved states carry over across reloads (no 'checking…' flash). - Credential-state reporting converges: /provider status recognizes OAuth logins (was 'api key: not set' + warning for a WORKING provider), doctor uses the shared HasConfiguredCredential definition plus OAuth awareness (its 'matching ProviderSnapshot.APIKeySet' claim is true again), and the manager's classifier falls through a stale stored-key marker to env/OAuth the way the runtime does. * Fix provider manager test formatting	2026-07-07 10:34:59 +08:00
PierrunoYTandCopilot Autofix powered by AI	008bc9b3f3	fix: address bugs found in a multi-agent codebase audit (#481) ... * fix: address bugs found in a multi-agent codebase audit Five parallel review agents each swept a different area (self-update, TUI keybindings, agent/session handling, sandbox/shell safety, and auth/credential storage) and surfaced nine concrete defects, all fixed here with regression tests: - sandbox: grant_scope path comparisons were case-sensitive, letting a persisted deny grant be bypassed by spelling a Windows path with different case - securefile/oauth: createSecretFile's Windows ERROR_ACCESS_DENIED handling was only applied to the oauth token store, not the shared credstore backend; both also masked the real lock-contention error behind a misleading "file does not exist" timeout - cli: firstUsableProvider ignored OAuth logins when picking a fallback provider, forcing unnecessary re-onboarding for an already-authenticated user - sessions: exec prompt summarization truncated on a raw byte offset, which could split a multi-byte UTF-8 rune and embed invalid UTF-8 into a resumed session's prompt - tui: configurable keybindings had no collision detection, so a remapped chord could silently make a hardcoded shortcut (or another configured binding) permanently unreachable - tools: the Windows shell-syntax pre-flight check wasn't quote-aware and could block harmless commands whose quoted arguments merely contained operator-like text - update: checksum verification didn't cross-check the archive filename it verified against the one being extracted; extractZip didn't reject symlink-mode entries like extractTarGz already did; replaceBinary's restore-on-failure rename had no retry, risking a permanently missing binary on a transient Windows file lock * fix: close remaining PR 481 review findings * Potential fix for pull request finding Co-authored-by: Copilot Autofix powered by AI <[email protected]> * fix(tools): drop stale ; test case after rebasing onto main's MSYS check Rebasing fix/bug-hunt-findings onto main pulled in #476, which replaced this file's regex-based windows_shell_syntax detection with a segment/word-anchored check and deliberately stopped treating ; as a command separator on Windows (cmd.exe doesn't split on ; the way bash does). detectShellCommandIssue itself came out of the rebase unchanged from main (our quote-masking version was fully superseded by #476's more thorough segment-based approach), but the quoted-content test this PR added still asserted the old bash-style semantics for `echo "foo" ; ls -la`, which now correctly returns no issue since it's a single echo invocation with literal arguments under cmd.exe, not a real ls invocation. --------- Co-authored-by: Copilot Autofix powered by AI <[email protected]>	2026-07-06 18:48:28 +08:00
KRATOS	efb6b79164	web_search robustness + chat UX: collapse, clipboard, scroll-pin, proactive search (#218) ... * web_search: fix #209 follow-ups (tolerant score, render gate, redaction, FQDN) Verified-and-fixed defects from a review of #209: - Tolerant score decode (lenientScore): a stringified/odd/null "score" no longer makes json.Unmarshal discard the entire response; it degrades to absent. - Score render gate rounds first (>= 0.01) so tiny positives and negatives no longer print a noisy "score 0.00". - Domain-filter error now goes through redactWebSearchText, matching the other error paths. - Trailing-dot FQDN symmetry: "react.dev." and "react.dev" normalize the same on both the allowlist and result sides. Left as-is (intended, tested): scheme-less host:port allowlist entries. Includes mixed-type domains-array tests. * web_search: only register the built-in tool when a backend is configured When no search backend is set (ZERO_WEBSEARCH_BASE_URL / ZERO_WEBSEARCH_BACKEND), CoreNetworkTools no longer offers the built-in web_search. Previously it was always registered, so a model with an MCP search tool (e.g. Exa) would burn several calls + high-risk permission prompts on the built-in tool — which can only return "no backend configured" — before falling back to the working MCP tool. Now: no backend -> only web_fetch (+ any MCP search tool) is offered, so the model uses the MCP search directly. A configured backend behaves exactly as before. Tests: registry/web_search/specialist tests that asserted web_search is always in CoreTools now configure a backend; added a test that it is omitted when none is set. (Pre-existing, unrelated: internal/cli doctor/exec tests fail in the sandbox on origin/main too — network/provider-dependent, not touched here.) * tui: copy selected transcript text to the native OS clipboard Selecting transcript text auto-copies on mouse release, but the copy went only through OSC52 (ansi.SetSystemClipboard), which macOS Terminal.app doesn't support at all and iTerm2 / some Windows Terminal builds have off by default — so the selection silently never reached the clipboard. Copy via the native clipboard (pbcopy / clip.exe / xclip) first, which works on local terminals regardless of OSC52, and fall back to OSC52 for remote/SSH sessions where no local clipboard utility is reachable. atotto/clipboard was already in the module graph (indirect); promoted to a direct import — no new dependency. Gates: gofmt/vet/build(host+linux+windows)/tui test -race/staticcheck all pass. * tui: collapse long tool output by default with click-to-expand Long, noisy tool output (web-search / MCP / read dumps) flooded the transcript. Now a tool result whose output exceeds the live body cap renders collapsed by default — head + a "▸ N lines — click to expand" footer — and you click the card (while it is live) to expand (▾) / collapse. Mirrors the existing reasoning-row collapse, so collapsed rows also flush to scrollback clean. Scoped to avoid hiding what matters: - diff tools (edit_file / apply_patch / write_file) never collapse — their body must stay reviewable (matches the deeper flush cap intent); - the uncapped detailed transcript view (Ctrl+O, bodyCap==0) never collapses — it still shows full output; - short output (<= cardBodyMaxLines) stays inline as before. Reuses transcriptRow.expanded + the row toggle (toggleReasoningRow generalized to toggleTranscriptRow); the card head is a clickable toggle line. Running tool cards (spinner) are unchanged. Tests: collapse/expand, short-output inline, diff tools never collapse, toggle, and the head exposes a click toggle; existing diff + detailed-view tests updated. Gates: gofmt/vet/build(host+linux+windows)/tui -race/staticcheck all clean. * cli: fall back to a usable saved provider instead of re-running onboarding If the active provider lacked a credential (e.g. it relies on an env var that isn't set, or an OAuth login that isn't stored), the TUI forced first-run onboarding on every launch — even when other saved providers were fully configured. Each onboarding then appended yet another provider. Now, when the active provider isn't usable, fall back to the first usable saved provider (prefer a remote keyed one over a local endpoint) for the session, and skip onboarding. Onboarding only runs when no configured provider is usable — a genuinely fresh setup. Saved logins persist; switch the active provider with `zero provider use <name>`. Tests: firstUsableProvider prefers remote keyed, falls back to local, accepts a keyless local proxy, and reports none-usable. Gates: gofmt/vet/build (host+linux+windows)/staticcheck clean; the only failing cli tests (doctor/exec, network-dependent) are pre-existing on origin/main. * tui: hold the scroll position while output streams in The chat viewport scroll offset is measured from the bottom, so when the transcript grew (a streaming turn) the window followed the new bottom and yanked the user off whatever they had scrolled up to read. syncChatScroll now pins the viewport: while the user is scrolled up, it bumps the offset by however many lines the body grew, so the absolute view holds where they are reading; at the bottom (offset 0) it follows the tail as before. Hooked into the single Update funnel; only the scrolled-up path renders the body, so the common (at-bottom) case stays cheap. Tests: pins by the growth delta while scrolled up; resets/follows at the bottom. Gates: gofmt/vet/build(host+linux+windows)/tui -race/staticcheck clean. * agent: search the web proactively instead of answering "I don't know" When a web search/fetch tool is available and the user asks about something external the model is unsure of or that may be current (a product, library, company, price, version, recent event), the system prompt now tells it to search and read results before answering, rather than replying that it doesn't know without checking. Addresses the model saying "I don't know about that" until the user explicitly asked it to search. Tests: prompt includes the new guidance ("search the web and read"). Existing prompt assertions use Contains, so the addition is non-breaking. * tui: show clean labels for MCP tool cards (mcp_exa_web_search_exa → web search) * agent: sharpen web-search guidance (disambiguate, refine, scale) Strengthen the search rule so the model searches before answering about an external entity/product/library/model/version/release it doesn't recognize, doesn't assume the most common meaning of an ambiguous name (disambiguates by the conversation's domain), and refines + re-searches when results look off-topic — rather than confidently answering the wrong interpretation (e.g. reading "Fable 5" as a video game instead of an AI model). Also: don't web-search timeless facts or workspace-answerable questions (use the file tools), keep queries short, and scale the number of searches to the question. Tests: prompt includes the new guidance ("search the web before answering", "do not recognize"). Existing assertions use Contains, so non-breaking. * test: gofmt setup_fallback_test.go (comment alignment) * Address review feedback on web_search + chat UX Resolve CodeRabbit review comments on this PR: - firstUsableProvider: skip a profile whose CatalogID no longer resolves *and* that has no explicit BaseURL (no endpoint → unusable). A stale CatalogID with a BaseURL still works as a custom endpoint and is kept. - providerProfileIsLocal: classify by parsed URL hostname (exact localhost / 127.0.0.1 / ::1) instead of substring matching, so hosts like "notlocalhost.com" are no longer misread as local. - lenientScore: reject non-finite scores (ParseFloat accepts "NaN"/"Inf") so the documented score filter actually holds and the renderer never emits a non-finite value. - syncChatScroll: shift the pinned from-bottom offset by the signed line delta so the view also holds when the body shrinks (card collapse, transcript clear), clamped at zero; previously only growth was handled. - copyTranscriptSelectionCmd: report "Copy failed" when both the native clipboard and the OSC52 fallback fail, instead of claiming success; keep the selection so the user can retry. - Drop vestigial ZERO_WEBSEARCH_BACKEND test env: the native backend was removed (MCP path), so the var is read by no product code and the references misrepresented it as a config knob. Adds tests for the local-host parsing, the catalog/BaseURL skip, and the non-finite score rejection.	2026-06-16 13:47:08 +05:30