Repository navigation
fix: replace obsolete subagent authentication verdicts - #915
Conversation
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 full github/main...HEAD diff and the surrounding snapshot store, boot verification, connection-time capability refresh, RPC consumers, and relevant history. I also checked the repository rules and Runtime context, backward compatibility with older/stale snapshot rows, and that the tests add coverage without weakening existing assertions. The revised persistence path keeps prior capabilities while replacing transient verdict fields, retries failed records on later boots, and prevents a stale authentication result from disabling an edited row. I found no concrete failure to raise.
Verification: the first uv run pytest tests/test_subagent_probe.py tests/test_rpc_subagents.py tests/test_subagent_acp.py -q attempt failed during shared fixture setup because the fresh environment lacked raven_everos; after uv sync --frozen --extra dev --dev, the same command passed with 525 tests. uv run ruff check and uv run ruff format --check passed for all four changed files, and git diff --check github/main...HEAD passed.
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 delta from f80d54e2716c and rechecked the full diff. The only new change makes DNS deterministic in the research-tool test module: resolver-failure cases override the fixture, literal private addresses still take the validator's direct-IP path, and the separate security-network tests continue to cover hostnames resolving to private addresses. It does not change production code or the previously reviewed authentication fix.
For this revision I covered the diff, affected validator and web-tool callers, test history, backward compatibility, test-strength preservation, and the repository's source-language, test-naming, domain-context, and architecture constraints. I found no concrete failure.
Verification: uv run pytest tests/test_agents_research_tools.py tests/test_security_network.py tests/test_subagent_probe.py tests/test_rpc_subagents.py tests/test_subagent_acp.py -q passed with 721 tests. Ruff check and format check passed for all five changed files, and git diff --check github/main...HEAD passed.
|
No blockers in the reviewed scope. Scope: I reviewed the full PR diff (5 changed files), including tests and documentation, and traced affected callers and contracts at Checked capability snapshot persistence, fingerprint/stale handling, manual and boot verification, failure retries, registry refresh, and RPC/Web/TUI authentication consumers. The latest delta only makes DNS deterministic in the research-tool tests: resolver-failure tests override it, literal private-address checks stay active, and the separate network-security suite covers private DNS results. Production authentication behavior is unchanged by that delta. Verification (native Windows, Python 3.12, isolated source snapshots; local JUnit-output options omitted below):
The original three-suite run at the previous reviewed head gave 467 passed, 57 failed, and 1 skipped; the merge-base control had the identical 57 failing IDs (457 passed, 1 skipped). The failures concern existing Windows executable/path and process-group behavior. The latest production diff is unchanged; the new-head run above also exercises the deterministic DNS fixture and the separate network-security tests. No new finding was identified.
|
|
Windows compatibility review: no new issue found in the inspected scope. Reviewed head Native Windows / Python 3.13.12, in a detached temporary checkout with uv run pytest tests/test_subagent_probe.py tests/test_rpc_subagents.py tests/test_agents_research_tools.py -k 'research or boot_replaces or manual_test_merges or credential_verdict' -n 0 -x --tb=short --basetemp "$reviewRunDir/pr915/tests"Result: 99 passed, 205 deselected. ACP verification and HTTP/DNS are substituted in this selection; no live agent login or external network service was tested. This scoped result does not establish full Windows compatibility. |
74cddde to
00f87b7
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.
This head rewrites the two branch commits but has the exact same tree (3b22dd265c0e509b142c3aaa5dd021bf3585cf2f) and merge base as the previously reviewed revision. I confirmed there is no file delta from 74cdddeaa311 and rechecked the full PR diff, commit history, project constraints, callers, backward compatibility, architecture boundaries, and test-strength considerations; nothing changes the prior clean assessment.
Verification on this checkout: uv run pytest tests/test_agents_research_tools.py tests/test_security_network.py tests/test_subagent_probe.py tests/test_rpc_subagents.py tests/test_subagent_acp.py -q passed with 721 tests. Ruff check and format check passed for all five changed files, and git diff --check github/main...HEAD passed.
|
Windows compatibility re-review: no new issue found in the inspected scope. Reviewed the complete 5-file PR diff at Git checks: source tree identical to previously reviewed Native Windows / Python 3.13.12, detached checkout, PR head: uv run pytest 'tests/test_subagent_probe.py' 'tests/test_rpc_subagents.py' 'tests/test_agents_research_tools.py' -k 'research or boot_replaces or manual_test_merges or credential_verdict' -n 0 -x --tb=short --basetemp 'C:\Users\livxu\AppData\Local\Temp\raven-windows-review\fa39f19b3fcb4d7daa4ad3b9472340f8\pr915\unit-pytest'
# 99 passed, 205 deselected in 11.05s; exit 0Limits: ACP verification and HTTP/DNS are substituted in this selection; no live agent login, external network service or full repository suite was tested. Previously reported broader Windows process/path failures were not remeasured here. This scoped result does not establish full Windows compatibility. |
A new non-authentication failure could leave an old needs_auth flag persisted, and a stale snapshot could still disable the edited agent's connection control. Carry current failure flags when preserving prior capabilities, recheck previously failed rows on subsequent boots, and omit stale authentication verdicts from the listing RPC.
Use deterministic DNS beside the mocked HTTP transport while retaining the real URL validation. Explicit resolver-failure tests keep their own overrides, and the strict idle ceiling remains enabled.
The two docstrings still described verifying only missing or stale rows. A row whose last verification failed is measured again at every start, so a timeout is not a permanent verdict.
00f87b7 to
f75ce7c
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 rebase onto 562c8457c790 and the new documentation commit. The newer target adds research-tool cases in the file carrying the DNS fixture; those cases remain correctly isolated and pass. The updated snapshot-verification docstrings now match the implementation's retry-on-next-start behavior. I rechecked the complete scoped diff, affected callers and history, project rules and context, backward compatibility, architecture boundaries, and whether the rebased tests lost coverage; I found no concrete failure.
Verification: uv run pytest tests/test_agents_research_tools.py tests/test_security_network.py tests/test_subagent_probe.py tests/test_rpc_subagents.py tests/test_subagent_acp.py -q passed with 724 tests. Ruff check and format check passed for all five changed files, and git diff --check github/main...HEAD passed.
|
Windows compatibility re-review: no new issue found in the inspected scope. Scope: the complete 5-file PR diff at head Native Windows, Python 3.13.12, isolated detached checkout and user/Raven/temp directories. Dependencies were synced with uv run pytest tests/test_subagent_probe.py::test_boot_replaces_an_auth_verdict_and_retries_until_ready tests/test_subagent_probe.py::test_manual_test_merges_current_failure_flags_with_old_capabilities tests/test_rpc_subagents.py::test_a_row_carries_the_credential_verdict_its_snapshot_measured -n 0 -x --tb=short --basetemp "$R/checks/915-regressions/pytest" --junitxml "$R/checks/915-regressions/junit.xml"
# 11 passed; real temporary snapshot persistence, simulated verifier results.
uv run pytest tests/test_agents_research_tools.py tests/test_security_network.py -n 0 -x --tb=short --basetemp "$R/checks/915-research/pytest" --junitxml "$R/checks/915-research/junit.xml"
# 199 passed; mocked HTTP/DNS and URL-validation cases.No real third-party ACP authentication, complete probe/ACP suite or full repository suite was run. The unchanged process-launch/teardown paths are not claimed fixed by this PR; Windows ACP teardown is addressed separately in #908. |
|
Not a blocker: full acceptance at head f75ce7c merged onto main 182daca found no blocking issue. It did find one narrow regression, and three texts that still describe the rule this PR replaces. 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. 1. A boot verify that fails late overwrites a Test that passed in the meantime.
Nothing serialises this against a manual Test that records through
On main a boot timeout is never written, so the Test's result survives. Here the new 2. Three texts still describe the old rule:
How I measured: the PR's own test fixtures ( |
|
No blockers; suggestions only, and they are marked inline. I independently reproduced @0xKT’s nonblocking finding through the production snapshot functions: a manual Test stored Verification remains green: the affected five-suite command passed with 724 tests. |
|
Fixed in #940 ( The boot read a row before its handshake and wrote its verdict from that read, so a Test recorded while the handshake ran was replaced by a late timeout, refusal or pass. It now reads the row again immediately before the write and leaves a newer measurement in place. The comparison is on Three mutants, each restored after: dropping the guard turns 3 tests red, comparing the whole record instead of the stamp turns 2 red, and reading the base before the await turns 1 red. |
## 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
3 participants
Summary
A new non-authentication failure could leave an old needs_auth flag persisted, and a stale snapshot could still disable an edited agent's connection control. Carry current failure flags when retaining prior capabilities, recheck failed rows on subsequent boots, and withhold stale authentication verdicts from the listing RPC. The start-up verification's docstrings now say that a failed row is measured again at every start.
Timeouts and transport failures remain non-ready and offer a retry path. Model-menu and resume capability data survive failed measurements. No UI or wire schema change is needed.
The research-tool unit tests also replace external DNS beside their mocked HTTP transport. Real URL validation still runs, and resolver-failure cases keep their explicit overrides. This removes the uncontrolled dependency from the test that exceeded the CI idle ceiling; the ceiling is not relaxed.
Type
Verification
At this head, rebased onto main, Linux, Python 3.12:
uv run --frozen --python 3.12 --all-extras pytest -qwith the proxy variables unset: 3 failed, 29651 passed, 158 skipped. The same 3 fail on main at the merge base on this machine (an npm on PATH, a root-only permission check, a LibreOffice conversion); 0 introduced.--idle-ceiling-strict: 616 passed.ruff check,ruff format --check,lint-imports,ty checkanddeptryover the Makefile targets,commitlint, the three repository gate scripts andpre-commit run --from-ref origin/main --to-ref HEAD: passed.Linux, Python 3.12, using the synchronized uv container environment:
613 passed, including the strict idle ceiling. Native Windows, with PYTHONUTF8=1 and the existing environment selected by UV_PROJECT_ENVIRONMENT:
88 passed. In an isolated reverse check, an external-DNS guard failed the original largest-read test at socket.getaddrinfo; the same test passed with the resolver fixture. The previous CI shard completed 7198 assertions but failed its idle ceiling. Its log did not measure DNS separately, so the exact cause of that historical wait is not asserted.
The authentication regressions cover real temporary snapshot persistence, retries until ready, previous-ready and previous-failed capability retention, and stale RPC verdicts. Earlier native Windows probe/RPC runs had 19 failures identical to the base; the Linux selection above is green. The unchanged extAgents source/page suites were previously checked with 154 passing tests.
Whole-branch gates on native Windows:
All listed gates passed. The full diff was checked for caller coverage, ownership, admission and failure behavior, compatibility, regression strength and architecture. The full repository suite and native macOS were not run locally. No screenshot change applies; affected code documentation is updated.
Risk
Previously failed rows are verified once per boot until ready, so some rows now incur a new handshake at startup. A timeout does not become a successful authentication verdict. Stale capability fallback data remains available, while stale authentication no longer disables the edited row.
The resolver fixture is confined to the research unit-test module; production network validation is unchanged. Revert the squash commit to roll back. No wire schema changes.
Related Issues
#554