Repository navigation
fix(node): an old uv no longer skips the index; the port check is an install's - #965
Conversation
…install's Three follow-ups to EverMind-AI#963, from its review. A machine's uv older than 0.4.23 ignores UV_DEFAULT_INDEX (uv's changelog adds it in 0.4.23, with --default-index) and installed from PyPI while the plan named the mirror pip is configured with there; before EverMind-AI#963 such a uv refused the --default-index argument instead. The probe now reports the found uv's version, and with an index configured a uv older than 0.4.23, or one that does not say its version, is passed over for the pinned uv fetched under the node dir; with nothing to fetch it with, the install says so in one sentence naming the version. Without an index an old uv is used as before. The check that refuses an index whose port is not a number from 0 to 65535 ran in plan(), before install-node reads whether the node is already there and before --prune, neither of which needs the index. It is now in refusal(), with the same sentence, so such a machine is still found to have its node and can be pruned. The real install test stood in for uv pip only when a step went through run; since EverMind-AI#963 the steps that run uv pip go through push when an index is configured, and with a system-wide pip.conf on the computer running it the test really installed packages. The push stand-in now stands in for them too, and the test runs with an index configured as well. Co-authored-by: Claude (claude-opus-5-5) <noreply@anthropic.com>
gloryfromca
left a comment
There was a problem hiding this comment.
No blockers; this can merge as far as I am concerned.
Reviewed github/main...HEAD, including the stacked MCP lending and DAG sandbox changes and the tip install follow-up. I checked the repository rules and context vocabulary, the changed code and tests, the install caller and probe/install/prune ordering, relevant history, compatibility at the node protocol boundary, credential handling, and whether tests were weakened. The uv 0.4.23 boundary also matches uv's documented introduction of UV_DEFAULT_INDEX.
Verification:
uv run --all-packages pytest tests/test_node_install.py tests/test_node_bridge.py tests/test_node_client.py tests/test_node_server.py tests/test_subagent_acp_mcp_lifecycle.py tests/test_subagent_acp_remote.py tests/test_subagent_dag_core.py tests/test_subagent_dag_runner.py tests/test_subagent_manager.py tests/test_subagent_third_party.py tests/test_agent_loop_playbook_entry.py tests/test_cli_playbook_commands.py -x- 1369 passed, with one asyncio resource warning.- The same default
uv run pytest ...invocation initially stopped in the autouse fixture becauseraven_everoswas not installed; enabling all workspace packages resolved the test environment without changing or skipping tests. scripts/check_large_files.py github/main..HEAD- passed.scripts/check_source_language.py github/main..HEAD- passed.git diff --check github/main...HEAD- passed.
|
No blockers in the reviewed scope. Scope: I reviewed only the What I examined: the follow-up to the observations F1, F2 and F3 I left on #963. With an index configured, a machine's uv older than 0.4.23 ( Observations outside the reviewed scope, numbered for this PR (the ones from #963 are named with its number):
Verification:
Checked and not reported: the index still travels only on stdin, |
… the old-uv case Follow-ups to EverMind-AI#965's review. The probe stopped at the first uv it found, and node_dir/.uv/uv came last. So on a machine whose own uv is older than 0.4.23 with an index configured, the first install passed that uv over and fetched uv 0.13.0 under node_dir, and the next install-node, which every Raven update asks for, found the old uv first again and fetched 0.13.0 a second time -- 25 MB more in the plan and a round trip to astral.sh on every update; and with no curl or wget by then it refused, asking to upgrade uv, while a uv that reads the index sat under node_dir. The probe now reports the uv under node_dir on its own as well, and the plan uses it in place of a machine uv that would ignore the index, so nothing is fetched twice and the uv and Python put there are reused by the next attempt, as the comment and the guide say. The machines guide said uv is fetched only when the machine had none, and its troubleshooting table had no row for the sentence an old uv earns; both pages now have them. A test docstring still said the look refuses a bad port; it says the install does. The real install test takes its uv as recent enough rather than reading this computer's: with a uv older than 0.4.23 here, its case with an index would have fetched one through a stand-in that stubs only pip install. Co-authored-by: Claude (claude-fable-5-1) <noreply@anthropic.com>
|
All three, and the test-host point from your "checked and not reported", are in 2f94f35.
Full suite: 31781 passed, the 3 failures are the ones that fail the same way on main on this computer; diff coverage 100% (30/30); |
gloryfromca
left a comment
There was a problem hiding this comment.
No blockers; this can merge as far as I am concerned.
Reviewed the delta from 390c970612e4 and checked it against the full target diff, install caller, probe/plan/steps behavior, relevant history, repository rules and context terms, backward compatibility, and the prior observations. The cached node-local uv is selected only when an index is configured, the first uv is too old or silent, and the cached uv is new enough; no-index and unusable-cache behavior remain unchanged. The documentation and test-host follow-ups match the implementation, and I found no weakened assertion or new concrete failure.
Verification:
uv run --all-packages pytest tests/test_node_install.py -x- 82 passed.uv run --all-packages pytest tests/test_docs_site.py tests/test_living_docs.py -x- 18 passed.uv run --frozen --python 3.12 --group docs mkdocs build -f docs-site/mkdocs.yml --strict- passed (dependency SyntaxWarnings only).scripts/check_large_files.py github/main..HEAD- passed.scripts/check_source_language.py github/main..HEAD- passed.git diff --check github/main...HEAD- passed.
The first direct docs-build attempt used an environment without the docs dependency group and could not find mkdocs; rerunning through the repository's documented docs group passed.
|
Windows compatibility review at No new Windows compatibility issue found in the inspected scope. Reviewed all four changed files, the CLI's plan/refusal/install/prune ordering and the SSH run/push callers. The added Native Windows / Python 3.13.12: 85 passed, 17 skipped, 18 deselected in the focused install/CLI/bundle selection. Sixteen skips require a POSIX shell; one requires symlink privileges this account lacks. An initial broader run stopped after 79 passes and 15 skips at Exact-head detached worktree; fresh $R = 'C:/Users/livxu/AppData/Local/Temp/raven-windows-review/c8dec77ada644245ae6438b441d39c59'
uv run pytest tests/test_node_install.py tests/test_cli_ops_connection_commands.py tests/test_node_bundle.py -k "node or install or prune" -n 0 -x --tb=short --basetemp "$R/checks/965-focused/pytest" --junitxml "$R/checks/965-focused/junit.xml"
# Exact base, same locked environment with base source imports: same FileNotFoundError
uv run pytest tests/test_cli_ops_connection_commands.py::test_adding_an_ssh_machine_writes_a_host_alias -n 0 -x --tb=short --basetemp "$R/checks/965-base-alias/pytest" --junitxml "$R/checks/965-base-alias/junit.xml"
|
… read (#977) ## Summary Follow-up to LivXue's review note on #957 (the pick-up's binding at `cli_agent.py:1393`). The machine's daemon kept an agent's stderr in the task's record (`stderr.log`) but sent it only to the connection holding the task when a line was written, and `agent/attach` replayed `frames.jsonl` alone. So what an agent said only on stderr while no connection held it never reached the host: - **The session id a CLI names only on stderr** (hermes does). The pick-up #957 added binds a create's handle to the id in its output; for such a CLI it found none, warned "not resumable", and the next turn to the instance opened a new session. - **The reason a command failed.** A run picked up after this computer stopped was reported as `exited 3` where the dispatch would have said `exited 3: token expired`. This part predates #957. - **The stderr written while a link was down**, on a run still being watched, the same way. The fix is in the node and the host's link to it (`raven/node/`): - The daemon numbers stderr lines as it numbers frames (`agent/stderr` carries `n`), and `agent/attach` takes an optional `stderr_after`: the stderr lines past that one are replayed after the frames, under the lock that forwards a new line, so none is sent twice or missed at the handover. It works the same for a live task, a finished one, and a record a daemon that never ran the task reads. An attach that does not ask hears no stderr, as before; a `stderr_after` that is not a whole number is refused like a bad `after`. - The host's `ChannelEndpoint` counts the stderr lines it got (`stderr_received`). A cli pick-up (`NodePool.attach`, a process that read none of them) asks from the first line; an acp pick-up from the line the run's record kept as the prompt went out (`RemoteRun.stderr_received`, follow-up commit below); a re-attach after a lost link (`NodePool._reattach`) asks from the last one it read. The cli dispatch and pick-up read stderr off the endpoint as before and now get all of it, so the reason and the session id are there. An acp pick-up's connection drains the same endpoint into the stderr tail it quotes in its errors and into its journal, so what the agent wrote during the turn while this computer was away reaches them too; the acp run record keeps the stderr line count as the prompt goes out, so the earlier prompts' lines are not read again (follow-up commit below). No protocol bump: the new fields are optional, and a node runs exactly the host's code anyway (protocol and digest are checked at every connection). ## Type - [x] Fix - [ ] Feature - [ ] Docs - [ ] CI / tooling - [ ] Refactor - [ ] Other ## Verification - `tests/test_node_stderr_replay.py`, new, 22 tests: the daemon numbers and keeps stderr lines; an attach replays exactly the lines past `stderr_after` (from a live task, a finished one, and a record a daemon that never ran it reads), none when not asked, none from a record with no `stderr.log`, and refuses a bad `stderr_after`; the endpoint counts by the node's numbers; a re-attach and a pick-up ask from the right line. End to end on the stand-in machine (real daemon, stand-in ssh): a left cli run's reason on stderr reaches its pick-up (said before and after this computer stopped), a left create that names its session only on stderr is bound to it by the pick-up (both orders), and a reason written while the link was down reaches the run watching it. 18 fail on main (2c27181); the 4 that pass there pin what does not change (no `stderr_after`, past the last line, past the end, no `stderr.log`). - Changed assertions: the four attach-parameter assertions in `tests/test_node_client.py` and the forwarded-stderr one in `tests/test_node_server.py` include the new fields. - Full suite: 32065 passed, 173 skipped, 2 xfailed, 3 failed -- the same 3 that fail on main (2c27181) on this computer when run alone there: a host without LibreOffice, and two simulation cases. - Diff coverage: 100.00% (30/30 executable changed lines; `coverage json`, then `scripts/coverage_gate.py diff --base-ref origin/main --threshold 90`). - `ruff check`, `ruff format --check`, `ty check`, `lint-imports`, pre-commit on the diff; `scripts/check_commit_messages.py`, `scripts/check_source_language.py` and `scripts/check_large_files.py` on `origin/main..HEAD`: clean. - Docs: CONTEXT.md's Raven node entry, the protocol module's notes, and the machines guide's "Disconnects and restarts" section in English and Chinese now say the daemon keeps and replays stderr; the changelog has an entry under Fixed. `tests/test_docs_site.py` and `tests/test_living_docs.py` pass (18), `mkdocs build --strict` passes. - Real machine, ravenx2, an isolated node directory per tree. The host process that dispatched was killed with SIGKILL and the run picked up by a new process; the link was dropped by killing the local ssh. The command writes `token expired` (or `session slow-A1`) on stderr: | Probe | main (2c27181) | this branch | |---|---|---| | reason written after the host process was killed | `exited 3` | `exited 3: token expired` | | reason written before it was killed | `exited 3` | `exited 3: token expired` | | reason written while the link was down | `exited 3` | `exited 3: token expired` | | create left before its commit, session id only on stderr | "not resumable", nothing bound | bound to `slow-A1`, no warning | The machine was cleaned after: the probe's two daemons ended by pid, its directory removed. - The runs above are on main 2c27181 (#957 merged). The branch was then rebased onto 400b30d, whose two new commits touch only TUI copy and i18n; the node, docs and range checks were run again there (238 passed, 1 skipped). It merges cleanly with the open #964 and #965 (`git merge-tree`). - [x] Relevant tests pass locally - [x] Relevant lint / type checks pass locally - [x] User-facing docs or screenshots are updated when needed - Follow-up commit e0468fa, after the agent review's F1 (an acp pick-up asked for the stderr from the agent's first line, across every earlier prompt of its process): `RemoteRun.stderr_received`, taken in `Tracker.prompt_sent` off the endpoint's count, and passed through `NodePool.attach(..., stderr_after=...)` by the pick-up; a cli pick-up keeps asking from 0; a record from before the field reads with 0. Regression on the stand-in machine (`test_a_pick_up_reads_the_turn_s_own_stderr_and_not_what_the_prompts_before_it_wrote`): two turns of 150 stderr lines, a third interrupted after its prompt, the next start's pick-up -- before the change its tail held 200 lines, 50 of them the second turn's; after it, the third turn's 150 and nothing else. Unit tests pin the record, the pick-up's attach arguments and the default of 0, and the tracker's wiring. The related files 259 passed; full suite 32067 passed with the three failures this Mac always has, plus one test whose stand-in `Tracker` lacked the new keyword, fixed in the same commit. Diff coverage of the changed lines: see CI. ## Risk The replayed lines are the agent's own stderr, already kept in the task's owner-only record and already sent live to whichever connection held the task; nothing new leaves the machine. `stderr_after` and `n` are optional: an attach without `stderr_after` behaves as before, and a line without a number counts on from the last. A cli pick-up replays the whole `stderr.log`, as it already replays every frame of the command; an acp pick-up replays the lines past the count its record kept as the prompt went out, so the replay is bounded to the turn, and the connection keeps a bounded tail of it. Rollback is reverting the commit. - [x] Security impact considered - [x] Backward compatibility considered - [x] Rollback path is clear for risky changes ## Related Issues Follow-up to #957, for this review note: #957 (comment) --------- Co-authored-by: Claude (claude-opus-5-5) <noreply@anthropic.com>
Summary
Three follow-ups to #963 (installing a Raven node on a registered machine),
from LivXue's review of it. Only
raven/node/install.pyand its test change.pip is configured with on the machine through
UV_DEFAULT_INDEX, which uvreads since 0.4.23 (uv's changelog adds it there together with
--default-index). A machine's own uv older than that ignores the variableand installed from PyPI while the plan named the mirror -- before fix(node): index off command lines, a bad port in one sentence, the node's own version #963 such a
uv refused the
--default-indexargument instead, loudly. The probe nowreports the found uv's version (
UV_VERSION, fromuv --version). With anindex configured, a uv older than 0.4.23, or one that does not say its
version, is passed over: the pinned uv is fetched under the node dir as for
a machine with none, and the plan says why ("the machine has uv 0.4.22, and
only uv 0.4.23 or later installs from the index pip is configured with").
With nothing to fetch it with, the install says so in one sentence naming
the version. Without an index configured an old uv is used as before.
whose port is not a number from 0 to 65535 in
plan(), whichinstall-nodecalls before it reads whether the node is already there and before
--prune; neither needs the index, so a machine that had its node and a badport in pip's configuration could not be told so or pruned. The check is
now in
refusal(), with the same sentence; the look takes the machine as itis.
runstand-in stood in for
uv pip install; since fix(node): index off command lines, a bad port in one sentence, the node's own version #963 the two steps that run itgo through
pushwhen an index is configured, and thepushstand-in ranthem for real -- on a computer with a system-wide
pip.confthe testinstalled packages, against the module docstring's promise. The
pushstand-in now stands in for them too, and the test also runs with an index
configured, checking both steps are fed it on stdin.
Type
Verification
Follow-up commit, after the review of this PR:
A uv an earlier install fetched under
node_dir/.uvis used in place of amachine uv that would ignore the index. The probe reports that uv on its
own (
NODE_UV,NODE_UV_VERSION) beside the one it found first, andplan()swaps it in when the first is older than 0.4.23 (or silent) andthe fetched one is not; without an index the machine's own uv serves as
before, and one under
node_dirthat is itself too old does not help. Twotests failed first: the plan from such a probe (uses the fetched uv, no
uvstep, 60 MB, no refusal without curl), and the probe under a real/bin/shreporting both uvs (then the one, then none).The machines guide, English and Chinese: the "What lands on the machine"
row for
uvnames the old-uv case, and the troubleshooting table has a rowfor the sentence an old uv earns.
tests/test_docs_site.pyandtests/test_living_docs.pypass (18),mkdocs build --strictpasses.The docstring of
test_a_plan_whose_index_port_cannot_be_read_still_describes_itself_and_its_failuressays the install, not the look, refuses the port. The real install test's
uv_versionis pinned toUV_VERSIONrather than read off this computer'suv, so a test host with a uv older than 0.4.23 does not reach astral.sh
through the stand-in that stubs only
pip install.tests/test_node_install.py-- 82 passed; with the cli, bundle, client andimport-cycle files -- 204 passed;
.venv/bin/python -m pytest -n 6-- 31781passed, 3 failed (the libreoffice and two simulation cases, which fail
the same way on main on this computer); diff coverage against main 100%
(30/30 changed executable lines).
Real machine (a registered Linux machine; everything under this check's
own directory, removed after): uv 0.4.22 from uv's release page was put
first on the PATH the check's own runner gives the probe, with an index
configured. Before this commit: the first install passed it over and
fetched uv 0.13.0 under
node_dir(85 MB planned), and the second lookfound 0.4.22 first again, planned to fetch 0.13.0 again (85 MB, an
uvstep), and with curl and wget taken away was refused with "upgrade uv
there" while 0.13.0 sat under
node_dir. After: the same first install,then the second look found the fetched 0.13.0, planned 60 MB with no
uvstep, and went ahead without curl or wget.
.venv/bin/python -m pytest tests/test_node_install.py-- 80 passed. Eachnew or changed case failed on main first: with an index configured, the
real install test's packages step really ran
uv pip installagainst thetest's mirror and failed resolving it (now stood in for, both steps fed the
index on stdin);
plan()of a machine whose index names a port that is nota port now succeeds,
refusal()gives the sentence,install()runsnothing, and a machine that has this node is found so and pruned; for a uv
at 0.4.23 and later, older, and silent about its version, with and without
curl or wget, the plan uses the machine's uv or fetches the pinned one and
says why, or
refusal()gives the exact sentence; an old uv still serves amachine with no index; under a real
/bin/shthe probe reports the versiona stand-in uv prints and nothing for one that prints none.
.venv/bin/python -m pytest tests/test_node_install.py tests/test_cli_ops_connection_commands.py tests/test_node_bundle.py tests/test_node_client.py tests/test_import_cycle_budget.py -n 6-- 202 passed.
.venv/bin/python -m pytest -n 6-- 31780 passed, 3 failed(
test_rpc_files.py::test_a_host_without_libreoffice_says_soand the twosimulation cases, which fail the same way on main on this computer); diff
coverage against main 100% (26/26 changed executable lines).
ruff check,ruff format --check,ty check raven/node/install.pyclean;
check_commit_messages.py,check_source_language.pyandcheck_large_files.pyonorigin/main..HEADclean.Real machine (a registered Linux machine whose pip is configured with an
HTTPS mirror; this check's own node dir, removed after): uv 0.4.22 from uv's
release page was put where the probe finds a uv, and the install was given
an index pointed at a port nothing listens on, so whether uv used it shows.
With main's code the old uv was used and the install succeeded -- uv went to
PyPI behind the plan. With this change the probe read 0.4.22, the plan
fetched uv 0.13.0 and said why, the packages step failed with "Connection
refused" (the index was used), and the node dir held uv 0.13.0 after.
Relevant tests pass locally
Relevant lint / type checks pass locally
User-facing docs or screenshots are updated when needed
The install module's docstring now says an old uv with an index configured
is passed over; the new
Planfield and the moved check are documentedwhere they are.
CONTEXT.md(the node entry) does not describe uv versions orthe index and still holds.
Risk
A machine whose own uv is older than 0.4.23 and whose pip is configured with
an index now gets the pinned uv fetched under the node dir (about 25 MB, as
for a machine with no uv), or one sentence asking to upgrade uv when it has
no curl or wget; every other machine is unchanged. A machine with a bad port
in pip's configuration can again be told it has the node and be pruned; an
install there is still refused with the same sentence. The change is to the
install plan and one probe line; rollback is a plain revert.
Related Issues
Follows the review of #963 (the observations F1, F2 and F3 left on it).