Repository navigation
fix: bound Windows environment expansion and preserve literal percent signs - #912
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.
Reviewed the full diff plus the Windows capture/launch callers and relevant history. I checked the repository rules and runtime terminology, failure/fallback behavior, compatibility of unknown and literal percent handling, cycle/depth/output bounds, and confirmed the tests add coverage without weakening existing assertions. The changes stay within the existing platform boundary and preserve PATH ordering.
Verification: PYTHONPATH="$PWD/plugins-dist/everos-memory${PYTHONPATH:+:$PYTHONPATH}" uv run pytest tests/test_subagent_host_env.py -q passed (29 tests); Ruff check and format-check passed for all changed Python files; git diff --check github/main...HEAD passed. GitHub's Windows and Python/lint checks are also green at review time.
|
Windows compatibility review: no new Windows defect found in the inspected changes; one inherited test failure remains. Reviewed all 4 changed files at head Native Windows / Python 3.13.12, detached temporary checkout, uv run pytest tests/test_subagent_host_env.py -k 'windows' -n 0 -x --tb=short --basetemp "$reviewRunDir/pr912/tests"
# Head: 20 passed, 1 failed, 2 deselected.
uv run pytest tests/test_subagent_host_env.py::test_windows_probe_finds_agents_on_the_captured_path -n 0 -x --tb=short --basetemp "$reviewRunDir/pr912/base-tests"
# Exact base: the same test failed with the same missing-executable result.
uv run pytest tests/test_subagent_host_env.py -k 'literal_percent or refuses_cycles or bounds_output or bounds_reference or invalid_expansion or non_strings or explicit_program' -n 0 -x --tb=short --basetemp "$reviewRunDir/pr912/targeted-tests"
# Head restored: 13 passed, 16 deselected.The inherited failure constructs an extensionless executable fixture; the probe reports Limits: the tests substitute the registry; no real registry edits, installed-agent launch, or full-suite validation was performed. This is not a full compatibility certification. |
|
No blockers in the reviewed scope. Scope: I reviewed the full PR diff (4 changed files), including tests and documentation, and traced affected callers and contracts at Checked literal-percent and unknown-reference handling, recursive expansion, cycle/depth/output bounds, memoization, PATH precedence, first-capture and refresh fallback, and CLI/ACP/probe consumers. Invalid expansion preserves the prior capture; the platform-specific documentation now matches the environment actually supplied to Windows children. Verification environment: native Windows; isolated source snapshots. Verification:
|
f6ab0de to
b94466f
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.
The tree is byte-for-byte identical to the previously reviewed revision; this rewrite only changes commit metadata. I also checked compatibility with the advanced main tip: the merge-tree is clean and the intervening changes do not touch this PR's files.
Verification: PYTHONPATH="$PWD/plugins-dist/everos-memory${PYTHONPATH:+:$PYTHONPATH}" uv run pytest tests/test_subagent_host_env.py -q passed (29 tests); git diff --check github/main...HEAD and the commit-message checker passed. The repository rules, full diff, callers/history, backward compatibility, test strength, and architecture constraints remain covered by the unchanged tree.
|
Windows compatibility re-review: no new issue found in the inspected scope. Reviewed the complete 4-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_host_env.py' -k 'literal_percent or refuses_cycles or bounds_output or bounds_reference or invalid_expansion or non_strings or explicit_program' -n 0 -x --tb=short --basetemp 'C:\Users\livxu\AppData\Local\Temp\raven-windows-review\fa39f19b3fcb4d7daa4ad3b9472340f8\pr912\unit-pytest'
# 13 passed, 16 deselected in 7.59s; exit 0Limits: Registry data is substituted in these tests; no real registry edits or installed-agent launch was performed. The broader host-environment suite was not rerun; previously reported inherited executable-fixture/process-group failures are outside this selection. This scoped result does not establish full Windows compatibility. |
… signs Recursive Windows environment expansion could grow without an output bound, while a literal percent in one PATH component could hide a later valid reference. Resolve names with memoization, detect cycles, limit reference depth and output size, and keep valid nested and case-insensitive references.
Two comments still said every sub-agent inherits the web keys through ~/.raven/env. Only POSIX cli and acp sub-agents read it, through their login shell capture; Windows children inherit Raven's own environment.
b94466f to
9674348
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 revision cleanly rebases the reviewed implementation onto current main and adds two consistent documentation corrections: the onboarding overview and settings-write comment now accurately limit ~/.raven/env inheritance to POSIX CLI/ACP sub-agents. I compared the range against the prior revision and rechecked the full six-file PR diff, repository rules, callers/history, compatibility, test strength, and architecture boundaries.
Verification: the focused host-environment file passed (29 tests) with uv run --no-sync pytest; Ruff check and format-check passed for all six changed Python files; commit-message, diff, merge-tree, and the currently completed GitHub checks passed.
|
Windows compatibility re-review: no new issue found in the inspected changes. One inherited Windows fixture failure remains. Scope: the complete 6-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_host_env.py -k windows -n 0 -x --tb=short --basetemp "$R/checks/912-windows/pytest" --junitxml "$R/checks/912-windows/junit.xml"
# Head: 20 passed, 1 failed; 2 deselected. -x left 6 selected tests unrun.
# Run from the detached exact-base checkout, with PYTHONPATH repinned there:
uv run pytest tests/test_subagent_host_env.py::test_windows_probe_finds_agents_on_the_captured_path -n 0 -x --tb=short --basetemp "$R/checks/912-control-base/pytest" --junitxml "$R/checks/912-control-base/junit.xml"
# Base 562c8457c79063e8fe8d1eacd48882e1948f11ed: same test failed.Reproduced on both head and exact base: |
|
Not a blocker: full acceptance at head 9674348 merged onto main 182daca found no blocking issue. One non-blocking change is worth a look, plus two small gaps. 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. For a stray I ran main's and this head's
Main agreed with the port on every row, and all three agree on the plain cases ( The module's contract is the terminal's environment ("it should see that terminal's environment rather than raven's", env.py:3-4). With such a value, a sub-agent's PATH now holds directories the user's own terminal does not have, or misses one it has. tests/test_subagent_host_env.py:291 pins the new result. If the goal is to match Windows, the not-found case should skip through the closing This only happens on Windows hosts, and only with a literal or malformed 2. Small gaps:
|
|
No blockers; suggestions only, and they are marked inline. The new acceptance evidence changes my standing verdict from clean to nonblocking. I independently reproduced all five malformed/literal-percent cases: the current scanner resumes at the next character and expands references that the Windows-compatible scan leaves untouched. This is a concrete behavior mismatch introduced by the PR, but it requires a malformed or literal percent in a persisted Windows PATH, so it does not hold up the change under the late-round reachability bar. The missing memoization regression test and stale |
|
Native Windows verification of the percent-scanning finding at Measured on Windows build 10.0.26300, CPython 3.13.12, in an isolated checkout with
The three controls (
This verifies these inputs on this Windows version, not general equivalence of recursive expansion with the native single pass. The memoization coverage and docstring notes are separate follow-ups; no registry writes, full-suite run, or installed-agent launch was performed. |
|
No blockers; suggestions only, and they are marked inline. The direct My stance remains nonblocking only for the two already-recorded minor follow-ups: add a memoization-specific regression and update |
|
Both notes are fixed in #940, after the native measurement in #912 (comment). 1. A stray percent. Not changed, and the reason is that measurement: 2a. The memo. Fixed in 2b. The docstring. Fixed in tests/test_subagent_host_env.py: 30 passed. |
…b key (#927) ## Summary Two gaps in how Raven stops its cli sub-agents and what environment it starts them with. **Stopping a cli sub-agent on Windows.** A cli sub-agent's timeout, cancel and stream-failure paths, and the `kimi` commands Raven runs to explain a failed Kimi Code connect, ended the process group with `os.killpg`, which Windows does not have. The `AttributeError` replaced the timeout error or the cancellation, and the CLI was left running. Where `killpg` is missing, the tree is now ended from the launcher's pid with `taskkill /T /F` while the launcher is still alive, and the launcher is then killed itself, the way `raven/ops/transport.py` already ends a command on Windows. Once the launcher has exited its pid may name another process, so nothing is killed by number. POSIX keeps the group kill. **A web key rotated through `settings.set`.** `settings.set` rewrites `~/.raven/env` when a web credential changes, but a POSIX cli or acp sub-agent reads that file only through the login-shell capture, which a running Raven takes once and keeps. Every later spawn started with the key the write had replaced, until a Connect, a Test, "Check again" or a restart took the capture again. A mirrored key's write now takes the capture again before the reply. Captures also run one at a time: a refresh that read the shell before the write, a Connect's for instance, could otherwise land after the write's own and put the replaced key back, and readers that arrive while the first capture runs now wait for it rather than each running the shell. Windows is unchanged for keys. Nothing sources `~/.raven/env` there, and a Windows sub-agent's environment is Raven's own with the registry's PATH, so a key held only in config does not reach it; #912 states this in the code. On POSIX the keys reach a sub-agent only through a `source` line in the user's shell startup files, which the onboard wizard adds only with the user's agreement, and a Windows delivery path would need a consent step of its own, so this PR does not add one. Not changed here: an acp sub-agent that is already connected keeps the environment it was launched with until its next launch; a key written by `raven onboard`, which runs as its own process, reaches a running Raven's sub-agents at the next Connect, Test, "Check again" or restart; and on Windows the wizard still prints the POSIX `source` line when `$SHELL` is not bash or zsh. ## Type - [x] Fix - [ ] Feature - [ ] Docs - [ ] CI / tooling - [ ] Refactor - [ ] Other ## Verification Linux, Python 3.12, rebased onto main 182daca: - `uv run --frozen --python 3.12 --all-extras pytest -q` with the proxy variables unset: 3 failed, 29667 passed, 158 skipped. The same 3 fail on main on this machine (an npm on PATH, a root-only permission check, a LibreOffice conversion); 0 introduced. - `uv run --frozen --python 3.12 --all-extras pytest tests/test_subagent_third_party.py tests/test_subagent_kimi_code.py tests/test_subagent_acp.py tests/test_subagent_host_env.py tests/test_rpc_console.py tests/test_rpc_settings.py tests/test_rpc_subagents.py -q`: 1080 passed. - Windows stop: `tests/_windows_kill.py` leaves a Windows host as it is, where the real `taskkill` runs, and elsewhere removes `os.killpg` and stands in for `taskkill` by ending the process group it is given, the launcher's tree here; either way it records each call. The launchers, their workers and the Kimi stand-in are Python started on `sys._base_executable`, because a Windows venv's `python.exe` is a redirector that runs the base interpreter as its child and returns a pid the script never sees. The tests have not run on a Windows host, and no CI job runs these test files there. - Windows stop, measured here: the six tests that use the helper pass. With `os.killpg` removed for the whole session and a `taskkill` on PATH that ends the group it is given, they pass again through that `taskkill` (3 calls), and the POSIX-only Kimi test beside them is skipped. With `sys.executable` replaced by a wrapper that runs Python as its child, as a venv's `python.exe` does on Windows, both modes stay green; started on that wrapper instead, the two cancel tests fail on the pid they compare. - Windows stop, against the fix: the six fail with `AttributeError` on main. Removing the alive check in the cli backend, the `taskkill` call or the kill after it in either module, or the cli backend's handling of a `taskkill` that times out, turns at least one of them red. Removing the POSIX group kill from the Kimi Code commands turns only the new test whose stand-in `kimi` starts a process of its own red (1 failed, 347 passed over the two test files). - Rotated key: `test_a_rotated_key_reaches_the_capture_the_next_spawn_reads` saves a key twice through `settings.set` and reads the capture after each save, through a real bash that sources the mirror; on main the second read still returns the first key. Removing the refresh turns 12 tests red; refreshing before the mirror is rewritten turns that test red. Two existing tests that write a web key now stub the refresh, so they do not run the host's login shell. - Concurrency: three tests force the interleavings with events: a refresh that read the shell first and lands after a later one, a second reader that arrives while the first capture runs, and readers waiting on a first capture that fails. Each failed in 3 of 3 runs on main and passes with the lock; removing the lock from either capture path, or either re-check inside the first one, turns one of them red. - With `--cov=raven --cov=raven_everos --cov-branch` added to the full run, `python scripts/coverage_gate.py diff --base-ref origin/main --threshold 90`: 93.22% of the changed lines (55 of 59). The four left are except clauses for a process group or a process that is already gone. - `ruff check`, `ruff format --check`, `lint-imports`, `ty check` and `deptry` over the Makefile targets, `commitlint`, the three repository gate scripts and `pre-commit run --from-ref origin/main --to-ref HEAD`: passed. - `git merge-file` of `raven/rpc/methods/console.py` with each open PR that touches it (#883, #900, #911, #912, #923, #925), and of `raven/agent/subagent/backends/env.py` with #912 in both orders: no conflicts. - [x] Relevant tests pass locally - [x] Relevant lint / type checks pass locally - [ ] User-facing docs or screenshots are updated when needed No user-facing docs describe either behavior. ## Risk - [x] Security impact considered - [x] Backward compatibility considered - [x] Rollback path is clear for risky changes Saving a web key now waits for the user's login shell to start before the reply, as Connect and Test already do; the capture's 15 s timeout bounds it, and it can also wait for a capture already in flight. A capture that fails keeps the previous one and does not fail the save. No credential reaches a sub-agent that did not before; it is the same shell reading the same file, read again. On Windows a stop now waits up to 5 s for `taskkill`, and a worker that a launcher started and left behind after exiting outlives the stop there, since Windows has no group to address and an exited launcher's pid is not trusted. If `taskkill` is missing, fails or does not answer within those 5 s, only the launcher is killed, and a cli stop then waits for any worker still holding the launcher's output pipes to exit. Revert this PR to roll back. ## Related Issues #908, #912
## 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
Recursive Windows environment expansion could grow without an output bound, while a literal percent in one PATH component could hide a later valid reference. Resolve names with memoization, detect cycles, limit reference depth and output size, and keep valid nested and case-insensitive references.
An invalid expansion fails the capture rather than publishing a truncated PATH; refresh retains the prior environment. Tests cover literal percent signs, unknown names, nesting, small injected limits, non-string registry values and explicit program paths. Capture descriptions now distinguish POSIX and Windows, including the settings writer's and the setup wizard's notes on
~/.raven/env, which only POSIX cli and acp sub-agents read through their login shell.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, 29654 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.uv run --frozen --python 3.12 --all-extras pytest tests/test_subagent_host_env.py -q: 29 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.Native Windows, Python 3.12. PYTHONUTF8=1; the existing environment was selected through UV_PROJECT_ENVIRONMENT, and PYTHONPATH pointed at this checkout.
Result: 13 passed in the targeted selection. The full host-environment file previously produced 26 passed and 3 failed on Windows. The same three failures reproduced on the unmodified base: two os.killpg calls and an executable-fixture lookup. On Linux the whole file passes at this head; the three Windows failures are the base's.
Whole-branch gates:
All listed gates passed. The whole-branch review covered callers, ownership, guard boundaries, failure paths, regression strength, architecture and compatibility. No screenshot change applies; directly affected code documentation is updated where needed. The full repository suite and Linux/macOS execution were not run locally; the local Docker Linux engine was unavailable.
Risk
Each expansion is limited to 32,767 characters and 16 references deep. Cyclic, deeper or oversized input now takes the existing capture-failure fallback instead of returning unresolved or excessively expanded text. PATH ordering and duplication policy are unchanged. Revert the squash commit to roll back.
Related Issues
#880