feat(agent): add remote helper design doc and NDJSON proto types - #1913
feat(agent): add remote helper design doc and NDJSON proto types#1913nitrobass24 wants to merge 6 commits into
Conversation
Condensed design doc (428 lines) replacing the 3,500-line whitepaper from PR #1820. Covers architecture, Backend interface, wire protocol, path safety, SSH credentials, schema, helper binary, and phased implementation plan (8 PRs). pkg/agent/proto is a leaf package (zero internal/ imports) defining the shared NDJSON wire types for qui <-> qui-helper communication: Command/Result envelopes, HelloBanner, and all op request/response payloads.
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughAdds a Remote Helper design doc, NDJSON-over-SSH protocol types/constants, filesystem operation request/response contracts, and comprehensive JSON round-trip tests for helper↔host communication. ChangesRemote Helper Protocol and Design
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@documentation/design/remote-helper.md`:
- Line 9: Add explicit language tags to the fenced code blocks shown (e.g. the
block containing "qui host seedbox" should be
```text, the block with the long ssh command beginning
`command="/home/user/.config/qui-helper/qui-helper serve --stdio ...` should be
```bash, and the block with "POST /api/instances/{id}/ssh-test -- test
SSH credentials, return host key fingerprint" should be ```text) so each fenced
block has a language label and avoids MD040 markdownlint warnings.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: e9a504da-75c7-4655-a3d8-3c19cf4e6672
📒 Files selected for processing (4)
documentation/design/remote-helper.mdpkg/agent/proto/ops.gopkg/agent/proto/proto.gopkg/agent/proto/proto_test.go
Replace monolithic callsite refactor with incremental feature-by-feature migration ordered by user-visible functionality. Each PR proves one feature works through the Backend interface before moving to the next.
Phase 2 is just missing files detection (proves the pattern). Phase 3 builds the remote helper infrastructure. Phase 5 iteratively adds remaining filesystem features after the helper is released, so each new feature works for both local and remote instances.
There was a problem hiding this comment.
🧹 Nitpick comments (1)
documentation/design/remote-helper.md (1)
208-210: ⚡ Quick winConfirm and document the minimum Go toolchain requirement for
os.Root.The design relies on
os.Root (Go 1.24+), but the implementation plan doesn’t explicitly gate rollout on repo toolchain compatibility. Please add an explicit “requires Go >= 1.24” note (or fallback plan) in the phase plan to avoid integration drift across PRs.#!/bin/bash set -euo pipefail echo "== go.mod files and declared Go versions ==" fd '^go\.mod$' -a | while read -r mod; do echo "--- $mod" rg -n '^\s*go\s+[0-9]+\.[0-9]+' "$mod" || true done echo echo "== Existing references to os.Root / toolchain assumptions ==" rg -n -C2 'os\.Root|Go 1\.24|toolchain|min(imum)? Go|openat2' documentation go.mod || trueAlso applies to: 340-342
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@documentation/design/remote-helper.md` around lines 208 - 210, Update the design doc section referencing os.Root to explicitly state "requires Go >= 1.24" and either gate rollout on that toolchain or describe a clear fallback plan; specifically edit the phase plan in documentation/design/remote-helper.md (the os.Root mentions around the phase plan and the repeated note at lines ~208-210 and ~340-342) to: 1) add a one-line minimum toolchain requirement "Go >= 1.24" next to the os.Root description, 2) describe the chosen rollout guard (e.g., bump repo go version in go.mod or CI check) OR a documented fallback behavior when older toolchains are used, and 3) reference the os.Root symbol and openat2 dependency so reviewers can find the rationale.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@documentation/design/remote-helper.md`:
- Around line 208-210: Update the design doc section referencing os.Root to
explicitly state "requires Go >= 1.24" and either gate rollout on that toolchain
or describe a clear fallback plan; specifically edit the phase plan in
documentation/design/remote-helper.md (the os.Root mentions around the phase
plan and the repeated note at lines ~208-210 and ~340-342) to: 1) add a one-line
minimum toolchain requirement "Go >= 1.24" next to the os.Root description, 2)
describe the chosen rollout guard (e.g., bump repo go version in go.mod or CI
check) OR a documented fallback behavior when older toolchains are used, and 3)
reference the os.Root symbol and openat2 dependency so reviewers can find the
rationale.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 55260473-b5b1-423c-9181-9827e891d666
📒 Files selected for processing (1)
documentation/design/remote-helper.md
|
Moving to draft as the backend FSOPS callsites dont depend on this. |
Replaces the deployable-agent direction from the remote-helper design (PR #1913): a single SSH connection per instance carrying the SFTP subsystem plus opt-in exec channels, with capabilities probed at connect. The key's own restrictions pick the tier — an internal-sftp key gets a degraded-but-working mode (no file identity, so hardlink features switch off visibly), an exec-capable key gets full functionality. Documents the op mapping, security model, slimmed schema scope for #1917, and the open questions (file identity wire form, SameFilesystem without fsids, BSD remotes).
|
Closing in favor of the SFTP-native direction — no deployed agent. The new design is |
2 participants
Summary
pkg/agent/proto— shared NDJSON wire types for qui ↔ qui-helper communicationProto package
Command,Result,HelloBannerenvelopesinternal/imports (CI-enforceable)Test plan
go build ./pkg/agent/proto/...— cleango test -race -count=1 ./pkg/agent/proto/...— 38 tests passgo list -deps ./pkg/agent/proto/... | grep -c 'qui/internal'— 0 (no internal imports)Summary by CodeRabbit
Documentation
New Features
Tests