Repository navigation
fix(agent): exec refuses a typed ssh to a machine the registry knows - #552
Conversation
gloryfromca
left a comment
There was a problem hiding this comment.
Blocking: the SSH detector must identify the executable and exact registered destination before refusing or naming a replacement.
I found three concrete failures in the new guard, detailed inline: ordinary path-qualified SSH invocations bypass it; same-host connections on different ports are resolved to the first row; and an unregistered address sharing a registered address's prefix is falsely refused.
Verification: uv run pytest tests/test_shell_machine_channel.py -q (32 passed), and uv run pytest tests/test_shell_comments.py tests/test_shell_approval.py tests/test_shell_background.py tests/test_rpc_spine.py tests/test_sandbox_unit.py -q (454 passed). I also reproduced all three cases directly against raw_ssh_target; the current tests do not cover them. GitHub's full check suite is green.
Coverage: I checked the repository rules and canonical Machine definition, the full diff, ExecTool callers and permission ordering, relevant registry/transport code and history, backward-compatibility claims, test changes for weakening, and the machine-identity architecture constraint.
84b22ed to
8ab4d06
Compare
gloryfromca
left a comment
There was a problem hiding this comment.
Blocking: compound-shell SSH and explicit -o Port destinations still need to be refused.
The three findings from the prior revision are fixed and their threads are resolved. This revision still has two concrete bypasses, detailed inline: valid compound shell lines can hide a registered SSH execution, and OpenSSH's explicit -o Port=... form is treated as port 22.
Verification: uv run pytest tests/test_shell_machine_channel.py -q passed 37 tests, and uv run pytest tests/test_shell_comments.py tests/test_shell_approval.py tests/test_shell_background.py tests/test_rpc_spine.py tests/test_sandbox_unit.py -q passed 454 tests. Direct reproductions confirmed both bypasses. GitHub's checks otherwise pass, but the coverage gate currently fails at 87.72% diff coverage (50/57 changed executable lines).
Coverage: I checked the repository rules and canonical Machine definition, the old-to-new delta and full diff, ExecTool callers and permission ordering, registry/transport behavior and relevant history, backward compatibility, test changes for weakening, and the machine-identity architecture constraints.
8ab4d06 to
244f610
Compare
gloryfromca
left a comment
There was a problem hiding this comment.
Blocking: the open compound-command and explicit-port findings still need code changes.
This revision adds four coverage tests only; raven/agent/tools/machine_exec.py is unchanged from the prior revision. Re-running the exact reproductions still returns None for both reported bypasses, so those two threads remain open. I found no new defect in the test-only delta, and the added tests do not weaken existing assertions.
Verification: uv run pytest tests/test_shell_machine_channel.py -q passed 41 tests, and uv run pytest tests/test_shell_comments.py tests/test_shell_approval.py tests/test_shell_background.py tests/test_rpc_spine.py tests/test_sandbox_unit.py -q passed 454 tests. At review time all completed GitHub checks pass; unit shard 4/4 is still running and the coverage gate has not yet reported.
Coverage: I checked the repository rules and canonical Machine definition, the prior-to-current delta and full diff, callers and permission ordering, relevant history, backward compatibility, test-strength changes, and the machine-identity architecture constraints.
244f610 to
4696085
Compare
gloryfromca
left a comment
There was a problem hiding this comment.
No blockers; this can merge as far as I am concerned.
Both open findings are fixed in this revision: compound commands now expose every SSH destination, including after unspaced operators, and -o Port=... participates in registry matching. I reproduced the original failing cases, replied to both threads, and resolved them. A sweep confirms every thread I opened is now resolved.
Verification: uv run pytest tests/test_shell_machine_channel.py -q passed 46 tests, and uv run pytest tests/test_shell_comments.py tests/test_shell_approval.py tests/test_shell_background.py tests/test_rpc_spine.py tests/test_sandbox_unit.py -q passed 454 tests. At review time every completed GitHub check passes; unit shard 4/4 is still running.
Coverage: I checked the repository rules and canonical Machine definition, the prior-to-current delta and full diff, callers and permission ordering, relevant history, backward compatibility, test-strength changes, and the machine-identity architecture constraints. I also applied the fourth-round convergence bar to parser edge cases and found none that justify another blocker.
|
Blocking: 1 finding needs correction before this revision can merge; see the inline notes. Scope: I reviewed the I reviewed the new detector and its single call site, re-ran every reproduction the earlier Confirmed findings:
Verification:
Checked and deliberately not reported: the detector is spelling-based, so |
4696085 to
c1f84cc
Compare
|
Revision c1f84cc answers both threads from the last round. Blocker (machine_exec.py, the -o branch): the option value is now split on an Nonblocking (the operator set): adding Tests: 12 new cases (two parametrised ExecTool tests for the spaced -o |
Two field runs on 2026-09-14 put a registered machine's address in the task statement, and the coding nodes typed "ssh -p <port> root@<ip> '... &'" from the local shell 58 times to start GPU work. The look-at-it channel is capped at 60 s and the on-call agent's job runner was not theirs to call, so the raw address was the path of least resistance. The cap, the process-group sweep that stops an orphan from holding a GPU, and the ledger all live on the other two paths, so none of that work was metered. The plain shell now refuses a command whose tokens run the ssh client to a destination the registry holds, naming both the machine and the two paths meant for it. The command is a shell line, not an argv. It is tokenised with punctuation_chars so an unspaced operator separates commands the way a shell reads them -- "true&&ssh" is two of them, and plain splitting hands back one word that is neither -- and every ssh in the line is read, because a compound line reaches each of its commands and stopping at the first let a registered destination through on the strength of an unregistered one beside it. A redirection is not the end of a command, and it is not a word either: "ssh 2>&1 -p 58717 root@host" is one ssh whose host follows the plumbing. Reading ">" as a terminator, or "&>" as the destination, lost the registered host after it; the operator and the file it names are stepped over instead, and the bare descriptor that punctuation_chars hands back in front of "2>&1" with it. The executable is recognised as "ssh", "/usr/bin/ssh" or "\ssh" alike: a leading backslash only suppresses alias lookup and an absolute path only skips PATH. The destination comes from ssh's own arguments rather than a search of the text, and the port from "-p" or from an "-o" port option in either spelling OpenSSH honours -- "-o Port=N" and "-o 'Port N'" print the same "port N" under ssh -G: the registry holds several machines at one address, so the address alone names the wrong one, and 203.0.113.70 is not 203.0.113.7. Untouched: an unregistered host still runs, scp and rsync still run (they move files and start nothing on the far side), and a registry that cannot be read or a line that cannot be tokenised recognises nothing, so ops being broken never takes the plain shell down with it. Co-authored-by: Claude (claude-fable-5-1) <noreply@anthropic.com>
gloryfromca
left a comment
There was a problem hiding this comment.
No blockers; suggestions only, and they are marked inline.
The prior blocker and operator-set note both reproduce as fixed: spaced -o Port values now match the registered port, and redirections before the destination no longer hide it. I withdrew my confirming blocker on LivXue's thread; that thread remains for its owner to resolve.
Named nonblocking follow-up: repeated conflicting Port settings use the wrong precedence. OpenSSH reports port 58717 for ssh -G -o Port=58717 -o 'Port 22' example.com because the first obtained value wins, while _ssh_destinations and the new test choose 22. This requires contradictory duplicate settings and removing the later duplicate is an immediate working path, so it does not meet the late-round blocker bar.
Verification: uv run pytest tests/test_shell_machine_channel.py -q passed 58 tests, and uv run pytest tests/test_shell_comments.py tests/test_shell_approval.py tests/test_shell_background.py tests/test_rpc_spine.py tests/test_sandbox_unit.py -q passed 454 tests. GitHub had not reported checks for this force-pushed revision at review time.
Coverage: I checked the repository rules and canonical Machine definition, the prior-to-current delta and full diff, callers and permission ordering, relevant history, backward compatibility, test-strength changes, and the machine-identity architecture constraints. All threads I opened remain resolved.
c1f84cc to
ecc9cc1
Compare
gloryfromca
left a comment
There was a problem hiding this comment.
No blockers; suggestions only, and they are marked inline.
The rebase onto current main preserved the reviewed detector and its tests byte-for-byte; the PR diff is still the same four files, and the prior fixes for spaced Port options and shell redirections still reproduce.
Named nonblocking follow-ups: the repeated conflicting-Port precedence mismatch remains as documented in the prior review. Separately, the detector treats any token named ssh as an executable position: echo ssh -p 58717 root@203.0.113.7 is refused as reaching the registered machine even though the shell only prints those words. That can obstruct diagnostics or script generation. This change introduces the false positive and it is reachable in ordinary use, but an operator has working alternatives and it does not reopen the raw-SSH bypass, so it does not meet the round-seven blocker bar.
Verification: uv run pytest tests/test_shell_machine_channel.py -q passed 58 tests; uv run pytest tests/test_shell_comments.py tests/test_shell_approval.py tests/test_shell_background.py tests/test_rpc_spine.py tests/test_sandbox_unit.py -q passed 454 tests. At review time, 17 GitHub checks passed, the coverage gate was still pending, and docs deploy was skipped.
Coverage: I checked the repository rules and canonical Machine definition, the full diff and rebase delta, the detector's single caller and permission ordering, relevant history, backward compatibility, test-strength changes, and the machine-identity architecture constraints. All threads I opened remain resolved; the two open LivXue threads are not mine to resolve.
Reviewed 2026-09-21. ssh takes the first obtained value for every option, and -p and -o Port queue together: `ssh -G -p 2222 -o Port=58717 host` prints 2222, and the two reversed prints 58717 (OpenSSH 9.9p2). The detector overwrote instead, so `-p 58717 -p 22` read as port 22 -- a command that really reaches the registered machine on 58717 read as unregistered and ran on the plain shell path, past the cap, the process-group sweep and the ledger. The prior test pinned the wrong semantics under the name "later-option-wins"; it is corrected and joined by six cases covering both spellings in both orders, all failing on the previous head. Co-authored-by: Claude (claude-opus-5) <noreply@anthropic.com>
|
Revision ba233ba: the named nonblocking precedence finding is fixed, and the You were right and my earlier test had it backwards. Measured on OpenSSH 9.9p2, The detector overwrote instead, so it was not only the conflicting-option case: The prior case in the redirection table asserted 22 under the id The second named follow-up -- Verification on this head: tests/test_shell_machine_channel.py 64 passed; all |
gloryfromca
left a comment
There was a problem hiding this comment.
No blockers; suggestions only, and they are marked inline.
The repeated-Port precedence finding is fixed. I compared all six new -p/-o Port orderings with OpenSSH and the parser now keeps the same first obtained value; the corrected expectation is backed by broader coverage rather than a weakened test. The merge-up preserves the prior fixes and, against the refreshed current main, the PR remains the same four-file scope.
The previously named executable-position false-positive remains the only nonblocking follow-up. I found no new defect on this revision.
Verification: uv run pytest tests/test_shell_machine_channel.py -q passed 64 tests; uv run pytest tests/test_shell_comments.py tests/test_shell_approval.py tests/test_shell_background.py tests/test_rpc_spine.py tests/test_sandbox_unit.py -q passed 454 tests. GitHub CI was still in progress at review time, with 16 checks passed and three unit shards pending.
Coverage: I checked the repository rules and current canonical Machine definition, the full diff and prior-to-current delta, the detector's caller and permission ordering, relevant history, backward compatibility, test-strength changes, and the machine-identity architecture constraints. All threads I opened remain resolved; the two open LivXue threads are not mine to resolve.
|
Blocking: 2 findings need correction before this revision can merge; see the inline notes. Scope: I reviewed the Both findings I filed at Confirmed findings:
All three share one mechanism: an option shape the scan does not model yields not an unknown Verification:
Checked and deliberately not reported: the documented deviation for a destination that is a bare |
Two spellings ssh honours let a connection to a registered machine past the guard (review on #552, 2026-09-21): - a bundled short-flag group: `ssh -vp 58717 root@h` was skipped as a group taking no argument, so 58717 became the destination; - an option after the host: OpenSSH re-enters its option loop once it has the destination and reads on until the first non-option word or `--`, so `ssh root@h -p 58717 true` connects on 58717 while the scan, which stopped at the host, read port 22. Groups are now read letter by letter until one takes a value, and the scan continues past the host until the remote command starts. The value-taking set is derived from ssh's own getopt string (OpenSSH_9.9p2) rather than kept by hand; the hand-kept one had lost `B`, so `ssh -B lo ...` read the interface name as the destination. The expectations are measured with `ssh -G`, and a parametrised test re-measures them against the ssh on the machine running the suite. Co-authored-by: Claude (claude-opus-5-5) <noreply@anthropic.com>
…ssh_to_registered_machine # Conflicts: # CHANGELOG.md
There was a problem hiding this comment.
No blockers; suggestions only, and they are marked inline.
I reproduced the three author-described parser corrections on this revision: bundled option groups now consume their values as getopt does, options after the destination affect the connection only until the remote command or --, and the value-taking option table includes -B and current -P. The new compatibility table agrees with the installed OpenSSH client, so the tests strengthen rather than mask the behavior. The merge-up leaves the pull request at the same four-file scope.
The previously named executable-position false positive remains the only nonblocking follow-up. I found no new defect on this revision.
Verification: uv run pytest tests/test_shell_machine_channel.py -q passed 94 tests; uv run pytest tests/test_shell_comments.py tests/test_shell_approval.py tests/test_shell_background.py tests/test_rpc_spine.py tests/test_sandbox_unit.py -q passed 475 tests. GitHub checks are green except unit shard 4/4, which is still running.
Coverage: repository rules and the canonical Machine definition, the full pull-request diff and revision delta, the detector's caller and ordering, relevant history, backward compatibility, test strength, and the stated architecture constraints. All threads I opened remain resolved; the remaining review threads were opened by another reviewer.
…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>
… can add a machine (#690) ## 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 home whose `connections.json` listed seven machines. It then read host/port/key out of the config and ran a raw `ssh root@<ip> -p <port> -i ~/.ssh/id_rsa`, putting the way in into the transcript. Two gaps: 1. **Read side.** `raven.ops.connections.store_path()` looked only beside the instance'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 the on-call launcher worked around this (it sets `RAVEN_CONNECTIONS` itself). 2. **Write side.** `ops_connection_add` (#545) lived in the on-call plugin. In the usual 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.json` if 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. - New `raven/ops/connection_add.py`: `probe`, `ssh_defaults`, `parse_probe`, `write`, `write_ssh_alias`, and `ASK_OWNER` (the questions relayed to the owner). The tool calls these functions directly; `raven ops connection add` (the terminal command for a person) calls the same ones, so the logic exists once. - New `raven/agent/tools/connection_add.py`: `ops_connection_add` as a trunk tool, **opt-in** via `tools.connectionAdd` (on in raven-code and raven-oncall, off in the 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 #545 review). - On-call plugin: `oncall_flow/connections.py` imports trunk's reader instead of keeping 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. - Raven-Code guide (`TOOLS_CODE.md`) gains when to leave this computer: write here; 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. - `SHOWN` made public in `raven.ops.connections` (the plugin may not import an underscored trunk name; `_SHOWN` stays as an alias). - Docs: `docs-site/docs/oncall.md` / `oncall.zh.md` updated for the new lookup and the tool. ## Not in scope - The pre-dispatch registry gate removed in a44664d is NOT restored. Where a 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. - #552 (exec refuses a typed ssh to a registered machine) is complementary: 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 trunk modules), including the ssh `-G` measurement that only the candidate key is offered under isolation. - Five new cases in `tests/test_ops_connections.py` for the lookup order. - Plugin parity tests replaced by an identity pin (the plugin's names ARE trunk's objects). - Code launcher face ledger gains a machine lane; oncall face unchanged. - Full suite: see CI. @LivXue -- review requested: this touches `raven/ops` and the `RAVEN_CONNECTIONS` handoff described in a44664d. Co-authored-by: Claude (claude-opus-5-5) <noreply@anthropic.com> Generated with Claude Code (https://claude.com/claude-code) --------- Co-authored-by: xiaotian.luo <xiaotian.luo@thetahealth.ai> Co-authored-by: Claude (claude-opus-5-5) <noreply@anthropic.com>
Two field runs on 2026-09-14 put a registered machine's address in the task
statement, and the coding nodes typed "ssh -p root@ '... &'" from
the local shell 58 times to start GPU work. The look-at-it channel is capped
at 60 s and the on-call agent's job runner was not theirs to call, so the raw
address was the path of least resistance. The cap, the process-group sweep
that stops an orphan from holding a GPU, and the ledger all live on the other
two paths, so none of that work was metered.
The plain shell now refuses a command that names ssh and a host the registry
knows, and the refusal names both the machine and the two paths that are
meant for it. Everything else is untouched: an unknown host still works, scp
and rsync still work (they move files and start nothing on the far side), and
a registry that cannot be read recognises nothing, so ops being broken never
takes the plain shell down with it.
Type: fix
Verification:
The four new ones cover a typed ssh to a registered machine being refused
with nothing run, an unknown host still running, a command that names the
host without ssh still running, and a malformed registry refusing nothing.
policy reasons, rpc): 316 passed, 0 failed.
Risk: low, but it does close a path some prompt may rely on. The refusal is
text, not an exception, and it names the replacement, so a model that reaches
for the old path is told where to go. This does not add the missing channel
itself: work between 60 s and a few minutes still has no first-class home
outside the on-call agent, which is a separate design question.