Repository navigation
Conversation
gloryfromca
left a comment
gloryfromca
left a comment
There was a problem hiding this comment.
No blockers; this can merge as far as I am concerned.
Reviewed the full github/main...HEAD diff and relevant surrounding code. I covered the repository rules and canonical terms in AGENTS.md / CONTEXT-MAP.md / the runtime and web context files; the persistence and explicit health-probe boundaries; RPC registration, models, wire schema, generated clients, and callers; the Connections hub source/store integration; backward compatibility; commit history; and whether tests were weakened to obtain a pass. I found no plain error or outstanding blocker.
Verification: uv run pytest tests/test_ops_robots.py tests/test_rpc_robots.py tests/test_rpc_schema_match.py tests/test_rpc_registration.py -x (551 passed); focused web tests (8 passed); web type-check and generated-client check passed; Ruff passed; direct source-language and large-file checks passed; git diff --check passed. The Make targets themselves were unavailable because this environment has no make, so I ran their underlying repository scripts directly.
|
Blocking: 1 finding needs an answer from the author before this revision can merge; it is stated below because no changed file is inside the reviewed fence. Scope: I reviewed only the What I examined: the diff from merge base 855a412 to head ad90940; Confirmed finding:
Questions for the author:
What is not in question: the implementation boundaries I checked look sound -- literal RFC1918 IPv4 only, Verification:
|
There was a problem hiding this comment.
Blocking: the GUI relabels the canonical Machine concept as "Servers" (zh: the word for "server"), which is the word CONTEXT.md's Machine entry rules out. CONTEXT.md is left unchanged, so it now contradicts the page.
What changed.
gui.hub.machinesgoes from{"en": "Machines", "zh": <"machine">}to{"en": "Servers", "zh": <"server">}(i18n/messages.json:3410).- The gui.machine.* block and its neighbours are reworded to "server" (for example "No server is registered; see the Servers tab."). The catalogue's count of the zh word for "server" goes from 14 to 56.
- The PR's own new text uses the same word:
ui-web/CONTEXT.md("not server registrations"),docs/robot-registration.md("separates Servers, where Raven may run an agent", "No robot is treated as a server"), and ui-web comments (state/hub.ts,HubHead.tsx,store.ts,manifest.ts).
What CONTEXT.md still says.
- Machine (CONTEXT.md:403), with
_Avoid_: "host" / "server" / "node" (too broad, no link to the ops connection registry that supplies the rows)at :504. - ":453 The registry itself is kept from the Agent Hub's Machines tab", which is now false.
- The model-facing instructions and the server's own refusal text elsewhere still say Machines. So a person sees "Servers" while the agent and the docs say "Machines", and "server" is already used on the same pages for the gateway, MCP servers and mail servers.
Rule. AGENTS.md section 6: naming a concept tracked in CONTEXT-MAP.md means using the canonical term, not a synonym; a new term is defined in the matching CONTEXT.md in the same change.
Fix, either:
- keep "Machines" (and the zh word for "machine") for the machine registry, and keep the robot work as its own tab and term; or
- if the product wants the rename, make it a CONTEXT.md decision in this change: rename the entry, move "server" out of Avoid with a reason, fix :453, and update the agent-facing text, so every surface says one word.
There was a problem hiding this comment.
Blocking: the Machine-to-Server relabel conflicts with the repository canonical terminology.
This newly raised conflict makes my prior clean stance stale. CONTEXT.md defines the registry concept as Machine, explicitly lists server under _Avoid_, and still names the Machines tab, while this revision relabels that same concept throughout the GUI and new documentation without changing the canonical definition. Until the author either keeps Machine or makes the product rename consistent in the canonical context and affected surfaces, I no longer consider this revision mergeable. This is a stance update based on the existing thread, not a separate finding.
No code has changed since the already reported test run; this objection is the repository term-contract mismatch itself.
There was a problem hiding this comment.
No blockers; this can merge as far as I am concerned.
My terminology objection is settled on daeee4b3ae79: the existing execution-host concept is again Machines in the catalogue, generated text, documentation, RPC module description, and UI comments, while Robots remains separate. I found no residual Machine-to-Server relabel in the feature diff.
I am not resolving this review thread because it was opened by another reviewer.
|
Not a blocker - non-blocking items from acceptance on Severity of this one finding, not a verdict on the pull request. Any blocking findings from this pass are review threads on the changed files; GitHub renders those collapsed, as a file name with no text. The network boundary holds up. The health-check validator rejected every hostile form I put to it:
No DNS is involved, so there is no rebinding. A 302 to the metadata address is not followed, requests are GET only with a bounded timeout and no proxy or netrc, and only the RPC owner can call it (no agent tool). Storage is 0700/0600, ids are uuid4, and the 100-row cap held under racing writers. Non-blocking:
Tests here on the merged tree: the changed Python test files via the board runner gave 541 passed. ui-web changed tests gave 8 passed. ui-web gate tests: 53 files, 239 passed (the goldens and snapshots match). |
|
Windows compatibility review — PR #1144 @ Scope: the full base-to-head diff at this head (35 files, +2373/−156): the new profile-scoped personal-robot registry ( Finding (low, Windows-specific, reproduced):
Note (not a defect): Positive, Windows-relevant: the status probe uses Verification on native Windows 11, detached worktree
Coverage gaps: the ui-web tests this PR adds ( |
|
Thanks @LivXue , @0xKT , and @gloryfromca for the reviews. I propose retaining Machines for the existing execution-host registry and keeping Robots as a separate concept. This would avoid coupling the robot feature to a broader terminology change. The current scope is an owner-managed, profile-scoped device inventory: registration, editing, recoverable removal, and explicit read-only health checks for supported HTTP endpoints. It does not add robot control, agent execution, scheduling, or dispatch. The Connections page is currently the only consumer. @LivXue, I understand the concern about adding a core store and five public RPC methods without another consumer. The existing authenticated transport and Connections UI provide an integration point, but the core-versus-plugin decision needs agreement before further implementation. Would you consider this bounded inventory-and-health-check feature appropriate for core with Machines retained, or would you prefer a product/plugin implementation? Is there a specific user workflow you would need this proposal to demonstrate? I’ve also noted the storage-path, TTL, schema-duplication, and Windows junction findings. Those are not fixed in the current revision. |
|
The motivation comes from my own use: I own several robots, and I plan to keep maintaining and extending this functionality in my local development version. I’d like to develop it in a structured, more general-purpose way rather than as a one-off setup for my own devices. That’s why I’m seeking feedback on the right abstraction and extension point in Raven. I’m open to keeping it in a product/plugin layer if that is a better fit than core. My goal is to establish a maintainable foundation for continued iteration, without expanding this PR into robot control or dispatch. |
|
Blocking: the proposed retention of Machines must land before this revision can merge. Retaining Machines for the existing execution-host registry and keeping Robots separate directly resolves my terminology objection once it is applied consistently to the catalogue, generated files, new docs, and UI comments. I agree that this avoids coupling the robot feature to a broader rename. I do not have an additional blocker on core-versus-plugin placement from my review; that is an architecture/product decision for the maintainers, and the current implementation has a concrete bounded inventory and explicit health-check workflow. My stance remains blocking only because the current head still contains the Machine-to-Server relabel. No code has changed, so there is no new test result to report. |
Rename GUI execution-host labels to Servers and add a personal robot registry using typed RPC. Keep registrations separate from executable nodes and restrict optional health checks to explicit read-only private IPv4 endpoints. Add the Robots tab's hub-head-tab line to both boot goldens and both affected page snapshots by hand to match the new shared header.
Restore the existing Machine catalogue and distinguish robot registrations from the Machine registry in documentation and UI comments.
ad90940 to
daeee4b
Compare
|
Windows compatibility re-review — PR #1144 @ Scope: the branch is rebased onto current main; the PR's own diff at this head is Result: the Windows finding from the previous review is unchanged and still open.
Verification on native Windows 11, detached worktree
Coverage gaps: the ui-web tests this PR adds still cannot run — the shared |
gloryfromca
left a comment
gloryfromca
left a comment
There was a problem hiding this comment.
No blockers; this can merge as far as I am concerned.
Reviewed the new revision and the delta from ad9094038dfc, then rechecked the full PR diff against current github/main. The new commit restores the canonical Machine term across the catalogue, generated text, docs, RPC description, and UI comments while keeping Robots separate, so my prior blocker is fixed. I also checked the repository/context rules, callers and generated contracts, compatibility, and that the rebase did not alter the previously reviewed feature patch. I found no new plain error.
The Windows junction hardening note remains on the record as the other reviewer labeled it: low severity and nonblocking. The author has also supplied the requested product motivation; core-versus-plugin placement is a maintainer architecture decision, not an additional blocker from my review.
Verification on this head: focused Python suite 551 passed; focused web suite 8 passed; web type-check and generated client check passed; TUI RPC and i18n generation checks passed; Ruff, source-language, large-file, and git diff --check checks passed.
Summary
The Connections hub now calls execution hosts Servers and provides a separate Robots tab for personal device registrations. Existing machine IDs, SSH/node behavior, configuration, and CLI/RPC names remain unchanged.
The new tab supports registration, filtering, editing, recoverable removal, and restore. SparkBot and generic HTTP devices optionally support explicit read-only health checks. Robot dogs, AGVs, mobile manipulators, and other device types remain metadata-only. Records are stored separately per configuration profile; registration never establishes availability or installs an executable node.
Five typed robots.* RPC methods use the existing authenticated transport. The page uses the native source seam, and offline fixtures never contact a physical device. English and Chinese copy and generated clients are updated. Both boot goldens and both affected page snapshots gain one hub-head-tab line, edited by hand to match the new shared header. The schema comparator now handles nullable type arrays consistently with Pydantic; its regression test also verifies that required nullable fields remain required on the wire.
Type
Verification
Commands and results:
uv run --frozen --no-sync --python 3.12 pytest tests/test_ops_robots.py tests/test_rpc_robots.py -q --cov=raven.ops.robots --cov=raven.rpc.methods.robots --cov-branch --cov-report=term-missing -o addopts='': 15 passed with 33 subtests, 100% line and branch coverage of both new backend modules.npm test --prefix ui-web: 4,475 passed in 261 files.make ci: all lint and selected tests passed: 36,909 Python tests, 33 subtests, 4,475 web tests, and 2,288 TUI tests. Python also reports 259 skips and two expected failures. The test subprocess has terminal color enabled and proxy variables removed so loopback and terminal tests are isolated from workstation settings. Two existing macOS failures are deselected as noted below. The build then caught the expected header change in the boot goldens.make build check-core-wheel: passed, including both boot DOM checks and the standalone kernel-wheel smoke test. No implementation files changed after the full test run.npm audit --prefix bridge --audit-level=critical: exit 0; dependency lockfiles are unchanged.The full command is:
env -u NO_COLOR -u HTTP_PROXY -u HTTPS_PROXY -u ALL_PROXY \ -u http_proxy -u https_proxy -u all_proxy -u no_proxy \ TERM=xterm-256color NO_PROXY=127.0.0.1,localhost,::1 \ PYTEST_ADDOPTS="--deselect=tests/test_simulation_scenario.py::test_a_fresh_trial_refreshes_the_subagent_homes_before_it_runs --deselect=tests/test_simulation_suite.py::test_a_stopped_run_is_marked_in_its_record_and_finished_with_its_evidence" \ make ciBoth deselected tests also fail on a clean upstream checkout on this macOS host: one assumes distinct user.md/USER.md files and the other assumes Linux process discovery. Those tests and their implementation are not modified by this PR. No tests in the new feature are deselected.
Risk
This adds owner-confirmed personal records and authenticated, on-demand outbound GET requests to literal RFC1918 IPv4 /health or /status endpoints. Checks reject credentials, query parameters, redirects, compressed bodies, oversized responses, and non-standard JSON constants; they use no proxy and expire after 60 seconds. An endpoint responding does not verify chassis, audio, or motion. Company devices, control, shell execution, ROS, MQTT, and media access are out of scope.
An older gateway reports registration as unavailable. Concurrent edits and late check results cannot overwrite a later edit/removal. Removal is recoverable. Reverting the feature leaves the separate local database intact; it does not alter the existing server registry or model/provider settings.
Related Issues
N/A