Skip to content

fix(*): a key the vendor refuses is not called connected - #1045

Open
Handsome-wzw wants to merge 6 commits into
mainfrom
fix/vendor_refused_key
Open

Handsome-wzw wants to merge 6 commits into
mainfrom
fix/vendor_refused_key

Conversation

@Handsome-wzw

@Handsome-wzw Handsome-wzw commented Oct 21, 2026 •

Copy link
Copy Markdown
Contributor

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:

  • the settings page and the first-run wizard stored the key, marked the row Connected and wrote "the provider rejects the saved key" beside it;
  • raven onboard printed "Connected! (468 models available)" and the first message then failed with an auth error;
  • raven provider test openrouter printed "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, /key for OpenRouter); only model.fetch_models with verify used it.

What changes:

  • A new read, model.check_key: the stored section with the form's fields laid over it (test_provider(section_override=...)), probed with the credential check. refused is the vendor's own refusal (credential_refused: invalid_key on 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.
  • The page's connect asks the vendor first. A refused key is not stored and the form says why, in the form (new string 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 string key_rejected). The label reads refused, not the invalid_key status: model.fetch_models with verify now answers refused (optional, computed with the same credential_refused), because the status table maps every 403 to invalid_key and a 403 can mean a region, a plan or a proxy -- that row keeps Connected in the warning colour, as before.
  • raven onboard step 1 and raven provider test pass check_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_key itself 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

  • Fix
  • Feature
  • Docs
  • CI / tooling
  • Refactor
  • Other

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 stored apiKey spelling, the refusal table, model.check_key refusing/reporting/not writing, the wizard and provider test passing the check and wording key_unchecked). Full suite pytest 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 run 4205 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/gates 218 passed (fixture-params names a refused key so the refusal's fields are drawn once); tsc --noEmit and eslint clean; gen:check clean after npm run gen.

  • ui-tui: gen:rpc and gen:i18n regenerated, lint:i18n clean, tsc --noEmit clean, vitest run 2255 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" from provider test against 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 for test_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 onboard with 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.md already says "test connectivity" and stays)

Risk

  • Security impact considered -- the key a form holds now travels to the gateway once more (in model.check_key, over the same signed-in RPC channel model.save_key uses) 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.
  • Backward compatibility considered -- model.save_key and test_provider's default behaviour are unchanged; check_credential adds 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.
  • Rollback path is clear for risky changes -- revert; no stored data changes shape.

Related Issues

N/A

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>
@Handsome-wzw
Handsome-wzw requested a review from 0xKT October 21, 2026 06:58

@gloryfromca gloryfromca left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 gloryfromca left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@0xKT

0xKT commented Oct 21, 2026

Copy link
Copy Markdown
Member

Merge conflict: @Handsome-wzw this PR no longer merges onto main. git merge-tree of 96103031 onto the current tip 4ec77de0 conflicts in CHANGELOG.md only.

@LivXue

LivXue commented Oct 22, 2026

Copy link
Copy Markdown
Member

Windows compatibility review: no new Windows-specific issue found in the inspected changes.

Scope: all 27 changed files at head 961030317bd38efc946a9ffac5e42adb4a24541d, against base/merge base d9c0a1d74222825ad621a0fd138b691c0edd537d. Read the complete local diff, credential probe and configuration readers, CLI callers, off-thread RPC check, Web connect/recheck consumers, generated Web/TUI clients, tests and all discussion pages. The new check reuses the existing synchronous HTTP client through asyncio.to_thread; no new OS/process/path primitive or dependency was introduced. The earlier 401/403 distinction is carried by refused, as covered in the resolved discussion.

Native Windows verification (Python 3.13.12; Node 25.9.0/npm 11.12.1; isolated detached checkouts/environments):

  • First Python selection: 457 passed, 1 failed, stopping at the unchanged OAuth test's stat.S_IMODE(...) == 0o600 assertion (test_cli_provider_commands.py:164). The same test fails identically at the exact base. This is an inherited POSIX-mode test limitation; that result does not establish Windows ACL exposure.
  • Remaining focused CLI/RPC/schema/CA selection: 535 passed, 4 skipped. The skips explicitly require Linux's stand-in certificate store.
  • Web: 103 passed; TypeScript and generated-client checks exited 0 (249 methods). Diff check and environment setup also exited 0.

Exact commands ($a names the task-owned audit root; Python selections ran in the corresponding head/base checkout with PYTHONPATH pinned there):

$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..961030317bd38efc946a9ffac5e42adb4a24541d

Coverage 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.

@Handsome-wzw

Copy link
Copy Markdown
Contributor Author

Merged origin/main (018f984) into the branch as 3400ce9, no rebase: the one conflict was CHANGELOG.md, where both sides had added entries at the top of ### Fixed; all are kept, this PR's first. The generated clients were regenerated on the merged tree and match the committed files (gen:check clean, gen:rpc / gen:i18n byte-identical). Re-run on the merge: the six Python files the change touches plus test_cli_system_ca.py, 1422 passed; ui-web settings + rpc + gates, 547 passed, tsc clean; ui-tui lint:i18n, tsc and vitest (2277 passed). The three branch gates (commit messages, source language, large files) pass on origin/main..HEAD.

@gloryfromca gloryfromca left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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:233 and ProviderDetail.test.tsx:327 expect exactly ['checkKey', 'provider'];
  • the base assertion expect(calls).toContainEqual(['fetchModels:verify', saved.slug]) was removed;
  • SetupBodies.test.tsx:320 still passes because it counts the reloadProviders call 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 from checkKey shown 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.

@0xKT

0xKT commented Oct 26, 2026

Copy link
Copy Markdown
Member

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:

  1. model.check_key can judge a key other than the one typed. section_override lays api_key over the flat field (update_providers.py:1433-1434). test_provider then picks its key through provider_endpoints (:1448-1450), and a stored endpoints list or Gemini's api_key_list replaces the flat key outright, as model_reveal_key already says at model.py:682-685. Reproduced with a loopback stub vendor through the production handler:

    • endpoints [sk-EP-REVOKED], typed good key: the wire carries Bearer sk-EP-REVOKED and the result is refused: True, so the page says "{vendor} rejected this key, so it was not saved" about a key that was never sent;
    • endpoints [sk-EP-GOOD], typed bad key: the result is valid, and the bad key is stored with the row marked valid;
    • controls with only a flat key put the typed key on the wire, as intended.

    A flat key saved under such a section was already unused at runtime on main, so the harm is a false verdict, not a broken runtime. Not blocking.

  2. An address-only connect of a local server can be refused as "rejected this key". For a local provider the form sends {slug, api_base} with no key (Providers.tsx:221-227; hosted_vllm draws no key field at all). A server behind a token answers the keyless GET /v1/models with 401, and credential_refused returns True for a credential that was never sent. Reproduced: server_saw=[('/v1/models', None)] -> refused: True. The address is then not stored, and the message asks the person to fix a key they do not have.

    The same rule splits one more way. When a stale key is stored, model_save_key clears it for this submission (model.py:633-644), but model_check_key overrides only when a key was sent (:769-773), so the stale key is sent to the new address and judged. Such a server refuses keyless requests at runtime anyway, so not blocking. One reading of "what this submission carries" for both check_key and save_key would fix 1 and 2 together.

Nits:

  • Nullable fields in the schema. rpc-schema/openrpc.json:1329-1337 declares http_status / models_count / error as bare integer and string, but model_check_key always sends them, mostly as null. A schema-validating client would reject most answers; store.ts uses ?? null, so nothing breaks today.
  • The timeout is per request. _CHECK_TIMEOUT_S (model.py:745) is described as a bound on the whole check but is an httpx per-request timeout. A public-catalogue vendor gets up to three sequential requests (probe, decoy, /key). Mutant "use _FETCH_TIMEOUT_S instead" survives the tests.
  • Decoy failures still read as valid. provider_commands.py:494-496 ("never read as a valid key") and the CHANGELOG line "a list that cannot confirm a key says so" do not hold when the decoy request gets anything but 200: _confirm_credential returns the original valid (update_providers.py:1741-1743). This is unchanged from main for that input.
  • Contradictory onboard lines. _pick_model with unchecked and an empty list prints "its model list answers without checking the key" and then "This provider has no model list to fetch" (onboard_commands.py:891). The mutant that drops "unchecked" from that tuple survives the tests.
  • Missing zh entries. The two new raven onboard lines (onboard_commands.py:764, :770) have no entry in raven/i18n/zh.py, so they print in English under zh while their neighbours are translated.
  • Wrong button name. gui.settings.providers.key_refused says "connect again" (and its zh text quotes the Connect button by name), but on a connected provider's page the button reads Update (ProviderDetail.tsx:146). The sibling key_unchanged takes {button} for this.
  • Status list incomplete. The ModelCheckKeyResult.status description omits unknown_provider, oauth_token_missing and the handler's own unknown fallback (model.py:784).
  • CHANGELOG overstates. "A stored key the vendor later refuses reads Key rejected on its row" holds only after someone presses Check in that session. Row probes are filled only by connect and recheck, and nothing fills them on load.

@gloryfromca gloryfromca left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@LivXue

LivXue commented Oct 26, 2026

Copy link
Copy Markdown
Member

Windows compatibility review — PR #1045 @ 3400ce9c08c022b1e9ded2ea0196d2001f0cea53

Scope: base 018f984dd963 → this head, all 27 changed files, re-read at this exact head. The head moved from the previously Windows-reviewed 96103031 by merging main (3400ce9c0); this pass re-verifies at the new head rather than repeating that review.

Delta: the merge changed generated RPC/UI files and shared UI modules; the PR's own probe and RPC logic (raven/config/update_providers.py, raven/rpc/methods/model.py, raven/cli/provider_commands.py, raven/cli/onboard_commands.py) carries no new code across it. I re-read the PR's Python and UI diff at this head anyway.

Result: no Windows-specific issue found.

What I checked: section_override/_with_override and credential_refused are pure mapping/string logic; model.check_key reuses the existing synchronous probe through asyncio.to_thread and introduces no OS, process, path, or encoding primitive; the CLI check_credential=True paths and the UI consumers use the same RPC shape; the test_cli_system_ca.py change only relaxes an assertion in a Linux-only test.

Verification (native Windows 11, CPython 3.12.13, isolated worktree D:\tmp\rv-1045 at this head, scratch HOME/USERPROFILE, PYTHONUTF8=1):

pytest tests/test_cli_provider_commands.py tests/test_rpc_model.py \
       tests/test_cli_system_ca.py tests/test_config_update_providers.py -q
→ 5 failed, 532 passed, 4 skipped in 35.56s

vitest run src/features/settings/SetupBodies.test.tsx \
            src/features/settings/providers/ProviderDetail.test.tsx \
            src/features/settings/source.test.ts
→ 3 files, 105 passed

All five Python failures are inherited: the same IDs fail at base 018f984dd (verified there). Three are POSIX S_IMODE == 0o600 assertions on this NTFS host; two are the WinINET-system-proxy trap (httpx's default trust_env=True routes the 127.0.0.1 TLS fixture through the host's registry proxy, so the handshake reads as network_error). The four skips are linux_only system-certificate-store tests.

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.

Handsome-wzw and others added 2 commits October 27, 2026 04:18
…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 gloryfromca left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@Handsome-wzw

Copy link
Copy Markdown
Contributor Author

On the blocking thread at ui-web/src/features/settings/store.ts (a successful web connect no longer wrote the served-models cache), fixed in d9f50ce after merging origin/main again as 6860d0e (the one conflict was CHANGELOG.md, both sides kept):

  • Taken the second way out: the connect's check is itself the catalogue read that remembers. model.check_key now asks test_provider for the whole catalogue (full_catalogue=True) and, when the vendor answered, writes what it listed through the same rows model.fetch_models builds -- the row building and the write moved into _served_rows / _remember_served, which both handlers call -- so there is still one probe per connect and one writer of cache/served-models.json, and store.ts:444-446 plus the handler's docstring are true again. The web flow is unchanged (checkKey, save_key, reloadProviders), so the two web tests that pin it stand.
  • Pinned by test_check_key_remembers_what_a_valid_key_serves (a valid key with zz-live-only-model leaves recall(slug) listing it, the probe was asked for the full catalogue, the config is still not written), and the refused and non-refusing cases now also assert that nothing is remembered.

From the non-blocking notes in the same pass, taken up in the same commit: the ModelCheckKeyResult schema declares http_status / models_count / error nullable and lists unknown_provider, oauth_token_missing and unknown among the status words (clients regenerated); gui.settings.providers.key_refused names no button any more (it reads Connect on the form and Update on a connected provider's page); the two raven onboard lines about an unchecked key have their zh entries; the CHANGELOG now says a row reads Key rejected after the key is checked again. Left for later, as they need a design of their own rather than a line: the check judging a stored endpoints key instead of the typed one (1), the address-only connect of a token-protected local server (2) and the stale-key send to a new address, the per-request timeout, decoy failures still reading as valid, and the contradictory _pick_model lines.

Verification on the new head: the six Python test files this PR touches plus test_rpc_schema_match.py, 1413 passed; diff coverage 100.00% (69/69 changed lines); ruff check, ruff format --check, ty check on the touched modules clean; ui-web settings domain and every gate (70 files / 473 tests), type-check clean; ui-tui lint:i18n, lint:rpc, type-check clean; the three repository gates pass. Full pytest tests on the head: 35971 passed, 255 skipped, 2 xfailed, 3 failed -- the three are this machine's baseline, failing the same way on a clean origin/main worktree with the same venv (test_rpc_files::test_a_host_without_libreoffice_says_so, test_simulation_scenario::test_a_fresh_trial_refreshes_the_subagent_homes_before_it_runs, test_simulation_suite::test_a_stopped_run_is_marked_in_its_record_and_finished_with_its_evidence).

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.

@Handsome-wzw
Handsome-wzw requested a review from 0xKT October 27, 2026 03:30
@LivXue

LivXue commented Oct 27, 2026

Copy link
Copy Markdown
Member

Windows compatibility re-review — PR #1045 @ d9f50ce79e4a6e0f7c88493dd8d4764e8670d991

Scope: delta from the previously reviewed head 3400ce9c — the PR's own 28 files, of which 10 changed (+194/−61): raven/rpc/methods/model.py (row building factored into _served_rows, shared by model_fetch_models and model_check_key; a successful connect check now remembers the live catalogue), rpc/models.py + schema, i18n, generated clients, and tests/test_rpc_model.py. The rest of the PR was covered by the 3400ce9c review.

Result: no Windows-specific issue.

  • The delta is provider-catalogue and RPC logic: a network round trip through the existing test_provider, row shaping from describe/catalogue_for, and served_models.remember off-thread. No subprocess, path, filesystem, permission, encoding, or signal primitive is added; the one scan hit was a docstring sentence.
  • The CLI/onboard and UI parts of the PR are unchanged in this delta; the earlier review's conclusions for them still hold.

Verification (native Windows 11, CPython 3.12.13, isolated worktree D:\tmp\rv-1045 at this head, scratch HOME/USERPROFILE, PYTHONUTF8=1):

pytest tests/test_rpc_model.py tests/test_config_update_providers.py -q
→ 2 failed, 453 passed in 24.47s

Both failures are the inherited WinINET-system-proxy TLS tests (test_a_codex_catalogue_behind_an_untrusted_certificate_is_named_for_what_it_is, test_a_minimax_token_renewal_behind_an_untrusted_certificate_is_named_for_what_it_is): with the host's registry proxy on, httpx routes the 127.0.0.1 fixture through it. They fail identically at base 4718421324 and were reported in the earlier review; tests/test_rpc_model.py passes in full.

Limitations: no live vendor endpoint; the settings UI was not re-run (unchanged in this delta); the TLS fixture limitation is environmental, not code.

@0xKT

0xKT commented Oct 27, 2026

Copy link
Copy Markdown
Member

Merge conflict: @Handsome-wzw this PR no longer merges cleanly into main.

Measured just now with git merge-tree --write-tree of main 26352da0 and this head d9f50ce7. Main moved with #1110 (Agent Plugins loader), #1064 (conditional branches) and #1066 (DAG file references), each of which changed CHANGELOG.md. Conflicting files: CHANGELOG.md.

After a rebase onto current main I re-check what moved before the grade comes back.

@gloryfromca gloryfromca left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 --prefix and selected no files; it was rerun with package-relative paths.
  • Web RPC generation and type-check passed.
  • TUI RPC and i18n generation checks passed.

@LivXue

LivXue commented Oct 27, 2026

Copy link
Copy Markdown
Member

Windows compatibility re-review — PR #1045 @ 2ac4ec686a27097a0afae312056e7f356f2ad6a4

Scope: the head moved from the previously reviewed d9f50ce7. The PR's own code — model.check_key/model_fetch_models, section_override, credential_refused, the CLI/onboard wiring and their tests — is unchanged; the only change among its files is main's #1119 SessionUsageResult.context_used description, which this PR does not own. The rest of the large delta is main's merges.

Result: no Windows-specific issue; the previous conclusion holds.

Verification (native Windows 11, CPython 3.12.13, isolated worktree D:\tmp\rv-1045-n2 at this head, scratch HOME/USERPROFILE, PYTHONUTF8=1):

pytest tests/test_rpc_model.py tests/test_config_update_providers.py -q
→ 2 failed, 453 passed in 19.96s

Both failures are the inherited WinINET-system-proxy TLS tests (httpx's default client routes the 127.0.0.1 fixture through the host's registry proxy on this machine); they fail identically at base and were reported in the earlier review.

Limitations: no live vendor endpoint; the settings UI was not re-run (unchanged).

@0xKT

0xKT commented Oct 30, 2026

Copy link
Copy Markdown
Member

Merge conflict: @Handsome-wzw, this PR no longer merges onto the current main tip 9d9bec90 (the #1125 merge). Measured with git merge-tree --write-tree 9d9bec90 2ac4ec68 -> exit 1, one conflict:

  • tests/test_cli_onboard_commands.py

The judge's verdict on 2ac4ec68 is clean, so a rebase onto main is what stands between this and the acceptance queue.

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants