feat: land RemoteKVM backlog completion + review hardening - #9
feat: land RemoteKVM backlog completion + review hardening#9dieteradant wants to merge 4 commits into
Conversation
Post-merge review of the backlog-completion work surfaced several issues; fix the high-confidence, locally-verifiable ones: - client/render.rs: NV12 odd-width chroma stride is 2*ceil(width/2), not width — the bare-width stride read one byte past the plane and panicked on legitimate odd-width frames. Fix the stride and add regression tests. - client/media.rs: cap a single in-flight H.264 access unit (16 MiB) so a peer that never sets the RTP marker bit can't exhaust memory; add CVPixelBuffer stride guards on the VideoToolbox copy paths. - server/websocket.rs: an authenticated agent could relay SignalingAnswer/ICE and inflate usage counters for sessions owned by *other* machines. Bind every relayed agent message to a session owned by that machine (session_belongs_to_machine) + add an integration test. - clippy.toml: pin msrv = 1.78 to match workspace rust-version; fix div_ceil / Default / op-ref lints flagged under -D warnings. Verified on macOS: builds (default + macos_v0 + server), clippy -D warnings clean, 34 client/protocol/transport + agent tests pass. Server integration suite passed at the merge base; the additive signaling guard compiles and ships a new test, but the suite could not be re-run here (Docker disk exhausted). Deferred to Linear (cannot verify in this environment): Windows NVENC NV12 UV-plane upload, macOS is_sync_sample keyframe detection, VT decode-callback aliasing, MF drain flag logic.
📝 WalkthroughWalkthroughAdds thread-safe ScreenCaptureKit completion callbacks on macOS agent, introduces a full client-side H.264/Opus media decode and render pipeline (VideoToolbox on macOS, Media Foundation on Windows, CPAL audio output, egui frame rendering), refactors server ChangesAgent macOS Capture Thread-Safety
Client Media Decode and Render Pipeline
Server Redis-Backed Signaling Security
Clippy MSRV Configuration
Sequence Diagram(s)sequenceDiagram
rect rgba(173, 216, 230, 0.5)
Note over ClientWS,Server: Client connect + signaling
ClientWS->>Server: ConnectRequest(session_id)
Server->>SignalingState: register_client_session(session_id)
Server->>SignalingState: notify_agent(machine_id, offer)
SignalingState->>AgentWS: forward ConnectRequest
end
rect rgba(255, 200, 150, 0.5)
Note over AgentWS,Server: Agent ownership-checked answer
AgentWS->>Server: SignalingAnswer(session_id, sdp)
Server->>DB: session_belongs_to_machine(session_id, machine_id)
DB-->>Server: ok / rejected
Server->>DB: mark_session_active(session_id)
Server->>SignalingState: notify_client(session_id, answer)
SignalingState->>ClientWS: ConnectResponse(accepted, sdp)
end
rect rgba(200, 255, 200, 0.5)
Note over AgentWS,Redis: Presence refresh
AgentWS->>Server: Heartbeat
Server->>Redis: refresh_agent_presence(machine_id, instance_id)
end
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes Possibly related PRs
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 docstrings
🧪 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: 13
🧹 Nitpick comments (8)
docs/agent/architecture.md (1)
217-223: ⚡ Quick winAdd language specifier to fenced code block.
The code block starting at line 217 should specify a language for proper syntax highlighting. Consider adding
textorplaintextas the language identifier.📝 Proposed fix
-``` +```text ScreenCaptureKit SCStream -> CVPixelBuffer -> VideoToolbox VTCompressionSession (H.264 constrained baseline)🤖 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 `@docs/agent/architecture.md` around lines 217 - 223, The fenced code block showing the pipeline ("ScreenCaptureKit SCStream -> CVPixelBuffer -> VideoToolbox VTCompressionSession (H.264 constrained baseline) -> Annex-B encoded frames -> WebRTC H.264 video track") is missing a language specifier; update the opening fence from ``` to ```text (or ```plaintext) so syntax highlighting treats it as plain text and renderers apply correct formatting—locate the block containing "ScreenCaptureKit SCStream" and add the language identifier to the opening backticks.packaging/windows/remotekvm-url-protocol.reg.template (2)
3-5: ⚡ Quick winUse explicit registry hive instead of
HKEY_CLASSES_ROOT.
HKEY_CLASSES_ROOTis a merged view that redirects writes toHKEY_LOCAL_MACHINE(admin) orHKEY_CURRENT_USER(non-admin), causing inconsistent behavior based on installation privileges.For predictable per-user installation, explicitly use:
[HKEY_CURRENT_USER\Software\Classes\remotekvm]For system-wide installation requiring admin, use:
[HKEY_LOCAL_MACHINE\Software\Classes\remotekvm]🤖 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 `@packaging/windows/remotekvm-url-protocol.reg.template` around lines 3 - 5, The template currently writes the URL protocol under the merged hive [HKEY_CLASSES_ROOT\remotekvm]; change it to an explicit hive so behavior is predictable: for per-user installs replace references to HKEY_CLASSES_ROOT\remotekvm with HKEY_CURRENT_USER\Software\Classes\remotekvm, or for system-wide installs use HKEY_LOCAL_MACHINE\Software\Classes\remotekvm; update the key name(s) and any subordinate values in the file (e.g., the default @ and "URL Protocol") to match the chosen explicit hive.
14-15: Deep-link URL parsing is validated, but token format is not sanitized.The client validates the incoming
%1deep-link URL:parse_deep_linkinapps/client/src/auth.rsparses it withurl::Url::parse, enforces theremotekvmscheme, and only accepts theremotekvm://auth(orremotekvm:///auth) route—other routes are rejected. However, the extractedtokenquery parameter is passed through as a raw string with no format, length, or charset validation before being used inspawn_fetch→ApiClient::set_token(&token).While command injection via the registry is not a concern (the URL is not executed as shell code), consider adding validation or length limits to the token parameter for defense-in-depth.
🤖 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 `@packaging/windows/remotekvm-url-protocol.reg.template` around lines 14 - 15, parse_deep_link currently extracts the token query param and passes it unchecked into spawn_fetch → ApiClient::set_token, so add defensive validation: after extracting the token in parse_deep_link (or at the start of ApiClient::set_token) enforce a strict format and length (e.g. allow only expected charset like URL-safe base64 or a limited set [A-Za-z0-9-_=.] and a maximum length such as 256–1024 chars), reject or return an error for malformed/oversized tokens, and ensure callers (spawn_fetch) stop processing when validation fails; update parse_deep_link, spawn_fetch, or ApiClient::set_token accordingly and surface a clear error path when the token is invalid.packaging/macos/io.remotekvm.agent.plist.template (1)
14-21: ⚖️ Poor tradeoffHard-coded video parameters limit deployment flexibility.
The video resolution, FPS, and bitrate are hard-coded. Deploying to machines with different display configurations or network conditions requires manually editing the plist for each installation.
Consider reading these parameters from a configuration file (e.g.,
/usr/local/etc/remotekvm-agent.conf) or accepting them as dynamic command-line arguments derived from the environment.🤖 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 `@packaging/macos/io.remotekvm.agent.plist.template` around lines 14 - 21, The plist currently hard-codes video params via the ProgramArguments entries (--width, --height, --fps, --bitrate-kbps); change this to load values from a configurable source instead of literals: either (A) replace these static arguments with a small wrapper script (e.g., remotekvm-agent-launch.sh) that reads /usr/local/etc/remotekvm-agent.conf or environment variables and invokes the real agent with --width, --height, --fps, --bitrate-kbps (with sane defaults if keys missing), or (B) use the plist EnvironmentVariables section to populate WIDTH/HEIGHT/FPS/BITRATE and update the agent invocation to reference those env vars; update the template entries for the arguments (the --width/--height/--fps/--bitrate-kbps strings) to be generated from the chosen source and ensure defaults are documented in the wrapper or config reader.apps/server/src/util.rs (1)
40-61: 💤 Low valueMinor: Redundant bucket reset check.
The check at lines 49-54 (
if bucket.reset_at <= now) is unreachable. Theretain()on line 43 already removes all buckets wherereset_at <= now, andor_insertcreates buckets withreset_at = now + window. Any bucket reaching line 49 will always havereset_at > now.This is harmless defensive code, so leaving it is fine.
🤖 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 `@apps/server/src/util.rs` around lines 40 - 61, The redundant reset logic in RateLimiter::check is unreachable because the initial buckets.retain(|_, bucket| bucket.reset_at > now) removes any expired buckets and buckets.entry(...).or_insert(...) sets reset_at = now + self.window, so remove the unnecessary if bucket.reset_at <= now { ... } block (the conditional that reassigns Bucket with count 0 and reset_at) to simplify check(), keeping the retain(), or_insert() usage, and the subsequent count check and increment.apps/server/src/config.rs (1)
85-90: 💤 Low valueParse error for
SIGNALING_TTL_SECONDSlacks context.If
SIGNALING_TTL_SECONDScontains an invalid integer, the?operator propagates a generic parse error without mentioning the variable name. Consider adding context:Proposed fix
let signaling_ttl_seconds = std::env::var("SIGNALING_TTL_SECONDS") .unwrap_or_else(|_| "90".to_string()) - .parse()?; + .parse() + .map_err(|e| anyhow::anyhow!("SIGNALING_TTL_SECONDS: {e}"))?;🤖 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 `@apps/server/src/config.rs` around lines 85 - 90, The parse step for SIGNALING_TTL_SECONDS currently uses .parse()? which yields a generic error; change to first capture the string into a variable (e.g. signaling_ttl_str) and then parse into signaling_ttl_seconds using parse().map_err or anyhow::Context to attach context mentioning SIGNALING_TTL_SECONDS and the actual string value (e.g. "Failed to parse SIGNALING_TTL_SECONDS ('{value}') as integer: {err}"), then keep the subsequent zero-check on signaling_ttl_seconds unchanged.apps/agent/src/macos/encode.rs (1)
245-249: 💤 Low valueConsider adding type bounds to
set_propertyfor safety.The function casts arbitrary
&Tto&CFTypevia pointer transmutation. While it's currently only called with CF types (CFBoolean, CFNumber, CFString), adding a trait bound or making it only accept&CFTypedirectly would prevent accidental misuse.♻️ Suggested improvement
-fn set_property<T>(session: &VTCompressionSession, key: &CFString, value: &T) -> i32 { +fn set_property(session: &VTCompressionSession, key: &CFString, value: &CFType) -> i32 { let session = unsafe { &*(session as *const VTCompressionSession as *const CFType) }; - let value = unsafe { &*(value as *const T as *const CFType) }; unsafe { VTSessionSetProperty(session, key, Some(value)) } }Then update callers to cast explicitly:
set_property(session, key, v.as_ref() as &CFType)🤖 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 `@apps/agent/src/macos/encode.rs` around lines 245 - 249, The helper set_property currently transmutes an arbitrary &T into &CFType which is unsafe; change its signature to accept a reference to CFType (e.g., fn set_property(session: &VTCompressionSession, key: &CFString, value: &CFType) -> i32) or add a trait bound that ensures T: AsRef<CFType>/Into<&CFType>, then remove the pointer casts and pass the value directly to VTSessionSetProperty; update all callers (where currently passing CFBoolean/CFNumber/CFString) to call set_property(session, key, v.as_ref() as &CFType) or otherwise cast explicitly so only actual CF types are passed.apps/agent/src/windows/audio.rs (1)
590-632: 💤 Low valueFloat32 buffer cast assumes proper alignment.
Line 596 casts
datapointer to*const f32and creates a slice. WASAPI typically provides aligned buffers, but if the buffer is misaligned, this would cause undefined behavior on some architectures. Consider usingread_unalignedfor safety, or document the alignment assumption.🤖 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 `@apps/agent/src/windows/audio.rs` around lines 590 - 632, The Float32 branch in copy_as_i16 unsafely casts data to *const f32 which can UB if the incoming buffer is misaligned; change the conversion to read each f32 sample via unaligned-safe access (e.g., treat data as *const u8 and for each sample use ptr::read_unaligned::<f32> from the appropriate byte offset or copy the bytes into a properly aligned f32 before converting) so SampleFormat::Float32 no longer depends on pointer alignment; keep the same clamping/scaling logic and use the same sample_count and byte offsets as in the other branches.
🤖 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 `@apps/agent/src/macos/mod.rs`:
- Around line 148-155: The Sample.timestamp is using SystemTime::now() instead
of the encoder PTS; update the code to derive Sample.timestamp (and set
Sample.packet_timestamp) from frame.pts when available by converting the PTS
into a SystemTime/Duration using the stream/frame timebase (e.g., compute
pts_seconds = frame.pts * time_base and do UNIX_EPOCH +
Duration::from_secs_f64(pts_seconds)), and fall back to SystemTime::now() only
if frame.pts is None or invalid; adjust Sample.packet_timestamp to store the raw
frame.pts value as well. Ensure you reference Sample, frame.pts,
Sample.timestamp and Sample.packet_timestamp in the change.
In `@apps/agent/src/windows/audio.rs`:
- Around line 174-206: The encode function currently assigns the same
packet.timestamp to every EncodedAudioPacket when push_packet yields multiple
frames; update the for loop in encode (the loop that iterates over frames
returned by normalizer.push_packet) to increment the timestamp for each
successive frame by OPUS_FRAME_DURATION (or add n * OPUS_FRAME_DURATION where n
is the frame index) so each EncodedAudioPacket.timestamp reflects its correct
20ms offset; ensure you reference EncodedAudioPacket, encode,
push_packet/normalizer.push_packet, OPUS_FRAME_DURATION and keep
OPUS_SAMPLE_RATE/OPUS_CHANNELS unchanged.
In `@apps/agent/src/windows/encode.rs`:
- Around line 1561-1606: The encoder currently bails in encode() when
drain_output() returns empty, but some Media Foundation H.264 MFTs buffer frames
and produce no immediate output; update encode() to tolerate empty output for
the first few frames instead of immediate failure: add a small buffering
threshold (e.g., check self.frame_index or a new counter like
self.frames_queued) and only call anyhow::bail! if output.is_empty() after that
threshold; alternatively, set the MFT realtime flag
(CODECAPI_AVEncCommonRealTime) during recreate_if_needed() to reduce internal
buffering—use symbols encode, drain_output, recreate_if_needed, and
frame_index/self.pending_keyframe to locate where to implement the check or
property set.
- Around line 570-594: The NVENC DirectX path is incorrectly supplying a manual
pitch and making assumptions about NV12 layout: in upload_nv12 (and where
bgra_to_nv12 produces a tightly-packed buffer) do not force SysMemPitch/row
pitch to self.config.width as a workaround; more importantly, for
NV_ENC_INPUT_RESOURCE_TYPE_DIRECTX set NvEncPicParams.input_pitch to 0 so NVENC
derives pitch from the D3D11 surface/texture metadata (or alternatively query
the actual D3D11 surface pitch if your driver/SDK requires it). Locate
upload_nv12, the UpdateSubresource call on input_texture, and the code that sets
pic.input_pitch and change the pitch value to 0 (or replace with a proper
D3D11-derived pitch retrieval) and remove any incorrect assumptions about
UV-plane separate pitch when using tightly-packed NV12 buffers.
In `@apps/agent/src/windows/input.rs`:
- Around line 92-114: send_key currently only toggles KEYEVENTF_KEYUP and never
sets KEYEVENTF_EXTENDEDKEY, causing arrow/extended keys simulated via SendInput
to be misinterpreted; update send_key to OR KEYEVENTF_EXTENDEDKEY into flags
when the resolved virtual key is an extended key (e.g., VK_LEFT, VK_RIGHT,
VK_UP, VK_DOWN or whenever hid_to_vk indicates an extended HID usage). Locate
send_key and use the vk from hid_to_vk to conditionally set flags = flags |
KEYEVENTF_EXTENDEDKEY for extended keys (while preserving KEYEVENTF_KEYUP for
key release), ensuring the INPUT / KEYBDINPUT payload includes the combined
flags so SendInput gets the correct extended-key semantics.
In `@apps/client/src/media.rs`:
- Around line 1401-1414: The loop that handles ProcessOutput in the match on
result currently treats MFT_OUTPUT_DATA_BUFFER_INCOMPLETE backwards: update the
conditional that checks output.dwStatus & MFT_OUTPUT_DATA_BUFFER_INCOMPLETE.0 as
u32 so that when the INCOMPLETE flag is set you continue the loop (call
ProcessOutput again), and when it is not set you break out (no more output).
Locate the match block around result, the usage of select_output_type(),
copy_output_sample(), and the existing check against
MFT_OUTPUT_DATA_BUFFER_INCOMPLETE and invert the branch logic accordingly.
In `@apps/server/Dockerfile`:
- Around line 8-20: The runtime Dockerfile runs the server as root; create a
non-root user and switch to it: in the runtime stage (the lines around WORKDIR
/app, COPY ... /usr/local/bin/remotekvm-server and CMD ["remotekvm-server"]) add
commands to create a minimal system user/group (e.g., app user), chown the
application directory and the binary (/app and /usr/local/bin/remotekvm-server)
to that user, and add a USER directive to run the process as that user instead
of root; ensure file ownership and permissions allow execution by that user and
keep ENV PORT and EXPOSE 8080 unchanged.
In `@apps/server/migrations/20260531120000_organization_invites.sql`:
- Around line 9-13: The unique constraint on organization_invites currently uses
(organization_id, email) which is case-sensitive while accept_pending_invites in
auth/routes.rs compares lower(email), causing mismatches; fix by making the DB
constraint case-insensitive—either drop the existing UNIQUE constraint on
organization_invites and create a functional unique index on (organization_id,
lower(email)) or add a persisted/lowercased computed column (e.g., email_lower)
and enforce UNIQUE(organization_id, email_lower); update migration SQL
accordingly so the DB uniqueness matches the lower(email) logic used in
accept_pending_invites.
In `@apps/server/src/auth/routes.rs`:
- Around line 349-353: The INSERT into organization_members uses ON CONFLICT
(organization_id, user_id) DO UPDATE SET role = EXCLUDED.role which can
downgrade an existing higher-privilege role; change the conflict handling in
this statement (affecting organization_members, role, EXCLUDED.role, and the
SELECT from accepted) to preserve the higher-privilege role or skip updates—for
example replace the DO UPDATE to conditionally set role =
GREATEST(existing.role, EXCLUDED.role) using an explicit comparison or use DO
NOTHING so existing memberships are kept; ensure the conflict clause references
organization_id and user_id as currently written and adjust the UPDATE
expression to prefer the higher privilege instead of unconditionally writing
EXCLUDED.role.
In `@apps/server/src/lib.rs`:
- Around line 127-133: The current rate-limit key fallback chain that reads
req.headers().get(header::USER_AGENT) and then unwrap_or_else("anonymous") is
insecure; remove the User-Agent fallback and instead require a client IP-derived
key (from x-forwarded-for or x-real-ip) only: if a trusted-proxy mode is
enabled, extract the IP from those headers and return a 400/Err when missing,
otherwise use the remote IP; eliminate the anonymous bucket/unwrap_or_else usage
and ensure the rate-limit key generation logic treats missing IPs as an error
rather than falling back to User-Agent or "anonymous".
In `@packaging/macos/io.remotekvm.agent.plist.template`:
- Around line 40-43: The plist currently writes logs to world-readable /tmp via
the StandardOutPath and StandardErrorPath keys; change those keys to point to a
user-specific log location (e.g. ${HOME}/Library/Logs/RemoteKVM/agent.log and
${HOME}/Library/Logs/RemoteKVM/agent.err.log) in the template and ensure the
installer or launch setup replaces ${HOME} with the target user's home and
creates the directory RemoteKVM with restrictive permissions (dir 0700, files
0600) before starting the agent so logs are not world-readable and survive
reboots.
- Around line 24-30: Remove the RKVM_REGISTRATION_TOKEN EnvironmentVariables
entry from the io.remotekvm.agent.plist.template and stop passing the secret on
the process environment; instead implement a secure retrieval path in the agent
(add a loadRegistrationToken() function and call it from main()/Agent::start())
that reads the token from a file with restricted permissions or from the macOS
Keychain, validate and handle errors (log without printing the token), and
ensure the token file is created with owner-only permissions so it is not
exposed via ps/launchctl.
- Around line 34-38: The plist's KeepAlive semantics currently treat clean exits
as successful so launchd won't restart the agent; update behavior so unexpected
server disconnects and similar failure paths return a non-zero exit (or change
the plist to remove/flip SuccessfulExit). In practice, modify the agent exit
paths in apps/agent/src/main.rs: where you handle requests.recv() == None (the
"server connection closed" branch) and the Ctrl+C/shutdown handler (main or run
loop), return an Err or call std::process::exit with a non-zero code instead of
Ok(()) for unexpected disconnects so launchd will restart the agent, or
alternatively update packaging/macos/io.remotekvm.agent.plist.template to set
KeepAlive/SuccessfulExit to true/omit it so clean exits are retried—pick one
approach and ensure the code paths reference the requests.recv() disconnect
handling and the main shutdown logic.
---
Nitpick comments:
In `@apps/agent/src/macos/encode.rs`:
- Around line 245-249: The helper set_property currently transmutes an arbitrary
&T into &CFType which is unsafe; change its signature to accept a reference to
CFType (e.g., fn set_property(session: &VTCompressionSession, key: &CFString,
value: &CFType) -> i32) or add a trait bound that ensures T:
AsRef<CFType>/Into<&CFType>, then remove the pointer casts and pass the value
directly to VTSessionSetProperty; update all callers (where currently passing
CFBoolean/CFNumber/CFString) to call set_property(session, key, v.as_ref() as
&CFType) or otherwise cast explicitly so only actual CF types are passed.
In `@apps/agent/src/windows/audio.rs`:
- Around line 590-632: The Float32 branch in copy_as_i16 unsafely casts data to
*const f32 which can UB if the incoming buffer is misaligned; change the
conversion to read each f32 sample via unaligned-safe access (e.g., treat data
as *const u8 and for each sample use ptr::read_unaligned::<f32> from the
appropriate byte offset or copy the bytes into a properly aligned f32 before
converting) so SampleFormat::Float32 no longer depends on pointer alignment;
keep the same clamping/scaling logic and use the same sample_count and byte
offsets as in the other branches.
In `@apps/server/src/config.rs`:
- Around line 85-90: The parse step for SIGNALING_TTL_SECONDS currently uses
.parse()? which yields a generic error; change to first capture the string into
a variable (e.g. signaling_ttl_str) and then parse into signaling_ttl_seconds
using parse().map_err or anyhow::Context to attach context mentioning
SIGNALING_TTL_SECONDS and the actual string value (e.g. "Failed to parse
SIGNALING_TTL_SECONDS ('{value}') as integer: {err}"), then keep the subsequent
zero-check on signaling_ttl_seconds unchanged.
In `@apps/server/src/util.rs`:
- Around line 40-61: The redundant reset logic in RateLimiter::check is
unreachable because the initial buckets.retain(|_, bucket| bucket.reset_at >
now) removes any expired buckets and buckets.entry(...).or_insert(...) sets
reset_at = now + self.window, so remove the unnecessary if bucket.reset_at <=
now { ... } block (the conditional that reassigns Bucket with count 0 and
reset_at) to simplify check(), keeping the retain(), or_insert() usage, and the
subsequent count check and increment.
In `@docs/agent/architecture.md`:
- Around line 217-223: The fenced code block showing the pipeline
("ScreenCaptureKit SCStream -> CVPixelBuffer -> VideoToolbox
VTCompressionSession (H.264 constrained baseline) -> Annex-B encoded frames ->
WebRTC H.264 video track") is missing a language specifier; update the opening
fence from ``` to ```text (or ```plaintext) so syntax highlighting treats it as
plain text and renderers apply correct formatting—locate the block containing
"ScreenCaptureKit SCStream" and add the language identifier to the opening
backticks.
In `@packaging/macos/io.remotekvm.agent.plist.template`:
- Around line 14-21: The plist currently hard-codes video params via the
ProgramArguments entries (--width, --height, --fps, --bitrate-kbps); change this
to load values from a configurable source instead of literals: either (A)
replace these static arguments with a small wrapper script (e.g.,
remotekvm-agent-launch.sh) that reads /usr/local/etc/remotekvm-agent.conf or
environment variables and invokes the real agent with --width, --height, --fps,
--bitrate-kbps (with sane defaults if keys missing), or (B) use the plist
EnvironmentVariables section to populate WIDTH/HEIGHT/FPS/BITRATE and update the
agent invocation to reference those env vars; update the template entries for
the arguments (the --width/--height/--fps/--bitrate-kbps strings) to be
generated from the chosen source and ensure defaults are documented in the
wrapper or config reader.
In `@packaging/windows/remotekvm-url-protocol.reg.template`:
- Around line 3-5: The template currently writes the URL protocol under the
merged hive [HKEY_CLASSES_ROOT\remotekvm]; change it to an explicit hive so
behavior is predictable: for per-user installs replace references to
HKEY_CLASSES_ROOT\remotekvm with HKEY_CURRENT_USER\Software\Classes\remotekvm,
or for system-wide installs use HKEY_LOCAL_MACHINE\Software\Classes\remotekvm;
update the key name(s) and any subordinate values in the file (e.g., the default
@ and "URL Protocol") to match the chosen explicit hive.
- Around line 14-15: parse_deep_link currently extracts the token query param
and passes it unchecked into spawn_fetch → ApiClient::set_token, so add
defensive validation: after extracting the token in parse_deep_link (or at the
start of ApiClient::set_token) enforce a strict format and length (e.g. allow
only expected charset like URL-safe base64 or a limited set [A-Za-z0-9-_=.] and
a maximum length such as 256–1024 chars), reject or return an error for
malformed/oversized tokens, and ensure callers (spawn_fetch) stop processing
when validation fails; update parse_deep_link, spawn_fetch, or
ApiClient::set_token accordingly and surface a clear error path when the token
is invalid.
🪄 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: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: b9ecb40a-706e-428a-a065-9f0a53a966ae
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (52)
Cargo.tomlapps/agent/Cargo.tomlapps/agent/src/input.rsapps/agent/src/macos/annex_b.rsapps/agent/src/macos/capture.rsapps/agent/src/macos/encode.rsapps/agent/src/macos/mod.rsapps/agent/src/main.rsapps/agent/src/signaling.rsapps/agent/src/windows/audio.rsapps/agent/src/windows/capture.rsapps/agent/src/windows/encode.rsapps/agent/src/windows/input.rsapps/agent/src/windows/mod.rsapps/agent/src/windows/service.rsapps/client/Cargo.tomlapps/client/src/app.rsapps/client/src/auth.rsapps/client/src/config.rsapps/client/src/main.rsapps/client/src/media.rsapps/client/src/render.rsapps/client/src/signaling.rsapps/server/.env.exampleapps/server/Cargo.tomlapps/server/Dockerfileapps/server/docker-compose.ymlapps/server/migrations/20260531120000_organization_invites.sqlapps/server/migrations/20260531130000_usage_billing.sqlapps/server/src/auth/routes.rsapps/server/src/config.rsapps/server/src/lib.rsapps/server/src/main.rsapps/server/src/routes/billing.rsapps/server/src/routes/machines.rsapps/server/src/routes/mod.rsapps/server/src/routes/organizations.rsapps/server/src/routes/sessions.rsapps/server/src/state.rsapps/server/src/util.rsapps/server/src/websocket.rsapps/server/tests/integration.rsclippy.tomldocs/agent/architecture.mddocs/agent/macos-launchd.mddocs/client/protocol-handler.mddocs/deploy/production.mddocs/server/api.mdfly.tomlpackaging/macos/RemoteKVM-client-Info.plist.templatepackaging/macos/io.remotekvm.agent.plist.templatepackaging/windows/remotekvm-url-protocol.reg.template
| let sample = Sample { | ||
| data: Bytes::from(frame.data), | ||
| timestamp: SystemTime::now(), | ||
| duration, | ||
| packet_timestamp: 0, | ||
| prev_dropped_packets: 0, | ||
| prev_padding_packets: 0, | ||
| }; |
There was a problem hiding this comment.
Sample timestamp uses wall-clock time instead of encoder PTS.
The Sample.timestamp is set to SystemTime::now() while the encoder's frame.pts (presentation timestamp) is logged but not used. If audio samples use their encoder PTS for timing, this mismatch could cause A/V drift. Consider using a consistent timing source.
🤖 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 `@apps/agent/src/macos/mod.rs` around lines 148 - 155, The Sample.timestamp is
using SystemTime::now() instead of the encoder PTS; update the code to derive
Sample.timestamp (and set Sample.packet_timestamp) from frame.pts when available
by converting the PTS into a SystemTime/Duration using the stream/frame timebase
(e.g., compute pts_seconds = frame.pts * time_base and do UNIX_EPOCH +
Duration::from_secs_f64(pts_seconds)), and fall back to SystemTime::now() only
if frame.pts is None or invalid; adjust Sample.packet_timestamp to store the raw
frame.pts value as well. Ensure you reference Sample, frame.pts,
Sample.timestamp and Sample.packet_timestamp in the change.
| pub fn encode(&mut self, packet: &AudioPacket) -> Result<Vec<EncodedAudioPacket>> { | ||
| if packet.sample_rate != self.sample_rate { | ||
| anyhow::bail!( | ||
| "audio sample-rate mismatch: expected {}, got {}", | ||
| self.sample_rate, | ||
| packet.sample_rate | ||
| ); | ||
| } | ||
| if packet.channels != self.channels { | ||
| anyhow::bail!( | ||
| "audio channel mismatch: expected {}, got {}", | ||
| self.channels, | ||
| packet.channels | ||
| ); | ||
| } | ||
|
|
||
| let frames = self.normalizer.push_packet(packet)?; | ||
| let mut encoded = Vec::with_capacity(frames.len()); | ||
| for frame in frames { | ||
| let data = self | ||
| .encoder | ||
| .encode_vec(&frame, OPUS_MAX_PACKET_SIZE) | ||
| .map_err(|error| anyhow::anyhow!("encode Opus frame: {error:?}"))?; | ||
| encoded.push(EncodedAudioPacket { | ||
| data, | ||
| codec: AudioCodec::Opus, | ||
| sample_rate: OPUS_SAMPLE_RATE, | ||
| channels: OPUS_CHANNELS, | ||
| duration: OPUS_FRAME_DURATION, | ||
| timestamp: packet.timestamp, | ||
| }); | ||
| } | ||
| Ok(encoded) |
There was a problem hiding this comment.
Encoded frame timestamps are identical when multiple frames are produced.
When push_packet returns multiple 20ms frames, all resulting EncodedAudioPackets receive the same timestamp (line 203). This could cause RTP timestamp issues on the receiving end. Consider incrementing the timestamp by OPUS_FRAME_DURATION for each subsequent frame.
Proposed fix
let mut encoded = Vec::with_capacity(frames.len());
- for frame in frames {
+ for (i, frame) in frames.iter().enumerate() {
let data = self
.encoder
- .encode_vec(&frame, OPUS_MAX_PACKET_SIZE)
+ .encode_vec(frame, OPUS_MAX_PACKET_SIZE)
.map_err(|error| anyhow::anyhow!("encode Opus frame: {error:?}"))?;
encoded.push(EncodedAudioPacket {
data,
codec: AudioCodec::Opus,
sample_rate: OPUS_SAMPLE_RATE,
channels: OPUS_CHANNELS,
duration: OPUS_FRAME_DURATION,
- timestamp: packet.timestamp,
+ timestamp: packet.timestamp + OPUS_FRAME_DURATION * i as u32,
});
}🤖 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 `@apps/agent/src/windows/audio.rs` around lines 174 - 206, The encode function
currently assigns the same packet.timestamp to every EncodedAudioPacket when
push_packet yields multiple frames; update the for loop in encode (the loop that
iterates over frames returned by normalizer.push_packet) to increment the
timestamp for each successive frame by OPUS_FRAME_DURATION (or add n *
OPUS_FRAME_DURATION where n is the frame index) so each
EncodedAudioPacket.timestamp reflects its correct 20ms offset; ensure you
reference EncodedAudioPacket, encode, push_packet/normalizer.push_packet,
OPUS_FRAME_DURATION and keep OPUS_SAMPLE_RATE/OPUS_CHANNELS unchanged.
| fn upload_nv12(&self, nv12: &[u8]) -> Result<()> { | ||
| let expected_len = | ||
| self.config.width as usize * self.config.height as usize * 3usize / 2usize; | ||
| if nv12.len() != expected_len { | ||
| anyhow::bail!( | ||
| "NV12 upload size mismatch: got {} bytes, expected {expected_len}", | ||
| nv12.len() | ||
| ); | ||
| } | ||
| let resource: ID3D11Resource = self | ||
| .input_texture | ||
| .cast() | ||
| .context("cast NVENC input texture to D3D11 resource")?; | ||
| unsafe { | ||
| self.context.UpdateSubresource( | ||
| &resource, | ||
| 0, | ||
| None, | ||
| nv12.as_ptr().cast(), | ||
| self.config.width, | ||
| nv12.len() as u32, | ||
| ); | ||
| } | ||
| Ok(()) | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check if there are any existing tests or documentation about NV12 upload
rg -n "NV12|nv12" apps/agent/src/windows/ --context 3Repository: Anchras/remotekvm
Length of output: 14445
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Show relevant code sections around upload_nv12 and texture creation
sed -n '540,620p' apps/agent/src/windows/encode.rs
echo "----"
sed -n '1210,1325p' apps/agent/src/windows/encode.rs
echo "----"
sed -n '1760,1885p' apps/agent/src/windows/encode.rs
echo "----"
rg -n "UpdateSubresource" apps/agent/src/windows/encode.rs
echo "----"
rg -n "bgra_to_nv12|converts_bgra_to_nv12|NV12 upload" apps/agent/src/windows/encode.rsRepository: Anchras/remotekvm
Length of output: 11701
🏁 Script executed:
#!/bin/bash
set -euo pipefail
rg -n "GetDesc|SysMem|Pitch|RowPitch|SrcRowPitch|chroma_offset|chroma|Map\(|Unmap\(|UpdateSubresource" apps/agent/src/windows/encode.rs
# Show the NVENC registration + mapped input / any chroma offsets or pitch computations
sed -n '420,560p' apps/agent/src/windows/encode.rs
sed -n '560,720p' apps/agent/src/windows/encode.rs
# Show NVENC map input resource struct usage if present
rg -n "NvEncMapInputResource|registered_input|registered_resource|chroma_offset" apps/agent/src/windows/encode.rsRepository: Anchras/remotekvm
Length of output: 11282
🏁 Script executed:
#!/bin/bash
set -euo pipefail
rg -n "UV|UV-plane|chroma_offset|input_pitch|NV12 upload|UpdateSubresource|row pitch|SysMemPitch|Nvenc.*pitch" apps/agent/src/windows/encode.rsRepository: Anchras/remotekvm
Length of output: 344
🏁 Script executed:
#!/bin/bash
set -euo pipefail
sed -n '1080,1265p' apps/agent/src/windows/encode.rsRepository: Anchras/remotekvm
Length of output: 5051
🏁 Script executed:
#!/bin/bash
set -euo pipefail
sed -n '440,520p' apps/agent/src/windows/encode.rs
sed -n '620,690p' apps/agent/src/windows/encode.rs
sed -n '690,760p' apps/agent/src/windows/encode.rsRepository: Anchras/remotekvm
Length of output: 7577
🌐 Web query:
ID3D11DeviceContext::UpdateSubresource NV12 SysMemPitch SysMemSlicePitch semantics
💡 Result:
For ID3D11DeviceContext::UpdateSubresource, the parameters SysMemPitch and SysMemSlicePitch (passed via the D3D11_SUBRESOURCE_DATA structure or directly to the method) follow specific semantics regarding how system memory is interpreted when uploading to a resource [1][2]. 1. SysMemPitch (Source Row Pitch): This represents the distance in bytes from the beginning of one line of data to the beginning of the next line [1]. For NV12, which is a planar format, the pitch must be set to the width of the luma (Y) plane in bytes (including any necessary padding) [3]. 2. SysMemSlicePitch (Source Depth Pitch): This represents the distance in bytes from the beginning of one depth level (or slice) to the next [1]. In D3D11, this parameter is primarily used for 3D textures [1][4]. For 2D textures, including NV12, this parameter should typically be set to 0 [4]. Semantics for NV12: NV12 is a planar format containing a luma (Y) plane followed by a packed chroma (UV) plane. D3D11's UpdateSubresource does not natively expose these planes as separate subresources [3]. When you call UpdateSubresource on an NV12 texture, the driver expects the data to be in a specific layout that effectively treats the entire NV12 surface as a single contiguous memory block [3]. Because the API does not provide first-class support for planar layouts, applications often cannot perform a single "clean" update if their source memory layout (e.g., separate Y and UV buffers) does not match the driver's expected memory layout for the subresource [3][5]. Key constraints for NV12: - UpdateSubresource generally expects a consistent stride. If your source data layout (especially for the chroma plane) differs from the destination resource's internal layout, you must perform manual copies to repack the data into a single buffer that matches the expected stride before calling UpdateSubresource [5]. - Some driver implementations may expect the total size of the subresource to include both planes, and inconsistencies in how the pitch is interpreted can lead to visual corruption or errors [3][6]. - Always verify that your row pitch account for any alignment requirements or padding added by the driver [1][3]. In summary, for 2D NV12 textures, set SysMemPitch to your row stride and SysMemSlicePitch to 0 [1][4]. Ensure your source memory is packed in a way that respects the driver's expected layout for the entire NV12 subresource [3][5].
Citations:
- 1: https://learn.microsoft.com/en-us/windows/win32/api/d3d11/ns-d3d11-d3d11_subresource_data
- 2: https://learn.microsoft.com/en-us/windows/win32/api/d3d11/nf-d3d11-id3d11devicecontext-updatesubresource
- 3: https://stackoverflow.com/questions/71906965/why-is-chrominance-lost-when-i-copy-dxgi-format-nv12-id3d11texture-from-a-d3d11d
- 4: https://stackoverflow.com/questions/53569263/directx-11-id3ddevicecreatetexture2d-with-initial-data-fail
- 5: https://stackoverflow.com/questions/58093597/copy-nv12-to-id3d11texture2d-at-once
- 6: https://gitlab.winehq.org/wine/wine/-/commit/35ad787138aab14ee10067931cb87c1301dbc497
🌐 Web query:
DXGI_FORMAT_NV12 UpdateSubresource SysMemPitch luma pitch UV pitch
💡 Result:
For DXGI_FORMAT_NV12 resources, using UpdateSubresource (or similar APIs like UpdateSubresources in D3D12) requires careful handling because NV12 is a planar format [1]. NV12 Layout and Pitch NV12 stores data in two distinct planes: a luminance (Y) plane and a chrominance (UV) plane [1]. 1. Luminance (Y) Plane: Occupies the first (SysMemPitch * height) bytes of the resource [1]. 2. Chrominance (UV) Plane: Occupies the remaining (SysMemPitch * (height / 2)) bytes [1]. The total size required for a staging resource or initialization data is (SysMemPitch * (height + (height / 2))) bytes [1]. Note that the width and height must be even [1]. Using UpdateSubresource When using UpdateSubresource to update an NV12 texture, the SrcRowPitch parameter typically refers to the pitch of the luminance plane [2]. The API treats the NV12 resource as having planar structure, where the Y and UV data are conceptually contiguous in system memory [1][3]. Key implementation details include: - Planar Mapping: Because NV12 is planar, you cannot map it as a single view for all planes simultaneously. You must map the Y plane and the UV planes separately using different shader resource view (SRV) formats (e.g., DXGI_FORMAT_R8_UNORM for Y and DXGI_FORMAT_R8G8_UNORM for UV) [1][4]. - UpdateSubresource Limitations: Some hardware and driver implementations may have limitations or bugs regarding how they handle pitch with NV12 when using UpdateSubresource [3]. For complex or high-performance scenarios, it is generally recommended to use Map/Unmap (for dynamic textures) [5][4] or use D3D12's GetCopyableFootprints to properly account for the planar layout and alignment requirements [6][7]. - Alignment: If you are using D3D12, ensure that row pitches and offsets comply with D3D12_TEXTURE_DATA_PITCH_ALIGNMENT (256 bytes) and D3D12_TEXTURE_DATA_PLACEMENT_ALIGNMENT (512 bytes) unless specific hardware features (like UnrestrictedBufferTextureCopyPitchSupported) are enabled [7]. In summary, when passing pitch to UpdateSubresource for NV12, provide the luminance row pitch. Ensure your source data is laid out correctly with the UV plane following the Y plane, maintaining the same row pitch for both planes [1]. If you encounter issues with this API, consider using dedicated copy functions or map/unmap operations to ensure full control over the planar data layout [5][7].
Citations:
- 1: https://learn.microsoft.com/en-us/windows/win32/api/dxgiformat/ne-dxgiformat-dxgi_format
- 2: https://learn.microsoft.com/en-us/windows/win32/api/d3d11/nf-d3d11-id3d11devicecontext-updatesubresource
- 3: https://gitlab.winehq.org/wine/wine/-/commit/35ad787138aab14ee10067931cb87c1301dbc497
- 4: https://github.com/microsoft/Windows-universal-samples/blob/main/Samples/HolographicFaceTracking/cpp/Content/NV12VideoTexture.cpp
- 5: https://stackoverflow.com/questions/14672005/how-to-use-updatesubresource-to-update-the-texture-in-direct3d
- 6: https://stackoverflow.com/questions/74995580/how-do-i-create-and-initialise-a-dxgi-format-nv12-resource-in-dx12-source-is-av
- 7: https://learn.microsoft.com/en-us/windows/win32/direct3d12/upload-and-readback-of-texture-data
🌐 Web query:
D3D11 UpdateSubresource planar/semi-planar formats NV12 pitch requirements
💡 Result:
In Direct3D 11, NV12 is a planar format (one Y plane and one interleaved UV plane) [1]. When using ID3D11DeviceContext::UpdateSubresource to update an NV12 texture, it is important to understand that the API does not treat NV12 as a multi-plane object with independent pitch parameters. Instead, it exposes the texture as a single subresource [2][1]. Regarding NV12 pitch and alignment requirements: 1. Consistency with Subresource Mappings: The safest way to handle pitch when updating NV12 textures is to match the row pitch expected by the D3D11 texture [2]. You should use ID3D11DeviceContext::Map to retrieve the D3D11_MAPPED_SUBRESOURCE, which provides the RowPitch that the driver-allocated texture requires [2]. Copying data line-by-line using this driver-provided pitch is the standard, reliable method, even if it requires an extra copy from your source memory [2]. 2. Plane Structure: NV12 data consists of a full-resolution Y plane followed by a half-resolution (in both dimensions) interleaved UV plane [1]. While the API abstracts this into one subresource, the memory layout typically requires the UV plane to begin immediately after the Y plane, often with its own row pitch alignment [1]. Some implementations may encounter issues if the box coordinates provided to UpdateSubresource are not aligned to 2x2 boundaries, as NV12 sub-sampling dictates 2-pixel alignment for both width and height [3][4]. 3. Hardware/Driver Behavior: Driver-specific implementations can sometimes behave inconsistently with UpdateSubresource on planar formats [3]. For instance, certain drivers may misinterpret the pitch or fail to copy correctly if the UpdateSubresource box does not strictly adhere to the alignment requirements of the format [3]. Because the exact pitch and alignment requirements for NV12 can be hardware-dependent and are not always transparently exposed by the D3D11 runtime, developers are strongly advised to: - Use ID3D11DeviceContext::Map and manually copy rows to the mapped destination [2]. - Ensure texture dimensions (width and height) are even, as NV12 requires this [4]. - If direct updates via UpdateSubresource are necessary, verify behavior with the D3D debug layer enabled to catch potential alignment or parameter errors [4].
Citations:
- 1: https://stackoverflow.com/questions/71906965/why-is-chrominance-lost-when-i-copy-dxgi-format-nv12-id3d11texture-from-a-d3d11d
- 2: https://stackoverflow.com/questions/58093597/copy-nv12-to-id3d11texture2d-at-once
- 3: https://gitlab.winehq.org/wine/wine/-/commit/35ad787138aab14ee10067931cb87c1301dbc497
- 4: https://stackoverflow.com/questions/65738868/id3ddevicecreatetexture2d-fails-with-e-invalidarg-in-nv12-format-for-certain-t
🌐 Web query:
D3D11 ID3D11DeviceContext::UpdateSubresource SrcDepthPitch should be 0 for 2D textures
💡 Result:
For 2D textures in ID3D11DeviceContext::UpdateSubresource, the SrcDepthPitch parameter should be set to 0 [1]. In Direct3D 11, SrcDepthPitch represents the size of one depth slice of source data [1][2]. Because a 2D texture has no depth component (it consists of only a single slice), its depth pitch is effectively zero [1][3]. Official Microsoft documentation examples consistently demonstrate this usage. For instance, when updating a 2D texture region, the API call is structured as: pd3dDeviceContext->UpdateSubresource( pDestTexture, 0, &destRegion, pData, rowPitch, 0); [1] While the parameter is mandatory in the function signature, providing 0 is the correct approach for 2D resources to signify that there is no depth slice data to traverse [1]. Failure to use 0 could lead to incorrect memory offsets if the driver interprets a non-zero value as a valid slice stride.
Citations:
- 1: https://learn.microsoft.com/en-us/windows/win32/api/d3d11/nf-d3d11-id3d11devicecontext-updatesubresource
- 2: https://learn.microsoft.com/en-us/windows/win32/api/d3d11_1/nf-d3d11_1-id3d11devicecontext1-updatesubresource1
- 3: https://learn.microsoft.com/en-us/windows/win32/api/d3d11/ns-d3d11-d3d11_subresource_data
🌐 Web query:
NvEncRegisterResource pitch meaning D3D11 NV12
💡 Result:
In the context of the NVIDIA Video Codec SDK and the NV_ENC_REGISTER_RESOURCE structure, the pitch parameter specifies the stride (the number of bytes from the start of one row to the start of the next row) of the input buffer [1][2]. When registering DirectX (D3D11) resources, you should set this pitch parameter to 0 [2]. For DirectX textures, the NVIDIA encoder interface retrieves the necessary memory layout and pitch information directly from the D3D11 resource metadata [2]. The pitch parameter is primarily intended for other resource types [2]: 1. For NV_ENC_INPUT_RESOURCE_TYPE_CUDADEVICEPTR, you must provide the pitch, typically obtained via cuMemAllocPitch or the buffer width in bytes if the buffer is non-pitched [2]. 2. For other types like OpenGL textures, the pitch is calculated based on the texture width and format (number of components) [2]. When working with NV12 D3D11 textures, simply pass 0 for the pitch in the NV_ENC_REGISTER_RESOURCE structure [2]. The hardware and driver handle the NV12 surface layout (which includes both Y and UV planes) automatically based on the texture's DXGI_FORMAT_NV12 description [3]. If you are encountering registration errors, ensure that your D3D11 texture was created with the correct dimensions and format, and that it is bound with appropriate flags (e.g., D3D11_BIND_RENDER_TARGET) to be compatible with the encoder [4][5].
Citations:
- 1: https://patches.ffmpeg.org/doxygen/3.2/structNV__ENC__REGISTER__RESOURCE.html
- 2: https://bytedeco.org/javacpp-presets/nvcodec/apidocs/org/bytedeco/nvcodec/nvencodeapi/NV_ENC_REGISTER_RESOURCE.html
- 3: https://stackoverflow.com/questions/40443387/nvenc-nvidia-encoder-d3d11-wrong-image-dimension-and-other-questions
- 4: https://github.com/NVIDIA/video-sdk-samples/blob/master/Samples/NvCodec/NvEncoder/NvEncoderD3D11.cpp
- 5: https://github.com/NVIDIA/video-sdk-samples/blob/master/Samples/NvCodec/NvEncoder/NvEncoderD3D11.h
🌐 Web query:
NvEncPicParams input_pitch meaning NV12
💡 Result:
In the context of the NVIDIA Video Codec SDK and the NVENC API, inputPitch in the NV_ENC_PIC_PARAMS structure specifies the stride (also known as pitch) of the input buffer, measured in bytes [1][2][3]. The pitch represents the distance in memory between the start of one row of pixels and the start of the next row [4]. This value is essential for the encoder to correctly interpret the memory layout of the frame, especially when the buffer contains padding or alignment bytes at the end of each row [5]. Regarding NV12 specifically: 1. Meaning: For NV12, the inputPitch generally refers to the stride of the luma (Y) plane, which is typically equal to the width of the frame (or slightly larger due to memory alignment requirements) [5][4]. 2. Best Practice: If the pitch of your input buffer is not explicitly known or if the buffer is tightly packed without padding, you can set inputPitch to the inputWidth [1][2][3]. 3. Usage: When working with dedicated GPU memory surfaces (such as those allocated via the NVENC API), it is highly recommended to retrieve the correct pitch from the buffer or surface allocation metadata rather than assuming it equals the width [5]. If the inputPitch is incorrectly set, the encoder will misinterpret the memory layout, typically resulting in visual corruption, such as slanted or sheared images, because the hardware will begin reading each new row of pixels at the wrong memory offset [5].
Citations:
- 1: https://www.ffmpeg.org/doxygen/3.2/structNV__ENC__PIC__PARAMS.html
- 2: https://www.ffmpeg.org/doxygen/3.3/structNV__ENC__PIC__PARAMS.html
- 3: http://bytedeco.org/javacpp-presets/nvcodec/apidocs/org/bytedeco/nvcodec/nvencodeapi/NV_ENC_PIC_PARAMS.html
- 4: https://docs.nvidia.com/cuda/nvimagecodec/samples/c_samples.html
- 5: https://ffmpeg.org/pipermail/ffmpeg-cvslog/2016-September/101961.html
🌐 Web query:
NV_ENC_INPUT_RESOURCE_TYPE_DIRECTX chroma_offset chroma_offset_in pitch required
💡 Result:
When using NV_ENC_INPUT_RESOURCE_TYPE_DIRECTX in the NVIDIA Video Codec SDK (via the NV_ENC_REGISTER_RESOURCE structure), the requirements for pitch, chroma_offset, and chroma_offset_in are as follows: Pitch: For DirectX resources (NV_ENC_INPUT_RESOURCE_TYPE_DIRECTX), the pitch parameter should be set to 0 [1][2]. The hardware driver determines the pitch directly from the DirectX surface properties [2]. Chroma Offset / Chroma Offset In: These fields are generally used for non-DirectX resource types (e.g., CUDA device pointers) where manual specification of planar offsets is required [1]. - chromaOffset: This field is used as an output parameter for the reconstructed buffer when using the D3D11 interface and requesting a reconstruction buffer (NV_ENC_OUTPUT_RECON) [1]. - chromaOffsetIn: This field is intended for input buffers (NV_ENC_INPUT_IMAGE) when using the NVCUVID interface [1]. It is specifically required only when luma and chroma allocations are not continuous or when planes are padded [1]. In standard DirectX-based workflows, the NVENC API handles the surface memory layout internally, making manual chroma offsets unnecessary for the primary input resource [2]. Always ensure your NV_ENC_REGISTER_RESOURCE struct version is correctly set to the version provided by your specific SDK header [3][1].
Citations:
- 1: https://bytedeco.org/javacpp-presets/nvcodec/apidocs/org/bytedeco/nvcodec/nvencodeapi/NV_ENC_REGISTER_RESOURCE.html
- 2: https://github.com/NVIDIA/video-sdk-samples/blob/master/Samples/NvCodec/NvEncoder/NvEncoderD3D11.cpp
- 3: https://patches.ffmpeg.org/doxygen/3.2/structNV__ENC__REGISTER__RESOURCE.html
NVENC DIRECTX input pitch handling looks wrong (UpdateSubresource width-as-pitch likely isn’t the real bug).
upload_nv12passesSysMemPitch = width, andbgra_to_nv12produces a tightly packed NV12 buffer where the interleaved UV plane rows are alsowidthbytes—so the “UV plane needs a different pitch” premise doesn’t match this buffer layout.- The more concerning issue is
NvEncPicParams.input_pitch = self.config.widthfor aNV_ENC_INPUT_RESOURCE_TYPE_DIRECTXinput. DIRECTX workflows typically expect NVENC to derive pitch from the D3D11 texture/surface metadata (i.e., don’t rely on a manually supplied pitch). Setpic.input_pitchto 0 (or derive the pitch from the actual D3D11 surface/mapping if your target SDK/driver requires it).
🤖 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 `@apps/agent/src/windows/encode.rs` around lines 570 - 594, The NVENC DirectX
path is incorrectly supplying a manual pitch and making assumptions about NV12
layout: in upload_nv12 (and where bgra_to_nv12 produces a tightly-packed buffer)
do not force SysMemPitch/row pitch to self.config.width as a workaround; more
importantly, for NV_ENC_INPUT_RESOURCE_TYPE_DIRECTX set
NvEncPicParams.input_pitch to 0 so NVENC derives pitch from the D3D11
surface/texture metadata (or alternatively query the actual D3D11 surface pitch
if your driver/SDK requires it). Locate upload_nv12, the UpdateSubresource call
on input_texture, and the code that sets pic.input_pitch and change the pitch
value to 0 (or replace with a proper D3D11-derived pitch retrieval) and remove
any incorrect assumptions about UV-plane separate pitch when using
tightly-packed NV12 buffers.
| fn encode( | ||
| &mut self, | ||
| frame: &crate::windows::capture::CapturedFrame, | ||
| params: EncodeParams, | ||
| ) -> Result<EncodedFrame> { | ||
| validate_frame(frame)?; | ||
| self.requested_bitrate_kbps = params.bitrate_kbps; | ||
| self.recreate_if_needed()?; | ||
|
|
||
| if params.force_keyframe || self.pending_keyframe { | ||
| if let Ok(force_keyframe) = self.transform.cast::<IWMVideoForceKeyFrame>() { | ||
| unsafe { | ||
| let _ = force_keyframe.SetKeyFrame(); | ||
| } | ||
| } | ||
| self.pending_keyframe = false; | ||
| } | ||
|
|
||
| let nv12 = bgra_to_nv12(frame)?; | ||
| let duration_hns = HNS_PER_SECOND / self.config.fps as i64; | ||
| let sample_time = self.frame_index as i64 * duration_hns; | ||
| let input = sample_from_bytes(&nv12, sample_time, duration_hns)?; | ||
|
|
||
| unsafe { | ||
| self.transform | ||
| .ProcessInput(0, &input, 0) | ||
| .context("feed frame to Media Foundation H.264 MFT")?; | ||
| } | ||
|
|
||
| let mut output = self.drain_output()?; | ||
| self.frame_index += 1; | ||
|
|
||
| if output.is_empty() { | ||
| anyhow::bail!("Media Foundation H.264 MFT accepted input but produced no output"); | ||
| } | ||
|
|
||
| output = ensure_annex_b(&output); | ||
| let is_keyframe = annex_b_contains_idr(&output); | ||
|
|
||
| Ok(EncodedFrame { | ||
| data: output, | ||
| is_keyframe, | ||
| timestamp: (sample_time / 10) as u64, | ||
| backend: EncoderBackendKind::MediaFoundation, | ||
| }) | ||
| } |
There was a problem hiding this comment.
Media Foundation encoder may require multiple frames before producing output.
Lines 1593-1594 bail if the MFT produces no output after accepting input. Some H.264 encoder MFTs buffer multiple frames before producing output (e.g., for B-frames or lookahead). This could cause immediate failure on first frame. Consider allowing empty output initially and only bailing if the pattern persists.
Possible approach
Track frame count and only require output after a few frames, or configure the MFT to minimize internal buffering with CODECAPI_AVEncCommonRealTime.
🤖 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 `@apps/agent/src/windows/encode.rs` around lines 1561 - 1606, The encoder
currently bails in encode() when drain_output() returns empty, but some Media
Foundation H.264 MFTs buffer frames and produce no immediate output; update
encode() to tolerate empty output for the first few frames instead of immediate
failure: add a small buffering threshold (e.g., check self.frame_index or a new
counter like self.frames_queued) and only call anyhow::bail! if
output.is_empty() after that threshold; alternatively, set the MFT realtime flag
(CODECAPI_AVEncCommonRealTime) during recreate_if_needed() to reduce internal
buffering—use symbols encode, drain_output, recreate_if_needed, and
frame_index/self.pending_keyframe to locate where to implement the check or
property set.
| fn send_key(hid_usage: u16, down: bool) -> anyhow::Result<()> { | ||
| let Some(vk) = hid_to_vk(hid_usage) else { | ||
| tracing::debug!(hid_usage, "no Windows virtual-key mapping for HID usage"); | ||
| return Ok(()); | ||
| }; | ||
| let flags = if down { | ||
| KEYBD_EVENT_FLAGS(0) | ||
| } else { | ||
| KEYEVENTF_KEYUP | ||
| }; | ||
| send_input(&[INPUT { | ||
| r#type: INPUT_KEYBOARD, | ||
| Anonymous: INPUT_0 { | ||
| ki: KEYBDINPUT { | ||
| wVk: vk, | ||
| wScan: 0, | ||
| dwFlags: flags, | ||
| time: 0, | ||
| dwExtraInfo: 0, | ||
| }, | ||
| }, | ||
| }]) | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
Does Windows SendInput require KEYEVENTF_EXTENDEDKEY for arrow keys?
💡 Result:
Yes, the KEYEVENTF_EXTENDEDKEY flag is typically required when simulating arrow keys (and other extended keys) using SendInput [1][2]. Under the hood, Windows treats arrow keys as extended keys, which are distinguished from their non-extended counterparts (like Numpad keys) by a prefix byte (0xE0) in their scan code [3][4][5]. When using SendInput, the KEYEVENTF_EXTENDEDKEY flag tells the system to treat the scan code as one of these extended keys [3][6]. If this flag is omitted, the system may interpret the input incorrectly—for example, treating an Up Arrow press as a Numpad 8 press [5]. Key points regarding its usage: 1. When to use: The flag is required for keys considered extended, including the arrow keys (VK_UP, VK_DOWN, VK_LEFT, VK_RIGHT), as well as keys like Home, End, Page Up, Page Down, Insert, and Delete [1]. 2. Functionality: In the INPUT structure's KEYBDINPUT member, setting dwFlags to KEYEVENTF_EXTENDEDKEY ensures the correct scan code sequence is sent to the operating system [3][7]. 3. Reliability: Do not rely on MapVirtualKey to automatically return the correct extended flag for all scenarios, as it may return the non-extended version of a virtual key code [1][5]. It is recommended to explicitly set the flag when simulating these specific keys [1][7].
Citations:
- 1: https://stackoverflow.com/questions/47309759/simulated-shiftright-key-does-not-always-work-on-windows
- 2: https://github.com/learncodebygaming/pydirectinput/blob/master/pydirectinput/__init__.py
- 3: https://learn.microsoft.com/en-us/windows/win32/api/winuser/ns-winuser-keybdinput
- 4: https://learn.microsoft.com/en-us/windows/win32/api/winuser/nf-winuser-keybd_event
- 5: https://stackoverflow.com/questions/78944994/why-is-mapvirtualkey-not-returning-the-extended-key-flag
- 6: https://learn.microsoft.com/en-us/previous-versions/ms909643(v=msdn.10)
- 7: https://stackoverflow.com/questions/58489992/struggling-with-sendinput
🏁 Script executed:
#!/bin/bash
set -euo pipefail
file="apps/agent/src/windows/input.rs"
if [ ! -f "$file" ]; then
echo "Missing $file"
exit 1
fi
# Show relevant sections with line numbers
nl -ba "$file" | sed -n '1,240p'Repository: Anchras/remotekvm
Length of output: 105
🏁 Script executed:
#!/bin/bash
set -euo pipefail
file="apps/agent/src/windows/input.rs"
if [ ! -f "$file" ]; then
echo "Missing $file"
exit 1
fi
nl -ba "$file" | sed -n '1,260p'Repository: Anchras/remotekvm
Length of output: 105
🏁 Script executed:
#!/bin/bash
set -euo pipefail
file="apps/agent/src/windows/input.rs"
echo "=== $file exists? ==="
test -f "$file" && echo "yes" || { echo "no"; exit 1; }
echo "=== Relevant excerpt (1-260) ==="
cat -n "$file" | sed -n '1,260p'Repository: Anchras/remotekvm
Length of output: 9312
🏁 Script executed:
#!/bin/bash
set -euo pipefail
rg -n "VIRTUAL_KEY\(|\.0\W*[,)]|KEYEVENTF_EXTENDEDKEY" -S .Repository: Anchras/remotekvm
Length of output: 9299
🏁 Script executed:
#!/bin/bash
set -euo pipefail
file="apps/agent/src/windows/input.rs"
echo "=== KEYEVENTF_EXTENDEDKEY occurrences in file ==="
rg -n "KEYEVENTF_EXTENDEDKEY" "$file" || true
echo "=== Any extended-key helpers in file ==="
rg -n "extended|is_extended|EXTENDEDKEY|E0" "$file" || true
echo "=== VIRTUAL_KEY field access (.0) occurrences in file ==="
rg -n "VIRTUAL_KEY\\([^\\)]*\\)|\\.0" "$file"Repository: Anchras/remotekvm
Length of output: 897
🌐 Web query:
windows-rs VIRTUAL_KEY tuple struct .0 field definition
💡 Result:
In the windows-rs crate, the VIRTUAL_KEY struct is defined as a tuple struct that wraps a 16-bit unsigned integer [1]. Its definition is: #[repr(transparent)] pub struct VIRTUAL_KEY(pub u16); The.0 field represents the underlying raw virtual-key code [1][2]. Because the struct is marked with #[repr(transparent)], it is guaranteed to have the same memory layout as the inner u16, allowing it to be used efficiently in FFI (Foreign Function Interface) calls when interacting with Windows APIs that expect a virtual-key code [1]. You can access this value directly using the.0 field notation (e.g., my_vkey.0) [2]. Note that while VIRTUAL_KEY uses a u16, some other types in the windows-rs crate, such as the windows::System::VirtualKey enum, may use different underlying types like i32 [3].
Citations:
- 1: https://microsoft.github.io/windows-docs-rs/doc/windows/Win32/UI/Input/KeyboardAndMouse/struct.VIRTUAL_KEY.html
- 2: tauri-apps/tao@93c256f
- 3: https://microsoft.github.io/windows-docs-rs/doc/windows/System/struct.VirtualKey.html
🏁 Script executed:
#!/bin/bash
set -euo pipefail
rg -n "KEYEVENTF_EXTENDEDKEY" -S .Repository: Anchras/remotekvm
Length of output: 43
Add KEYEVENTF_EXTENDEDKEY for arrow/extended keys in send_key
apps/agent/src/windows/input.rs maps HID arrow usages to VK_LEFT/VK_RIGHT/VK_UP/VK_DOWN, but send_key currently only sets KEYEVENTF_KEYUP vs down and never sets KEYEVENTF_EXTENDEDKEY. When simulating extended keys via SendInput, omitting this flag can cause incorrect interpretation in some apps (e.g., confusion with non-extended/numpad variants).
🔧 Suggested fix
fn send_key(hid_usage: u16, down: bool) -> anyhow::Result<()> {
let Some(vk) = hid_to_vk(hid_usage) else {
tracing::debug!(hid_usage, "no Windows virtual-key mapping for HID usage");
return Ok(());
};
- let flags = if down {
- KEYBD_EVENT_FLAGS(0)
+ let extended = is_extended_key(vk);
+ let mut flags = if down {
+ KEYBD_EVENT_FLAGS(0)
} else {
KEYEVENTF_KEYUP
};
+ if extended {
+ flags |= KEYEVENTF_EXTENDEDKEY;
+ }
// ...
}
+fn is_extended_key(vk: VIRTUAL_KEY) -> bool {
+ matches!(vk.0, 0x21..=0x28 | 0x2D..=0x2E | 0x5B..=0x5C) // PgUp/Dn, End, Home, Arrows, Ins, Del, Win keys
+}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| fn send_key(hid_usage: u16, down: bool) -> anyhow::Result<()> { | |
| let Some(vk) = hid_to_vk(hid_usage) else { | |
| tracing::debug!(hid_usage, "no Windows virtual-key mapping for HID usage"); | |
| return Ok(()); | |
| }; | |
| let flags = if down { | |
| KEYBD_EVENT_FLAGS(0) | |
| } else { | |
| KEYEVENTF_KEYUP | |
| }; | |
| send_input(&[INPUT { | |
| r#type: INPUT_KEYBOARD, | |
| Anonymous: INPUT_0 { | |
| ki: KEYBDINPUT { | |
| wVk: vk, | |
| wScan: 0, | |
| dwFlags: flags, | |
| time: 0, | |
| dwExtraInfo: 0, | |
| }, | |
| }, | |
| }]) | |
| } | |
| fn send_key(hid_usage: u16, down: bool) -> anyhow::Result<()> { | |
| let Some(vk) = hid_to_vk(hid_usage) else { | |
| tracing::debug!(hid_usage, "no Windows virtual-key mapping for HID usage"); | |
| return Ok(()); | |
| }; | |
| let extended = is_extended_key(vk); | |
| let mut flags = if down { | |
| KEYBD_EVENT_FLAGS(0) | |
| } else { | |
| KEYEVENTF_KEYUP | |
| }; | |
| if extended { | |
| flags |= KEYEVENTF_EXTENDEDKEY; | |
| } | |
| send_input(&[INPUT { | |
| r#type: INPUT_KEYBOARD, | |
| Anonymous: INPUT_0 { | |
| ki: KEYBDINPUT { | |
| wVk: vk, | |
| wScan: 0, | |
| dwFlags: flags, | |
| time: 0, | |
| dwExtraInfo: 0, | |
| }, | |
| }, | |
| }]) | |
| } | |
| fn is_extended_key(vk: VIRTUAL_KEY) -> bool { | |
| matches!(vk.0, 0x21..=0x28 | 0x2D..=0x2E | 0x5B..=0x5C) // PgUp/Dn, End, Home, Arrows, Ins, Del, Win keys | |
| } |
🤖 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 `@apps/agent/src/windows/input.rs` around lines 92 - 114, send_key currently
only toggles KEYEVENTF_KEYUP and never sets KEYEVENTF_EXTENDEDKEY, causing
arrow/extended keys simulated via SendInput to be misinterpreted; update
send_key to OR KEYEVENTF_EXTENDEDKEY into flags when the resolved virtual key is
an extended key (e.g., VK_LEFT, VK_RIGHT, VK_UP, VK_DOWN or whenever hid_to_vk
indicates an extended HID usage). Locate send_key and use the vk from hid_to_vk
to conditionally set flags = flags | KEYEVENTF_EXTENDEDKEY for extended keys
(while preserving KEYEVENTF_KEYUP for key release), ensuring the INPUT /
KEYBDINPUT payload includes the combined flags so SendInput gets the correct
extended-key semantics.
| INSERT INTO organization_members (organization_id, user_id, role) | ||
| SELECT organization_id, $1, role | ||
| FROM accepted | ||
| ON CONFLICT (organization_id, user_id) DO UPDATE SET | ||
| role = EXCLUDED.role |
There was a problem hiding this comment.
Accepting an invite may downgrade existing membership role.
The ON CONFLICT ... DO UPDATE SET role = EXCLUDED.role will overwrite an existing membership's role with the invite's role. If a user is already an admin and accepts a member invite (perhaps sent before their promotion), they'll be downgraded.
Consider preserving the higher-privilege role or skipping the update:
Option: Skip update if already a member
ON CONFLICT (organization_id, user_id) DO UPDATE SET
- role = EXCLUDED.role
+ role = CASE
+ WHEN organization_members.role = 'owner' THEN organization_members.role
+ WHEN organization_members.role = 'admin' AND EXCLUDED.role = 'member' THEN organization_members.role
+ ELSE EXCLUDED.role
+ ENDAlternatively, DO NOTHING if existing membership should always take precedence.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| INSERT INTO organization_members (organization_id, user_id, role) | |
| SELECT organization_id, $1, role | |
| FROM accepted | |
| ON CONFLICT (organization_id, user_id) DO UPDATE SET | |
| role = EXCLUDED.role | |
| INSERT INTO organization_members (organization_id, user_id, role) | |
| SELECT organization_id, $1, role | |
| FROM accepted | |
| ON CONFLICT (organization_id, user_id) DO UPDATE SET | |
| role = CASE | |
| WHEN organization_members.role = 'owner' THEN organization_members.role | |
| WHEN organization_members.role = 'admin' AND EXCLUDED.role = 'member' THEN organization_members.role | |
| ELSE EXCLUDED.role | |
| END |
🤖 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 `@apps/server/src/auth/routes.rs` around lines 349 - 353, The INSERT into
organization_members uses ON CONFLICT (organization_id, user_id) DO UPDATE SET
role = EXCLUDED.role which can downgrade an existing higher-privilege role;
change the conflict handling in this statement (affecting organization_members,
role, EXCLUDED.role, and the SELECT from accepted) to preserve the
higher-privilege role or skip updates—for example replace the DO UPDATE to
conditionally set role = GREATEST(existing.role, EXCLUDED.role) using an
explicit comparison or use DO NOTHING so existing memberships are kept; ensure
the conflict clause references organization_id and user_id as currently written
and adjust the UPDATE expression to prefer the higher privilege instead of
unconditionally writing EXCLUDED.role.
| .or_else(|| { | ||
| req.headers() | ||
| .get(header::USER_AGENT) | ||
| .and_then(|value| value.to_str().ok()) | ||
| .map(|value| format!("ua:{value}")) | ||
| }) | ||
| .unwrap_or_else(|| "anonymous".to_string()); |
There was a problem hiding this comment.
User-Agent fallback for rate limiting is easily bypassed.
Falling back to User-Agent when IP headers are missing allows attackers to bypass rate limits by varying the User-Agent header. The "anonymous" fallback then shares a single bucket for all such requests, which could either be too restrictive for legitimate users or too permissive if the bucket is sized for distributed traffic.
Consider using only IP-based keys, or if behind a trusted proxy, requiring x-forwarded-for / x-real-ip to be present (returning 400 if missing in production).
🤖 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 `@apps/server/src/lib.rs` around lines 127 - 133, The current rate-limit key
fallback chain that reads req.headers().get(header::USER_AGENT) and then
unwrap_or_else("anonymous") is insecure; remove the User-Agent fallback and
instead require a client IP-derived key (from x-forwarded-for or x-real-ip)
only: if a trusted-proxy mode is enabled, extract the IP from those headers and
return a 400/Err when missing, otherwise use the remote IP; eliminate the
anonymous bucket/unwrap_or_else usage and ensure the rate-limit key generation
logic treats missing IPs as an error rather than falling back to User-Agent or
"anonymous".
| <key>EnvironmentVariables</key> | ||
| <dict> | ||
| <key>RKVM_REGISTRATION_TOKEN</key> | ||
| <string>__REGISTRATION_TOKEN__</string> | ||
| <key>RUST_LOG</key> | ||
| <string>info,remotekvm_agent=debug</string> | ||
| </dict> |
There was a problem hiding this comment.
Registration token exposure via environment variable.
The RKVM_REGISTRATION_TOKEN is visible to all processes via ps e or launchctl list io.remotekvm.agent. If this token grants machine registration or authorization, exposure in the process environment creates a privilege escalation risk.
Consider passing the token via a restricted-permission file (e.g., ~/Library/Application Support/RemoteKVM/token) and having the agent read it at startup, or using macOS Keychain for credential storage.
🤖 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 `@packaging/macos/io.remotekvm.agent.plist.template` around lines 24 - 30,
Remove the RKVM_REGISTRATION_TOKEN EnvironmentVariables entry from the
io.remotekvm.agent.plist.template and stop passing the secret on the process
environment; instead implement a secure retrieval path in the agent (add a
loadRegistrationToken() function and call it from main()/Agent::start()) that
reads the token from a file with restricted permissions or from the macOS
Keychain, validate and handle errors (log without printing the token), and
ensure the token file is created with owner-only permissions so it is not
exposed via ps/launchctl.
| <key>KeepAlive</key> | ||
| <dict> | ||
| <key>SuccessfulExit</key> | ||
| <false/> | ||
| </dict> |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Description: Check agent main.rs for exit codes and expected lifecycle behavior
# Search for exit calls and return codes in agent main
rg -n -C 5 'std::process::exit|exit\(|return\s+Ok\(\)|return\s+Err\(' apps/agent/src/main.rsRepository: Anchras/remotekvm
Length of output: 43
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# 1) Confirm plist KeepAlive/SuccessfulExit settings in the template
echo "== Plist template KeepAlive settings =="
rg -n --hidden --no-ignore -S "<key>KeepAlive</key>|SuccessfulExit|RunAtLoad" packaging/macos/io.remotekvm.agent.plist.template
# 2) Find the actual Rust entrypoint(s) for the agent
echo
echo "== Locate candidate agent main.rs files =="
fd -t f -i 'main.rs' apps packaging src || true
# 3) Search for explicit exits / returns in likely agent entrypoints
echo
echo "== Search for process::exit and other exit paths =="
rg -n -C 5 --hidden --no-ignore -S "std::process::exit|process::exit|\\bexit\\(|return\\s+Ok\\(|return\\s+Err\\(" apps packaging src || true
# 4) Narrow search to anything that mentions "agent" lifecycle or run loop
echo
echo "== Search for run loop / signal handling indicators =="
rg -n -S --hidden --no-ignore -i "signal|SIGTERM|Shutdown|ctrlc|ctrl-c|daemon|spawn|loop\\s*\\{|tokio::select!" apps || trueRepository: Anchras/remotekvm
Length of output: 50485
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# 1) Show exact SuccessfulExit value in the plist template
echo "== Plist template SuccessfulExit value =="
rg -n "<key>SuccessfulExit</key>|<false/|<true/" packaging/macos/io.remotekvm.agent.plist.template
echo
# 2) Inspect agent entrypoint main.rs (size first)
echo "== apps/agent/src/main.rs size =="
wc -l apps/agent/src/main.rs
echo
# 3) Print the whole main.rs if small; otherwise key regions around main()
echo "== apps/agent/src/main.rs excerpt =="
if [ "$(wc -l < apps/agent/src/main.rs)" -le 220 ]; then
cat -n apps/agent/src/main.rs
else
# show around main() and any early return/exit-y patterns
rg -n --no-heading "\bfn main\b|tokio::main|async fn main|return Ok\(|return Err\(|std::process::exit" apps/agent/src/main.rs
# print surrounding regions near those matches
for line in $(rg -n --no-heading "\bfn main\b|tokio::main|async fn main|return Ok\(|return Err\(|std::process::exit" apps/agent/src/main.rs | cut -d: -f1 | head -n 20); do
start=$((line-40)); [ "$start" -lt 1 ] && start=1
end=$((line+80))
sed -n "${start},${end}p" apps/agent/src/main.rs | nl -ba -w2 -s: -ba
echo "----"
done
fi
echo
# 4) Search the agent crate for shutdown/signal handlers
echo "== apps/agent shutdown/signal handling patterns =="
rg -n -S --hidden --no-ignore -i "ctrlc|SIGTERM|SIGINT|signal::|shutdown|graceful shutdown" apps/agent/src || true
echo
# 5) Specifically look for patterns that indicate main can end on clean exit
echo "== apps/agent main/agent-runner returns =="
rg -n -S "return Ok\(\(\)|return Ok\(|return Err\(|\\bOk\\(\\(\\)\\)\\b|\\bErr\\(" apps/agent/src/main.rs apps/agent/src/macos/mod.rs apps/agent/src/macos/*.rs 2>/dev/null || trueRepository: Anchras/remotekvm
Length of output: 380
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== Plist template SuccessfulExit value =="
rg -n "<key>SuccessfulExit</key>|<false/|<true/" packaging/macos/io.remotekvm.agent.plist.template
echo
echo "== Show main() region in apps/agent/src/main.rs =="
rg -n --no-heading "\bfn main\b|tokio::main|async fn main" apps/agent/src/main.rs
main_line=$(rg -n --no-heading "\bfn main\b|tokio::main|async fn main" apps/agent/src/main.rs | head -n1 | cut -d: -f1)
start=$((main_line-80)); [ "$start" -lt 1 ] && start=1
end=$((main_line+140))
sed -n "${start},${end}p" apps/agent/src/main.rs | cat -n -ba -w2 -s': '
echo
echo "== Search for explicit process-exit or return that ends main =="
rg -n --no-heading "std::process::exit|process::exit" apps/agent/src/main.rs apps/agent/src/macos/mod.rs apps/agent/src/main.rs || true
rg -n --no-heading "return Ok\(|return Err\(|Ok\(\(\)\)|Err\(|return; " apps/agent/src/main.rs || true
echo
echo "== Search agent crate for shutdown/signal handling / run loop termination =="
rg -n -S --hidden --no-ignore -i "ctrlc|SIGTERM|SIGINT|signal::|shutdown|graceful shutdown" apps/agent/src || true
echo
echo "== Search for top-level run loop functions in agent crate =="
rg -n -S --hidden --no-ignore "run\\(|start\\(|serve\\(|serve\\_|main\\s*\\(" apps/agent/src/main.rs apps/agent/src/macos/mod.rs apps/agent/src -g"*.rs" | head -n 50 || trueRepository: Anchras/remotekvm
Length of output: 354
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== apps/agent/src/main.rs (full) =="
cat -n apps/agent/src/main.rs
echo
echo "== Show where main() calls into runner/run loop =="
rg -n "main\\(\\).*|\\.await\\b|\\.run\\b|run\\b|serve\\b|start\\b" apps/agent/src/main.rs
echo
echo "== Inspect runner entrypoints used by main =="
# Print candidate functions referenced from main.rs
rg -n "^[[:space:]]*(let|tokio::spawn|spawn|[a-zA-Z0-9_]+::)?[a-zA-Z0-9_]+\\s*=.*|[a-zA-Z0-9_]+::(run|serve|start)|\\.(run|serve|start)\\(" apps/agent/src/main.rs -o --no-filename || true
echo
echo "== apps/agent shutdown/signal handling =="
rg -n -S --hidden --no-ignore -i "ctrlc|SIGTERM|SIGINT|signal::|shutdown|graceful shutdown" apps/agent/src || trueRepository: Anchras/remotekvm
Length of output: 13607
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== apps/agent/src/server_client.rs =="
wc -l apps/agent/src/server_client.rs
sed -n '1,220p' apps/agent/src/server_client.rs | cat -n -ba -s': '
echo
echo "== Find how requests.recv() becomes None / closes =="
rg -n "recv\\(|requests|mpsc|channel|None|closed|disconnect|websocket.*close|close_reason" apps/agent/src/server_client.rs apps/agent/src/signaling.rs apps/agent/src/main.rsRepository: Anchras/remotekvm
Length of output: 219
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== apps/agent/src/server_client.rs (full) =="
cat -n apps/agent/src/server_client.rs
echo
echo "== Search for where requests channel/stream is closed/None is returned =="
rg -n "recv\\(|requests\\b|mpsc|channel|None\\b|close\\b|disconnect|websocket|ConnectionClosed|Closed" apps/agent/src/server_client.rs
echo
echo "== Also inspect signaling ConnectionRequest handling =="
rg -n "struct ConnectionRequest|enum.*ConnectionRequest|ConnectionRequest" apps/agent/src/signaling.rsRepository: Anchras/remotekvm
Length of output: 7768
🌐 Web query:
launchd KeepAlive SuccessfulExit false restart on normal exit semantics
💡 Result:
In launchd.plist(5), the KeepAlive dictionary allows you to define specific conditions under which a job should be restarted [1][2]. When SuccessfulExit is set to false, it defines a conditional restart behavior based on the job's termination status [1][3]. Specifically, the semantics of KeepAlive SuccessfulExit false are: 1. Restart on Failure: The job will be restarted if it exits with a non-zero exit status (i.e., an error or failure) [1][4]. 2. Stop on Success: The job will not be restarted if it exits with a zero exit status (i.e., a successful or normal exit) [1][4]. This configuration is frequently used to ensure that a background service or daemon automatically recovers from crashes or unexpected failures while allowing it to shut down gracefully when a user or the system explicitly stops it with a successful exit code [4]. Important Implementation Notes: - Polarity: Boolean values in the KeepAlive dictionary act as filters for conditions, not as simple "enable/disable" switches [5]. Setting a key to false often inverts the expected condition (e.g., "restart when NOT successful") [1][5]. - Implicit RunAtLoad: Using the KeepAlive key implies RunAtLoad is set to true, because launchd must run the job at least once to observe its initial exit status and evaluate the KeepAlive conditions [1][3]. - Multiple Conditions: If you define multiple keys within the KeepAlive dictionary (e.g., combining SuccessfulExit with other conditions like Crashed or PathState), launchd evaluates them using an OR operation [1][2]. This means if any condition evaluates to true, the job will be restarted [1][2]. Use caution, as unintended combinations can lead to infinite restart loops if one condition is always true [5].
Citations:
- 1: https://www.manpagez.com/man/5/launchd.plist/
- 2: https://keith.github.io/xcode-man-pages/launchd.plist.5.html
- 3: https://leancrew.com/all-this/man/man5/launchd.plist.html
- 4: https://github.com/tjluoma/launchd-keepalive/blob/master/README.md
- 5: macOS: launchd home-manager agent loops endlessly due to
Crashed = falsein KeepAlive dict ryantm/agenix#372
🏁 Script executed:
#!/bin/bash
set -euo pipefail
sed -n '20,50p' packaging/macos/io.remotekvm.agent.plist.template | cat -n -w2 -s': 'Repository: Anchras/remotekvm
Length of output: 127
🏁 Script executed:
#!/bin/bash
set -euo pipefail
sed -n '20,50p' packaging/macos/io.remotekvm.agent.plist.template | cat -n -baRepository: Anchras/remotekvm
Length of output: 127
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== packaging/macos/io.remotekvm.agent.plist.template KeepAlive block =="
sed -n '25,45p' packaging/macos/io.remotekvm.agent.plist.template | awk '{print NR+24 ":" $0}'Repository: Anchras/remotekvm
Length of output: 663
Fix launchd KeepAlive semantics vs agent shutdown paths.
packaging/macos/io.remotekvm.agent.plist.template sets KeepAlive -> SuccessfulExit=false (launchd won’t restart the agent when it exits with code 0). In apps/agent/src/main.rs, the agent exits with Ok(()) when the server WebSocket closes (requests.recv() returns None, “server connection closed”) and when Ctrl+C triggers shutdown—so those are “successful” exits and won’t be restarted.
Change SuccessfulExit and/or make unexpected disconnects cause a non-zero exit / reconnect instead of exiting cleanly.
🤖 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 `@packaging/macos/io.remotekvm.agent.plist.template` around lines 34 - 38, The
plist's KeepAlive semantics currently treat clean exits as successful so launchd
won't restart the agent; update behavior so unexpected server disconnects and
similar failure paths return a non-zero exit (or change the plist to remove/flip
SuccessfulExit). In practice, modify the agent exit paths in
apps/agent/src/main.rs: where you handle requests.recv() == None (the "server
connection closed" branch) and the Ctrl+C/shutdown handler (main or run loop),
return an Err or call std::process::exit with a non-zero code instead of Ok(())
for unexpected disconnects so launchd will restart the agent, or alternatively
update packaging/macos/io.remotekvm.agent.plist.template to set
KeepAlive/SuccessfulExit to true/omit it so clean exits are retried—pick one
approach and ensure the code paths reference the requests.recv() disconnect
handling and the main shutdown logic.
| <key>StandardOutPath</key> | ||
| <string>/tmp/remotekvm-agent.out.log</string> | ||
| <key>StandardErrorPath</key> | ||
| <string>/tmp/remotekvm-agent.err.log</string> |
There was a problem hiding this comment.
Log files in world-readable /tmp directory.
Log files in /tmp are world-readable and ephemeral (cleared on reboot). If logs contain session data, error details, or debugging information, this creates an information disclosure risk. Additionally, losing logs on reboot hampers troubleshooting.
Relocate logs to a user-specific directory such as ~/Library/Logs/RemoteKVM/agent.log with restrictive permissions.
🤖 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 `@packaging/macos/io.remotekvm.agent.plist.template` around lines 40 - 43, The
plist currently writes logs to world-readable /tmp via the StandardOutPath and
StandardErrorPath keys; change those keys to point to a user-specific log
location (e.g. ${HOME}/Library/Logs/RemoteKVM/agent.log and
${HOME}/Library/Logs/RemoteKVM/agent.err.log) in the template and ensure the
installer or launch setup replaces ${HOME} with the target user's home and
creates the directory RemoteKVM with restrictive permissions (dir 0700, files
0600) before starting the agent so logs are not world-readable and survive
reboots.
Add two macos_v0 runtime tests that exercise the never-executed macOS video path on actual hardware: - encode.rs: feed a synthetic NV12 CVPixelBuffer through the real VideoToolbox H.264 encoder; assert non-empty Annex-B NALs + a keyframe. Permission-free. (Enables objc2-core-video CVBuffer/CVReturn features so a CVPixelBuffer can be constructed in-test.) - capture.rs: drive live ScreenCaptureKit capture -> VideoToolbox -> Annex-B; skips gracefully if Screen Recording permission is not granted (TCC -3801), otherwise asserts real H.264 frames. Verified locally on macOS: synthetic encode = 5 frames / 14.5 KB / 5 keyframes; live capture = 5 frames / 73.7 KB / 5 keyframes. Both runs report every frame as a keyframe — empirical confirmation of the is_sync_sample stub bug (ADA-453).
There was a problem hiding this comment.
🧹 Nitpick comments (2)
apps/agent/src/macos/encode.rs (2)
245-249: 💤 Low valueConsider adding a trait bound or safety comment to
set_property.This generic function casts any
TtoCFTypewithout compile-time enforcement thatTis actually a Core Foundation type. Currently safe because it's private and only called withCFBoolean,CFNumber, andCFString, but a future refactor could inadvertently pass an incompatible type.A brief
// SAFETY: caller ensures T is a CFTypecomment would document this invariant.🤖 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 `@apps/agent/src/macos/encode.rs` around lines 245 - 249, The generic function set_property unsafely casts any T to CFType which lacks compile-time enforcement; either add a trait bound (e.g., require T: AsCFType or a new unsafe trait marker like CFTypeable) to ensure only Core Foundation types are allowed, or add a SAFETY comment above set_property documenting the invariant ("SAFETY: caller ensures T is a CFType such as CFBoolean, CFNumber, CFString") and keep the function private; reference set_property, CFType, CFBoolean, CFNumber, and CFString when making the change so future callers follow the constraint.
370-374: 💤 Low valueKeyframe assertion is tautological given
is_sync_samplestub.Since
is_sync_sample()unconditionally returnstrue,keyframeswill always equalframes, makingkeyframes > 0redundant withframes > 0on line 371. The test validates Annex-B output but doesn't actually verify keyframe detection.Consider adding a comment acknowledging this limitation, or changing the assertion to document the stub behavior:
- assert!(keyframes > 0, "expected at least one keyframe (IDR)"); + // NOTE: is_sync_sample is stubbed (always true), so keyframes == frames. + // This confirms the stub behavior; real keyframe detection requires + // CFDictionary attachment lookup (see is_sync_sample comment). + assert_eq!(keyframes, frames, "stub: every frame reported as keyframe");🤖 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 `@apps/agent/src/macos/encode.rs` around lines 370 - 374, The keyframe assertion is redundant because is_sync_sample() currently always returns true, so keyframes will always equal frames; update the test to reflect this by either (A) replacing the assert!(keyframes > 0, ...) with an explicit assert_eq!(keyframes, frames, "is_sync_sample stub returns true so every frame is treated as a keyframe") to document the stub behavior, or (B) add a clear comment near the is_sync_sample() stub and above the assertions noting that keyframe detection is not implemented and therefore the second assertion is tautological; locate symbols is_sync_sample, keyframes, and frames in the encode.rs test and apply one of these changes to make the intent explicit.
🤖 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 `@apps/agent/src/macos/encode.rs`:
- Around line 245-249: The generic function set_property unsafely casts any T to
CFType which lacks compile-time enforcement; either add a trait bound (e.g.,
require T: AsCFType or a new unsafe trait marker like CFTypeable) to ensure only
Core Foundation types are allowed, or add a SAFETY comment above set_property
documenting the invariant ("SAFETY: caller ensures T is a CFType such as
CFBoolean, CFNumber, CFString") and keep the function private; reference
set_property, CFType, CFBoolean, CFNumber, and CFString when making the change
so future callers follow the constraint.
- Around line 370-374: The keyframe assertion is redundant because
is_sync_sample() currently always returns true, so keyframes will always equal
frames; update the test to reflect this by either (A) replacing the
assert!(keyframes > 0, ...) with an explicit assert_eq!(keyframes, frames,
"is_sync_sample stub returns true so every frame is treated as a keyframe") to
document the stub behavior, or (B) add a clear comment near the is_sync_sample()
stub and above the assertions noting that keyframe detection is not implemented
and therefore the second assertion is tautological; locate symbols
is_sync_sample, keyframes, and frames in the encode.rs test and apply one of
these changes to make the intent explicit.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 016d4df9-5762-4afc-9176-dcf45b8f57ba
📒 Files selected for processing (3)
apps/agent/Cargo.tomlapps/agent/src/macos/capture.rsapps/agent/src/macos/encode.rs
🚧 Files skipped from review as they are similar to previous changes (1)
- apps/agent/src/macos/capture.rs
main (PR #13) and this branch were independent takes on the backlog with no common ancestor, so every overlapping file conflicted. Resolved as the union of both: main's refinements are taken as the base (DB-probing health check, billing webhook org-admin recheck, recvonly video transceiver, real is_sync_sample keyframe detection, trimmed objc2-core-video features), and this branch's review hardening that main lacked is layered back on top: - websocket.rs: per-tenant session_belongs_to_machine guard on relayed agent SignalingAnswer/ICE (+ its integration test). - client/render.rs: odd-width NV12 chroma stride (2·⌈w/2⌉) fix + regression tests. - client/media.rs: 16 MiB in-flight H.264 access-unit cap and CVPixelBuffer stride guards (kept `% != 0` over is_multiple_of to stay MSRV 1.78). - agent/macos/capture.rs: real-hardware capture+encode test. - clippy.toml: msrv = "1.78". Co-authored-by: linear-code[bot] <222613912+linear-code[bot]@users.noreply.github.com>
There was a problem hiding this comment.
🧹 Nitpick comments (2)
apps/server/tests/integration.rs (2)
1741-1746: 🔒 Security & Privacy | 🔵 Trivial | ⚡ Quick winAssert the rejected member webhook leaves subscription state unchanged.
Line 1741 only checks the plan remains
free; a regression could still persiststripe_subscription_id = 'sub_member'and pass this test. Query both fields before the admin webhook.Proposed test tightening
- let org_plan: String = sqlx::query_scalar("SELECT plan FROM organizations WHERE id = $1") - .bind(org) - .fetch_one(&pool) - .await - .unwrap(); + let (org_plan, subscription_id): (String, Option<String>) = + sqlx::query_as("SELECT plan, stripe_subscription_id FROM organizations WHERE id = $1") + .bind(org) + .fetch_one(&pool) + .await + .unwrap(); assert_eq!(org_plan, "free"); + assert_eq!(subscription_id, None);🤖 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 `@apps/server/tests/integration.rs` around lines 1741 - 1746, The webhook rejection test currently only verifies the organization plan stays free, so it can miss a stale Stripe subscription record. Update the integration test around the org subscription assertions to query both plan and stripe_subscription_id before and after the admin/member webhook flow, using the existing org/org_plan checks in the test, and assert that both fields remain unchanged when the member webhook is rejected.
1055-1055: 🔒 Security & Privacy | 🔵 Trivial | ⚡ Quick winCover forged ICE candidates in the isolation regression.
The new test exercises a foreign agent forging
SignalingAnswer, but the same tenant boundary also protects agent-originatedIceCandidatemessages. Add a forged candidate attempt from the non-owner agent and assert the client receives nothing so both guarded relay paths stay covered.🤖 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 `@apps/server/tests/integration.rs` at line 1055, The isolation regression test currently covers forged SignalingAnswer handling but not agent-originated IceCandidate relay. Update the existing sqlx::test in the integration test to also send a forged IceCandidate from a non-owner agent using the same tenant boundary setup, then assert the client receives no message. Keep the new assertion alongside the existing forged-answer checks so both guarded relay paths are covered in the same test.
🤖 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 `@apps/server/tests/integration.rs`:
- Around line 1741-1746: The webhook rejection test currently only verifies the
organization plan stays free, so it can miss a stale Stripe subscription record.
Update the integration test around the org subscription assertions to query both
plan and stripe_subscription_id before and after the admin/member webhook flow,
using the existing org/org_plan checks in the test, and assert that both fields
remain unchanged when the member webhook is rejected.
- Line 1055: The isolation regression test currently covers forged
SignalingAnswer handling but not agent-originated IceCandidate relay. Update the
existing sqlx::test in the integration test to also send a forged IceCandidate
from a non-owner agent using the same tenant boundary setup, then assert the
client receives no message. Keep the new assertion alongside the existing
forged-answer checks so both guarded relay paths are covered in the same test.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: bb4c566a-3e12-43a2-ac6b-7927c244cccd
📒 Files selected for processing (1)
apps/server/tests/integration.rs
1 participant
Summary
Lands the RemoteKVM backlog-completion work (Phases 0–4 feature set) onto
main, plus review-driven hardening. This consolidates the previously-unmergedcodex/remotekvm-backlog-completionbranch and adds fixes from a multi-reviewer pass.What this lands
media.rs) — WebRTC track receive, H.264 RTP depacketize/assemble, hardware decode (VideoToolbox / Media Foundation), YUV→RGB render, Opus/cpal audio playback; keyring token storage;remotekvm://protocol-handler packaging.TrackLocalStaticSample(behind--features macos_v0); control-channel SetBitrate/RequestKeyframe; Windows NVENC + Media Foundation encoders, WASAPI audio, service controller (cfg-gated).Review hardening (this PR's top commit)
2·⌈width/2⌉, notwidth; the bare-width stride read one byte past the plane and panicked on legitimate odd-width frames. Fixed + regression tests.SignalingAnswer/ICE and inflate usage counters for sessions owned by other machines (inject SDP into another tenant's negotiation). Now binds every relayed agent message to a session owned by that machine (session_belongs_to_machine) + integration test.msrv = "1.78"to matchworkspace.rust-version; cleared-D warnings.Verification (macOS, this environment)
cargo build --workspace✅ ·cargo build -p remotekvm-agent --features macos_v0✅ ·cargo build -p remotekvm-server✅cargo clippy … -- -D warnings✅ clean (incl. agentmacos_v0)DATABASE_URL=… cargo test -p remotekvm-serverin CI.Known gaps / not verifiable here (follow-up Linear issues filed)
cfg-gated and cannot be compiled/tested on macOS — including a likely critical NVENC NV12 UV-plane upload bug (chroma plane may be left uninitialized). Needs Windows + GPU.is_sync_sampleis a stub that marks every frame a keyframe (SPS/PPS on every frame); VideoToolbox decode-callback aliasing; Media Foundationdrain_outputINCOMPLETE-flag logic. Need live hardware to validate.Relates to ADA-317, ADA-320, ADA-321, ADA-331 (video pipeline).
Summary by CodeRabbit
New Features
Bug Fixes