Repository navigation
fix(*): a key the vendor refuses is not called connected - #1045
Handsome-wzw wants to merge 6 commits into
Conversation
OpenRouter's model list answers anyone, so a mistyped key read as connected on every surface: the settings page and the first-run wizard stored it, marked the row "Connected" and wrote "the provider rejects the saved key" beside it; `raven onboard` and `raven provider test` reported "Connected! (468 models available)" and the first message then failed with an auth error. The probe already knew how to confirm a key where the list says nothing (`check_credential`: a decoy key against the list, then the vendor's own key endpoint); nothing that answers a person used it. The page now asks the vendor before storing (`model.check_key`, a read: the stored section with the form's fields laid over it, through the same probe). The vendor's own refusal -- a 401 to this key, or the key named invalid -- is the one answer that holds a key back: the form says so and nothing is written. Every other answer stores the key and says what is still worth knowing, as before; a check that cannot run stores it and tests on the side. A stored key the vendor later refuses reads "Key rejected" on its row rather than "Connected". The wizard and `provider test` pass `check_credential`, and a list that cannot confirm a key says so instead of claiming it valid. Co-authored-by: Claude (claude-fable-5-1) <noreply@anthropic.com>
gloryfromca
left a comment
There was a problem hiding this comment.
Blocking: preserve the backend's distinction between a rejected key and other HTTP 403 failures.
I reviewed the repository rules and context documents, the full diff, surrounding callers and probe implementation, relevant history, backward-compatibility behavior, test changes, and the Runtime-to-RPC-to-Web architecture boundary. I found one concrete rendering defect inline.
Verification:
uv run --python 3.13 --all-packages pytest tests/test_config_update_providers.py tests/test_rpc_model.py tests/test_cli_provider_commands.py tests/test_cli_onboard_commands.py -x- 884 passed.npm test --prefix ui-web -- --run src/features/settings/SetupBodies.test.tsx src/features/settings/providers/ProviderDetail.test.tsx src/features/settings/source.test.ts- 102 passed.npm run type-check --prefix ui-web- passed.npm run gen:check --prefix ui-web- passed; generated client matches the 249-method contract.
Two earlier Python attempts did not reach tests: the plain environment lacked the optional raven_everos workspace package, and --all-extras could not build datrie because this runner lacks gcc. The successful workspace-package run above exercised all four affected Python files without skipping or weakening tests.
The row's chip and the detail head read "Key rejected" off the probe's invalid_key status. That status is broader than a refusal: the status table maps every 403 to it, and a 403 can mean a region, a plan, a permission or a proxy policy -- a stored key that may well be fine. model.fetch_models with verify now answers `refused` from the same credential_refused() the check uses, the page keeps it on the probe, and only that turns the label. The TLS trust test asserted "custom OK" from `provider test`; its stand-in's list answers anyone, so the probe's truthful word is now "reached, key not confirmed", and the test asserts the handshake either way. Co-authored-by: Claude (claude-fable-5-1) <noreply@anthropic.com>
gloryfromca
left a comment
There was a problem hiding this comment.
No blockers; this can merge as far as I am concerned.
The prior blocker is fixed: the runtime exposes the narrow refused verdict, the RPC schema and generated clients carry it, and both settings views use it rather than the broader invalid_key status. I rechecked the repository rules and context, the full diff and revision delta, callers and relevant history, backward compatibility of the optional RPC field, test changes, and the Runtime-RPC-Web boundary. I found no new defect worth raising.
Verification:
uv run --python 3.13 --all-packages pytest tests/test_config_update_providers.py tests/test_rpc_model.py tests/test_cli_provider_commands.py tests/test_cli_onboard_commands.py tests/test_cli_system_ca.py -x- 897 passed.npm test --prefix ui-web -- --run src/features/settings/SetupBodies.test.tsx src/features/settings/providers/ProviderDetail.test.tsx src/features/settings/source.test.ts- 103 passed.npm run type-check --prefix ui-web- passed.npm run gen:check --prefix ui-web- passed; generated client matches the 249-method contract.
|
Merge conflict: @Handsome-wzw this PR no longer merges onto |
|
Windows compatibility review: no new Windows-specific issue found in the inspected changes. Scope: all 27 changed files at head Native Windows verification (Python 3.13.12; Node 25.9.0/npm 11.12.1; isolated detached checkouts/environments):
Exact commands ( $a = 'C:\Users\livxu\AppData\Local\Temp\raven-windows-review\a130fd4dd4344beabbc69e870abc412a'
uv sync --frozen --python 3.13 --extra dev
uv run --frozen --no-sync pytest tests/test_config_update_providers.py tests/test_rpc_model.py tests/test_cli_provider_commands.py tests/test_cli_onboard_commands.py::test_verify_provider_confirms_the_key_itself tests/test_cli_onboard_commands.py::test_verify_provider_says_an_unconfirmed_key_is_unconfirmed tests/test_cli_onboard_commands.py::test_a_certificate_the_machine_does_not_trust_points_at_the_certificate_store tests/test_cli_onboard_commands.py::test_a_refused_certificate_names_the_refusals_a_ca_does_not_fix tests/test_rpc_schema_match.py tests/test_rpc_registration.py tests/test_cli_system_ca.py -n 0 -x --tb=short --basetemp "$a\checks\1045-python\pytest" --junitxml "$a\checks\1045-python\junit.xml"
uv run --frozen --no-sync pytest tests/test_cli_provider_commands.py::test_test_command_confirms_the_key_itself tests/test_cli_provider_commands.py::test_test_command_says_an_unconfirmed_key_is_not_confirmed tests/test_cli_onboard_commands.py::test_verify_provider_confirms_the_key_itself tests/test_cli_onboard_commands.py::test_verify_provider_says_an_unconfirmed_key_is_unconfirmed tests/test_cli_onboard_commands.py::test_a_certificate_the_machine_does_not_trust_points_at_the_certificate_store tests/test_cli_onboard_commands.py::test_a_refused_certificate_names_the_refusals_a_ca_does_not_fix tests/test_rpc_schema_match.py tests/test_rpc_registration.py tests/test_cli_system_ca.py -n 0 -x --tb=short --basetemp "$a\checks\1045-cli-contract\pytest" --junitxml "$a\checks\1045-cli-contract\junit.xml"
# Exact base control (exit 1 at the same assertion):
uv run --frozen --no-sync pytest 'tests/test_cli_provider_commands.py::test_a_sign_in_leaves_its_credential_readable_only_by_its_owner[openai-codex-written0]' -n 0 -x --tb=short --basetemp "$a\checks\1045-base-mode\pytest" --junitxml "$a\checks\1045-base-mode\junit.xml"
# In the head's ui-web directory:
npm.cmd ci --ignore-scripts --no-audit --no-fund
npm.cmd test -- --run src/features/settings/SetupBodies.test.tsx src/features/settings/providers/ProviderDetail.test.tsx src/features/settings/source.test.ts
npm.cmd run type-check
npm.cmd run gen:check
# In the head checkout:
git diff --check d9c0a1d74222825ad621a0fd138b691c0edd537d..961030317bd38efc946a9ffac5e42adb4a24541dCoverage gaps: mocked provider/gateway responses; no live vendor credentials, Windows enterprise certificate-store/TLS end-to-end check, real-browser walkthrough or full TUI run. The complete Python selection was not green. This scoped result makes no full-compatibility or merge claim. |
# Conflicts: # CHANGELOG.md
|
Merged |
gloryfromca
left a comment
There was a problem hiding this comment.
No blockers; this can merge as far as I am concerned.
I reviewed the merge revision, including the full github/main...HEAD diff, the effective feature diff against merged main, the CHANGELOG.md conflict resolution, overlapping RPC/settings/generated-file changes, callers and history, backward compatibility, test changes, repository rules/context, and the Runtime-RPC-Web architecture boundary. The reviewed fix is preserved, both changelog sides are retained, generated clients are synchronized, and git merge-tree --write-tree HEAD github/main is clean. I found no new defect worth raising.
Verification:
uv run --python 3.13 --all-packages pytest tests/test_config_update_providers.py tests/test_rpc_model.py tests/test_cli_provider_commands.py tests/test_cli_onboard_commands.py tests/test_cli_system_ca.py tests/test_rpc_schema_match.py -x- 1,416 passed.npm test --prefix ui-web -- --run src/features/settings src/rpc- 314 passed.npm run type-check --prefix ui-web- passed.npm run gen:check --prefix ui-web- passed; generated client matches the 249-method contract.npm run lint:rpc --prefix ui-tui- passed.npm run lint:i18n --prefix ui-tui- passed.
The first TUI RPC check attempt could not start because ui-tui dependencies were absent; after npm ci --prefix ui-tui, both locked generation checks passed.
There was a problem hiding this comment.
Blocking: a successful web connect no longer writes the vendor's live model list to the served-models cache, so the pickers fall back to the bundled catalogue.
On main, connect() in ui-web/src/features/settings/store.ts saved the key and then always ran recheck(slug), which calls fetchModels(slug, true). That reaches model.fetch_models with verify, and that is the only writer of cache/served-models.json: raven/rpc/methods/model.py:889-892 (if asked: ... remember(slug, ...)). The reloadOffer() that follows then lets the role slots and the composer picker list what the vendor serves. This is the behaviour #751 shipped ("OpenRouter went from 110 bundled ids to 500+"). The rule is still written at raven/providers/served_models.py:7-9: "Whenever raven does ask the vendor (a connect's test, a "check again", the add-model sheet), the answer is written here".
On this head, the connect's test is model.check_key. It calls test_provider and returns (model.py:751-790), with no remember and no catalogue (full_catalogue is not set). connect() now runs recheck only when checkKey itself throws (store.ts:438-439). When the check returns a verdict, the sequence is checkKey, save_key, reloadProviders, so reloadOffer() at store.ts:437 reloads an unchanged offer. model.options then reads recall(slug) (model.py:165-173) and gets nothing for this provider.
Where else could the cache get written? git grep "recheck(\|sheetOpen(\|fetchModels(" over ui-web on this head finds only:
- the add-model sheet (
store.ts:471-476); - the detail page's Check link (
ProviderDetail.tsx:190, shown only while there is no probe this session; connect has just set one); - the row's Retry (
Providers.tsx:196, shown only after an answer other than a plain yes).
So after a valid connect in the first-run wizard (SetupBodies.tsx <Providers setup />), nothing writes the cache. In settings, only opening the add-model sheet does.
Reproduced on head 3400ce9 merged onto main 018f984. Backend probe: scratch RAVEN_HOME, network denied, test_provider on an httpx.MockTransport that lists zz-live-only-model for the good key.
head path (model_check_key, then set_provider_fields as save_key does):
check_key -> ok True, status valid, refused False
served-models.json exists: False recall(openai): {} offer has zz-live-only-model: False
main path (model_fetch_models verify=True after the save):
served-models.json exists: True recall(openai): {openai/zz-live-only-model, openai/gpt-4o} offer has it: True
The web half is from reading store.ts:409-440 on both trees; JS was not executed in this pass. The PR's tests pin the missing fetch as intended:
SetupBodies.test.tsx:233andProviderDetail.test.tsx:327expect exactly['checkKey', 'provider'];- the base assertion
expect(calls).toContainEqual(['fetchModels:verify', saved.slug])was removed; SetupBodies.test.tsx:320still passes because it counts thereloadProviderscall and cannot see the server cache.
Two ways out, either keeps one writer:
- after a non-refused save, run the verified read as main did (
void recheck(slug)unconditionally once stored, with the verdict fromcheckKeyshown until it lands); - or make the connect's test itself the catalogue read that remembers, so there is still one probe per connect.
Whichever is chosen, a test that asserts the served-models cache (or a fetchModels:verify call) after a valid connect would pin it. Also, store.ts:444-446 and model_check_key's docstring ("The same probe model_fetch_models runs with verify") would then be true again.
|
Not a blocker (two non-blocking points and some nits from the same acceptance pass, head 3400ce9 onto main 018f984; the blocking point is in its own thread). Severity of this one finding, not a verdict on the pull request. Any blocking findings from this pass are review threads on the changed files; GitHub renders those collapsed, as a file name with no text. Non-blocking:
Nits:
|
gloryfromca
left a comment
There was a problem hiding this comment.
Blocking: restore the served-model cache refresh after a successful web connect.
The new blocking thread changes my stance on this unchanged revision. I independently confirmed its facts: main called recheck(slug) after every successful save; this head calls it only when checkKey throws. model.check_key neither requests the full catalogue nor calls served_models.remember, while model.fetch_models remains the cache writer used by recheck. Therefore a normal successful connect now reloads provider state without first recording the vendor's live models, so model pickers can fall back to the smaller bundled catalogue. The existing web tests explicitly expect only checkKey then provider on the successful path and therefore pass without covering the lost cache write.
This is the outstanding finding opened by @0xKT; I did not duplicate it, re-grade it, or resolve their thread. The separately posted non-blocking notes do not require another blocking finding from me.
Verification on the unchanged head:
uv run --python 3.13 --all-packages pytest tests/test_rpc_model.py -x- 251 passed.npm test --prefix ui-web -- --run src/features/settings/SetupBodies.test.tsx src/features/settings/providers/ProviderDetail.test.tsx- 83 passed.
These green results confirm the current tests do not catch the reported regression; they do not refute it.
|
Windows compatibility review — PR #1045 @ Scope: base Delta: the merge changed generated RPC/UI files and shared UI modules; the PR's own probe and RPC logic ( Result: no Windows-specific issue found. What I checked: Verification (native Windows 11, CPython 3.12.13, isolated worktree All five Python failures are inherited: the same IDs fail at base Limitations: mocked provider responses; no live vendor credentials; no Windows certificate-store end-to-end run (the system-CA suite is Linux-only by design); no real-browser walkthrough. |
# Conflicts: # CHANGELOG.md
…pickers list it The connect's test used to be the verified catalogue read, which is the one writer of the served-models cache; `model.check_key` replaced it and wrote nothing, so a valid connect left every picker on the bundled catalogue until an add-model sheet was opened. The check now asks for the whole catalogue and remembers it through the same rows the catalogue read builds, which keeps one probe per connect and one writer. Also from the same acceptance pass: the check's answer declares its nullable fields and every status word it can send, the refused-key line names no button (it reads Connect on the form and Update on a connected provider's page), the two onboard lines about an unchecked key have their zh text, and the changelog says when a row reads Key rejected. Co-authored-by: Claude (claude-fable-5-1) <noreply@anthropic.com>
gloryfromca
left a comment
There was a problem hiding this comment.
No blockers; this can merge as far as I am concerned.
The served-model cache regression is fixed on this revision. model.check_key now requests the full catalogue, feeds the response through the same row builder as model.fetch_models, and writes only the vendor-listed rows through the shared served-model cache path. The regression test confirms a live-only model is recalled while the unsaved key is still absent from config. I reviewed the revision delta, full current PR diff, affected callers/history, schema compatibility, generated clients, tests, repository rules/context, and the Runtime-RPC-Web boundary; I found no new defect worth raising.
The cache thread was opened by @0xKT and remains theirs to resolve; I did not resolve or re-grade their thread.
Verification:
uv run --python 3.13 --all-packages pytest tests/test_rpc_model.py tests/test_rpc_schema_match.py tests/test_cli_onboard_commands.py tests/test_i18n_boundary.py -x- 1,129 passed.npm run gen:check --prefix ui-web- passed; generated client matches the 249-method contract.npm run type-check --prefix ui-web- passed.npm run lint:rpc --prefix ui-tui- passed.npm run lint:i18n --prefix ui-tui- passed.PYTHONPATH=. uv run --frozen --python 3.12 --extra dev python scripts/check_source_language.py github/main..HEAD- passed.git merge-tree --write-tree HEAD github/main- clean.
An initial Python command named the nonexistent tests/test_i18n_catalog.py and ran zero tests; the corrected command above is the actual passing result.
|
On the blocking thread at
From the non-blocking notes in the same pass, taken up in the same commit: the Verification on the new head: the six Python test files this PR touches plus I could not reply inside the thread from this session, so this comment stands in for it; the thread is left open for its author to close. |
|
Windows compatibility re-review — PR #1045 @ Scope: delta from the previously reviewed head Result: no Windows-specific issue.
Verification (native Windows 11, CPython 3.12.13, isolated worktree Both failures are the inherited WinINET-system-proxy TLS tests ( Limitations: no live vendor endpoint; the settings UI was not re-run (unchanged in this delta); the TLS fixture limitation is environmental, not code. |
|
Merge conflict: @Handsome-wzw this PR no longer merges cleanly into Measured just now with After a rebase onto current main I re-check what moved before the grade comes back. |
# Conflicts: # CHANGELOG.md
gloryfromca
left a comment
There was a problem hiding this comment.
No blockers; this can merge as far as I am concerned.
Reviewed the new merge revision 2ac4ec686a27. The only manual conflict was CHANGELOG.md; the resolution preserves this PR's entry and the incoming main entries. git range-diff shows the PR's three commits unchanged, the current PR diff remains the same 28-file change, and a merge-tree check against github/main is clean.
This pass covered the repository rules, merge delta and conflict resolution, overlapping RPC schema/generated-client integration, the previously reviewed callers and history, backward compatibility, test changes, and the existing architecture constraints. I found no weakened test or concrete regression introduced by the merge.
Verification:
uv run --python 3.13 --all-packages pytest tests/test_rpc_model.py tests/test_rpc_schema_match.py tests/test_cli_onboard_commands.py tests/test_cli_provider_commands.py tests/test_config_update_providers.py tests/test_cli_system_ca.py tests/test_i18n_boundary.py -x: 1,419 passed.- Corrected focused Web settings run: 4 files, 122 tests passed. The initial invocation used repo-root paths under
--prefixand selected no files; it was rerun with package-relative paths. - Web RPC generation and type-check passed.
- TUI RPC and i18n generation checks passed.
|
Windows compatibility re-review — PR #1045 @ Scope: the head moved from the previously reviewed Result: no Windows-specific issue; the previous conclusion holds. Verification (native Windows 11, CPython 3.12.13, isolated worktree Both failures are the inherited WinINET-system-proxy TLS tests (httpx's default client routes the Limitations: no live vendor endpoint; the settings UI was not re-run (unchanged). |
|
Merge conflict: @Handsome-wzw, this PR no longer merges onto the current
The judge's verdict on |
Summary
A key the vendor refuses was reported as connected on every surface, because OpenRouter's model list answers anyone and the surfaces that answer a person read a 200 from that list as a valid key:
raven onboardprinted "Connected! (468 models available)" and the first message then failed with an auth error;raven provider test openrouterprinted "OK (468 models available)".The probe already knew how to confirm a key where the list says nothing (
test_provider(check_credential=True): a decoy key against the list, then the vendor's own key endpoint,/keyfor OpenRouter); onlymodel.fetch_modelswithverifyused it.What changes:
model.check_key: the stored section with the form's fields laid over it (test_provider(section_override=...)), probed with the credential check.refusedis the vendor's own refusal (credential_refused:invalid_keyon a 401, or on the 400 Google uses to name a key), the one answer that holds a key back. 403 is deliberately not a refusal: vendors answer it for a region, a plan or a permission, and a proxy answers it for anything.key_refused). Every other answer stores the key and the row carries the verdict at once, without the second test on the side; a check that cannot run (the gateway unreachable) stores the key and tests on the side as before. A stored key the vendor later refuses reads Key rejected on its row and in the detail head rather than Connected (new stringkey_rejected). The label readsrefused, not theinvalid_keystatus:model.fetch_modelswithverifynow answersrefused(optional, computed with the samecredential_refused), because the status table maps every 403 toinvalid_keyand a 403 can mean a region, a plan or a proxy -- that row keeps Connected in the warning colour, as before.raven onboardstep 1 andraven provider testpasscheck_credential=True. A list that cannot confirm a key (key_unchecked: a public catalogue with no key endpoint) is reported as reached but unconfirmed, and the wizard goes on to the test message, which is the check that remains.Not changed:
model.save_keyitself still stores what it is given (the TUI's picker and the Docker first-run bootstrap write through it unverified, as before); the wizard's CLI path still writes the key before verifying and offers "Re-enter key" on a refusal, as it did.Type
Verification
Python:
pytest tests/test_rpc_model.py tests/test_config_update_providers.py tests/test_cli_onboard_commands.py tests/test_cli_provider_commands.py tests/test_rpc_schema_match.py tests/test_rpc_registration.py -n 4-- 1408 passed on the first head, 778 on the second for the four files it touches (18 new: the override judged and nothing written, the override winning over the storedapiKeyspelling, the refusal table,model.check_keyrefusing/reporting/not writing, the wizard andprovider testpassing the check and wordingkey_unchecked). Full suitepytest tests -n 8: 34175 passed, 3 failed -- the local baseline (the libreoffice host check and two simulation cases, which fail the same way on main here).ui-web:
vitest run4205 passed on the first head, 528 for settings + rpc + gates on the second (7 connect-flow cases rewritten for ask-then-store, 4 new: refused key not saved and said in the form, a check that cannot run falls back to save-then-test, the detail head says Key rejected, a re-check refused on 401 reads Key rejected while a 403 keeps Connected);scripts/gates218 passed (fixture-paramsnames a refused key so the refusal's fields are drawn once);tsc --noEmitandeslintclean;gen:checkclean afternpm run gen.ui-tui:
gen:rpcandgen:i18nregenerated,lint:i18nclean,tsc --noEmitclean,vitest run2255 passed.CI on the first head: shard 4/4 was red for
tests/test_cli_system_ca.py(linux-only, so never run on this Mac), which asserted "custom OK" fromprovider testagainst a TLS stand-in whose list answers anyone -- the truthful line is now "reached, key not confirmed", and the test asserts the handshake either way; and fortest_acp_mirasim_server.py::test_a_failed_re_read_keeps_the_menus_in_hand_and_is_not_tried_on_every_session, a catalog read timing out on the runner (QUERY_TIMEOUT_S = 0.2), which this PR does not touch and which passes locally.Real CLI, same bogus OpenRouter key, before (installed 0.2.5) and after (this branch): "OK (468 models available)" -> "failed: invalid_key / Run: raven provider set openrouter --api-key ". Against two local fake vendors: a list that 401s a wrong bearer ->
invalid_key, the right bearer -> "OK (2 models available)"; a list that answers anyone -> "reached, key not confirmed".raven onboardwith the bogus key: "Auth failed: the API key is invalid" and the Re-enter key menu, where before it printed "Connected! (468 models available)".Real page (isolated gateway from this branch, empty home, first-run wizard, headless Chrome): bogus OpenRouter key -> the form stays open with the refusal line, provider count stays 0, no key in config.json; strict fake vendor with a wrong key -> refused the same way; the right key -> Connected / Verified, 2 models, key in config; public fake vendor -> stored with "this provider's model list doesn't check keys ... saved". The gateway and the fake vendors were stopped by pid afterwards.
Relevant tests pass locally
Relevant lint / type checks pass locally
User-facing docs or screenshots are updated when needed (CHANGELOG;
commands.mdalready says "test connectivity" and stays)Risk
model.check_key, over the same signed-in RPC channelmodel.save_keyuses) and is laid over the section in memory only; the probe never logs it, and the decoy check sends a constant that is no vendor's key.model.save_keyandtest_provider's default behaviour are unchanged;check_credentialadds one request for a vendor whose list refuses the decoy and two for one that does not, on the two CLI paths and the page's connect only, not on catalogue listing. A vendor that 403s a wrong key is still stored and marked, as before this PR.Related Issues
N/A