Skip to content

feat(*): a Raven daemon on a registered machine, reached over one watched ssh link - #920

Open
Handsome-wzw wants to merge 13 commits into
EverMind-AI:mainfrom
Handsome-wzw:feat/subagent_server_node
Open

Handsome-wzw wants to merge 13 commits into
EverMind-AI:mainfrom
Handsome-wzw:feat/subagent_server_node

Conversation

@Handsome-wzw

@Handsome-wzw Handsome-wzw commented Oct 12, 2026 •

Copy link
Copy Markdown
Contributor

Summary

First of three changes that let a Raven dispatch work to the machines in
its machine registry on a design where the machine owns the work: a Raven
daemon runs there, the ssh is only the way to it, and an ssh that goes
mid-task does not end the task. This one puts the daemon on a machine and
keeps the link to it; nothing dispatches to a machine yet. The next change
runs acp sub-agents through it, the one after reads and writes files there
by address.

It follows #893 (the ssh line and the remote working directory) and
replaces #897 and #901, closed when the design changed: their code is in
here, reworked to the daemon.

After the review's second round, four more things hold: a page read
(read_file, here and on a machine) reads the file a chunk at a time
and decodes only the page's own lines, so no one step holds the
interpreter for the whole file and the file is never whole in memory;
the whole-file calls on a machine go through one gate (readers
together, a writer alone, a waiting writer first), so a read never
answers with the old page and the new digest; the install reads the
index pip is configured with on the machine the way pip does, names it
in the plan and hands it to uv pip install; and a machine without uv
gets one pinned version as an unmanaged install, which writes no
receipt under the home directory and changes no PATH, while a node
directory that was there keeps its mode.

