feat(agent): RDM messages and pipe passthrough logic #1538
Conversation
Let maintainers know that an action is required on their side
|
There was a problem hiding this comment.
Pull Request Overview
This draft PR introduces support for Remote Desktop Manager (RDM) application lifecycle management through the NOW protocol, including capabilities detection, application launching, and window control operations.
- Adds RDM-specific message handlers for capabilities, app start, and app actions
- Implements registry-based RDM installation detection and version retrieval
- Adds process monitoring and window management functionality for RDM instances
Reviewed Changes
Copilot reviewed 6 out of 7 changed files in this pull request and generated 15 comments.
Show a summary per file
| File | Description |
|---|---|
| devolutions-session/src/main.rs | Adds devolutions_agent_shared import for RDM registry access |
| devolutions-session/src/dvc/task.rs | Implements RDM message handlers, process spawning/monitoring, and window control |
| devolutions-session/Cargo.toml | Updates now-proto-pdu to 0.4.0 and adds devolutions-agent-shared dependency |
| crates/devolutions-agent-shared/src/windows/registry.rs | Adds product version encoding variants and install location retrieval function |
| crates/devolutions-agent-shared/src/windows/mod.rs | Defines RDM_UPDATE_CODE constant for registry lookups |
| crates/devolutions-agent-shared/src/lib.rs | Updates agent version retrieval to use new encoding parameter |
| Cargo.lock | Updates dependency versions including now-proto-pdu 0.4.0 |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| let short_year = match version_encoding { | ||
| ProductVersionEncoding::Agent => (product_version >> 24) + 2000, | ||
| ProductVersionEncoding::Rdm => (product_version >> 24) + 0x700, |
There was a problem hiding this comment.
Magic number: The constant 0x700 (1792 in decimal) for RDM version encoding is unexplained. This magic number makes the code difficult to understand and maintain. Consider adding a comment explaining what this value represents and why it's used for RDM version calculation, or define it as a named constant with documentation.
There was a problem hiding this comment.
Good question, I found this const empirically, @thenextman, by any chance, maybe you know how RDM version is encoded in registry?
vnikonov-devolutions
force-pushed
the
feat/agent-rdm-messages-bootstrap
branch
from
9241bee to
6d370a2
Compare
vnikonov-devolutions
changed the title
vnikonov-devolutions
force-pushed
the
feat/agent-rdm-messages-bootstrap
branch
from
6d370a2 to
4ab4249
Compare
vnikonov-devolutions
changed the title
Technical details
Additional changes
|
vnikonov-devolutions
force-pushed
the
feat/agent-rdm-messages-bootstrap
branch
from
4ab4249 to
3146dc4
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 12 out of 13 changed files in this pull request and generated 11 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
@copilot fix/apply last batch of copilot comments/suggestions. If I left comments in those conversations - do as comment says for resolution. PR should contain all changes in a single batch. |
|
@vnikonov-devolutions I've opened a new pull request, #1647, to work on those changes. Once the pull request is ready, I'll request review from you. |
Addresses review comments on PR for RDM message handling and pipe
passthrough logic.
## Safety & Correctness
- **Integer overflow**: Use `saturating_mul` for timeout_ms calculation
(prevents overflow when timeout_secs is large)
- **Race condition**: Replace check-then-set pattern with
`compare_exchange` in `process_app_start` to atomically transition
Disconnected→Connecting
- **State recovery**: Reset `connection_state` to Disconnected when
`process_app_start_impl` fails, allowing retry
- **Channel closure**: Explicitly handle `None` from `dvc_rx.recv()` to
avoid busy loop when channel closes
- **Path comparison**: Use `eq_ignore_ascii_case` for RDM process
detection (Windows paths are case-insensitive)
## Code Quality
- **Error visibility**: Log errors when READY notification fails instead
of silent `let _ =`
- **Documentation**: Clarify capabilities negotiation semantics
(negotiated subset vs. downgraded)
- **Magic numbers**: Document 0x700 offset in RDM MSI version encoding
(empirically derived)
- **Dead code**: Remove unused environment block creation and HashMap
import
## Example: Race Condition Fix
```rust
// Before: Non-atomic check-then-set allows duplicate spawns
match current_state {
Disconnected => {
self.connection_state.store(Connecting, Ordering::Release);
}
}
// After: Atomic transition prevents race
loop {
match current_state {
Disconnected => {
match self.connection_state.compare_exchange(
Disconnected, Connecting, Ordering::AcqRel, Ordering::Acquire
) {
Ok(_) => break, // Won the race
Err(actual) => current_state = actual; continue, // Retry
}
}
}
}
```
<!-- START COPILOT CODING AGENT TIPS -->
---
💬 We'd love your input! Share your thoughts on Copilot coding agent in
our [2 minute survey](https://gh.io/copilot-coding-agent-survey).
---------
Co-authored-by: copilot-swe-agent[bot] <198982749+Copilot@users.noreply.github.com>
Co-authored-by: vnikonov-devolutions <246051166+vnikonov-devolutions@users.noreply.github.com>
Adds RDM (Remote Desktop Manager) message handling capabilities to devolutions-session, enabling bidirectional communication between the agent and RDM through the NOW protocol over the named pipe.
Blocked by: Devolutions/now-proto#57
Issue: PI-651