Repository navigation
refactor(*): one registry reader and one writer, and the coding agent can add a machine - #690
Conversation
… can add a machine Two products run work on the owner's machines, and in the usual chain the coding agent runs first: it builds a case where the solver is and smoke-tests it there, then hands off to the on-call agent. Yesterday's run showed the seam between them was broken twice over. Read side. `raven.ops.connections.store_path()` resolved the registry "beside the config". A sub-agent runs on a rendered config in a state directory of its own, so Raven-Code read an empty directory while the owner's seven machines sat one level up in the home the host had handed it as RAVEN_HOME; `exec(machine=...)` answered "No connection is registered", and the model reached for the raw ssh address instead. The path now resolves to the env var, else the owner's home when a registry is there, else beside the config when THAT exists, else the home -- which is also where a first add lands, so what the coding agent writes is what the on-call agent reads. The on-call launcher's RAVEN_CONNECTIONS pointer keeps working and is no longer the only thing that does. Write side. The tool that takes the owner's answer and writes a machine (`ops_connection_add`, #545) lived in the on-call plugin; the coding agent, which meets the empty registry first, had no way to consume the answer. The probe, the writer and the ssh alias now live in `raven.ops.connection_add`, behind both the tool (`raven/agent/tools/connection_add.py`) and `raven ops connection add`; the plugin's copies are gone. Key isolation for a candidate the tool picked itself moved with it, as `raven.ops.transport.make_ssh_runner(identities_only=)`, and the probe rides the same transport `exec`'s machine channel uses. The tool is opt-in per product (`tools.connectionAdd`): on in raven-code and raven-oncall, off elsewhere, because five product tool faces are pinned and a lane that never runs work on the owner's machines has no use for a tool that writes to their ssh config. The on-call plugin's `connections` module kept a hand-aligned copy of trunk's reader (eleven functions) on the ground that trunk's module was not a contracts paper; the plugin already imports trunk's utilities and hooks, and a third copy for the coding agent was the alternative. It imports the reader now and keeps only what a campaign needs (lookup by id, display name, meta resolution, the model-facing listing). The ask the model relays to the owner is one text (`ASK_OWNER`), shared by the on-call listing reply and the tool. Raven-Code's guide gains the rule for leaving this computer at all: write here; verify where the software is; ask the owner for a machine only when installing the software here would be wrong (GPU, memory) or heavy (a compiled solver, a toolchain, a long download); never type an ssh from an address in the task statement. Tests: the connection-add suite moves to trunk and patches the kernel modules; the plugin's two "aligned by hand" parity tests become one identity pin; five new cases pin the path resolution; the code product's face ledger gains a machine lane. Complements #552 (exec refuses a typed ssh to a machine the registry knows), which only fires once the registry is visible. Co-authored-by: Claude (claude-opus-5-5) <noreply@anthropic.com>
gloryfromca
left a comment
There was a problem hiding this comment.
Blocking: the shared SSH probe no longer enforces its advertised timeout.
I reviewed the full github/main...HEAD diff and relevant surrounding code, callers, and history. I also checked AGENTS.md plus the Runtime/agent terminology and architecture in CONTEXT-MAP.md, CONTEXT.md, and agents/README.md; backward compatibility of registry-path resolution; the moved writer and both product launch paths; and the test changes for weakening. The home-first precedence is deliberate and covered, and the reader/writer consolidation otherwise preserves the existing contracts.
Verification:
uv run pytest tests/test_ops_connection_add.py tests/test_ops_connections.py tests/test_cli_ops_connection_commands.py tests/test_shell_machine_channel.py tests/test_agents_oncall_flow_connections.py tests/test_agents_oncall_flow_watcher.py tests/test_agents_oncall_launcher.py tests/test_agents_code_launcher.py -x- 195 passed.uv run pytest- 19,932 passed, 130 skipped, 74 failed, 283 errors; the failures/errors are outside this diff and are dominated by optional packages/plugins absent from this checkout (for examplewebsockets,raven_everos,raven_design, andraven_ppt).- A direct patched call to
probe(..., timeout=0.25)confirmed that the remote runner invokessubprocess.runwithout a timeout.
Once the SSH execution deadline is restored, I see no other blocker in this revision.
…efusals The shared probe passed its 30 s timeout to `runner_from`, which applied it only to the local runner; the ssh runner called `subprocess.run` with no timeout, and `ConnectTimeout` is over once the session is up. A machine that accepted and then stopped answering held `raven ops connection add` (and the tool's worker thread) forever, where the old CLI gave up at 30 s (review on #690). `make_ssh_runner` takes `cap_seconds`, returns `TIMED_OUT_RC` when it fires (the code the local runner already used), and `runner_from` passes the cap to both runners. The probe reads 124 as "no answer within Ns" on both of its ssh paths. The machine channel now gets its own backstop on ssh too, behind the wrapper that caps the far side. Tests drive a stand-in ssh on PATH for the silent, refusing and missing cases, and cover each refusal and remark the tool can return; diff coverage on the branch measures 99%. Co-authored-by: Claude (claude-opus-5-5) <noreply@anthropic.com>
gloryfromca
left a comment
There was a problem hiding this comment.
No blockers; this can merge as far as I am concerned.
I re-read the full github/main...HEAD diff and the change since ac98cb2. I checked the repository rules and Runtime/agent architecture documents, the shared transport callers and history, registry-path and CLI backward compatibility, and the changed tests for weakening. The SSH deadline now reaches the normal and isolated-key probe paths, returns the transport's existing rc 124 on timeout, and is rendered as a no-write refusal. I found no new issue in the revision.
Verification: uv run pytest tests/test_ops_connection_add.py tests/test_ops_connections.py tests/test_cli_ops_connection_commands.py tests/test_shell_machine_channel.py tests/test_agents_oncall_flow_connections.py tests/test_agents_oncall_flow_watcher.py tests/test_agents_oncall_launcher.py tests/test_agents_code_launcher.py -x - 213 passed. I also replied to and resolved my prior timeout thread.
…egistry_one_copy # Conflicts: # raven/agent/loop/_shared.py # raven/agent/loop/wiring.py
gloryfromca
left a comment
There was a problem hiding this comment.
No blockers; this can merge as far as I am concerned.
This revision merges current main; after refreshing github/main, the PR remains the same 28-file registry change. git range-diff shows both authored commits are unchanged from the revision I previously reviewed. I rechecked the full PR diff, repository rules and Runtime/agent architecture constraints, callers and history, backward compatibility, and the tests for weakening, with particular attention to the newer machine-exec guard now on the base. I found no new issue, and my earlier timeout thread remains resolved.
Verification: git diff --check github/main...HEAD passed; uv run pytest tests/test_ops_connection_add.py tests/test_ops_connections.py tests/test_cli_ops_connection_commands.py tests/test_shell_machine_channel.py tests/test_agents_oncall_flow_connections.py tests/test_agents_oncall_flow_watcher.py tests/test_agents_oncall_launcher.py tests/test_agents_code_launcher.py -x - 283 passed.
|
Blocking: 1 finding(s) need correction before this revision can merge; see the inline notes. Scope: I reviewed only the What I examined: the new tool against its plugin predecessor (the execution path is unchanged; the model-facing description and success reply now also name exec's Confirmed findings:
Outside the reviewed scope, disclosed here because no inline note can be filed there:
Verification, all at 257acb1 against an extracted head tree, with the imported
Checked and not reported: the plugin's face roster serves only tools with |
LivXue
left a comment
There was a problem hiding this comment.
Requesting changes for the blocking finding at raven/agent/tools/connection_add.py:207. The scoped summary and the other notes are in the comments above.
gloryfromca
left a comment
There was a problem hiding this comment.
Blocking: exec's model-facing schema must gain the machine parameter after the first registry write.
The new review evidence changes my prior clean stance on this revision. I reproduced the first-add transition against ToolRegistry: ExecTool.parameters becomes machine-aware after a row is added, but the registry continues serving the frozen schema admitted at startup, so the provider is not offered the parameter that ops_connection_add tells it to use. Direct dispatch accepts the undeclared argument, but that does not make it a dependable model-facing lane.
The separate settings-table note also holds but remains nonblocking: the coverage guard builds with connection_add=False, and the opt-in tool is absent from toolGroups.json.
Verification: uv run pytest tests/test_ops_connection_add.py tests/test_shell_machine_channel.py tests/test_settings_tool_groups.py tests/test_tool_registry_execute.py -x - 151 passed. A direct before/after registry reproduction showed live_has_machine=True while both the admitted and subsequently rendered definitions remained False. I replied with the measured facts on both new threads; they were opened by another reviewer, so I did not resolve them.
…onfig's own list Review findings on the registry-writer PR (2026-09-24). Blocker: a process that started with no machine kept an `exec` schema without `machine` after the first `ops_connection_add`, because the registry serves a tool from the copy it took at admission and `ExecTool` authored no `to_schema` -- so the reply, the typed-ssh refusal from #552 and the new Raven-Code guide all sent the model to `machine=<id>` while the schema it was shown had no such parameter. `ExecTool.to_schema` is authored now, which is how a tool declares a dynamic shape; the shape moves only when the registry goes from empty to not. The registry falls back to the owner's home only when there is none beside the instance's own config, in `raven.ops.connections.store_path` and in the on-call launcher's `connections_registry`. Home-first was right for a sub-agent, whose config sits in a state directory nothing writes, but it silently moved a `--config /x/config.json` host off `/x/connections.json` as soon as a registry appeared in the home -- which an agent's first add now writes with no owner action. A sub-agent still resolves the home: nothing is ever written beside its rendered config. Also: `ops_connection_add` joins the settings tool table's `run` group, and the guard that diffs that table against a loop's registry turns the flag on, so it can see opt-in tools; `CONTEXT.md`'s Machine entry and both on-call doc pages describe the lookup and the tool; the plugin's orphaned `_PROBE` is gone; and #552's ssh-version test asks the client whether `-P` takes a value instead of looking for the string "tag", which an older client prints as the hostname -- so it never skipped on OpenSSH 8.9. Co-authored-by: Claude (claude-opus-5-5) <noreply@anthropic.com>
gloryfromca
left a comment
There was a problem hiding this comment.
No blockers; this can merge as far as I am concerned.
Reviewed the new revision and the full PR against the repository rules and Machine architecture. The previous schema blocker is fixed: ExecTool now explicitly declares a dynamic schema, so ToolRegistry.get_definitions() reflects the first real registry write on the next model call. The settings-table follow-up is also addressed by grouping ops_connection_add and enabling the opt-in tool in the coverage guard.
I also checked the added registry-path compatibility correction against its callers and history: an existing config-adjacent registry remains authoritative, while rendered configs without their own registry still fall back to the owner's home. The tests exercise both sides. I found no weakened coverage or backward-compatibility regression in the delta.
Verification: uv run pytest tests/test_ops_connection_add.py tests/test_ops_connections.py tests/test_cli_ops_connection_commands.py tests/test_shell_machine_channel.py tests/test_settings_tool_groups.py tests/test_agents_oncall_flow_connections.py tests/test_agents_oncall_flow_watcher.py tests/test_agents_oncall_launcher.py tests/test_agents_code_launcher.py tests/test_tool_registry_execute.py -x (314 passed). I covered the project rules and domain terminology, the full diff and new delta, relevant callers and history, backward compatibility, test changes, and the one-reader/one-writer architecture.
…s row The settings Tools page now draws one more switch row (ops_connection_add joined the run group), so the skeleton test expects 23, and ruff format wraps the store_path docstring. Co-authored-by: Claude (claude-opus-5-5) <noreply@anthropic.com>
|
Addressing the review summary (a525fcf, 5146f0a). Both inline findings are fixed and replied to; the out-of-scope notes:
Locally: the connection, launcher, tool-face and settings suites pass (365), the settings UI suite passes (183), and the full Python suite has one failure, |
gloryfromca
left a comment
There was a problem hiding this comment.
No blockers; this can merge as far as I am concerned.
Reviewed the revision delta and rechecked it against the full PR, prior findings, repository rules, relevant callers/history, backward compatibility, test integrity, and the one-reader/one-writer architecture. The added blank line is formatter-only, and raising the settings skeleton count from 22 to 23 matches the newly grouped ops_connection_add row; the rendered test passes rather than merely accepting a changed fixture. No prior fix is disturbed.
Verification: uv run pytest tests/test_ops_connections.py tests/test_settings_tool_groups.py tests/test_shell_machine_channel.py -x (124 passed); npm test -- src/features/settings/Skeletons.test.tsx (11 passed).
|
No blockers in the reviewed scope. This supersedes my summary at 257acb1, which opened Scope: I reviewed only the What I examined: the delta since 257acb1. Last round, now verified at 5146f0a:
Outside the reviewed scope, disclosed here because no inline note can be filed there:
Verification, at 5146f0a against an extracted head tree, with the imported
Checked and not reported: |
… exists Review question on #690 (2026-09-24): beside-the-config-first could not tell a stale copy beside a sub-agent's rendered config from the owner's list beside a `--config` host's config, and it broke the first the way home-first broke the second -- a copy read first hid every row added to the home since, and hid the coding agent's own adds from the on-call agent. `store_path()` now keys on the process role: a sub-agent (the host launched it with `RAVEN_SUBAGENT`) reads the owner's home whenever the home holds a registry, and its copy only when it does not (an install older than the home registry); any other instance, the `--config` host included, reads the list beside its config first, then the home. The on-call launcher's `connections_registry()` goes back to home-first for the same reason. Tests: the sub-agent fixture now carries `RAVEN_SUBAGENT`; the `--config` case runs as a host; two new cases pin both reproductions (a stale copy beside a sub-agent's config loses to the home, and one sub-agent's add reaches another whose state directory holds a copy). Both fail with the role check removed. Co-authored-by: Claude (claude-opus-5-5) <noreply@anthropic.com>
|
On the lookup question (d7c5220): yes, keyed on the process role now, as you suggested.
Both of your reproductions are pinned and fail with the role check removed:
The Locally: 658 passed across the connection, launcher, tool-face, settings, tool-search, registry and sub-agent suites; ruff check and format clean. |
gloryfromca
left a comment
There was a problem hiding this comment.
No blockers; suggestions only, and they are marked inline.
The role-sensitive lookup resolves the stale-copy versus custom-config ambiguity correctly, and the writer/read-through and launcher cases are covered without weakening prior tests. I rechecked the new delta against the full reviewed PR, repository rules and Machine terminology, callers/history, backward compatibility, and the one-reader/one-writer architecture. The only remaining item is the ordinary ops-command import-cost regression marked inline; it does not prevent merging.
Verification: uv run pytest tests/test_ops_connections.py tests/test_ops_connection_add.py tests/test_agents_oncall_launcher.py tests/test_agents_code_launcher.py tests/test_subagent_host_env.py tests/test_agent_loop_subagent_role.py tests/test_shell_machine_channel.py tests/test_settings_tool_groups.py -x (262 passed).
store_path() imported raven.agent.subagent.role, which made raven.ops and raven.agent import each other and pushed the mutual-pair count to 14, over the ceiling of 13. The role variable, its reader and its writer now live in raven.home, which every shelf may import; raven.agent.subagent.role re-exports them so existing callers are unchanged. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
gloryfromca
left a comment
There was a problem hiding this comment.
No blockers; this can merge as far as I am concerned.
The new delta fixes the only outstanding suggestion without changing the role-sensitive registry behavior: the role flag now lives at the lightweight raven.home boundary, while raven.agent.subagent.role preserves its public imports. A clean store_path() call no longer loads the eager sub-agent/provider graph.
I reviewed the delta against the full previously reviewed PR, repository rules and Machine terminology, relevant callers/history, backward compatibility, test integrity, and the one-reader/one-writer architecture. Verification: uv run pytest tests/test_ops_connections.py tests/test_ops_connection_add.py tests/test_agents_oncall_launcher.py tests/test_agents_code_launcher.py tests/test_subagent_host_env.py tests/test_agent_loop_subagent_role.py tests/test_shell_machine_channel.py tests/test_settings_tool_groups.py -x (262 passed).
Why
In a research -> code -> oncall chain run on 2026-09-22, Raven-Code called
exec(machine="conn_cpu_32c")and got "No connection is registered", on a homewhose
connections.jsonlisted seven machines. It then read host/port/key outof the config and ran a raw
ssh root@<ip> -p <port> -i ~/.ssh/id_rsa, puttingthe way in into the transcript.
Two gaps:
raven.ops.connections.store_path()looked only beside theinstance's own config. A sub-agent runs on a rendered config in its own state
directory, so it looked in an empty directory. The host already hands every
sub-agent
RAVEN_HOME; the registry just was not looked for there. Only theon-call launcher worked around this (it sets
RAVEN_CONNECTIONSitself).ops_connection_add(feat(raven-oncall): let the on-call instance add the machine the owner describes #545) lived in the on-call plugin. In theusual chain the coding agent runs first, meets the empty registry first, and
had no way to take the owner's answer.
What changes
store_path(): env var ->raven_home()/connections.jsonif it exists ->beside the config if THAT exists -> the home (where a first add lands). An
install that kept its own list beside its config keeps working.
raven/ops/connection_add.py:probe,ssh_defaults,parse_probe,write,write_ssh_alias, andASK_OWNER(the questions relayed to the owner).The tool calls these functions directly;
raven ops connection add(theterminal command for a person) calls the same ones, so the logic exists once.
raven/agent/tools/connection_add.py:ops_connection_addas a trunk tool,opt-in via
tools.connectionAdd(on in raven-code and raven-oncall, off inthe other three products, whose pinned tool faces are unchanged).
raven.ops.transport.make_ssh_runner(identities_only=)+ISOLATE_IDENTITY,moved from the plugin: a key the tool picked itself is probed with that key
alone (see feat(raven-oncall): let the on-call instance add the machine the owner describes #545 review).
oncall_flow/connections.pyimports trunk's reader instead ofkeeping a hand-aligned copy (758 -> 212 lines); keeps only campaign lookups
(
get,display_name,resolve_into,describe). Its own tool file,factory and manifest row are removed.
TOOLS_CODE.md) gains when to leave this computer: writehere; verify where the software is; ask the owner for a machine only when
installing the software here is wrong (GPU, memory) or heavy (compiled solver,
toolchain, long download); never ssh from an address in the task statement.
SHOWNmade public inraven.ops.connections(the plugin may not import anunderscored trunk name;
_SHOWNstays as an alias).docs-site/docs/oncall.md/oncall.zh.mdupdated for the new lookupand the tool.
Not in scope
job runs is still decided inside the agent that runs it; this only makes that
agent find the owner's registry and able to add to it.
only fires once the registry is visible, which this makes true for Raven-Code.
Tests
tests/test_ops_connection_add.py(moved from the plugin suite, patches trunkmodules), including the ssh
-Gmeasurement that only the candidate key isoffered under isolation.
tests/test_ops_connections.pyfor the lookup order.objects).
@LivXue -- review requested: this touches
raven/opsand theRAVEN_CONNECTIONShandoff described in a44664d.
Co-authored-by: Claude (claude-opus-5-5) noreply@anthropic.com
Generated with Claude Code (https://claude.com/claude-code)