What a daemon is (raven/node/):

  • This Raven's own code, running as a daemon on a registered machine.
    The host's ssh runs a relay there (python -I -m raven.node --attach --dir <node_dir>, raven/node/relay.py): it bridges the ssh session
    to the daemon's unix socket under the node directory, starts the daemon
    when none answers (--serve, detached, its words in log/serve.log),
    and leaves with the ssh. The daemon leaves on its own after ten minutes
    with no connection and no running agent; the next relay starts it again,
    so nothing is registered as a service and a reboot costs one connect.
    It opens no network port; the socket is the registered user's own
    (mode 600) in run/ (0700), or, when that path is too long for a
    socket, in a directory of the user's own under the temp directory
    (0700, checked before use: a directory of the user's, never a link,
    never another user's, narrowed when wider). The relay forwards only to
    a socket of the user's and starts no daemon beside anything else; a
    daemon whose socket's place holds a thing it cannot remove says so in
    one line and leaves rather than listen beside it. ssh login is the
    authentication.
  • It speaks newline-framed JSON-RPC 2.0, the framing ACP uses, so the
    host drives it with AcpClient (over an Endpoint seam this change
    adds: a process, or a channel of a link). Protocol 3.
  • File calls (fs/read, list, find, grep, stat, write, edit,
    pull) run Raven's own file tools there, fenced to the roots each
    call names, so a remote read comes back exactly as a local one would.
    A write may carry expect, the digest the file had when it was read,
    and is refused with CHANGED when it no longer matches. A page read
    with its digest, a stat with one, a chunk and a checked write run in a
    thread of the daemon's, writers one at a time under a lock, so the
    loop that answers the heartbeat and forwards every agent's frames is
    not held for the length of a large file.
  • Agents (agent/launch, frame, stderr, exit, cancel, close):
    the command named is started in the machine user's login environment
    (captured once by the daemon itself, under bash or zsh -lic),
    in its own process group, under a task id of the host's. Its ACP
    frames travel on a numbered channel of the connection that holds it,
    and are kept in the task's record on disk (<node_dir>/tasks/<task>/:
    task.json, frames.jsonl, stderr.log; the directories 0700 and the
    files 0600, as the host's own journal is) as they arrive. A connection
    that goes leaves the agent running; agent/attach on a later
    connection replays the record from the frame the host last read and
    takes the task over, and a finished task is replayed from its record
    alone, even by a daemon that never ran it. agent/cancel ends the
    whole process tree: the tree is read off ps before the first signal,
    so a command the agent put in a session of its own (Claude Code's shell
    tool does) goes too. task/list names the tasks running or remembered;
    records are kept seven days. node/shutdown ends agents only under
    --stdio, the one mode that ties them to the connection (tests, and a
    fallback).
  • Probes (probe/machine, probe/agent) and session/dir, which makes
    a session directory and says where it is as the machine resolves it.
  • Small on purpose: it imports the file tools and nothing that talks to a
    model, reads a config or opens a port; beside Raven's code the machine
    needs uv (or curl or wget, to fetch one with), a Python 3.12 if it has
    one (else uv brings one), loguru and pillow (about 55 MB, more with a
    fetched uv or Python), ripgrep-bin when its index has it. A test runs
    the node with nothing importable beyond the standard library, its own
    code and those two packages, and asks every file call, so the next
    import added under raven/agent/tools is caught here rather than on
    every installed node.

The same code at both ends (raven/node/bundle.py): the code is named by
a digest of the package's files, and node/hello compares it, so a
daemon of other code is refused with the command that updates it. An
install lives in <node_dir>/<version>-<digest>.

The host side (raven/node/client.py):

  • One link per machine, shared by every caller in the process; a link
    idle for ten minutes with no agent on it and no call on its way to it
    is closed the next time any link is asked for. A heartbeat every 5 s;
    one unanswered for 15 s means the link is lost.
  • A lost link no longer fails the agents on it. Each endpoint waits,
    holding what the host writes meanwhile (up to 1000 frames); the link is
    tried again on a backoff (2, 4, 8, 16 s) for as long as any agent
    waits; a link that comes back attaches every task again from the frame
    the host last read, sends what waited and any cancel asked in between.
    Only a task the machine no longer knows ends an endpoint, in one
    sentence. A link closed on purpose just closes: the daemon keeps its
    agents, and tasks() on a later link lists them. The listing says
    "link lost: ...; N tasks continue on the machine; reconnecting".
  • Every failure is one sentence that names the machine and what to do,
    never its address: not installed (with the install command),
    unreachable, login refused, other code, a refused handshake, a call
    that ran out of time, or the daemon's own last word.

The registry's package (raven/ops/remote_path.py) tells a remote path
from a local one (a UNC or extended Windows path, and on a Windows host a
drive-relative one, are this computer's), resolves it against the
registry, and lends the tools the pool through one seam (use_nodes,
nodes) -- the tools must not import the node package, which imports
them. raven/acp_client/remote.py keeps the ssh command, the machine
lookup, the posix wrapping of a remote command and the one-sentence
reading of an ssh failure from #893, and loses the launch command, the
env preamble and the session-directory ssh that #897 would have used:
the daemon does those. The ssh command is handed to the connection as an
argv, and AcpClient.launch takes one as it is, so no host's own grammar
reads it again (a Windows host's parser split the line into twelve
words). What reaches a model is a path on the machine written the way
scp writes it, address included, because that is how a file there is
named to the tools; the way in -- the key path, ssh's options, ssh's own
messages -- reaches no config file, log line or model, and an error names
a machine by id and name only. The lane stays
debug-only (RAVEN_DEBUG_MACHINES=1, ruled on #897): with it off no
daemon is started and no install is planned, each saying why.

Installing (raven ops connection install-node <id>,
raven/node/install.py): one look at the machine (the install, its uv or
a curl or wget to fetch one with, a Python 3.12 if it has one, the free
space), then a question, then everything through uv, as the repository's
dependency rule asks: a uv fetched under the node directory from its
installer when the machine has none, a virtualenv uv makes of the
machine's Python 3.12 or of one it brings, the pinned packages with
uv pip, the code as a tarball on ssh's stdin, each step one ssh call
and a failed step removing the half-made install; the node directory is
made 0700. A machine with neither uv nor a way to fetch one is refused
with the installer's address. --dir keeps installs elsewhere and
records it on the row; --prune removes other versions. Nothing else on
the machine changes. raven ops connection doctor reports the registry
as before.

Also: CONTEXT.md defines the Raven node and seats raven.node inner in
the import contract (pyproject.toml, lint-imports); the repo layout
table gets its row; the gateway, the TUI and the RPC entrance close the
node pool with the ACP pool on shutdown.

Type

  • Fix
  • Feature
  • Docs
  • CI / tooling
  • Refactor
  • Other

Verification

  • uv run pytest tests/test_node_bundle.py tests/test_node_server.py tests/test_node_client.py tests/test_node_relay.py tests/test_node_install.py tests/test_acp_client_teardown.py tests/test_acp_remote.py tests/test_ops_remote_path.py tests/test_cli_ops_connection_commands.py tests/test_ops_connection_add.py tests/test_ops_transport.py tests/test_subagent_acp.py tests/test_acp_client*.py tests/test_cli_gateway*.py
    -- 859 passed on this tree; diff coverage of this change's lines from
    these groups alone passes the 90% gate. Covered, beside what feat(*): run a Raven node on a registered machine to answer file calls there #901's
    tests covered (parity of every file call with the tool's own answer,
    the fence, each malformed field, the handshake, the stdio loop, the
    install's scripts and one real install on this computer, through uv):

    • The socket's directory is the user's own or not used (a file, a
      link and another user's directory refused; wider bits narrowed); a
      daemon refused its socket's place says so and leaves; the relay
      forwards to nothing that is not the user's and starts no daemon
      beside it; a task's records are 0700 and 0600; a ping is answered
      while a file's digest is taken; a call on its way keeps its link;
      the ssh argv reaches the connection as it is and AcpClient.launch
      takes an argv; the installer fetches uv when the machine has none
      (curl or wget) and refuses a machine with neither; a drive-relative
      path is this computer's on a Windows host and scp's elsewhere, an
      extended or UNC path this computer's anywhere; the node answers
      every file call with only its requirements importable.
    • A task is recorded as it runs and goes on when its connection ends;
      a second connection attaches from the frame the host had not read and
      hears the exit; a finished task is replayed from its record by a
      daemon that never ran it; a record left "running" by a daemon that
      died reads as lost; an attach for a task nobody knows is refused by
      code; the task list and the sweep of old records; shutdown from a host
      leaves a daemon's tasks running.
    • The daemon on its socket: two connections in turn, the agent surviving
      the first, the daemon leaving when idle and leaving at once when
      another serves the same directory; --serve and --attach need
      --dir.
    • The link, over a stand-in ssh that hands the line to a real login
      shell and starts the real relay and daemon: a request made while the
      link is down is answered after the link is back, on the same daemon;
      a daemon told to stop takes its agents and the turn says so; a link
      closed on purpose leaves the agent for the next link to list, attach
      and cancel; a heartbeat lost parks the endpoint and the reconnect
      attaches it again; a task the machine no longer knows ends its
      endpoint in one sentence; an attach the link cannot carry parks every
      waiting endpoint again; held frames are capped; a file fetched whole
      and checked, and refused outside the roots.
    • The relay in this process: it starts a daemon when none answers,
      bridges bytes both ways, reaches the same daemon again without
      starting another, and gives up in one sentence when none comes up.
    • Cancel ends a child the agent put in a session of its own; the tree
      is read off ps; the daemon's own group is never signalled.
  • uv run pytest --collect-only -- no import error; lint-imports --
    10 kept, 0 broken. The daemon's two test modules skip on win32 before
    importing it (fcntl, unix sockets), and the two POSIX teardown tests
    lend a SIGKILL where the platform has none.

  • uv run pytest on the final tree of the three changes, which includes
    this one -- 29217 passed, 121 skipped, 3 failed:
    test_rpc_files.py::test_a_host_without_libreoffice_says_so,
    test_simulation_scenario.py::test_a_fresh_trial_refreshes_the_subagent_homes_before_it_runs
    and test_simulation_suite.py::test_a_stopped_run_is_marked_in_its_record_and_finished_with_its_evidence,
    which fail the same way on main on this machine. The groups above
    also pass under --idle-ceiling-strict, CI's idle ceiling.

  • Real machines (the final tree of the three changes, two registered
    Linux machines, a scratch registry and RAVEN_HOME):

    • Install through uv on both: on the machine without uv, uv fetched
      with the machine's curl under the node directory, then a virtualenv
      of the machine's own Python 3.12, 16 s in all; on the machine whose
      home disk was full, under /tmp, with a Python uv brought, 11 s.
      Both nodes answered the handshake with this code's digest.
    • Connect and handshake 0.7 to 0.9 s; session/dir 11 to 18 ms;
      agent/launch 13 ms; a request's round trip 12 ms.
    • With a real Claude Code running a 25 s shell loop: the host's ssh
      killed mid-turn, the link read as lost in 5 s, the turn finished 27 s
      after the kill with the agent's reply, 25 of 25 lines written, the
      same daemon on both sides of the gap; a cancel left 0 processes 3 s
      later; three sessions at once answered in 8 s; with every link closed
      the daemon stayed with its records and no agent process remained.
  • The second round's four notes (tests/test_filesystem_tools.py tests/test_node_install.py tests/test_node_server.py tests/test_node_client.py tests/test_read_file_image.py -- 337
    passed, 100% diff coverage of that commit's lines): a page is the
    lines splitlines gives whatever the chunk edge, over every break it
    knows; a 120 MiB file pages with every ping answered within 0.25 s; a
    read with a slowed digest beside a write of the same file answers with
    the old page and the old digest and the write lands after; a waiting
    writer goes before new readers; the probe reads pip.conf and
    PIP_INDEX_URL as pip does under a real shell, the plan names the
    index and the install hands it to uv; the uv step fetches one pinned
    version unmanaged; a node directory that was there keeps its mode and
    one the install makes is owner-only. On a registered Linux machine
    with the node reinstalled: a page read of a 1 GiB file there beside
    pings every 50 ms kept the longest wait of a ping at 0.34 s; the reads
    sent during a write all answered with a page and a digest of one
    version; the probe found the mirror that machine's pip is configured
    with; an install into a fresh node directory, forced down the fetched
    path, put uv 0.13.0 under it unmanaged in 24 s, with the receipt under
    the home directory untouched and the directory at 700.

  • The full review's five findings (tests/test_node_client.py tests/test_node_server.py tests/test_node_relay.py -- 223 passed, the
    seven new tests failing on the code before the fix, diff coverage of
    that commit's lines past the gate): a link the heartbeat has not yet
    seen die, replaced by an ordinary call, brings its parked agents along
    (agent/attach on the new link, nothing left parked); a replay the new
    link cannot carry keeps every frame it did not take, parks the endpoint
    being replayed and every one not yet reached, and the link is tried
    again; a cancel asked on a dead link, or failing on the new one, stays
    wanted until a link delivers it, and a refusal clears it; two installs
    on one long node directory get sockets apart; a launch still capturing
    the login environment is counted, so a second one at the limit is
    refused before the first spawns. On a registered Linux machine, one
    script run before and after the fix (the local relay killed with the
    heartbeat kept out of the way, then a plain call; a cancel asked in
    that gap; eleven launches at once against the limit of eight): before,
    the agent stayed parked and never answered, the cancelled task ran on
    with its record saying running, nine launches started and ten tasks ran
    there; after, the call took 0.3 s and the agent answered through the
    new link, the task ended within the reattach with its record saying
    so, eight started and three were refused with eight running, and
    nothing was left running on the machine. The other half of the first
    finding, a relaunch that fails under the call with agents parked: on the
    machine, the relay killed and the relaunch made to fail once, before the
    agent was still parked 30 s after the machine was reachable again and no
    reconnect was under way; after, the reconnect the heartbeat loss
    schedules runs here too, the agent was attached again on the first retry
    after the machine answered and answered through the restored link.

  • scripts/check_source_language.py, scripts/check_commit_messages.py
    and scripts/check_large_files.py on the branch range, commitlint on
    the commit and the prospective squash header, ruff -- all clean.

  • Relevant tests pass locally

  • Relevant lint / type checks pass locally

  • User-facing docs or screenshots are updated when needed

The command describes itself in --help; user docs follow with the
change that runs sub-agents on a machine.

Risk

Opt-in and inert: nothing starts a daemon unless the debug lane is on and
a caller asks for a machine, and an install runs only when the owner asks,
after saying what it will do. On a machine it adds one directory under
~/.raven-node (or the row's node_dir): the install, a run/ with the
socket, lock and pid of this install, a tasks/ with the records, a
log/; removing the directory uninstalls it. The daemon runs as the
registered user, listens on a socket in a directory only that user can
open (and the relay forwards to a socket of that user's only), reads and
writes only inside the roots each call names, and leaves when idle. A
task's record holds the agent's ACP frames, which may quote what the
agent read: it is the user's alone (0700, 0600) and is removed after
seven days. The way in stays in the ssh argv, in memory; no error names
the address, and a model sees it only as part of a path on the machine,
written the way scp writes it. Rollback is a plain revert; an installed
daemon is then an inert directory that leaves once idle.

  • Security impact considered
  • Backward compatibility considered
  • Rollback path is clear for risky changes

Related Issues

Follows #893; replaces #897 and #901.

…ched ssh link

A registered machine gets this Raven's own code as a daemon (raven/node):
started by the relay the host's ssh runs there (`python -I -m raven.node
--attach --dir <node_dir>`) when none answers on its socket under the
node directory, and left on its own once idle. It answers file calls with
Raven's own tools fenced to the directories each call names (fs/read,
list, find, grep, stat, write, edit, pull), starts acp agents under task
ids of the host's in their own process groups in the machine user's
login environment, keeps each agent's frames in the task's record on
disk so a connection that goes leaves the agent running and the next one
takes it over from the frame the host last read (agent/attach), lists
the tasks it runs or remembers, and probes the machine and an agent's
command. It opens no network port; only --stdio ties the agents to the
connection. `raven ops connection install-node <id>` puts it on a
machine after one look and one question, and `--prune` removes older
installs.

The host keeps one link per machine (raven/node/client.py): the ssh line
runs the relay, a heartbeat watches the link, a lost link parks the
agents on it and is tried again on a backoff for as long as any waits,
and a link that comes back attaches every task again by its id, sending
what the host wrote meanwhile and any cancel asked in between. The
registry's package tells a remote path from a local one and lends the
tools the pool through one seam (raven/ops/remote_path.py); the machine
lane stays debug-only (RAVEN_DEBUG_MACHINES=1).

First of three changes on this design; the next runs acp sub-agents on
a machine through this daemon, the one after reads and writes files
there by address. Nothing dispatches to a machine yet.

Co-authored-by: Claude (claude-fable-5-1) <noreply@anthropic.com>
@Handsome-wzw
Handsome-wzw requested a review from LivXue October 12, 2026 11:06

@gloryfromca gloryfromca left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Blocking: the node installer must use uv instead of pip.

I reviewed the full github/main...HEAD diff and covered the repository rules (AGENTS.md, CLAUDE.md, CONTEXT-MAP.md, and the Raven node additions to CONTEXT.md), affected ACP/node/file-path callers, the single-commit history, backward compatibility of the generalized ACP endpoint, test changes, and the documented layer-seat constraints. I found no weakened tests.

Verification after uv sync --all-extras --all-groups:

  • uv run pytest tests/test_acp_client_teardown.py tests/test_acp_remote.py tests/test_cli_ops_connection_commands.py tests/test_node_bundle.py tests/test_node_client.py tests/test_node_install.py tests/test_node_relay.py tests/test_node_server.py tests/test_ops_connection_add.py tests/test_ops_remote_path.py tests/test_ops_transport.py -q - 443 passed, 4 skipped because /bin/tcsh and /bin/csh are unavailable.
  • uv run pytest tests/test_subagent_acp.py tests/test_acp_protocol.py tests/test_acp_journal.py -q - 365 passed.
  • Focused Ruff, source-language, large-file, commit, and diff-whitespace checks passed (the make executable is unavailable, so the two repository check scripts were invoked directly).

The initial focused pytest attempt exercised no tests because the fresh environment lacked the local raven_everos development package; synchronizing the declared extras/groups resolved that setup issue.

One blocker is marked inline.

Comment thread raven/node/install.py Outdated
@LivXue

LivXue commented Oct 12, 2026

Copy link
Copy Markdown
Member

Windows compatibility review: two findings.

Scope: complete 34-file PR diff at 71f47d1354976ef26da43ed1dddfd53becc35e23, API base 82c76a82a9a7d312ef6cd75e560404bccd81d292, merge base 829eb28e1db877dffdd3df4f835d707e1f049d3a, including source, callers, tests and surrounding platform guards. Inspected isolated exact-SHA source archives; all changed-file blob hashes and all patch addition/deletion counts matched GitHub.

  1. P2 - Serialize the host SSH argv with the host's quoting rules. Reproduced. remote.py:188 uses shlex.join, but NodePool._launch sends that string to AcpClient.launch and its Windows command_argv parser. With the machine lane enabled, the actual node launch's one remote-command argument becomes 12 arguments; the reconstructed remote shell command starts with the program name exec /bin/sh -c echo;, rather than executing exec. Node handshakes, including the final install-node check, therefore cannot start correctly from Windows. Pass argv directly or serialize it with the same platform grammar as the consumer, keeping POSIX quoting inside the remote-command value. The base's older launch_command already had this quoting pattern, but has no production callers in the base tree; the new node path makes it reachable.

  2. P2 - Gate POSIX-only tests before importing their implementation. Reproduced. test_node_server.py:29 and test_node_relay.py:17 import the POSIX daemon unconditionally, stopping Windows collection at import fcntl. The new teardown test also forces _POSIX=True without providing SIGKILL, and fails on Windows. Skip the daemon modules at module level before those imports, and skip or fully emulate the POSIX teardown branch. This finding concerns host test collection; the daemon itself is intentionally POSIX. The real Windows teardown branch passed its two selected tests.

Native Windows / Python 3.13.12, PYTHONUTF8=1, UV_NO_SYNC=1, isolated user/Raven/temp directories. A pre-synced uv environment supplied the unchanged locked dependencies; imports were asserted to come from each inspected snapshot. Commands ran from the named snapshot root.

uv run python 'C:\Users\livxu\AppData\Local\Temp\raven-windows-review\5f9cc24f22014021985a6c637edc57e3\probe_windows.py' node-ssh
# 12 remote-command arguments instead of 1; native producer/parser mismatch
uv run pytest 'tests/test_node_bundle.py' 'tests/test_ops_remote_path.py' -n 0 -x --tb=short --basetemp 'C:\Users\livxu\AppData\Local\Temp\raven-windows-review\5f9cc24f22014021985a6c637edc57e3\pr920\node-tests-pytest'
# 93 passed, 1 skipped (symlink privilege)
uv run pytest 'tests/test_node_client.py' -n 0 -x --tb=short --basetemp 'C:\Users\livxu\AppData\Local\Temp\raven-windows-review\5f9cc24f22014021985a6c637edc57e3\pr920\client-tests-pytest'
# 24 passed, 29 skipped (POSIX-shell fixtures)
uv run pytest 'tests/test_node_server.py' 'tests/test_node_relay.py' -n 0 -x --collect-only --tb=short --basetemp 'C:\Users\livxu\AppData\Local\Temp\raven-windows-review\5f9cc24f22014021985a6c637edc57e3\pr920\collect-pytest'
# collection failed: ModuleNotFoundError: fcntl; exit 1
uv run pytest 'tests/test_acp_client_teardown.py' -n 0 -x --tb=short --basetemp 'C:\Users\livxu\AppData\Local\Temp\raven-windows-review\5f9cc24f22014021985a6c637edc57e3\pr920\teardown-pytest'
# 1 failed: signal.SIGKILL absent; exit 1
uv run pytest 'tests/test_acp_client_teardown.py' -k 'without_process_groups or platform_is_read' -n 0 -x --tb=short --basetemp 'C:\Users\livxu\AppData\Local\Temp\raven-windows-review\5f9cc24f22014021985a6c637edc57e3\pr920\windows-teardown-pytest'
# 2 passed, 2 deselected

The argument probe uses the real NodePool._prepare and command_argv, with only the registry lookup substituted. No live SSH, remote install, daemon execution, live agent credentials or full suite was exercised. The existing uv-only installer finding remains a separate discussion.

@LivXue

LivXue commented Oct 12, 2026

Copy link
Copy Markdown
Member

Blocking: 1 finding needs correction before this revision can merge; it is the first of the notes below.

Scope: none of this PR's 34 files is under raven/agent/**, the part of the tree I review, so nothing is filed inline. That package is still reached: the node runs its file tools on the registered machine (eight raven.agent modules, raven/agent/tools/filesystem.py and file_search.py among them), the AcpClient endpoint seam sits under the ACP backends in raven/agent/subagent, and make_ssh_runner serves exec(machine=...). I read every non-test file of the diff, and the tests where a note turns on them; the notes below are about the files that carry the package there, stated here rather than inline. The Blocking note at raven/node/install.py:187 and the Windows compatibility review above stand on their own and are not repeated here.

Notes:

  1. Blocker - keep the daemon's socket in the owner-only run/ directory, or check the listener before forwarding to it. socket_path (raven/node/server.py:545) puts the socket under <node_dir>/run/, which the daemon makes 0700, only while that path fits in 100 bytes. Past that -- a node directory longer than 72 bytes with this head's install name -- it uses raven-node-<digest of the node directory>.sock in the temp directory (/tmp unless TMPDIR says otherwise), outside any directory the machine user owns. The relay (raven/node/relay.py:63) then forwards the host's lines to whatever is listening at that path, without checking that the listener is the machine user's own daemon, so nothing guarantees that the link's traffic, including the env each agent/launch carries, reaches that daemon. A file at that path that the user cannot remove also keeps the daemon from starting: sock.unlink() (server.py:1214) suppresses only FileNotFoundError. Run as one account: with a 78-byte node directory the socket path moved to /tmp/raven-node-<16 hex>.sock; a listener already bound there received the line the relay forwarded, and the relay started no daemon. The cross-account case is code-reading. The default ~/.raven-node stays under the limit for an ordinary home; a longer --dir is what reaches the fallback. The Risk section's "a socket only that user can open" holds for the socket the daemon binds; the relay does not check that the socket it reaches is that one.

  2. Nonblocking - create the task records owner-only, as the host's own frame journal is. The daemon makes tasks/ and each tasks/<task>/ with the default mode and opens frames.jsonl and stderr.log the same way (raven/node/server.py:798, :834, :617; task.json through _write_json, :760), and the installer's mkdir -p "$dir" makes the node directory that way too (raven/node/install.py:178, :184); only run/ (0700) and the socket (0600) are narrowed. Under umask 022 a real daemon (--serve) left the node directory, tasks/ and the task's directory at 0755 and all three record files at 0644. frames.jsonl holds the agent's output frames verbatim -- its replies and tool results, which can quote any file it read -- so any account that can traverse the user's home can read them. The host writes the same kind of record at 0600 (raven/acp_client/journal.py:161), and the Risk section describes the record as living "in the user's own directory".

  3. Nonblocking - keep whole-file work off the daemon's event loop. One fs/read runs ReadFileTool.execute, which reads and decodes the whole file (raven/agent/tools/filesystem.py:264), and then hashes the whole file (raven/node/server.py:253); both run on the daemon's only event loop, which also answers the heartbeat and forwards every agent's frames. fs/stat with hash, the first fs/pull chunk and an fs/write with expect hash the whole file the same way. Measured with python -m raven.node --stdio at this head, pinging every 0.2 s, on a 1 GiB file read from the page cache: asking for its first 10 lines took 3.50 s, with no ping answered for 3.46 s of it (baseline 0.2 ms); fs/stat with hash took 0.88 s, with a 0.78 s gap. Scaled linearly, a file of about 4.3 GiB holds the loop past the 15 s heartbeat limit, after which the host treats the link as lost and parks every agent on the machine; that last step is extrapolated, not run, and a cold or network disk reaches it sooner. Separately, NodePool.call treats any call that outlasts its timeout as a lost link (raven/node/client.py:347), even while the heartbeat is being answered.

  4. Nonblocking - pin the node's import budget with a test. A node is installed with loguru and pillow only (protocol.REQUIREMENTS), yet it imports eight raven.agent modules, and nothing on the host fails if one of them gains another third-party import: the one real install (tests/test_node_install.py:569) stands in for pip and then runs only --version, which needs the standard library alone, and the stand-in ssh tests run the node with the test's own interpreter (exec {sys.executable}). The budget holds today -- with every other third-party import blocked, all eight file calls and node/hello ran at this head (31 raven modules loaded, 8 under raven.agent) -- so this is about the next change to raven/agent/tools, which would break every installed node while the suite stays green. A test that runs the file calls with only the declared requirements importable would catch it.

  5. Nonblocking - count a call in flight as use before closing a link as idle. _close_idle (raven/node/client.py:767) closes another machine's link when it has no agent channel and its used is older than IDLE_CLOSE_S, and used moves only when a call returns (:358). A call that starts on a link near that limit is closed under itself as soon as a call for any other machine acquires a link, and it fails with "the Raven node on machine ... stopped", which blames the node for the host's own close. Reproduced with two stand-in links and idle_close_s=5: link A last used 4.5 s earlier, a 2 s call on A, a call on B one second in; A's link was closed and its call failed with that sentence.

  6. Nonblocking - make the absent directory in the login-shell test one the test controls. test_a_command_reads_the_same_under_every_login_shell (tests/test_acp_remote.py:385) checks [ ! -d /nonexistent ], and some machines have that directory: on the Linux box I ran it on, /nonexistent is nobody's home and exists, so the sh, dash and bash cases fail with exit 1 and an empty stderr. A path under tmp_path that the test knows is absent keeps the check without the dependency.

  7. Question - is RemotePath.display meant to reach the model? display (raven/ops/remote_path.py:127) renders a remote path from the registry row as user@host:path, or scp://user@host:port//path, and remote.shown (raven/acp_client/remote.py:401) calls it "what the file tools read back as a remote path". The same PR's remote.py says no address "reaches a config file, a log line or a model" (lines 18-19) and that the registry "keeps the address out of anything a model reads" (lines 74-75). Nothing calls either function here; in feat(*): read, write and deliver files on a registered machine by address #922 the file tools put display() into their results and errors (raven/agent/tools/remote_files.py, raven/agent/tools/deliver.py), and the remote agent's result note uses remote.shown (raven/acp_client/acp_agent.py), so the model would read registry addresses there. Which of the two is meant to hold?

Verification:

  • 17 test files at this head, with a copy of the dev environment whose editable install points at this head, so the relay's and the daemon's -I children import it too: tests/test_node_*.py (5 files), test_acp_client_teardown.py, test_acp_remote.py, test_acp_protocol.py, test_acp_journal.py, test_subagent_acp.py, test_ops_connection_add.py, test_ops_remote_path.py, test_ops_transport.py, test_cli_ops_connection_commands.py, test_scope_canon.py, test_l4_entrances.py, test_kernel_budget.py: 816 passed, 6 skipped, 3 failed (note 6).
  • lint-imports at this head: 10 kept, 0 broken.
  • The probes named in each note, run from this head on Linux with Python 3.12. No ssh to a real machine and no second account on my side.

Checked and deliberately not reported:

  • The debug gate: every way onto a machine passes remote.machine(), which refuses while RAVEN_DEBUG_MACHINES is off, and remote_path.target() then names no machine for any path; the gate is an environment variable, so the model cannot turn it on through raven_config.
  • The node pool is closed at the same three shutdown points as the ACP pool (gateway, TUI, RPC), after it.
  • find and grep, the Python fallback included, run off the event loop; fs/write compares expect and writes with no await in between.
  • The ACP backends under raven/agent/subagent construct no AcpClient themselves, so the endpoint seam reaches them unchanged, and test_subagent_acp.py passes. make_ssh_runner's new stdin=DEVNULL also covers exec(machine=...) (raven/agent/tools/machine_exec.py:344), whose remote command no longer inherits the caller's stdin.
  • feat(*): run an acp sub-agent on a registered machine through its daemon, add it from a chat #921 and feat(*): read, write and deliver files on a registered machine by address #922 are stacked on this commit and do not change raven/node/, raven/acp_client/remote.py or raven/ops/remote_path.py again, so these notes are filed here.

@LivXue LivXue left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Requesting changes: 1 blocking finding, with 5 nonblocking notes and 1 question, is in the PR-level summary comment. None is filed inline, as no file in this PR is under raven/agent/**, the part of the tree I review; the summary also carries the scope statement and the verification log.

Handsome-wzw and others added 2 commits October 13, 2026 01:15
…when the machine has none

The install ran `python -m venv` and `python -m pip install` on the
machine, against the repository's rule that uv is the only Python package
manager. Everything Python now goes through uv: `uv venv` of the machine's
own Python 3.12 or of one uv keeps inside the node directory, `uv pip
install` for the pinned packages and the optional search package. A
machine without uv gets one under `<node_dir>/.uv` first, from uv's own
installer fetched with the machine's curl or wget and told to touch
nothing else; the probe looks for curl and wget, and for a uv an earlier
install put there; with no uv and no way to fetch one the install stops
and says so. The plan counts the room uv takes and says when it is fetched.

Co-authored-by: Claude (claude-fable-5-1) <noreply@anthropic.com>
…h argv goes as it is

The socket's fallback for a long node directory lay in the temp
directory itself, so the relay forwarded the host's lines to whatever
listened at that path, and a file there the user could not remove kept
the daemon from starting: the fallback now lies in a directory of the
user's own (0700, checked, never a link or another's), the relay
forwards only to a socket of the user's, and a daemon refused its
socket's place says so in one line and leaves. The task records -- the
agent's frames as they were -- were made with the default mode: tasks/,
each record and its three files are the owner's alone now, as the
host's journal is, and the installer makes the node directory so. A
page read, a digest, a chunk and a checked write ran on the daemon's
one loop, which also answers the heartbeat: they run in a thread now,
writers one at a time under a lock. The ssh argv went to the connection
as one shell-quoted line, which a Windows host's own grammar split into
twelve words: it goes as the argv, which no host reads again. A call on
its way to a node counts as use, so a link is not closed under it as
idle. A drive-relative or extended Windows path (C:file, \\?\C:\...)
no longer reads as a machine's. The daemon's tests skip where there is
no fcntl, the POSIX teardown tests lend a SIGKILL where the platform
has none, the login-shell test checks a directory of its own instead
of /nonexistent, a node is run with nothing importable beyond its
requirements, and remote.py says what does reach a model: a path on
the machine, written the way scp writes it.

Co-authored-by: Claude (claude-fable-5-1) <noreply@anthropic.com>
@Handsome-wzw

Copy link
Copy Markdown
Contributor Author

Thank you for the two reviews. Every point is in the commits now on this branch; the uv-only installer is the commit before them.

  1. Blocker, the socket's fallback: it now lies in a directory of the user's own under the temp directory (raven-node-<uid>/, 0700, checked before use: a directory of the user's, never a link, never another user's, narrowed when wider); run/ is checked the same way; the relay forwards only to a socket of the user's and starts no daemon beside anything else; a daemon whose socket's place holds a thing it cannot remove says so in one line in log/serve.log and leaves rather than listen beside it (own_dir, owned, socket_path in server.py; connect in relay.py; tests for a file, a link and another user's directory in the socket's place, for the daemon and for the relay).
  2. Records: tasks/, each record and task.json, frames.jsonl, stderr.log are made 0700 and 0600 (own_file), log/ and serve.log too, and the installer makes the node directory 0700 (test).
  3. Whole-file work: fs/read, fs/stat, fs/pull, fs/write and fs/edit run in a worker thread (_apart), writers one at a time under a lock so a write's check and its write stay one stretch; a ping is answered while a file's digest is being taken (test). NodePool.call reading its own timeout as a lost link is left as it is for now.
  4. Import budget: test_a_node_answers_every_file_call_with_only_its_requirements_importable runs the node with nothing importable beyond the standard library, its own code, loguru and pillow, and asks every file call.
  5. A call in flight counts as use: _Node.calls, and _close_idle leaves a link with one (test with a link that answers slowly while another machine is asked for).
  6. The login-shell test checks a directory under tmp_path.
  7. RemotePath.display is meant to reach the model: a path on a machine is written the way scp writes it (the design's ruling), and that is how the file tools and deliver_files take it. What stays out of models and logs is the way in -- the key path, ssh's options, ssh's own messages -- and an error names a machine by id and name only. The two docstrings in remote.py said more than that; they now say this.

Windows: remote.ssh_command returns the argv and AcpClient.launch takes an argv as it is, so no host grammar reads the remote command again (tests/test_acp_client_launch.py, and the node's argv checked at the connection in test_node_client.py); test_node_server.py and test_node_relay.py skip on win32 before importing the daemon, and the two POSIX teardown tests lend a SIGKILL where the platform has none.

@Handsome-wzw
Handsome-wzw requested a review from LivXue October 12, 2026 18:23

@gloryfromca gloryfromca left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No blockers; suggestions only, and they are marked inline.

Reviewed the new-head delta and its integration with the full PR diff, callers, and history. The earlier uv-only blocker is fixed: node setup now uses uv venv and uv pip install, obtains a private uv when possible, and refuses installation when uv cannot be obtained. The host-safe SSH argv, owner-only socket/records, off-loop whole-file work, in-flight call accounting, Windows collection guards, import-budget coverage, and remote-path documentation changes also match the failures previously reported.

One nonblocking test-portability issue is marked inline. It is introduced and reproducible under an ordinary umask 0002, but it affects only the assertion, has the umask 0022 workaround, and does not leave an operator without a working product path, so it does not meet the round-four blocker bar.

Verification: the affected suite produced 462 passed, 4 skipped, 1 failed on Python 3.12.3; the sole failure is the inline umask assumption. The isolated case failed the same way on Python 3.13.2 and passed under umask 022. ACP regressions passed (365). Ruff, all 10 import contracts, source-language and large-file gates, and git diff --check passed. make is unavailable in this environment, so I ran the two canonical gate scripts directly with their documented revision range.

Coverage included AGENTS.md/CLAUDE.md and CONTEXT routing, the diff and new commits, callers, history, backward compatibility, test weakening, and the stated architecture boundaries. The Windows module skips occur before POSIX imports and do not skip those tests on POSIX.

Comment thread tests/test_node_server.py
…test sets its own mode

The record opener handed os.fdopen a splatted keyword map, which no
overload of it matches under the type check: the binary and the text
mode are two calls now. The test of another user's directory asserted
the mode mkdir leaves, which is the umask's (0775 under 0002, where it
failed): the test sets 0755 first and asserts it is left so.

Co-authored-by: Claude (claude-fable-5-1) <noreply@anthropic.com>

@gloryfromca gloryfromca left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No blockers; this can merge as far as I am concerned.

Reviewed the complete delta from the previously assessed head. The explicit chmod(0755) fixes the only outstanding suggestion under both tested umasks, and splitting the binary/text os.fdopen calls preserves runtime behavior while satisfying the type-check overloads. I found no new issue.

Verification: the isolated ownership test passed under umask 0002 and umask 022; all 90 node-server tests passed; the previously affected 12-file suite passed with 463 tests and 4 environment skips; focused Ruff and ty check raven/node/server.py passed; git diff --check passed.

Coverage for this revision included the new diff, its own_file callers, commit history, backward compatibility, test-strength preservation, and the already-reviewed AGENTS.md/CLAUDE.md, CONTEXT terminology, and architecture constraints. This delta changes no dependency, domain term, public interface, or architectural edge.

@LivXue

LivXue commented Oct 12, 2026

Copy link
Copy Markdown
Member

Windows compatibility re-review: the previously reported SSH argv and POSIX-import/teardown failures are addressed in the exercised paths; one test expectation needs updating.

Scope: all 35 changed files at 2979e5617a552dcfae5afdb588971baedc0e5702; API base 82c76a82a9a7d312ef6cd75e560404bccd81d292, merge base 829eb28e1db877dffdd3df4f835d707e1f049d3a. Rechecked the complete PR scope and revision delta, using blob identity for unchanged previously reviewed files and tracing the affected callers.

P2 - Update the unknown-machine fixture for Windows drive-relative paths. Reproduced. tests/test_ops_remote_path.py:101 still expects a:b.txt to raise RemotePathError. The new Windows branch intentionally recognizes it as a local drive-relative path, so this case fails with DID NOT RAISE. Keep that runtime correction; use a multi-character unknown host in this fixture, or make its POSIX expectation platform-specific. This is part of the shared #920 change.

The native launch probe confirmed that the real node preparation passes an argv list, preserves a key path containing spaces, and supplies exactly one remote-command argument to the intercepted connection. A real local file was readable through absolute, drive-relative and extended Windows spellings with the machine lane enabled. Both POSIX daemon test modules now skip before importing fcntl; the four teardown tests pass.

Native Windows / Python 3.13.12. Git fetching failed; the isolated fixed-SHA snapshots were reconstructed and every blob plus the complete Git tree was verified. A fresh environment was created with uv sync --frozen --python 3.13 --extra dev after the first environment-reuse attempt failed before tests ran. Tests used isolated user/Raven/temp/registry paths, PYTHONUTF8=1, UV_NO_SYNC=1, UV_PYTHON=3.13, and UV_PROJECT_ENVIRONMENT=$R/venv; PYTHONPATH selected the tested snapshot and plugin roots, with the loaded source asserted. $R = 'C:/Users/livxu/AppData/Local/Temp/raven-windows-review/2d5904b4d92f4f238e9db27e2ade2ddf'.

uv run pytest tests/test_node_bundle.py tests/test_ops_remote_path.py tests/test_acp_client_launch.py tests/test_acp_client_teardown.py tests/test_acp_remote.py tests/test_node_client.py tests/test_node_install.py tests/test_node_server.py tests/test_node_relay.py -k 'not path_that_names_no_registered_machine and not each_agent_gets_an_owner_only_ssh_log' -n 0 -x --tb=short --basetemp "$R/checks/920-final-host/pytest" --junitxml "$R/checks/920-final-host/junit.xml"
# 235 passed, 58 skipped, 13 deselected. POSIX daemon/shell/install and symlink-privilege cases skipped.
uv run pytest tests/test_ops_remote_path.py -k path_that_names_no_registered_machine -n 0 -x --tb=short --basetemp "$R/checks/920-final-path/pytest" --junitxml "$R/checks/920-final-path/junit.xml"
# 11 passed, 1 failed (a:b.txt), 79 deselected.
uv run pytest C:/Users/livxu/AppData/Local/Temp/raven-windows-review/2d5904b4d92f4f238e9db27e2ade2ddf/test_windows_resolved_paths.py -k 'not executable_spellings' -s -n 0 -x --tb=short --basetemp "$R/checks/920-final-native/pytest" --junitxml "$R/checks/920-final-native/junit.xml"
# 2 passed, 1 deselected; scratch argv/path probes, no SSH connection.
# Exact API-base control, with source imports pinned to that snapshot:
uv run pytest tests/test_acp_remote.py::test_each_agent_gets_an_owner_only_ssh_log -n 0 -x --tb=short --basetemp "$R/checks/920-platform-control/pytest" --junitxml "$R/checks/920-platform-control/junit.xml"
# 1 failed: the same POSIX 0700 mode assertion as at the inspected head.

The host selection excludes the separately reproduced path-error group and test_each_agent_gets_an_owner_only_ssh_log. The latter also failed on exact base 82c76a82...; it is an inherited Windows mode assertion, not a new finding. Earlier runs on the immediately preceding head stopped at those two assertions; their source is unchanged by the final two-file daemon/type-check delta.

The daemon ownership/umask discussion is already resolved and is not repeated here. Remote daemon hardening was code-reviewed; no live SSH, remote installation, remote daemon execution or full suite ran. Windows skips do not establish those POSIX behaviors.

@gloryfromca

Copy link
Copy Markdown
Member

No blockers; suggestions only, and they are marked inline.

The new Windows evidence supersedes my earlier clean stance. I verified the reported mismatch at 2979e5617a55: with the Windows branch selected, parse("a:b.txt", rows) correctly returns None, while the shared fixture still expects every listed value to raise RemotePathError. The native-platform tests/test_ops_remote_path.py suite remains green here (91 passed), confirming this is the platform-specific expectation rather than a runtime regression.

I am carrying this as the one nonblocking follow-up already identified in the Windows re-review; I am not duplicating it as another inline finding. The production correction is sound, and updating the fixture is straightforward.

…host reads as a drive

On a Windows host one letter before the colon is a drive, so `a:b.txt`
is this computer's there and the test that expects a refusal failed
(reviewed 2026-10-12); a two-letter name reads as a machine's everywhere.

Co-authored-by: Claude (claude-fable-5-1) <noreply@anthropic.com>

@gloryfromca gloryfromca left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No blockers; this can merge as far as I am concerned.

Reviewed the complete delta from the previously assessed head. Replacing the ambiguous one-letter fixture with ab:b.txt fixes the Windows expectation without weakening coverage: a:b.txt remains exercised as a Windows drive-relative local path, while the two-letter name remains an unknown remote host on both platform branches. I found no new issue.

Verification: tests/test_ops_remote_path.py passed all 91 tests; direct probes exercised both _WINDOWS branches and preserved the intended distinction; focused Ruff and git diff --check passed.

Coverage for this revision included the changed test, the parser behavior it asserts, callers and history, backward compatibility, test-strength preservation, and the already-reviewed AGENTS.md/CLAUDE.md, CONTEXT terminology, and architecture constraints. This delta changes no production code, dependency, domain term, interface, or architectural edge.

@Handsome-wzw

Copy link
Copy Markdown
Contributor Author

The fixture is updated in 7b3757c, now on this branch: the colon-in-a-name case names a two-letter host (ab:b.txt), which reads as a machine's on every host, and the drive-relative runtime correction stays as it is.

@LivXue

LivXue commented Oct 12, 2026

Copy link
Copy Markdown
Member

Windows compatibility re-review: the drive-relative fixture is corrected; one intermittent test failure remains.

Scope: the complete 35-file PR at 7b3757c128b9a907e190e458ac5362242e34ccc4, API base 82c76a82a9a7d312ef6cd75e560404bccd81d292, merge base 829eb28e1db877dffdd3df4f835d707e1f049d3a. Rechecked the full scope, existing discussions and revision delta; all 4,375 source blobs in the isolated snapshot matched the head tree. Since the preceding reviewed head, only the unknown-machine fixture changed.

P2 - Do not require reattachment to be the last heartbeat request. Reproduced intermittently on native Windows. tests/test_node_client.py:901 checks again.asked[-1] after waiting for reconnection. This test runs heartbeats every 10 ms, while _until polls every 20 ms: a normal node/ping can follow the successful agent/attach before the assertion. Both the combined run and the standalone file run failed with node/ping != agent/attach; the selected test alone passed. The logs show successful reattachment in both failures. Assert that the expected attach occurred, or synchronize on that request, without requiring unrelated heartbeat traffic to stop. This is a test race, not evidence that production reconnection failed. The test is new relative to the PR base, but unchanged by this latest fixture correction; the same source/test blobs are carried by #921 and #922.

Native Windows, Python 3.13.12; fresh run-owned environment from uv sync --frozen --extra dev --dev, UV_NO_SYNC=1, PYTHONUTF8=1, isolated user/Raven/temp directories. Imports were verified against this snapshot. Exact test commands, from its root:

$R = 'C:\Users\livxu\AppData\Local\Temp\raven-windows-review\abf44f96254d43bc81b1be33079fb26b'
uv run pytest tests/test_ops_remote_path.py tests/test_acp_client_launch.py tests/test_acp_client_teardown.py tests/test_node_bundle.py tests/test_node_client.py tests/test_node_server.py tests/test_node_relay.py -n 0 -x --tb=short --basetemp "$R\checks\920-host\pytest" --junitxml "$R\checks\920-host\junit.xml"
# 116 passed, 32 skipped, 1 failed (heartbeat assertion).
uv run pytest tests/test_node_client.py::test_a_heartbeat_unanswered_loses_the_link_tells_its_agents_and_reconnects -n 0 -x --tb=short --basetemp "$R\checks\920-heartbeat\pytest" --junitxml "$R\checks\920-heartbeat\junit.xml"
# 1 passed.
uv run pytest tests/test_node_client.py -n 0 -x --tb=short --basetemp "$R\checks\920-node-client\pytest" --junitxml "$R\checks\920-node-client\junit.xml"
# 14 passed, 29 skipped, 1 failed (same assertion).

All 91 remote-path tests passed, including the corrected ab:b.txt fixture. The skips cover POSIX-only daemon/SSH fixtures and unavailable symlink privilege. Both failing runs stopped at the first failure; later node-client tests, live SSH, remote installation/daemon operation, and the full suite were not exercised.

@gloryfromca

Copy link
Copy Markdown
Member

No blockers; suggestions only, and they are marked inline.

The new Windows evidence supersedes my clean stance. I confirmed the race at 7b3757c128b9: the complete tests/test_node_client.py file passed here (55 tests), but a deterministic probe that waited one heartbeat after successful reconnection recorded the valid sequence agent/attach, node/ping, node/ping. Therefore again.asked[-1] == agent/attach depends on scheduler timing even though reattachment succeeded.

I am carrying the intermittent assertion as the one nonblocking follow-up already identified in the Windows re-review, without duplicating it as my own inline finding. Production reconnection behaved correctly in the probe, and the test can assert that the expected attachment occurred without requiring heartbeat traffic to stop.

…equests, not as its last

The test of a link that comes back read the new link's last request as
the attach, and its heartbeat, every 10 ms there, can follow the attach
before the check: it now looks for the attach among the requests
(reproduced on Windows, 2026-10-12).

Co-authored-by: Claude (claude-fable-5-1) <noreply@anthropic.com>
@Handsome-wzw

Copy link
Copy Markdown
Contributor Author

Fixed in 418923f: the reconnect test looks for the attach among the new link's requests instead of reading its last request, so a heartbeat that follows the attach no longer fails it.

@gloryfromca gloryfromca left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No blockers; this can merge as far as I am concerned.

Reviewed the complete delta from the previously assessed head. The reconnect test now searches the replacement client's requests for the exact agent/attach method and payload instead of requiring it to remain last after heartbeat scheduling resumes. That fixes the reproduced race without weakening the behavior under test. I found no new issue.

Verification: all 55 tests/test_node_client.py tests passed with xdist disabled, focused Ruff passed, and git diff --check passed.

Coverage for this revision included the changed assertion, the heartbeat/reconnect ordering and callers it exercises, commit history, backward compatibility, test-strength preservation, and the already-reviewed AGENTS.md/CLAUDE.md, CONTEXT terminology, and architecture constraints. This test-only delta changes no production behavior, dependency, interface, domain term, or architectural edge.

A review date in a source comment is the task's context, not the
code's: the four comments that carried one keep their reason and drop
the day.

Co-authored-by: Claude (claude-fable-5-1) <noreply@anthropic.com>

@gloryfromca gloryfromca left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No blockers; this can merge as far as I am concerned.

Reviewed the complete delta from the previously assessed head. It removes transient review-date wording while retaining the durable explanations for Windows argv handling and heartbeat interleaving. No executable behavior changed, and I found no new issue.

Verification: the directly associated ACP launch/remote and node-client suites passed with 145 tests and 4 unavailable-shell skips; focused Ruff, the source-language gate, and git diff --check passed.

Coverage for this revision included all changed lines, their documented callers and history, backward compatibility, test-strength preservation, and the already-reviewed AGENTS.md/CLAUDE.md, CONTEXT terminology, and architecture constraints. This comment-only delta changes no dependency, domain term, public interface, or architectural edge.

@LivXue

LivXue commented Oct 12, 2026

Copy link
Copy Markdown
Member

Windows compatibility re-review: the reported heartbeat-test race is fixed. No additional Windows compatibility issue was found in the inspected scope.

Reviewed the complete 35-file PR at 9fa6129cdcf77fa5448ee663e154c9bd6c555dac, against API base 82c76a82a9a7d312ef6cd75e560404bccd81d292. The isolated snapshot matched all 4,375 head-tree blobs. Since the preceding reviewed head, the functional change is the test's exact agent/attach membership assertion; the other edits remove dated wording from comments/docstrings. Runtime logic is unchanged. The assertion still checks the replacement connection, task and replay offset, while allowing a later heartbeat.

Native Windows / Python 3.13.12; fresh run-owned environment prepared with uv sync --frozen --extra dev --dev, UV_NO_SYNC=1, PYTHONUTF8=1, isolated user/Raven/temp directories, and imports checked against this snapshot. Exact command from the snapshot root:

$R = 'C:\Users\livxu\AppData\Local\Temp\raven-windows-review\0605c160aa864b11bad6b7d18f24455f'
uv run pytest tests/test_ops_remote_path.py tests/test_acp_client_launch.py tests/test_acp_client_teardown.py tests/test_node_bundle.py tests/test_node_client.py tests/test_node_server.py tests/test_node_relay.py -n 0 -x --tb=short --basetemp "$R\checks\920-host\pytest" --junitxml "$R\checks\920-host\junit.xml"
# 127 passed, 33 skipped.

The complete selected node-client file passed, including the previously failing reconnect test. Skips cover POSIX-only SSH/daemon fixtures and unavailable symlink privilege. No live SSH, remote installation/daemon operation, or full repository suite was run.

@LivXue

LivXue commented Oct 13, 2026

Copy link
Copy Markdown
Member

No blockers in the reviewed scope.

Scope: this re-review covers the six commits since 71f47d135497 and re-runs, at 9fa6129cdcf7, the checks behind my first-round notes. As in that round, none of this PR's 35 files is under raven/agent/**, the part of the tree I review, so nothing is filed inline; the node still runs raven/agent/tools on the machine, and the notes below are about the files that carry it there. The first round's Blocking note is fixed, so for this head the verdict above replaces that summary's.

First-round notes, at this head:

  1. Fixed - the socket fallback. Reproduced with two accounts on one Linux box (the relay ran as uid 0 and the other account was uid 65534; the new checks compare uids). At 71f47d135497 the other account bound the fallback path first and received the agent/launch line the relay forwarded, its env included. At this head the other account created raven-node-<uid>/ first, as a directory and then as a link, and listened inside it: the relay refused with "is not a directory only this user may open" and forwarded nothing, and --serve printed the same sentence and exited 1.
  2. Fixed - owner-only records. Under umask 022 a real daemon made run/, tasks/ and the task's directory 0700, and frames.jsonl, stderr.log and task.json 0600.
  3. Partly fixed - whole-file work and the event loop. The digest is off the loop now; the page read is not (note 8).
  4. Fixed - the import budget. The new test fails on a third-party import added at the top of raven/agent/tools/tree_walk.py, and on one added inside EditFileTool.execute. Under the same import hook, probe/machine, session/dir, probe/agent, agent/launch and task/list also ran.
  5. Fixed - a call in flight counts as use. With two stand-in links and an idle limit of 5 s, the 2 s call on link A returned its answer and A's link stayed open.
  6. Fixed - the login-shell test checks a directory under tmp_path.
  7. Answered - RemotePath.display is meant to reach the model. The docstrings in raven/acp_client/remote.py now say so.

New notes, all nonblocking:

  1. Nonblocking - keep the page read off the loop too, not only the digest. On one 1 GiB file, with runs of both heads alternated in one session: during fs/stat with hash, the longest unanswered ping was 0.08 s at this head (4.2-4.7 s at 71f47d135497), but during fs/read of the first 10 lines it was 7.9-9.4 s (17.0-18.6 s before). _apart (raven/node/server.py:1189) does move the call into a worker thread, but ReadFileTool.execute decodes and splits the whole file (raven/agent/tools/filesystem.py:264), and those two C calls hold the GIL until they finish, so the loop's thread cannot run in the meantime. Measured on the same file, one step at a time in a worker thread while the main thread wakes every 5 ms: read_bytes took 3.30 s with a longest main-thread gap of 0.09 s, bytes.decode 2.70 s with a 2.71 s gap, str.splitlines 4.80 s with a 4.80 s gap, and the sha256 0.88 s with a 0.10 s gap. The new test stands time.sleep in for _sha256, and time.sleep releases the GIL, so the test passes either way. A page read that does not decode the whole file, or a separate process for the whole-file calls, would keep the heartbeat answered. NodePool.call reading its own timeout as a lost link is left as it is, as the reply says.

  2. Nonblocking - keep a read from landing inside a write of the same file. Until this head the file handlers ran on the daemon's one loop without yielding to it, so a read and a write of one file never overlapped. Now each runs in a worker thread and only writers take _WRITE_LOCK (raven/node/server.py:1118-1119); _read reads the page and then reads the whole file again for sha256 (:241-254), and WriteFileTool writes in place (raven/agent/tools/filesystem.py:442). Reproduced by calling the daemon's handle on one loop, as a connection does: one fs/write of a 400,000-line file with fs/read calls of that file while the write ran, 12 trials per run, three runs. Per run, 47-59 reads landed during the write; 2-4 of them returned the old page with the new file's digest, and 1-4 returned a digest of neither version. At 71f47d135497 no read landed during a write. Nothing in this PR uses that digest yet; in feat(*): read, write and deliver files on a registered machine by address #922, raven/agent/tools/remote_files.py sends it as expect with the next write_file, so after a read that returned the old page with the new digest, that write replaces content the model never read. A lock that readers take as well (shared for reads, exclusive for writes), or a digest taken from the same bytes as the page, closes it.

  3. Nonblocking - give uv the package index the machine is configured with. The plan says the packages come "from the machine's package index" (raven/node/install.py:100-101), and at 71f47d135497 pip read that index from pip's own configuration; uv does not read pip's configuration. Simulated here with pypi.org unreachable and only a mirror, named in ~/.config/pip/pip.conf, reachable: the first round's install command (pip in the node's virtualenv) installed loguru==0.7.3 from the mirror; this head's uv pip install went to pypi.org and failed with "Failed to fetch" (exit 2); with UV_DEFAULT_INDEX set to the mirror, uv installed it. Passing uv the index pip would use (--default-index), or naming the index in the plan so the person can set it, keeps such a machine installable.

  4. Nonblocking - keep install-node inside the node directory, as its plan says. The plan a person approves ends "Nothing else on the machine changes" (raven/node/install.py:102; the module docstring says so at :7, and at :14-15 that uv's installer is "told to touch nothing else"), but two steps change things outside the node. On a machine without uv, the uv step (:220) runs uv's installer with UV_INSTALL_DIR, UV_NO_MODIFY_PATH and INSTALLER_NO_MODIFY_PATH, and the installer still writes an install receipt to ~/.config/uv/uv-receipt.json (or under $XDG_CONFIG_HOME): run with an empty HOME, the step left that receipt as the one file outside the node directory. The installer writes the receipt with >, so an existing receipt, from a uv the user installed with the same installer, is replaced; that part is code-reading. With UV_UNMANAGED_INSTALL="$dir/.uv" the installer puts the same binary there and writes no receipt and no PATH change; run the same way, it left nothing outside. The step also installs whichever uv is current (0.13.0 today); uv's per-version installer would pin it, as this Raven pins the packages it installs. Separately, chmod 700 "$dir" (:220, :225) narrows a --dir that already existed, a directory where prune_script expects the owner to keep other things (:256-258): the virtualenv step, run on an existing 0755 directory holding a file of the owner's, left it 0700. Making the directory owner-only only when the step creates it keeps note 2's fix, since run/, tasks/ and log/ are 0700 on their own.

Verification:

  • 18 test files at this head, run with a copy of the dev environment whose editable install points at this head, so the relay's and the daemon's -I children import it too: the first round's 17 plus tests/test_acp_client_launch.py: 839 passed, 6 skipped (no zsh, tcsh or csh here), 0 failed.
  • lint-imports at this head: 10 kept, 0 broken.
  • The probes named in each note, run from this head and from 71f47d135497 on Linux with Python 3.12. No ssh to a real machine.

Checked and deliberately not reported:

…p's index, stay in node_dir

Four notes of the review, each measured on this branch:

- A page read decoded and split the whole file, two C calls that hold
  the interpreter for as long as the file takes (7.5 s on 1 GiB), so a
  node's heartbeat went unanswered and a host's turn stood still.
  `read_file` now reads the file a chunk at a time, counts the line
  breaks `splitlines` knows as the bytes come, holds back what could
  begin a break at a chunk's edge, and decodes only the page's own
  lines; the file is never whole in memory. Same lines, same footer, and
  a byte that is not UTF-8 refuses the page it is on as it refused the
  file.
- In a worker thread a page read and the digest taken beside it could
  land inside a write of the same file and leave with the old page and
  the new digest, which a later write carrying that digest as `expect`
  would take as leave. The whole-file calls now go through one gate:
  readers together, a writer alone, a waiting writer before new readers.
- uv reads no pip configuration, so a machine that reaches only the
  mirror pip is configured with could not be installed. The probe reads
  that index the way pip does (`PIP_INDEX_URL`, then `index-url` in
  pip's files, user over system, install over global), the plan names
  it, and `uv pip install` gets it as `--default-index`.
- The uv fetched for a machine without one wrote a receipt under the
  home directory and was whichever version was current; it is now one
  pinned version, installed unmanaged, which writes no receipt and
  changes no PATH. And `node_dir` is made owner-only only when the
  install makes it: one that was there is the owner's.

Co-authored-by: Claude (claude-fable-5-1) <noreply@anthropic.com>
@Handsome-wzw

Copy link
Copy Markdown
Contributor Author

The four nonblocking notes are fixed in 01b0e5ad2 (carried to #921, #922 and #934 the same way as the earlier commits):

  1. A page read no longer decodes or splits the whole file. ReadFileTool reads the file a chunk at a time (4 MiB), counts the line breaks splitlines knows as the bytes come (holding back what could begin a break at a chunk's edge), and decodes only the page's own lines; the file is never whole in memory, and no one step holds the interpreter for longer than one chunk's. Same lines, same footer; a byte that is not UTF-8 refuses the page it is on, as it refused the file. Pinned by a chunk-edge test over every break splitlines knows (test_filesystem_tools.py), and by a heartbeat test on a 120 MiB file through handle (test_node_server.py: the longest wait of a ping during the read stays under 0.25 s). On a registered machine, a page read of a 1 GiB file beside 50 ms pings is in the verification below.

  2. The whole-file calls go through one gate (_FileGate): readers together, a writer alone, a waiting writer before new readers, so what a read answers with is one state of the file. Your reproduction, through handle on one loop with a slowed digest, now returns the old page with the old digest and the write lands after (test_a_read_never_lands_inside_a_write_of_the_same_file); the writer-first rule has its own test.

  3. The probe reads the index pip is configured with on the machine the way pip reads it -- PIP_INDEX_URL, else index-url in pip's files, the user's over the system's and [install] over [global] -- by whichever Python it found, since uv reads none of that itself; the plan names it ("from (the index pip is configured with there)", else PyPI), and uv pip install gets it as --default-index, the search package first on it and then with the public index beside it. Pinned under a real shell with a pip.conf and with the variable set (test_the_probe_reads_the_index_pip_is_configured_with_as_pip_reads_it).

  4. A machine without uv now gets one pinned version (UV_VERSION, served under its number at astral.sh) as an unmanaged install (UV_UNMANAGED_INSTALL="$dir/.uv"), which writes no receipt under the home directory and changes no PATH; and node_dir is made owner-only only when the install makes it, so one that was there keeps its mode and what the owner keeps in it (test_a_node_dir_that_was_there_keeps_its_mode_and_one_the_install_makes_is_owner_only). The docstring and the plan's "Nothing else on the machine changes" hold again.

Verification: tests/test_filesystem_tools.py tests/test_node_install.py tests/test_node_server.py tests/test_node_client.py tests/test_read_file_image.py -- 337 passed, 100% diff coverage of the commit's changed lines; lint-imports 10 kept, ty and ruff clean, the range gates clean. On this computer a 1 GiB file of 22 million lines pages in 2.4 s at 9 ms a chunk. On a registered Linux machine with the node reinstalled: a page read of a 1 GiB file there beside pings every 50 ms kept the longest wait of a ping at 0.34 s (7.9-9.4 s in your measurement at the previous head); the reads sent during a write all answered with a page and a digest of one version; the probe found the mirror that machine's pip is configured with and the plan named it; an install into a fresh node directory, forced down the fetched path, put uv 0.13.0 under it as an unmanaged install in 24 s, left the receipt under the home directory untouched and the directory at 700.

@Handsome-wzw
Handsome-wzw requested a review from LivXue October 13, 2026 08:12

@gloryfromca gloryfromca left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Blocking: the install confirmation must not expose credentials embedded in the configured package-index URL.

Reviewed the complete delta from 9fa6129cdcf7 to b6351d970434, including the repository rules and canonical Machine/Raven-node terms, all six changed files, the affected file-tool and installer callers, relevant history, compatibility, test assertions, and the daemon's read/write architecture. I found one blocker, filed inline. It meets the post-fourth-round bar: this commit introduces the disclosure; authenticated private indexes are an ordinary configuration for the package-index support added here; and an operator who invokes install-node has no escape because the plan is printed before either confirmation or installation, including with --yes. I found no weakened tests.

Verification: uv run pytest tests/test_filesystem_tools.py tests/test_node_install.py tests/test_node_server.py -q passed all 175 tests; 1,000 randomized valid-Unicode/chunk-boundary cases matched str.splitlines(); Ruff, the source-language gate, and git diff --check passed. I also verified the pinned uv 0.13.0 installer URL resolves and its unmanaged mode disables PATH changes and receipts.

Comment thread raven/node/install.py Outdated
…in pip may keep inside it

pip takes an index with a sign-in in the URL (`https://user:password@host/simple`),
and the plan that names the index is printed before the confirmation, and under
`--yes`, so the password reached the terminal and any transcript of it. The plan
now shows the host and the path only, saying so when a sign-in was left out; uv
still gets the full URL, and a failed step's sentence, which quotes uv's own last
line, is shown the same way.

Co-authored-by: Claude (claude-fable-5-1) <noreply@anthropic.com>

@gloryfromca gloryfromca left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No blockers; this can merge as far as I am concerned.

Reviewed the revision delta from b6351d970434 and the credential flow through the install command, plan display, uv execution, and bounded failure text. The fix removes package-index userinfo from every operator-visible sentence while preserving the full shell-quoted URL for uv. The added tests cover plan text, execution, failure output, and IPv6, and they strengthen rather than weaken the existing assertions.

I also rechecked the repository rules, relevant callers and history, backward compatibility, and the existing SSH/node architecture constraints. Verification: uv run pytest tests/test_node_install.py -q (58 passed), uv run ruff check raven/node/install.py tests/test_node_install.py, the source-language gate, and git diff --check github/main...HEAD all pass. No security candidate survived the focused scan.

@LivXue

LivXue commented Oct 13, 2026

Copy link
Copy Markdown
Member

Windows compatibility re-review: no new Windows compatibility issue found in the inspected scope.

Scope: the complete 37-file PR at 131427c4679b35b4d8c75094bab13545eb2b900c; API base 82c76a82a9a7d312ef6cd75e560404bccd81d292, merge base 829eb28e1db877dffdd3df4f835d707e1f049d3a. Rechecked the full scope, the six-file revision delta and its callers; unchanged previously inspected files were compared by blob identity. The isolated snapshot matched all 4,375 head-tree blobs, and the complete PR diff passed git diff --check.

Native checks covered drive-relative/UNC/extended-path classification, host SSH argv handling, ACP teardown and node reconnection. The changed paging tests preserve CRLF and Unicode line boundaries across chunk sizes and report invalid UTF-8 on the requested page. Pure installer-plan tests passed for the inherited remote package index, credential redaction, pinned unmanaged uv and command quoting; no installer was executed. This corroborates the existing resolved credential-display discussion, without filing it again.

Native Windows / Python 3.13.12; fresh per-head environment prepared with uv sync --frozen --extra dev --dev, isolated user/Raven/temp directories, PYTHONUTF8=1, UV_NO_SYNC=1, and imports verified against this snapshot. Exact commands from the snapshot root:

$R = 'C:/Users/livxu/AppData/Local/Temp/raven-windows-review/09bd90f70d7a4516ab71a23a267eb9e3'
uv run pytest tests/test_ops_remote_path.py tests/test_acp_client_launch.py tests/test_acp_client_teardown.py tests/test_node_bundle.py tests/test_node_client.py tests/test_node_server.py tests/test_node_relay.py -n 0 -x --tb=short --basetemp "$R/checks/920-host/pytest" --junitxml "$R/checks/920-host/junit.xml"
# 127 passed, 33 skipped; exit 0.
uv run pytest tests/test_filesystem_tools.py::test_a_page_is_the_lines_splitlines_gives_whatever_the_chunk_edge tests/test_filesystem_tools.py::test_read_file_pages_a_file_without_holding_all_of_it tests/test_node_install.py::test_the_index_pip_is_configured_with_is_handed_to_uv_and_named_in_the_plan tests/test_node_install.py::test_a_sign_in_inside_the_index_url_reaches_uv_and_never_a_sentence tests/test_node_install.py::test_a_machine_without_uv_fetches_one_version_of_it_under_the_node_dir_and_nowhere_else tests/test_node_install.py::test_the_steps_build_a_virtualenv_with_uv_then_fill_it_with_uv tests/test_node_install.py::test_uv_makes_its_python_inside_the_node_dir -n 0 -x --tb=short --basetemp "$R/checks/920-changed/pytest" --junitxml "$R/checks/920-changed/junit.xml"
# 13 passed; exit 0.

Limits: the skips cover POSIX-only daemon/SSH fixtures and unavailable symlink privilege. The reader/writer gate and remote shell installation changes were code-reviewed only; their POSIX integration tests did not run on Windows. No live SSH, remote installation/daemon operation, prospective merged-tree test or full repository suite was run.

Comment thread raven/agent/tools/filesystem.py Outdated
@LivXue

LivXue commented Oct 13, 2026

Copy link
Copy Markdown
Member

Blocking: 1 finding needs correction before this revision can merge; see the inline note.

Scope: this re-review covers the two commits since 9fa6129cdcf7 (b6351d970, 131427c46) and re-runs, at 131427c4679b, the checks behind the blocking finding below. One of this PR's 37 files is under raven/agent/**, the part of the tree I review: raven/agent/tools/filesystem.py. The remaining 36 files were read as context, and the three further findings below sit in raven/node/**, outside that zone, so no note is filed against them and they are listed here instead.

What I examined: the raven/agent/** diff, which is the paged read this review is about; raven/node/server.py and raven/node/install.py as context, since they call the tools the fence covers; the callers of the changed read; the test files added beside it; and the daemon end to end through its own stdio protocol.

Confirmed findings:

  • _page_of_lines re-scans its whole accumulated buffer once per 4 MiB chunk in the inline note's terms, so a read of a file with no line break is quadratic in its size: a 256 MiB single-line file takes 32.66 s where the previous revision took 0.87 s, and 33.37 s through a real daemon. Filed inline at raven/agent/tools/filesystem.py:73.
  • _FileGate (raven/node/server.py:1122) admits readers together and a writer alone across the whole machine, not per file: with a read of one 256 MiB file in flight, a write of a different small file waited 32.95 s (0.05 s on its own). Outside the review zone, so stated here rather than filed. Nonblocking.
  • The install step hands uv the configured package index as --default-index in the remote command (raven/node/install.py:278), which puts any sign-in inside that URL into the ssh argv on the host and the sh -c argv and uv's argv on the machine; /proc/<pid>/cmdline is readable by every account on the machine, and a note on this file's display already settles that the plan and the failure sentence must not show the sign-in. Reproduced from an unprivileged account (uid 65534) reading a sibling process's argv. Outside the review zone. Nonblocking.
  • _shown_index (raven/node/install.py:358) raises ValueError when the URL's port is not an integer, and install_node catches InstallError only, so a hand-written port in pip's configuration reaches the person as a traceback. Outside the review zone. Nonblocking.

Verification:

  • Both revisions extracted from their own shas (131427c4679b, 9fa6129cdcf7) with git archive, run on one Python 3.12 against the same files, each printing the filesystem.py path it imported. Fixtures: 64/128/256 MiB single-line files and a 256 MiB file of 63-byte lines, all built for this run.
  • Paged read, ReadFileTool.execute(limit=10): old 0.22 / 0.45 / 0.87 s at 64 / 128 / 256 MiB and 0.88 s for the short-line file; new 2.37 / 8.58 / 32.66 s and 1.05 s. Peak RSS 539 -> 803 MiB on the 256 MiB single-line file, 768 -> 41 MiB on the short-line one.
  • End to end: python -m raven.node --stdio from the new tree, ping every 0.2 s, fs/read limit=10 on the 256 MiB single-line file answered in 33.37 s with the slowest ping at 517 ms.
  • The gate: calling the daemon's handle as a connection does, a write of a small file alone took 0.05 s and took 32.95 s while a read of a different 256 MiB file was in flight.
  • _CALL_TIMEOUT_S read at the head: 120.0, raven/node/client.py:71.

The previous round's four nonblocking notes were addressed in b6351d970 and re-checked at that head before this one; they are not re-litigated here. The four findings above stand beside that record rather than replacing it.

This summary supersedes the No blockers in the reviewed scope. summary of this same PR at 9fa6129cdcf7. That verdict was accurate for its head; the paging commit b6351d970 postdates it and moves the read's worst case, which is what the note above is against.

No note is filed against raven/node/**: it is outside the zone this review covers, and the three findings there are recorded as observations, with the mechanism and the reproduction named so they can be acted on where they belong.

…han a chunk costs its length

The paged read held the line in progress whole and split it again with
every chunk, so a file with no line break in it cost time square in its
size: a 256 MiB single-line file took half a minute where the whole-file
read took under a second, and one near 1 GiB would have outlasted the
node's call timeout. Minified bundles, single-line JSON and machine-written
CSV are that shape.

Each chunk is split once now, and the bytes of the line in progress are
kept only while that line may be on the page; off the page it is counted,
not held. The same 256 MiB single-line file reads in a third of a second,
on the page or off it, and the page is still exactly what splitlines gives.

Co-authored-by: Claude (claude-fable-5-1) <noreply@anthropic.com>
@Handsome-wzw

Copy link
Copy Markdown
Contributor Author

The blocking finding is fixed in 6a0ec2c and answered on its thread: the paged read looks at each chunk once, so a line longer than a chunk costs its length and no more, and the 256 MiB single-line file reads in a third of a second here, on the page or off it.

The three observations on raven/node/** are noted. They are outside this change's correction: the gate's span, the index inside the remote argv and the port that is not a number each want a change of their own, and I will take them up apart from this PR so its review can close on what it covers. The four notes of the previous round stand as re-checked.

@Handsome-wzw
Handsome-wzw requested a review from LivXue October 13, 2026 10:05

@gloryfromca gloryfromca left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No blockers; suggestions only, and they are marked inline.

The long-line regression is fixed: each chunk is split once, requested line bytes are retained only while needed, and both on-page and off-page probes scale linearly. The new property tests preserve splitlines behavior and directly prevent the repeated-scan failure rather than weakening coverage.

I reviewed the revision delta, helper callers, relevant history and backward compatibility, repository rules, and the existing node/file-tool architecture constraints. Verification: the filesystem, node-client, and node-server suites pass all 178 tests; Ruff and git diff --check github/main...HEAD also pass.

Named maintainer-deferred follow-ups from the record: make the machine file gate per-file rather than global, avoid exposing authenticated index URLs through process argv, and turn a nonnumeric configured index port into a handled install error rather than a traceback. These remain nonblocking for this change.

@LivXue

LivXue commented Oct 13, 2026

Copy link
Copy Markdown
Member

No blockers in the reviewed scope.

Scope: this supersedes my Blocking: summary of 131427c4679b. That finding was against _page_of_lines in raven/agent/tools/filesystem.py, the one file of this PR's 38 under raven/agent/**, the part of the tree I review. It is fixed in 6a0ec2c12cf4; I reproduced the fix there and the numbers are on the thread. The other files were read as context, and the three nonblocking observations below still sit in raven/node/**, outside the zone that may carry a note.

Confirmed findings:

  • None at this head. The paged read is linear in the file again, on the page and off it, and the page is still what splitlines gives.

Still open, not filed against this change:

  • _FileGate (raven/node/server.py:1122) admits readers together and a writer alone across the whole machine rather than per file: with a read of one 256 MiB file in flight, a write of a different small file waited 32.95 s (0.05 s alone). Outside the review zone, nonblocking.
  • The install step passes the configured package index as --default-index inside the remote command (raven/node/install.py:278), so a sign-in in that URL is in the ssh argv on the host and the sh -c argv on the machine, where /proc/<pid>/cmdline is readable by every account. Reproduced from uid 65534. Outside the review zone, nonblocking.
  • _shown_index (raven/node/install.py:358) raises ValueError on a port that is not an integer, and install_node catches InstallError only, so a hand-written port in pip's configuration surfaces as a traceback. Outside the review zone, nonblocking.

The author has said these three want a change of their own and will be taken up apart from this PR, which is a reasonable split; they are recorded here so they are not lost.

Verification:

  • Both revisions extracted from their own shas with git archive, run on one Python 3.12 against the same files, each printing the filesystem.py path it imported. Fixtures built for this run: 64 / 128 / 256 MiB single-line files; a 256 MiB file of 63-byte lines; a file with a 32 MiB first line followed by 200,000 short lines.
  • Paged read, ReadFileTool.execute(limit=10) on a single-line file: 0.77 / 1.40 / 2.41 s here against 0.22 / 0.45 / 0.87 s at 131427c4679b and 2.37 / 8.58 / 32.66 s before that fix.
  • Off-page long line: the 32 MiB-first-line file read at offset=2 peaks at 41 MiB against 38 MiB for the same file without the long line, and 126 vs 62 MiB at 131427c4679b.
  • End to end: python -m raven.node --stdio from this revision, ping every 0.2 s, fs/read limit=10 on the 256 MiB single-line file answered in 1.72 s with the slowest ping at 151 ms.
  • PYTHONPATH set to the extracted revision: tests/test_filesystem_tools.py 30 passed unmodified, and 12 failed / 18 passed with the previous loop planted back verbatim, which is what pins the property.

The two conclusions this replaces: my Blocking: summary at 131427c4679b, and the No blockers in the reviewed scope. summary before it, which was accurate for its own head.

@LivXue

LivXue commented Oct 13, 2026

Copy link
Copy Markdown
Member

Windows compatibility re-review: no additional Windows compatibility issue found in the inspected scope. The long-line paging regression's new native tests pass.

Scope: the complete 37-file PR at 6a0ec2c12cf4ae928de4e4b6464c85e99516937b; API base 82c76a82a9a7d312ef6cd75e560404bccd81d292, merge base 829eb28e1db877dffdd3df4f835d707e1f049d3a. Rechecked the two-file delta and pager callers, with blob identity for previously inspected unchanged source. All 4,375 snapshot blobs matched the head tree; the full diff passed git diff --check.

Native Windows / Python 3.13.12, fresh per-head environment from uv sync --frozen --extra dev --dev, UV_NO_SYNC=1, PYTHONUTF8=1, isolated user/Raven/temp directories, and imports verified against the exact-head snapshot. Commands from that snapshot:

$R = 'C:/Users/livxu/AppData/Local/Temp/raven-windows-review/d1d0b660deeb48e3a2e4d079a3316cbc'
uv run pytest tests/test_filesystem_tools.py::test_a_page_is_the_lines_splitlines_gives_whatever_the_chunk_edge tests/test_filesystem_tools.py::test_a_line_longer_than_a_chunk_is_whole_on_its_page_and_only_counted_off_it tests/test_filesystem_tools.py::test_a_long_line_is_looked_at_once_whatever_its_length tests/test_filesystem_tools.py::test_read_file_pages_a_file_without_holding_all_of_it -n 0 -x --tb=short --basetemp "$R/checks/920-paging/pytest" --junitxml "$R/checks/920-paging/junit.xml"
# 12 passed; 1 advisory idle-ceiling warning.
uv run pytest tests/test_ops_remote_path.py tests/test_acp_client_launch.py tests/test_acp_client_teardown.py tests/test_node_bundle.py tests/test_node_client.py tests/test_node_server.py tests/test_node_relay.py -n 0 -x --tb=short --basetemp "$R/checks/920-host/pytest" --junitxml "$R/checks/920-host/junit.xml"
# 127 passed, 33 skipped.

Paging coverage includes CRLF/Unicode separators, chunk boundaries, long lines on and off the requested page, and the property that bytes are split once. Host checks cover Windows path classification, SSH argv, ACP teardown and node reconnect behavior. No native performance benchmark or remote daemon was run; skipped cases require POSIX fixtures or symlink privilege.

The already recorded node/installer follow-ups remain separate. No live SSH, installation, UI execution or full repository suite.

@LivXue LivXue left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Blocking: this full-PR review found five correctness issues (2 P1, 3 P2) in node recovery and daemon state management. The five inline comments describe the failing states and repair directions.

This review includes raven/node and supersedes any merge-readiness impression from my earlier agent-only and Windows compatibility notes. The earlier long-line paging finding remains resolved; its 12 regression cases pass.

Reviewed head: 6a0ec2c12cf4ae928de4e4b6464c85e99516937b. Verification used native Windows, Python 3.13.12, and a freshly synced isolated environment. All 4,375 blobs in the source archive were checked against the GitHub tree, and the source was checked again after the probes.

  • Relevant repository tests: 338 passed, 10 failed, 61 skipped. All 10 failing test IDs also failed in the same-machine merge-base control (829eb28e1db877dffdd3df4f835d707e1f049d3a); the comparison used JUnit test IDs, not just counts.
  • Controlled recovery probes: 4 failed, 1 passed. Daemon state probes: 2 failed, 2 passed. These are six violating states covering the five findings, with three positive controls.
  • uv run --frozen --no-sync lint-imports: 10 contracts kept.

Repository test command:

uv run --frozen --no-sync pytest tests/test_node_bundle.py tests/test_node_client.py tests/test_node_install.py tests/test_node_server.py tests/test_node_relay.py tests/test_acp_client_launch.py tests/test_acp_client_teardown.py tests/test_acp_remote.py tests/test_ops_remote_path.py tests/test_cli_ops_connection_commands.py tests/test_filesystem_tools.py tests/test_ops_connection_add.py tests/test_ops_transport.py -n 0 -q --tb=short

Verification limits: no real SSH, remote installation, or POSIX daemon end-to-end run. The socket finding uses the production path function with a fixed POSIX UID substitute; the launch-limit probe uses production request/launch logic with mocked subprocess creation and POSIX directory setup. An initial selection also included unchanged tests/test_read_file_image.py, whose unguarded os.geteuid call prevents Windows collection; that file was excluded from the completed selection, and the base has the same call.

The existing file-gate/index observations are not filed again. A separate error-redaction hypothesis was excluded because the real local uv failure did not establish the proposed credential leak.

Comment thread raven/node/client.py
Comment thread raven/node/client.py Outdated
Comment thread raven/node/client.py
Comment thread raven/node/server.py Outdated
Comment thread raven/node/server.py Outdated
…ry, a launch reserves its place

A full review of the link's recovery and the daemon's state found five
states they got wrong. Each is reproduced by a test of its own before the
fix, and three of them on a real machine as well: a dead link replaced by a
plain call left its agent parked for good; a cancel asked while the link
was dead never arrived and the task ran on; eleven launches at once
started nine against a limit of eight.

A link the heartbeat had not yet seen die was replaced by the next call
without its agents: the pool now attaches whatever waits for the machine
on every link it makes, and the call hears why when that link goes too. A
replay the new link could not carry escaped the reconnect with the frames
already dropped and the endpoints after it in no list: a frame leaves the
held list only once the link took it, the endpoint being replayed and
every one not yet reached are parked again with what they hold, and the
link is tried again. A cancel is wanted from the moment it is asked until
the node takes it or says there is nothing to take; a link that dies under
the request, or had died unnoticed, keeps it for the next link.

The daemon's long-path socket is named for the install as well as the
directory, as the socket under run/ is, so two installs on one long
directory listen apart. A launch takes its place against the agent limit
before its first await, so launches served together cannot pass the check
as one.

Co-authored-by: Claude (claude-fable-5-1) <noreply@anthropic.com>
@Handsome-wzw

Copy link
Copy Markdown
Contributor Author

All five are fixed in 093966f and answered on their threads, each with the test that reproduces your probe. The three a real machine can show were run there before and after the fix, from the same script (the relay killed and a plain call made before the heartbeat could notice; a cancel asked in that gap; eleven launches at once): the run before reproduced each as you described it, the run after passes. The node test groups pass with the seven new tests, and the fix is carried to #921, #922 and #934.

@Handsome-wzw
Handsome-wzw requested a review from LivXue October 13, 2026 17:54

@gloryfromca gloryfromca left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No blockers; suggestions only, and they are marked inline.

Reviewed the delta from 6a0ec2c12cf4: ordinary link replacement now reattaches parked endpoints; replay removes frames only after delivery and reparks the remaining endpoints on failure; cancellation intent survives failed delivery; long socket fallbacks include the install identity; and launches reserve capacity before their first await. The new tests exercise each reported race or collision and do not weaken prior assertions.

I also checked the affected callers, reconnect and daemon lifecycle, relevant history and backward compatibility, repository rules, and the existing node architecture constraints. uv run pytest tests/test_node_client.py tests/test_node_server.py -q passes all 155 tests; Ruff and git diff --check github/main...HEAD pass. The previously named file-gate scope, authenticated-index argv, and invalid-index-port follow-ups remain nonblocking.

@gloryfromca gloryfromca left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No blockers; suggestions only, and they are marked inline.

Reconfirmed the checked-out and live PR head are both 093966f8fa6d; the already-reviewed revision delta and its verification leave the three previously named maintainer-deferred follow-ups as nonblocking. This revision can merge as it stands.

@LivXue

LivXue commented Oct 13, 2026

Copy link
Copy Markdown
Member

Windows compatibility re-review: no additional Windows compatibility issue found in the inspected scope.

Scope: all 37 changed files at 093966f8fa6d107cbff0ad7ffd32c26067188531, base 82c76a82a9a7d312ef6cd75e560404bccd81d292, merge base 829eb28e1db877dffdd3df4f835d707e1f049d3a. Compared the complete PR scope with the previously reviewed tree 6a0ec2c12cf4ae928de4e4b6464c85e99516937b: 33 files are unchanged, and the four-file node client/server/test delta was re-inspected with its callers. Existing discussions and threads were read. git diff --check passed.

Native Windows / Python 3.13.12: 144 passed, 33 skipped. The selected tests cover Windows path classification, ACP argv/teardown, bundling, file paging and the new ordinary-call reconnect, failed replay and cancellation-retention cases. Those recovery cases use controlled client substitutes. Socket fallback namespacing and launch reservations were checked in the POSIX daemon code; its Unix-socket/process-group tests do not run on this host.

Exact verification commands

Fresh isolated uv environment and detached checkout; source imports verified against this head, with separate home/temp/token directories. Setup succeeded: uv sync --frozen --python 3.13 --all-packages --extra dev. Tests used PYTHONUTF8=1 and UV_NO_SYNC=1.

$R = 'C:/Users/livxu/AppData/Local/Temp/raven-windows-review/c281ddf96c3446c98e4f4127722b59c6'
$cases = @(
  'tests/test_ops_remote_path.py',
  'tests/test_acp_client_launch.py',
  'tests/test_acp_client_teardown.py',
  'tests/test_node_bundle.py',
  'tests/test_node_client.py',
  'tests/test_node_server.py',
  'tests/test_node_relay.py',
  'tests/test_filesystem_tools.py::test_a_page_is_the_lines_splitlines_gives_whatever_the_chunk_edge',
  'tests/test_filesystem_tools.py::test_a_line_longer_than_a_chunk_is_whole_on_its_page_and_only_counted_off_it',
  'tests/test_filesystem_tools.py::test_a_long_line_is_looked_at_once_whatever_its_length',
  'tests/test_filesystem_tools.py::test_read_file_pages_a_file_without_holding_all_of_it'
)
uv run pytest @cases -n 0 -x --tb=short --basetemp "$R/checks/920-host/pytest" --junitxml "$R/checks/920-host/junit.xml"

Limits: no real SSH/remote install, POSIX daemon end-to-end execution, full repository suite or Windows ACL audit. Platform skips do not establish equivalent Windows support.

…ackoff for the agents that wait

The previous fix had a link replaced by an ordinary call bring the parked
agents along, but only when the relaunch succeeded. When it failed -- the
machine unreachable at that moment -- the call raised and nothing tried
again: the agents stayed parked with no reconnect task, where the same
failure under the heartbeat would have been retried on the backoff. On a
real machine, the relay killed and the relaunch made to fail once, the
agent was still parked thirty seconds after the machine was reachable
again. A failed relaunch with agents waiting now schedules the same
reconnect the heartbeat loss does; the agent is attached again on the
first retry once the machine answers.

Co-authored-by: Claude (claude-fable-5-1) <noreply@anthropic.com>
@Handsome-wzw

Copy link
Copy Markdown
Contributor Author

One more commit, a6cc40d: the ordinary-call path of the first finding was fixed only for a relaunch that succeeds. A relaunch that fails with agents parked now schedules the reconnect the heartbeat loss does, so the agents come back on the backoff once the machine answers, as the module's contract says. Reproduced on a real machine before the fix (the relay killed, the relaunch made to fail once: parked = 1, no reconnect task, still parked 30 s after the machine was back) and gone after it (a reconnect task scheduled at once, the agent attached again on the first retry after the machine answered, nothing left running there); the unit test fails on the previous head and passes here. Carried to #921, #922 and #934.

@gloryfromca gloryfromca left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No blockers; suggestions only, and they are marked inline.

Reviewed the two-file delta from 093966f8fa6d, the surrounding acquisition/reconnect state transitions, the added regression case, and the existing project constraints. The failed ordinary-relaunch path now schedules the same guarded backoff recovery used after heartbeat loss, while an in-progress reconnect remains unique. uv run pytest tests/test_node_client.py -q passes all 61 tests, Ruff passes on both changed files, and git diff --check github/main...HEAD is clean. The three previously named maintainer-deferred node follow-ups remain nonblocking.

@LivXue

LivXue commented Oct 14, 2026

Copy link
Copy Markdown
Member

Windows compatibility re-review at a6cc40dddabaf71a4408de002c59cc5737a073ec, base 82c76a82a9a7d312ef6cd75e560404bccd81d292, merge base 829eb28e1db877dffdd3df4f835d707e1f049d3a.

Reconciled the complete 37-file PR diff with the previous review at 093966f8fa6d107cbff0ad7ffd32c26067188531: 35 files are unchanged; only raven/node/client.py and its test changed. Inspected the new failed-acquisition retry path, lock release, reconnect deduplication/backoff, shutdown guard and callers. Existing discussions/threads were read, and git diff --check passed.

No additional Windows issue found in this retry change. Native host/paging checks: 145 passed, 33 skipped. The new controlled-client regression confirms that a failed ordinary relaunch schedules a retry and reattaches the parked endpoint when a later launch succeeds. POSIX daemon and stand-in shell cases remain skipped.

Exact verification commands

Native Windows / Python 3.13.12, fresh isolated uv environment and detached exact-head checkout; imports verified, with separate home/temp/token directories. Setup succeeded: uv sync --frozen --python 3.13 --all-packages --extra dev. Tests used PYTHONUTF8=1 and UV_NO_SYNC=1.

$R = 'C:/Users/livxu/AppData/Local/Temp/raven-windows-review/615c117f238046cca9805459d4b37a77'
$cases = @(
  'tests/test_ops_remote_path.py',
  'tests/test_acp_client_launch.py',
  'tests/test_acp_client_teardown.py',
  'tests/test_node_bundle.py',
  'tests/test_node_client.py',
  'tests/test_node_server.py',
  'tests/test_node_relay.py',
  'tests/test_filesystem_tools.py::test_a_page_is_the_lines_splitlines_gives_whatever_the_chunk_edge',
  'tests/test_filesystem_tools.py::test_a_line_longer_than_a_chunk_is_whole_on_its_page_and_only_counted_off_it',
  'tests/test_filesystem_tools.py::test_a_long_line_is_looked_at_once_whatever_its_length',
  'tests/test_filesystem_tools.py::test_read_file_pages_a_file_without_holding_all_of_it'
)
uv run pytest @cases -n 0 -x --tb=short --basetemp "$R/checks/920-host/pytest" --junitxml "$R/checks/920-host/junit.xml"
# 145 passed, 33 skipped

Limits: no real SSH, remote installation, POSIX daemon end-to-end run, full repository suite or Windows ACL audit. Passing host tests does not establish equivalent Windows daemon support.

…node

# Conflicts:
#	raven/acp_client/client.py
@0xKT

0xKT commented Oct 14, 2026

Copy link
Copy Markdown
Member

Merge conflict: @Handsome-wzw the main branch tip has advanced to d106a5c and this PR no longer merges onto it cleanly. Re-running the merge check against the live tip:

git merge-tree --write-tree d106a5c6 a6cc40dd

reports a content conflict in raven/acp_client/client.py (the other files auto-merge). The judge has cleared the current head, so a rebase onto the latest main, resolving raven/acp_client/client.py, is the one step left before this PR can enter the acceptance lane. Posted by the review board after re-running the merge check.

@gloryfromca

Copy link
Copy Markdown
Member

Blocking: the branch must be rebased onto current main and the conflict resolved.

Confirmed against live github/main at d106a5c6: git merge-tree --write-tree HEAD github/main exits 1 with a content conflict in raven/acp_client/client.py. This is not a new code finding; it supersedes my nonblocking stance because the current revision cannot enter the merge lane as it stands.

@gloryfromca gloryfromca left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No blockers; suggestions only, and they are marked inline.

Reviewed the merge revision against live main, including the raven/acp_client/client.py conflict resolution, its process and remote-endpoint callers, and the retained PR diff. The resolution preserves the node endpoint/argv work while integrating main's process-tree teardown and leftover-agent bookkeeping. The live merge check and git diff --check github/main...HEAD pass; the focused ACP lifecycle and node client/server suites pass all 242 tests, and Ruff passes on the integrated files. This settles my merge-conflict blocker. The three previously named maintainer-deferred node follow-ups remain nonblocking.

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants