Skip to content

Commit ea9bb50

Browse files
authored
fix(*): close the follow-ups from four merged pull requests (#940)
## 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_info` for any id carrying an Ollama segment, including ids LiteLLM answers offline: `ollama/Llama3`, `litellm_proxy/ollama/llama3` and `github/ollama/llama3` were 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 stored `ollama-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_config` and 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.fooBar` present, a write to `tools.mcpServers.foo_bar.enabled` selected `fooBar`, disabling a server the caller never named. The catalog knows which segments a path binds, so a bound segment is now matched as written. `providers` keeps the tolerant rule, because its runtime reader folds those spellings through `names_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 expects `ollama_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. The `ollama/` completion prefix is preserved because LiteLLM keys its offline table rows under it; non-Ollama prefixes are unchanged. ## Type - [x] Fix - [ ] Feature - [ ] Docs - [ ] CI / tooling - [ ] Refactor - [ ] Other ## Verification - [x] Relevant tests pass locally - [x] Relevant lint / type checks pass locally - [ ] User-facing docs or screenshots are updated when needed Latest review fix (Linux, Python 3.12.13, LiteLLM 1.85.0; proxy variables unset; consumer tests rechecked after rebase onto current main): - New and strengthened regression cases ran before the fix: 6 failed / 108 passed, covering the memo spelling, both no-fetch metadata lookups, the conservative guard fallback, and an id produced by `stored_model_id` that 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 importing `raven.providers.rates`, plus `test_provider_registry_lookup.py` and `test_provider_wire_model.py`. The skip is the permission test that cannot deny a root process with `chmod 000`; existing Pydantic warnings remain. - Applying the repository's `parse_changed_lines` and `calculate_diff_coverage` to 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. - Six independent in-memory mutants were caught: raw memo prefilter, raw endpoint guard, raw shared predicate, omitted window spelling conversion, omitted ceiling spelling conversion, and replacing only the endpoint conversion with `wire_model` (which loses the `ollama/` 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.py` also passed. - Against a loopback stand-in daemon, the actual stored `ollama-chat/qwen2.5` id makes no request in cache-only mode before a fetch. One allowed `/api/show` request learns 8192, after which the cache-only lookup and `ModelBinding.context_window` both return 8192 without another request, and the output limit is 4096. - The daemon-path parameter cases clear LiteLLM's own id-keyed cache so alias cases cannot inherit a previous case's successful response. Earlier author verification, before this review fix: Commands recorded at the original PR revision and main baseline, with proxy variables unset: ``` uv run --frozen --python 3.12 --all-extras pytest -n 4 -q ``` 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), and `test_what_cannot_be_read_names_nothing` (load-dependent). Focused runs on the changed files: - `tests/test_provider_rates.py` 110 passed - `tests/test_config_self_surface.py` `tests/test_rpc_settings.py` `tests/test_config_update.py` `tests/test_raven_config_tool.py` `tests/test_rpc_console.py` 805 passed - `tests/test_subagent_host_env.py` 30 passed Gates: `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..HEAD` and `scripts/check_large_files.py origin/main..HEAD` all pass. `pre-commit run --all-files` fails 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/show` request answers 8192, `ollama/qwen2.5` read with `allow_fetch=False` answers 8192 with no second request, and `ModelBinding.context_window` on that model answers 8192 where it read the fallback before. ## Risk - [x] Security impact considered - [x] Backward compatibility considered - [x] Rollback path is clear for risky changes 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_cache` the catalogue uses. Stored `ollama-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
1 parent e3d0135 commit ea9bb50

13 files changed

Lines changed: 817 additions & 62 deletions

File tree

‎raven/agent/subagent/backends/env.py‎

Lines changed: 3 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -233,8 +233,9 @@ def _capture_windows(*, consequence: str) -> dict[str, str] | None:
233233
upper-case names ``os.environ`` gives them on Windows, which is the spelling
234234
every reader of this capture asks for.
235235
236-
``None`` when neither store can be read, which is the "capture failed"
237-
signal the caller falls back on.
236+
``None`` when neither store can be read, or when a stored Path cannot be
237+
expanded (a cyclic, too deeply nested or oversized reference): either is
238+
the "capture failed" signal the caller falls back on.
238239
"""
239240
import winreg
240241

‎raven/agent/subagent/probe.py‎

Lines changed: 32 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -1176,12 +1176,13 @@ def _note_menu_re_measured(snapshot: Any, name: str) -> None:
11761176
def capabilities_wanted(cfg: Any) -> bool:
11771177
"""Does this acp entry lack a fresh, complete capability record?
11781178
1179-
The same four cases the boot backfill re-measures: no snapshot, one whose
1180-
launch config has changed, one written before the model menu was recorded,
1181-
or one of raven's own whose menu came back empty -- that last one bounded as
1182-
``_own_row_missing_its_menu`` bounds it. A row with a complete record keeps
1183-
it -- a connect must not spend a handshake re-measuring what is already
1184-
known.
1179+
Four cases: no snapshot, one whose launch config has changed, one written
1180+
before the model menu was recorded, or one of raven's own whose menu came
1181+
back empty -- that last one bounded as ``_own_row_missing_its_menu`` bounds
1182+
it. A recorded failure, a credential refusal included, is not one of them;
1183+
the boot backfill re-measures those as well as these four. A row with a
1184+
complete record keeps it -- a connect must not spend a handshake re-measuring
1185+
what is already known.
11851186
"""
11861187
snapshot = acp_snapshot_for(cfg)
11871188
if snapshot is None or snapshot.stale or not getattr(snapshot, "model_menu_measured", True):
@@ -1222,6 +1223,16 @@ def _test_record(snapshot: Any, previous: Any) -> Any:
12221223
)
12231224

12241225

1226+
def _measured_at(snapshot: Any) -> int | None:
1227+
"""When the record's verdict was measured; ``None`` for no record.
1228+
1229+
Every measurement stamps its own time (``_test_record`` carries it over), and
1230+
a modes relearn keeps the stamp it found, so a changed stamp means a newer
1231+
verdict and nothing else does.
1232+
"""
1233+
return None if snapshot is None else getattr(snapshot, "measured_at_ms", 0)
1234+
1235+
12251236
_SCHEDULED = False
12261237
"""One auto-verify per process. Manual Test runs and probe=true listings are
12271238
peer paths to this backfill, not causes to run it again."""
@@ -1336,7 +1347,9 @@ async def _verify_missing_snapshots(manager: Any, rows: list[Any], *, configured
13361347
once at boot. After the run the table is refreshed so the rows rebuilt at
13371348
startup pick up the snapshots this task just recorded -- the alternative is
13381349
a roster that reports an agent stateful and dispatches it stateless until
1339-
the next restart or hot-apply.
1350+
the next restart or hot-apply. The newest measurement wins: a row measured
1351+
while its handshake was in flight -- a manual Test, a connect -- keeps that
1352+
verdict, whatever the handshake returned.
13401353
"""
13411354
from raven.acp_client.capabilities import SnapshotStore, verify_agent
13421355

@@ -1380,11 +1393,22 @@ async def _verify_missing_snapshots(manager: Any, rows: list[Any], *, configured
13801393
continue
13811394
result = await verify_agent(cfg, **_launch_kw(cfg))
13821395
_note_menu_re_measured(result, name)
1396+
# Read again with nothing awaited between here and the write: the
1397+
# handshake held the row for up to the ready timeout. A measurement
1398+
# recorded meanwhile -- a manual Test, a connect -- is newer evidence
1399+
# than this one and stands. A dispatch relearning the agent's modes
1400+
# measures nothing and keeps the stamp, so it does not stop this
1401+
# write, and a failure below is kept over the re-read record, modes
1402+
# included.
1403+
current = acp_snapshot_for(cfg)
1404+
if _measured_at(current) != _measured_at(snapshot):
1405+
logger.info("acp agent {!r}: auto-verify {} superseded by a newer measurement", name, result.status)
1406+
continue
13831407
# Replace an old failure with what this attempt actually measured.
13841408
# Failed rows are retried next boot, so a timeout is not a permanent
13851409
# verdict. A formerly working row still keeps its old capabilities.
13861410
if result.status == "ready" or getattr(result, "needs_auth", False) or failed_before:
1387-
record = _test_record(result, snapshot) if snapshot is not None and result.status != "ready" else result
1411+
record = _test_record(result, current) if current is not None and result.status != "ready" else result
13881412
store.record(record)
13891413
recorded = True
13901414
logger.info("acp agent {!r}: auto-verify {}", name, result.status)

‎raven/config/keys.py‎

Lines changed: 34 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -1,20 +1,45 @@
1-
"""Choose existing config key spellings without letting null aliases hide values."""
1+
"""Choose the config key spelling the loaders read, so a write updates it in place."""
22

33
from collections.abc import Mapping
44
from typing import Any
55

66
from pydantic.alias_generators import to_camel, to_snake
77

88

9-
def key_as_written(node: Mapping[str, Any], name: str) -> str:
10-
"""Prefer a non-null spelling, then a present null spelling, then ``name``.
9+
def key_as_written(node: Mapping[str, Any], name: str, *, top_level: bool = False, exact: bool = False) -> str:
10+
"""The spelling of ``name`` already in ``node`` that the loader reads, else ``name``.
1111
12-
Extension loading skips null blocks, so a null alias must not hide the
13-
block it reads. False, zero and other non-null values still reach the
14-
caller's validation rather than being replaced as if they were absent.
12+
The config models accept every key as camelCase or snake_case, so a write
13+
has to land on the spelling being read or it grows a second key beside it.
14+
Which spelling is read depends on where the key sits:
15+
16+
- Below the top level pydantic reads every key. It takes the camelCase
17+
alias whenever it is present, null or not, and the snake_case name only
18+
when the alias is absent, so the first spelling present is the one read.
19+
``name`` is tried first, so a key present exactly as written wins; a
20+
schema field is passed as its alias, the spelling pydantic tries first.
21+
- At the top level ``load_raven_config`` drops a null extension block
22+
before pydantic sees it, so a non-null spelling is the one read there and
23+
a null one only when nothing else is present. False, zero and other
24+
non-null values still count, so they reach the caller's validation.
25+
- ``exact`` is for a segment that is not a field at all: the caller names a
26+
*map entry* (a provider, a channel, an MCP server), and the map is read by
27+
the key itself. Folding there would reach a different entry that happens
28+
to be a spelling of the same word -- ``foo_bar`` and ``fooBar`` are two
29+
MCP servers, and a write aimed at one disabling the other.
30+
``providers`` is the exception the caller keeps tolerant: its runtime
31+
reader folds spellings through ``names_same_provider``, so exactness
32+
there would update a section nothing reads.
33+
34+
``exact`` is only consulted for a name the node does not carry as written:
35+
a caller spelling an entry the way the file already files it lands on that
36+
key either way.
1537
"""
38+
if exact:
39+
return name
1640
spellings = (name, to_camel(name), to_snake(name))
17-
for spelling in spellings:
18-
if spelling in node and node[spelling] is not None:
19-
return spelling
41+
if top_level:
42+
for spelling in spellings:
43+
if node.get(spelling) is not None:
44+
return spelling
2045
return next((spelling for spelling in spellings if spelling in node), name)

‎raven/config/self_surface.py‎

Lines changed: 52 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -35,6 +35,7 @@
3535
from collections.abc import Iterable
3636
from dataclasses import dataclass, field
3737
from enum import StrEnum
38+
from functools import lru_cache
3839
from pathlib import Path
3940
from typing import Any
4041

@@ -783,6 +784,47 @@ def find(path: str) -> tuple[Setting, list[str]] | None:
783784
return None
784785

785786

787+
@lru_cache(maxsize=1024)
788+
def _wildcard_indexes(path: str) -> frozenset[int]:
789+
"""The positions in ``path`` that stand for a map entry rather than a field.
790+
791+
Asked of the longest catalog entry whose path is a *prefix* of this one, not
792+
of a full match: the catalog carries ``tools.mcpServers.*.enabled`` and the
793+
tool may not write ``tools.mcpServers.fooBar.command``, but a map entry is
794+
named the same way however deep inside it a path reaches. The binding is a
795+
property of the segment, so it is read before the rest of the path is known.
796+
797+
A path with no catalog entry anywhere along it (an extension section, a
798+
second-level sub-agent field) has no wildcard to find, and the tolerant rule
799+
that was there before is what reads it. The catalog is the only thing that
800+
knows a segment was named by the caller, so it is asked rather than guessed
801+
from the shape of the name.
802+
803+
Cached on the path: the catalog is a module constant, so the answer cannot
804+
change while the process runs, and ``lookup`` is called once per leaf on
805+
every settings read. Uncached, this walked the 95 catalog entries once per
806+
prefix -- 48us a call against the 22us the walk itself costs, on a path a
807+
page reload repeats for every row it draws.
808+
"""
809+
parts = path.split(".")
810+
wildcards: frozenset[int] = frozenset()
811+
for end in range(1, len(parts) + 1):
812+
found = find(".".join(parts[:end]))
813+
if found is not None:
814+
pattern = found[0].path.split(".")
815+
wildcards = frozenset(index for index, part in enumerate(pattern) if part == "*")
816+
return wildcards
817+
818+
819+
def _provider_bound(path: str) -> bool:
820+
"""Whether a wildcard in ``path`` binds a provider name.
821+
822+
``providers`` is the one map whose reader folds spellings, so it is the one
823+
map where a segment naming an entry keeps the tolerant rule.
824+
"""
825+
return path.split(".", 1)[0] == "providers"
826+
827+
786828
def section_of(path: str) -> Section | None:
787829
for section in SECTIONS:
788830
if section.name == path:
@@ -1424,11 +1466,12 @@ def read_raw(config_path: Path | None = None) -> dict[str, Any]:
14241466

14251467
def lookup(data: dict[str, Any], path: str) -> tuple[bool, Any]:
14261468
"""``(present, value)`` for ``path`` in a raw config dict, either spelling."""
1469+
wildcards = _wildcard_indexes(path) if not _provider_bound(path) else frozenset()
14271470
node: Any = data
1428-
for part in path.split("."):
1471+
for index, part in enumerate(path.split(".")):
14291472
if not isinstance(node, dict):
14301473
return False, None
1431-
key = _spelled(node, part)
1474+
key = _spelled(node, part, top_level=node is data, exact=index in wildcards)
14321475
if key not in node:
14331476
return False, None
14341477
node = node[key]
@@ -1559,13 +1602,17 @@ def remove_value(path: str, *, config_path: Path | None = None) -> Any:
15591602
def _mutate(path: str, value: Any, *, remove: bool, config_path: Path | None) -> Any:
15601603
target = config_path or get_config_path()
15611604
parts = path.split(".")
1605+
# The segments standing for a map entry, which the file's own key names:
1606+
# see ``_wildcard_indexes``. Read once, outside the retry, since the
1607+
# catalog does not depend on the file's contents.
1608+
wildcards = _wildcard_indexes(path) if not _provider_bound(path) else frozenset()
15621609

15631610
def _apply(_text: str | None) -> tuple[str | None, Any]:
15641611
data = read_raw_or_raise(target) if target.exists() else {}
15651612
before = copy.deepcopy(data)
15661613
node = data
1567-
for part in parts[:-1]:
1568-
key = _spelled(node, part)
1614+
for index, part in enumerate(parts[:-1]):
1615+
key = _spelled(node, part, top_level=node is data, exact=index in wildcards)
15691616
child = node.get(key)
15701617
if child is None:
15711618
if remove:
@@ -1574,7 +1621,7 @@ def _apply(_text: str | None) -> tuple[str | None, Any]:
15741621
if not isinstance(child, dict):
15751622
raise ValueError(f"{path}: {key} is not an object in config.json")
15761623
node = child
1577-
leaf = _spelled(node, parts[-1])
1624+
leaf = _spelled(node, parts[-1], top_level=node is data, exact=len(parts) - 1 in wildcards)
15781625
previous = node.get(leaf)
15791626
if remove:
15801627
if leaf not in node:

‎raven/config/update.py‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -243,7 +243,7 @@ def set_skill_blocked(
243243

244244
def _apply(_text: str | None) -> tuple[str | None, tuple[list[str], bool]]:
245245
data = read_raw_or_raise(path)
246-
sf_key = key_as_written(data, "skillForge")
246+
sf_key = key_as_written(data, "skillForge", top_level=True)
247247
section = data.get(sf_key)
248248
if section is None:
249249
section = data[sf_key] = {}

0 commit comments

Comments
 (0)