feat(wizard): validate the LLM credential during onboarding, with failure recovery#4055
feat(wizard): validate the LLM credential during onboarding, with failure recovery#4055Harshvardhan86 wants to merge 13 commits into
Conversation
Wire validate_provider_credentials into run_wizard's two credential collection sites via _collect_validated_llm_credential, with a bounded failure menu (re-enter / save anyway / repick) and a keyring-persist recovery menu replacing the hard exit. Fixes Tracer-Cloud#3591.
…d tests The working tree was reverted by a concurrent session between the original restore and commit; this reinstates the verified state (flow.py wiring, 67-test contract, amended onboarding smoke tests) from the tagged recovery point wave-3591-true-state.
…L gap Socket-level autouse guard for the wizard package: no wizard test may open a real outbound connection. This is load-bearing for Tracer-Cloud#3591 — validation.py caches the provider SDK classes in module globals, tests/conftest.py loads .env with override=True, and dev machines routinely export ANTHROPIC_API_KEY, so a test that patches the wrong name would quietly call api.anthropic.com with a live key and still go green. The guard derives from BaseException so the SDKs' own `except Exception` (which re-raises transport faults as APIConnectionError) cannot swallow it. It immediately caught a pre-existing impure test: run_wizard's model-change test reaches _configure_grafana_local -> seed_logs -> wait_for_loki -> requests.get against a local Loki. The connection error is swallowed today, so it passes — but on a machine with Loki running, that unit test seeds a live Loki. Fixed in the follow-up commit. Also adds the RED tests for the MiniMax gap: `minimax` is a live api_key provider that reaches the wizard's key prompt, but _get_provider_base_url has no branch for it, so a valid key is sent to api.openai.com and rejected. MINIMAX_BASE_URL has been in config all along, never wired in. Latent today; a hard block once onboarding gates on validation. Refs Tracer-Cloud#3591.
Deltas 3-6 + X2. Every new test fails on missing behaviour (AssertionError) or on a helper the approved design contract obliges GREEN to create.
…nd recover from failures
Greptile code reviewThis repo uses Greptile for automated review. Before merge, aim for Confidence Score: 5/5 with zero unresolved review threads — see CONTRIBUTING.md. Run a review — add a PR comment with: Give it ~5-10 minutes (sometimes longer) for results, then fix feedback and re-trigger until you reach Confidence Score: 5/5. Optional: automate with the greploop skill. |
opensre-3591-demo.mp4Demo — a ~40s terminal recording of the wizard on this branch. Fresh onboarding: the model is chosen first, a typo'd key is rejected live (✗ Anthropic · claude-opus-4-7 · Failed — the failure names the model we validated against), the recovery menu appears, "Save anyway" completes onboarding, and the summary honestly reports unverified. Recorded against the real Anthropic API (a bad key returns a genuine 401). |
|
@greptile review |
Greptile SummaryThis PR brings LLM credential validation in the onboarding wizard up to the same standard as every other integration: model selection is hoisted before the credential prompt so the live probe runs against the model that will be persisted, and a shared three-verb recovery menu handles both validation failures and keyring-persist failures. Two latent bugs are fixed in the same change: MiniMax was being validated against
Confidence Score: 5/5Safe to merge — the new validate-then-retry loop is well-guarded (escape hatches at every failure site, no hard gate), the MiniMax and Azure placeholder fixes are correct, and the test coverage is thorough. The new validate-then-retry loop is well-guarded with escape hatches at every failure site, the MiniMax and Azure placeholder fixes are correct, and the test coverage is thorough with the socket guard ensuring mis-patched tests fail loudly. All findings are non-blocking: an inaccurate module name in the conftest error message and a misleading recovery-menu label for the uncommon Ollama .env write-failure path. tests/cli/wizard/conftest.py — the remediation text in the socket-guard error message references the wrong module. surfaces/cli/wizard/llm_credential.py — the "Retry saving to the system keychain" label should be conditioned on credential_kind for Ollama. Important Files Changed
Flowchart%%{init: {'theme': 'neutral'}}%%
flowchart TD
A[run_wizard: LLM Provider step] --> B{change_provider?}
B -- Yes --> C[Choose provider + auth method]
C --> D{is Azure?}
D -- No --> E[choose_provider_model]
D -- Yes --> F[defer model pick into credential block]
E --> G[_prompt_validated_llm_credential]
F --> G
B -- No --> H[use saved provider/model]
H --> I{has_api_key?}
I -- No --> G
I -- Yes --> K
G --> G1[prompt credential]
G1 --> G2[ensure_azure_endpoint_settings]
G2 -- None / back --> REPICK
G2 -- ok --> G3{is Azure?}
G3 -- Yes --> G4[export key to env, choose_azure_deployment]
G3 -- No --> G5[validate_provider_credentials]
G4 --> G5
G5 -- ok --> PERSIST
G5 -- fail --> G6{recovery menu}
G6 -- Re-enter --> G7[clear Azure endpoint env, retry loop]
G7 --> G1
G6 -- Save anyway --> PERSIST
G6 -- Repick --> REPICK
G6 -- Escape --> CANCEL
PERSIST[_persist_llm_credential_with_recovery] --> P1{write ok?}
P1 -- Yes + validated --> RET_OK[return OK]
P1 -- Yes + unvalidated --> RET_UNVERIFIED[return UNVERIFIED]
P1 -- No --> P2{persist recovery menu}
P2 -- Retry --> PERSIST
P2 -- Continue unsaved --> P3[export to os.environ only, return UNSAVED]
P2 -- Repick --> REPICK
P2 -- Escape --> CANCEL
RET_OK --> K[sync_provider_env]
RET_UNVERIFIED --> K
P3 --> K
K --> L[re-apply session_env_sink if UNSAVED]
L --> M[Integrations step]
M --> N[Summary: system keychain / unverified / .env / not saved]
REPICK --> A
CANCEL --> EXIT1[return 1]
%%{init: {'theme': 'base', 'themeVariables': {"darkMode": true, "background": "#0d1117", "primaryColor": "#21262d", "primaryTextColor": "#e6edf3", "primaryBorderColor": "#8b949e", "lineColor": "#8b949e", "textColor": "#e6edf3", "edgeLabelBackground": "#161b22", "actorBkg": "#21262d", "actorBorder": "#8b949e", "actorTextColor": "#e6edf3", "actorLineColor": "#8b949e", "signalColor": "#8b949e", "signalTextColor": "#e6edf3", "noteBkgColor": "#373320", "noteBorderColor": "#d4a72c", "noteTextColor": "#f0e6c0", "labelBoxBkgColor": "#21262d", "labelBoxBorderColor": "#8b949e", "labelTextColor": "#e6edf3", "loopTextColor": "#e6edf3", "activationBkgColor": "#30363d", "activationBorderColor": "#8b949e"}}}%%
flowchart TD
A[run_wizard: LLM Provider step] --> B{change_provider?}
B -- Yes --> C[Choose provider + auth method]
C --> D{is Azure?}
D -- No --> E[choose_provider_model]
D -- Yes --> F[defer model pick into credential block]
E --> G[_prompt_validated_llm_credential]
F --> G
B -- No --> H[use saved provider/model]
H --> I{has_api_key?}
I -- No --> G
I -- Yes --> K
G --> G1[prompt credential]
G1 --> G2[ensure_azure_endpoint_settings]
G2 -- None / back --> REPICK
G2 -- ok --> G3{is Azure?}
G3 -- Yes --> G4[export key to env, choose_azure_deployment]
G3 -- No --> G5[validate_provider_credentials]
G4 --> G5
G5 -- ok --> PERSIST
G5 -- fail --> G6{recovery menu}
G6 -- Re-enter --> G7[clear Azure endpoint env, retry loop]
G7 --> G1
G6 -- Save anyway --> PERSIST
G6 -- Repick --> REPICK
G6 -- Escape --> CANCEL
PERSIST[_persist_llm_credential_with_recovery] --> P1{write ok?}
P1 -- Yes + validated --> RET_OK[return OK]
P1 -- Yes + unvalidated --> RET_UNVERIFIED[return UNVERIFIED]
P1 -- No --> P2{persist recovery menu}
P2 -- Retry --> PERSIST
P2 -- Continue unsaved --> P3[export to os.environ only, return UNSAVED]
P2 -- Repick --> REPICK
P2 -- Escape --> CANCEL
RET_OK --> K[sync_provider_env]
RET_UNVERIFIED --> K
P3 --> K
K --> L[re-apply session_env_sink if UNSAVED]
L --> M[Integrations step]
M --> N[Summary: system keychain / unverified / .env / not saved]
REPICK --> A
CANCEL --> EXIT1[return 1]
Reviews (6): Last reviewed commit: "merge: reconcile #4055 extraction with u..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
Tracer-Cloud#3591) Greptile P1: the parametrized codex/opencode repick smoke test was missed when the smoke tests were reordered for Tracer-Cloud#3591. It still expected the pre-hoist order (key prompt, then model) and the stale string 'failed validation. What next?'. Post-hoist the real flow is model -> key -> 'could not be verified. What next?', matching test_onboard_interactive_smoke. The test is skipped without codex or opencode on PATH, so the staleness never failed locally; it would hang until timeout once either CLI is installed.
|
@greptile review Fixed the P1 in |
|
@greptile review again |
|
@Harshvardhan86 nice work, will take a deeper look later 👍 |
| ) | ||
|
|
||
|
|
||
| def _recovery_action(*, prompt: str, retry_label: str, retry_hint: str, escape: Choice) -> str: |
There was a problem hiding this comment.
all such logic should be extracted to a different file? could you check where it fits appropriately, create new file if necessary
There was a problem hiding this comment.
Good call — moved the whole LLM-credential cluster out of flow.py into a new surfaces/cli/wizard/llm_credential.py in 8671b4df: the validate-then-retry prompt, the recovery menu, the render/persist/summary helpers, and the Azure endpoint helpers. run_wizard just imports and calls them now, so flow.py drops ~460 lines and the credential logic lives in one place.
|
|
||
|
|
||
| def _render_credential_validation(label: str, model: str, result: ValidationResult) -> None: | ||
| """Render the live-probe outcome, naming the provider *and* the probed model. |
There was a problem hiding this comment.
same for all such functions in this class, cehck once
There was a problem hiding this comment.
Same — this one moved too; it's all in llm_credential.py now (8671b4df).
| credential_outcome = _prompt_validated_llm_credential( | ||
| provider, model=model, session_env_sink=session_env_sink | ||
| ) | ||
| if credential_outcome == "cancel": |
There was a problem hiding this comment.
use constants pls for such keyworks like "cancel", "repick" and re-use acorss this PR
There was a problem hiding this comment.
Done in 8671b4df — the outcome keywords are now named constants in llm_credential.py (RETRY / REPICK / CANCEL / SAVE_ANYWAY / CONTINUE_UNSAVED, plus the OK / UNSAVED / UNVERIFIED states) and reused across the PR instead of the string literals. The values are unchanged so the menu default (retry) and the outcome checks still line up.
| migration_outcome = _persist_llm_credential_with_recovery( | ||
| provider, legacy_api_key, session_env_sink=session_env_sink | ||
| ) | ||
| if migration_outcome == "cancel": |
There was a problem hiding this comment.
Same — covered by the constants in 8671b4df.
|
The recovery flow handles keyring failures, but I think there is still a gap for host-based credentials. _persist_llm_credential() calls sync_env_values() for values like OLLAMA_HOST without handling write errors. If writing to .env fails, the user may get an exception instead of the retry menu. Could this use the same recovery flow? It would also be good to add a test for a .env write failure. |
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…ts; host .env recovery (Tracer-Cloud#4055 review) Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
|
@cerencamkiran nice catch — you're right, the host path ( |
|
@greptile review Addressed the review in |
|
Could you please resolve the conflicts? Thanks! |
…lm-key-validation # Conflicts: # tests/cli/wizard/test_flow.py
|
@cerencamkiran done — merged latest |
|
@muddlebee — addressed your review in |
|
@Harshvardhan86 thank you I will test this locally :) |
…nai module (Tracer-Cloud#4117) Delegate llm_credential.py's Azure helpers to upstream's new azure_openai module: drop the duplicated endpoint helpers, import ensure_endpoint_settings and choose_provider_model, and route the Azure deployment pick through choose_provider_model so Tracer-Cloud#4117's live deployment discovery is preserved. The deployment pick moves inside _prompt_validated_llm_credential (after endpoint+key collection, before validation) so Tracer-Cloud#3591's "validate the model the user picked" invariant holds while discovery has its credentials. All Tracer-Cloud#3591 fixes survive: model-before-validate, shared recovery menu, continue_unsaved, honest summary, Azure empty-key default, Azure endpoint re-prompt on retry/repick, and host .env-write recovery. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
|
Merged latest |
4 participants
Fixes #3591
Describe the changes you have made in this PR -
The onboarding wizard was the one credential-entry surface in the product that saved its credential without validating it. Every other credential in the same wizard (Dagster, Slack, GitLab, OpenSearch, Telegram) already prompts → validates → retries on failure — the LLM key skipped that convention, so a typo'd key sailed through onboarding and only surfaced later, deep inside an investigation. If the keyring write failed, the wizard hard-exited with no way to recover.
This PR brings the LLM credential up to the same standard:
return 1exits are gone..env.system keychain,system keychain (unverified),project .env, ornot saved — re-enter next run.Two latent bugs that gating-on-validation would have weaponized are fixed in the same change: a valid MiniMax key was being sent to
api.openai.com(its base URL was never wired in), and pressing Enter at the Azure key prompt silently persisted the placeholder endpoint URL as the API key. A typo'd Azure endpoint is now re-promptable on retry/repick instead of being an unrecoverable dead-end.Tests: a new autouse socket guard makes any real network call in a wizard test a hard failure (the validator caches SDK classes in module globals, so a mis-patched test could otherwise hit
api.anthropic.comwith a live key and pass for the wrong reason). Every new behaviour ships with a test written to fail first.make lint && make format-check && make typecheck && make test-scopeall green (857 passed, 3 skipped); the wizard suite is 224 passing.Known follow-up (not in this PR): after 10 consecutive failed retries the wizard returns exit 1 instead of dropping back to the provider menu — bounded, since the escape hatches are offered every round. Out of scope: the two onboarding GIFs in
docs/quickstart.mdxrecord the old key→model order and are now slightly stale (the prose is order-agnostic); validating an already-stored key that is never re-prompted; and a pre-existing double-Ctrl-C-exits-0 behaviour.Demo/Screenshot for feature changes and bug fixes -
✗ Anthropic · claude-opus-4-7 · Failed), the recovery menu appears, "Save anyway" completes onboarding, and the summary honestly reportsunverified.Code Understanding and AI Usage
Did you use AI assistance (ChatGPT, Claude, Copilot, etc.) to write any part of this code?
If you used AI assistance:
Explain your implementation approach:
The problem is that the wizard persisted the LLM API key straight to the keyring with no live check, while every other integration in the same wizard already validated-then-retried — so a bad key only failed much later. My approach was to reuse that existing pattern (
validate_provider_credentialsalready existed and is used byopensre auth) rather than invent anything new.The one non-obvious decision was ordering: the key was prompted before the model was chosen, so a naive "validate before save" would have probed the provider's default model. That breaks Ollama (fails on a model the user never pulled), so I hoisted model selection above the credential block in both branches. I considered hard-gating onboarding on a passing validation, but rejected it because the validator can't distinguish a bad key from a dead network / proxy / rate-limit / capped account — a hard gate would lock out real users, so "Save anyway" is mandatory and "Re-enter" is the default.
Key pieces:
_prompt_validated_llm_credential(the validate-then-retry loop),_recovery_action(one shared 3-verb menu used by both the validation-failure and keyring-persist-failure sites), and_persist_llm_credential_with_recovery(replaces the three bare hard-exits). Thecontinue_unsavedpath exports toos.environonly and is re-applied aftersync_provider_envso the in-process shell handoff can read it, but is never written to.env.Checklist before requesting a review
Note: Please check Allow edits from maintainers if you would like us to assist in the PR.