Repository navigation
fix(*): close the follow-ups from four merged pull requests - #940
Conversation
gloryfromca
left a comment
gloryfromca
left a comment
There was a problem hiding this comment.
Blocking: support the accepted ollama-chat/ spelling in the endpoint cache and no-fetch guard.
I reviewed the full github/main...HEAD diff and the surrounding production callers/history. I covered the repository rules and Runtime context, all changed code and tests, caller behavior, backward compatibility, test-strength concerns, and the relevant provider/config/ACP architecture boundaries. I found one concrete blocker, posted inline; I found no other plain errors worth raising.
Verification: uv run --frozen --python 3.12 --all-packages pytest tests/test_provider_rates.py tests/test_config_self_surface.py tests/test_rpc_settings.py tests/test_subagent_host_env.py tests/test_subagent_probe.py -q passed (485 tests), and tests/test_subagent_acp.py::test_the_connect_records_what_a_test_would passed. The requested --all-extras environment could not be built here because datrie needs the unavailable x86_64-linux-gnu-gcc; that is an environment/toolchain failure, not a test failure. git diff --check also passed.
|
Windows compatibility review: no newly introduced Windows compatibility issue found in the inspected scope. One inherited ACP teardown failure limits the broader run. Scope: all 13 changed files at Native Windows / Python 3.13.12; reused uv environments with identical $R = 'C:/Users/livxu/AppData/Local/Temp/raven-windows-review/0d8ab4cfce454f15b5d09028fe345dbb'
uv run pytest tests/test_provider_rates.py -n 0 -x --tb=short --basetemp "$R/checks/940-provider/pytest" --junitxml "$R/checks/940-provider/junit.xml"
# 110 passed, including local loopback-server metadata/cache checks.
uv run pytest tests/test_config_self_surface.py tests/test_rpc_settings.py::test_settings_leaf_write_lands_on_the_alias_pydantic_reads -n 0 -x --tb=short --basetemp "$R/checks/940-config/pytest" --junitxml "$R/checks/940-config/junit.xml"
# 125 passed.
uv run pytest tests/test_subagent_host_env.py -n 0 -x --tb=short --basetemp "$R/checks/940-env/pytest" --junitxml "$R/checks/940-env/junit.xml"
# 1 failed: AcpClient.close calls unavailable os.killpg; stopped at the first test.
uv run pytest tests/test_subagent_host_env.py -k 'not acp_child and not windows_probe' -n 0 -x --tb=short --basetemp "$R/checks/940-env-focused/pytest" --junitxml "$R/checks/940-env-focused/junit.xml"
# 27 passed, 3 deselected; includes the new expansion-memo regression.
uv run pytest tests/test_subagent_probe.py::test_a_record_written_while_the_boot_verify_ran_outranks_its_late_verdict tests/test_subagent_probe.py::test_a_modes_relearn_during_the_boot_verify_does_not_stop_its_write tests/test_subagent_acp.py::test_the_connect_records_what_a_test_would -n 0 -x --tb=short --basetemp "$R/checks/940-verify/pytest" --junitxml "$R/checks/940-verify/junit.xml"
# 6 passed; emitted a PytestUnraisableExceptionWarning from a Windows Proactor pipe transport destructor.The broader environment run reaches client.py:315, whose unconditional The five new concurrent-measurement cases passed with real temporary snapshot writes. Config tests cover alias/null handling, distinct map-entry keys and actual persisted/live values. The existing provider-spelling finding remains separate and is not duplicated. Limits: no full repository suite, Windows registry modification, ACL audit or external provider/agent session. The observed teardown failure and transport warning prevent a clean broad Windows verdict. |
The cache-only gate skipped get_model_info for any id with an Ollama segment, including ids LiteLLM answers from its table, so ollama/Llama3, litellm_proxy/ollama/llama3 and github/ollama/llama3 lost the window main reads offline. The gate now puts LiteLLM's own precondition to it, keyed on the caller's id: the id routes to Ollama and no name derived from it is in the table. A LiteLLM without those helpers gets the cautious answer.
The boot read a row before its handshake and wrote its verdict from that read, so a Test recorded while the handshake ran was overwritten by a late timeout, refusal or pass. It now reads the row again right before the write and leaves a newer measurement in place. A modes relearn keeps the record's stamp, so it does not stop the write, and a failure is kept over the re-read record.
capabilities_wanted and four test docstrings still described the rule this branch replaced.
Below the top level pydantic reads the camelCase alias whenever it is present, null or not, so preferring a non-null spelling there sent a write to a key the schema ignores, and load_config and the live readers then disagreed. Only the top level drops a null block, so only there does a non-null spelling win; below it the first spelling present is the one read. Every caller now states which case it is in.
A branching reference graph reads each name once. Without the memo the output is the same and the reads grow on every level, which no test saw.
A stored Path that cannot be expanded fails the capture too.
A caller that may fetch resolves a local model's window by asking its daemon, and one that may not -- the binding, read inside a running turn on the event loop -- answered the 200000-token default for the same model, on every iteration. The gauge already asked; it just discarded what it learned. The answer is now kept per process, under the id the request goes out under, so ollama/ and ollama_chat/ share one entry, and a cache-only read finds it before deciding the endpoint is unreachable. Only a real figure is stored. A daemon that is down comes back as a row of nulls, and keeping that would answer the default for the life of the process, which is the one state the daemon coming up later cannot correct. The output ceiling is deliberately not stored beside it: the driver writes one number into both fields, so a stored ceiling always equals the stored window and _trustworthy_ceiling rejects it as a window filed twice. The caller bounds its own fallback by the window instead.
…ntry Below the top level key_as_written falls back to the other spelling of a name, which is right for a schema field and wrong for a map entry: a path naming an MCP server reaches the server called that, or nothing. Falling back disabled the server called fooBar when the caller asked for foo_bar. The catalog knows which segments a path binds -- it is the only thing that named them -- so the reader and the writer ask it once per path, and a bound segment is matched as written. providers keeps the tolerant rule, because its runtime reader folds those spellings through names_same_provider: there the section that gets read is the one the write has to find. Found in review of #923: colliding custom ids plus a null target, where the write selected the neighbour and returned its previous value.
Normalize endpoint vendor segments before reading the memo or deciding whether a metadata lookup may contact the daemon. Translate the stored ollama-chat prefix at both metadata entry points, using the registry's LiteLLM spelling and preserving ollama/ for its offline table rows. Cover the stored id from configuration through one daemon request to cache-only lookups and ModelBinding. Clear LiteLLM's metadata cache in the daemon parameter cases so aliases cannot reuse another case's answer.
65adf6e to
6b27299
Compare
gloryfromca
left a comment
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 final github/main...HEAD diff and the new fix against the prior blocker. The stored ollama-chat/ path now reaches LiteLLM under ollama_chat/, reuses the endpoint window for cache-only and ModelBinding reads, and preserves ollama/ static-table lookup behavior. The original thread is resolved.
Coverage this round included the changed provider callers and registry normalization contract, backward compatibility across all three Ollama spellings, no-fetch network behavior, regression-test strength, the rebased final diff, and repository/Runtime architecture rules. I found no new plain error.
Verification at 6b2729988e24: tests/test_provider_rates.py passed 114 tests; the focused config, ACP, and Windows set passed 376 tests; git diff --check github/main...HEAD passed. Patch-risk assessment: merge, human review required (moderate bounded runtime impact, low regression likelihood, strong focused protection, easy revert, high confidence).
|
Windows compatibility re-review: no additional Windows issue found in the inspected changes. Scope: all 13 changed files at Native Windows / Python 3.13.12: 272 passed across four focused runs; 3 environment cases deselected. The 114 provider tests include a real loopback HTTP stand-in: the stored Fresh isolated uv environment, detached exact-head checkout, imports verified, and separate home/temp/token directories. Setup $R = 'C:/Users/livxu/AppData/Local/Temp/raven-windows-review/615c117f238046cca9805459d4b37a77'
uv run pytest tests/test_provider_rates.py -n 0 -x --tb=short --basetemp "$R/checks/940-provider/pytest" --junitxml "$R/checks/940-provider/junit.xml"
# 114 passed
uv run pytest tests/test_config_self_surface.py tests/test_rpc_settings.py::test_settings_leaf_write_lands_on_the_alias_pydantic_reads -n 0 -x --tb=short --basetemp "$R/checks/940-config/pytest" --junitxml "$R/checks/940-config/junit.xml"
# 125 passed
uv run pytest tests/test_subagent_host_env.py -k 'not acp_child and not windows_probe' -n 0 -x --tb=short --basetemp "$R/checks/940-env-focused/pytest" --junitxml "$R/checks/940-env-focused/junit.xml"
# 27 passed, 3 deselected
uv run pytest tests/test_subagent_probe.py::test_a_record_written_while_the_boot_verify_ran_outranks_its_late_verdict tests/test_subagent_probe.py::test_a_modes_relearn_during_the_boot_verify_does_not_stop_its_write tests/test_subagent_acp.py::test_the_connect_records_what_a_test_would -n 0 -x --tb=short --basetemp "$R/checks/940-verify/pytest" --junitxml "$R/checks/940-verify/junit.xml"
# 6 passed; Windows Proactor pipe destructor emitted PytestUnraisableExceptionWarningThe three exclusions are the two real ACP-child cases and the extensionless executable-discovery fixture described in the previous review. The ACP client blob is still identical to base ( No full repository suite, real registry modification, external provider/agent session or ACL audit. This is a focused Windows result. |
3 participants
Summary
Follow-ups to the review threads left on four merged pull requests: #909, #912, #915 and #923. Each is a defect the review named, reproduced before any change, and repaired with a test that fails against the old code. This also addresses the Ollama spelling issue raised in this PR's review.
A cache-only window lookup no longer skips a model LiteLLM answers from its table (from #909). The gate that keeps metadata lookups off a model's own server skipped
get_model_infofor any id carrying an Ollama segment, including ids LiteLLM answers offline:ollama/Llama3,litellm_proxy/ollama/llama3andgithub/ollama/llama3were sized on the 200000-token fallback where main read 8192. The gate now puts LiteLLM's own precondition to LiteLLM, keyed on the caller's id, and answers cautiously only for an id with an Ollama segment when the installed release does not ship that check.A window the model's own server reported is reused inside the turn (the second item of #909). A caller allowed to fetch resolved a local model's window by asking its server and threw the answer away; the next caller, which may not fetch, answered the fallback for the same model on every iteration, for the life of the process. The answer is now kept under the id the request goes out under, so
ollama/,ollama_chat/and the storedollama-chat/spelling share one entry. The table is consulted before the memo, so a row LiteLLM ships still outranks a previous ask; only a real figure is stored, so a server that was down is asked again.A measurement taken while the boot verify ran is no longer overwritten (from #915). The boot read a row before its handshake and wrote its verdict from that read, so a Test recorded during the handshake was replaced by a late timeout, refusal or pass. It reads the row again immediately before the write and leaves a newer measurement in place. The comparison is on
measured_at_ms, so a modes relearn does not stop the write. Four docstrings that described the replaced rule are corrected.The Windows environment capture's memo is pinned, and its docstring names the second failure mode (from #912).
A key is written under the spelling the loaders read (from #923). Below the top level pydantic reads the camelCase alias whenever it is present, null or not, so preferring a non-null spelling sent a write to a key the schema ignores, and
load_configand the live readers then disagreed. Only the top level drops a null block; below it the first spelling present is the one read, and every caller now states which case it is in.A write aimed at a wildcard map entry reaches that entry (the follow-up #923's review asked for). Below the top level the spelling helper fell back to another spelling of a key, which is right for a schema field and wrong for a map entry: with only
tools.mcpServers.fooBarpresent, a write totools.mcpServers.foo_bar.enabledselectedfooBar, disabling a server the caller never named. The catalog knows which segments a path binds, so a bound segment is now matched as written.providerskeeps the tolerant rule, because its runtime reader folds those spellings throughnames_same_provider.Configured Ollama models reach the metadata driver under its accepted spelling. The wizard and model picker store Ollama ids as
ollama-chat/<model>, while LiteLLM expectsollama_chat/. The window and output-ceiling lookups now translate that endpoint prefix using the provider registry before consulting the candidate list. The memo and no-fetch prefilters use the same registry normalization. Theollama/completion prefix is preserved because LiteLLM keys its offline table rows under it; non-Ollama prefixes are unchanged.Type
Verification
Latest review fix (Linux, Python 3.12.13, LiteLLM 1.85.0; proxy variables unset; consumer tests rechecked after rebase onto current main):
stored_model_idthat could not reach the daemon.uv run --no-sync pytest tests/test_provider_rates.py -q -n 0 --tb=short: 114 passed after the fix.uv run --no-sync pytest -q --tb=short <consumer-test-files> --cov=raven.providers.rates --cov-branch --cov-report=json:<coverage-json>: 1312 passed, 1 skipped. The selection contains 16 test modules importingraven.providers.rates, plustest_provider_registry_lookup.pyandtest_provider_wire_model.py. The skip is the permission test that cannot deny a root process withchmod 000; existing Pydantic warnings remain.parse_changed_linesandcalculate_diff_coverageto the fix commit after rebase yields 100% (13/13 executable changed lines). This is coverage of this fix, not a new full-PR coverage or full-suite ratchet result.wire_model(which loses theollama/table rows). The worktree was not rewritten by the mutation checks.UV_NO_SYNC=1 make lint-python lint-imports lint-deps lint-types: passed after rebase; 2268 Python files formatted, 10 import contracts kept, no dependency issues. Both commit-message validators and the source-language and large-file gates also passed.uv run --no-sync ty check raven/providers/rates.py tests/test_provider_rates.pyalso passed.ollama-chat/qwen2.5id makes no request in cache-only mode before a fetch. One allowed/api/showrequest learns 8192, after which the cache-only lookup andModelBinding.context_windowboth return 8192 without another request, and the output limit is 4096.Earlier author verification, before this review fix:
Commands recorded at the original PR revision and main baseline, with proxy variables unset:
3 failed, 29796 passed, 158 skipped, 2 xfailed. A same-day run of the original main control gives the same 3 failed with 29771 passed, so the failure set is identical and the 25 extra passes are the tests added here. The three are environment-local:
test_resolve_node_dir_answers_each_case_it_exists_for(npm on PATH),test_a_legacy_workbook_is_converted_and_read(LibreOffice), andtest_what_cannot_be_read_names_nothing(load-dependent).Focused runs on the changed files:
tests/test_provider_rates.py110 passedtests/test_config_self_surface.pytests/test_rpc_settings.pytests/test_config_update.pytests/test_raven_config_tool.pytests/test_rpc_console.py805 passedtests/test_subagent_host_env.py30 passedGates:
ruff check,ruff format --check,ty check,lint-imports,deptry,scripts/check_commit_messages.py origin/main..HEAD,scripts/check_source_language.py origin/main..HEADandscripts/check_large_files.py origin/main..HEADall pass.pre-commit run --all-filesfails two hooks on three files this branch does not touch; the same three fail the same way at the base, so they are pre-existing.Every new test was checked by reverting the fix: six single-construct mutants over the two repaired rules, each restored and compared after running. Two survived the first pass and both were findings rather than tests to tighten. The guard that refused a zeroed window was unreachable (the one caller already has a window), so the parameter is required instead of optional. And the memo originally ranked above LiteLLM's own table, so a previous ask could have overridden a row a caller could have read offline; the table now comes first.
Reproduced end to end against a stand-in server on a loopback port: one
/api/showrequest answers 8192,ollama/qwen2.5read withallow_fetch=Falseanswers 8192 with no second request, andModelBinding.context_windowon that model answers 8192 where it read the fallback before.Risk
No stored config changes shape or meaning. The alias-spelling fix changes where a write lands when a file carries both spellings of one key, which is the file that did not load correctly before. The wildcard fix changes where a write lands when two map entries differ only by spelling; both are left as written, where the old code silently edited the neighbour.
The window memo is process-wide and holds context-window and optional output-limit counts per model, cleared by the same
reset_openrouter_cachethe catalogue uses. Storedollama-chat/ids can now make a metadata request to the configured daemon when fetch is allowed; cache-only callers continue to reuse local data. Other hyphenated provider prefixes are outside this fix. Roll back by reverting the merged change; the memo is not persisted.Related Issues
#909, #912, #915, #923