refactor: centralize safe command execution and native file ops#17
Open
mjc wants to merge 16 commits into
Open
refactor: centralize safe command execution and native file ops#17mjc wants to merge 16 commits into
mjc wants to merge 16 commits into
Conversation
📝 WalkthroughWalkthroughThis PR refactors command execution across the NanoKVM server by introducing centralized utilities (command execution with retry logic, atomic file copy, service restart) and updating all service files to use these helpers instead of direct ChangesCommand Execution Centralization
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes 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 |
There was a problem hiding this comment.
Pull request overview
This PR refactors multiple service paths to stop using shell-form sh -c execution and instead run commands via argv-style exec.Command(...), reducing shell parsing/injection risk and improving argument handling. It also replaces a USB gadget reset sequence that previously relied on shell redirection/pipelines with direct sysfs file operations.
Changes:
- Replace
exec.Command("sh","-c", "...")with argv-formexec.Command(bin, args...)across VM, application update, networking, HID, and Tailscale service paths. - Refactor chained shell
&&sequences into explicit sequential command execution with error checks (e.g., cp then start). - Implement USB gadget UDC reset via
os.WriteFile+os.ReadDirrather than shell pipelines/redirection.
Reviewed changes
Copilot reviewed 12 out of 12 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| server/service/vm/virtual-device.go | Converts virtual device mount/unmount command lists from shell strings to structured argv execution. |
| server/service/vm/tls.go | Uses argv-form init script restart instead of sh -c. |
| server/service/vm/swap.go | Uses argv-form swap setup/teardown commands and improves logging detail. |
| server/service/vm/ssh.go | Runs SSH init script with explicit args instead of formatting a shell command. |
| server/service/vm/mdns.go | Replaces shell chaining and kill command string with explicit argv commands and PID validation. |
| server/service/storage/image.go | Replaces shell-based USB reset with direct sysfs writes via resetUSBGadgetUDC(). |
| server/service/picoclaw/runtime_start_stop.go | Runs Picoclaw script actions via a helper wrapping exec.CommandContext argv form. |
| server/service/network/wifi.go | Uses argv-form to stop WiFi script instead of sh -c. |
| server/service/hid/status.go | Uses argv-form to call USB device script action restart_phy. |
| server/service/extensions/tailscale/cli.go | Replaces shell commands with argv-form for script lifecycle and tailscale CLI operations. |
| server/service/application/update.go | Uses argv-form restart of S95nanokvm after update. |
| server/service/application/update_offline.go | Uses argv-form restart of S95nanokvm after offline update. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
mjc
changed the title
mjc
force-pushed
the
mjc/cmd-argv-cleanups
branch
from
June 8, 2026 16:30
bdde989 to
dfc4618
Compare
Replace shell-form command execution with argv-based exec calls across application, vm, tailscale, network, storage, hid, and picoclaw services, while leaving vm/script.go untouched for the dedicated script-runner branch.
Add source-level guard tests for refactored command execution paths and new helper-level tests in vm for PID parsing, swap command building, and virtual-device argv command specs.
Update the mount-image error message to reflect the refactored resetUSBGadgetUDC path instead of the old generic command wording.
Refactor repeated argv command patterns into shared helpers across tailscale, vm command sequences, and NanoKVM restart paths. Add targeted tests for new helpers and update regression checks accordingly.
mjc
force-pushed
the
mjc/cmd-argv-cleanups
branch
from
June 8, 2026 16:34
dfc4618 to
e506eef
Compare
Further collapse tailscale command wrappers and simplify PR guard tests to focus on key signals rather than exhaustive snippet tables. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
mjc
changed the title
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 9
🤖 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 `@server/service/application/update.go`:
- Line 43: The call to utils.RestartNanoKVM() is currently discarding errors;
change the call from ignoring the result to capturing the error (err :=
utils.RestartNanoKVM()) and handle failures by logging the error and returning a
failure from the surrounding update function (or wrapping and returning the
error) so a successful response isn't returned when the restart fails; ensure
the surrounding function that calls utils.RestartNanoKVM (the update handler in
update.go) does not emit success before the restart completes and propagate the
error up to the caller.
In `@server/service/extensions/tailscale/cli.go`:
- Around line 99-100: The code returns whatever extractLoginURL(line) finds and
strips whitespace from the entire line which can produce malformed output;
update extractLoginURL (and its call sites) to locate the URL token (e.g., split
the line into tokens or use a regex), trim surrounding punctuation/whitespace
(not remove internal whitespace), and validate it using net/url.Parse ensuring a
valid scheme (http/https) and non-empty host before returning; if validation
fails, continue scanning rather than returning the raw line.
In `@server/service/picoclaw/runtime_start_stop.go`:
- Around line 237-242: The per-PID os.ReadFile(commPath) call currently returns
the whole function on any read error; change this so only os.ReadDir(procRoot)
failures remain fatal and per-PID read errors are treated as skip-and-continue
(optionally log/debug the error) instead of returning false, err—i.e., inside
the loop around commPath/ReadFile, on error if os.IsNotExist(err) or any other
read error, continue to the next PID rather than returning; update the ReadFile
error handling near the commPath variable in runtime_start_stop.go accordingly.
In `@server/service/storage/image_test.go`:
- Around line 9-58: The tests for resetUSBGadgetUDCPaths don't verify that the
"clear" write to the UDC file is a non-empty newline (which is the required
sysfs behavior); update resetUSBGadgetUDCPaths to accept an injectable write
helper (e.g., a writer function or interface param used for the two writes:
clear and rebind) and then change the tests to use a fake writer that records
each write call and asserts the first write is exactly "\n" (not zero-length)
and the second write contains the controller name "musb-hdrc.0"; keep function
name resetUSBGadgetUDCPaths and tests
TestResetUSBGadgetUDCPathsRebindsFirstController/TestResetUSBGadgetUDCPathsFailsWithoutController
so the diff is limited to adding the writer injection and asserting the newline
invariant in the test harness.
In `@server/service/storage/image.go`:
- Around line 130-132: The error returned from resetUSBGadgetUDC is never
logged; update the branch that handles its error so the underlying error is
recorded and included in the API response: when resetUSBGadgetUDC() returns err,
log the error using the project's logger (e.g., log.Errorf or the existing
server logger) including context like "reset usb gadget failed" and/or include
err.Error() in the rsp.ErrRsp call (replace the static message "reset usb gadget
failed" with a message that appends err.Error() or pass the error string as the
response detail) so both logs and the API response reflect the actual failure
from resetUSBGadgetUDC.
In `@server/service/vm/hostname.go`:
- Around line 65-68: The handler currently calls rsp.OkRsp(c) before applying
the live hostname and ignores errors from syscall.Sethostname(data); move the
rsp.OkRsp(c) call so it only runs after syscall.Sethostname succeeds, check the
return value of syscall.Sethostname(data) and if it returns an error log and
return an appropriate error response (do not swallow the error), and ensure the
function returns immediately on failure; update code around the
syscall.Sethostname and rsp.OkRsp calls (in the hostname handler function) to
reflect this flow.
In `@server/service/vm/virtual-device.go`:
- Around line 108-113: mountVirtualDeviceNetwork currently leaves the marker
created by ensureVirtualDeviceFile(virtualNetwork) if restartUSBDeviceScript()
fails; modify mountVirtualDeviceNetwork so that after ensureVirtualDeviceFile
returns success you attempt restartUSBDeviceScript() and on any error you
remove/rollback the marker (i.e. delete the file created for virtualNetwork)
before returning the error; implement the same rollback pattern for the
corresponding disk activation function (the similar block around
ensureVirtualDeviceFile and restartUSBDeviceScript for the disk path) so that
GetVirtualDevice() remains consistent when restart fails.
- Around line 115-125: unmountVirtualDeviceNetwork currently returns immediately
on intermediate errors after calling utils.Run(usbDevScript, "stop"), leaving
S03usbdev down; change it to record that stop succeeded and install a deferred
best-effort restart (call utils.Run(usbDevScript, "start")) that always runs
unless the function performs the final explicit restart successfully, and
suppress that defer when the final utils.Run succeeds; apply the same pattern to
the analogous function handling the other path (the function around lines
135-145) and reference usbDevScript, utils.Run, networkConfigPath and
virtualNetwork when making the change so the defer only runs if stop succeeded
and is cancelled when the final restart succeeds.
In `@server/utils/command.go`:
- Around line 64-66: The RunSequenceWithDelay function is sleeping after the
final successful command, adding unnecessary tail latency; change its loop so
time.Sleep(delay) is only invoked between commands (i.e., when there is a next
command) and not after the last one. Concretely, in RunSequenceWithDelay check
that the current index is not the last (e.g., i < len(cmds)-1) and that the
command succeeded before calling time.Sleep(delay), or move the sleep to the
start of the next-iteration path; keep existing handling of errors and the delay
variable unchanged.
🪄 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: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 6bd79e49-3fc0-4ce0-8d34-8b1465acd7ab
📒 Files selected for processing (26)
server/service/application/update.goserver/service/application/update_offline.goserver/service/extensions/tailscale/cli.goserver/service/extensions/tailscale/cli_test.goserver/service/hid/status.goserver/service/network/dns.goserver/service/network/wifi.goserver/service/network/wol.goserver/service/picoclaw/runtime_start_stop.goserver/service/picoclaw/runtime_start_stop_test.goserver/service/storage/image.goserver/service/storage/image_test.goserver/service/vm/command_helpers_test.goserver/service/vm/command_runner.goserver/service/vm/hostname.goserver/service/vm/mdns.goserver/service/vm/ssh.goserver/service/vm/swap.goserver/service/vm/system.goserver/service/vm/tls.goserver/service/vm/virtual-device.goserver/utils/command.goserver/utils/command_test.goserver/utils/copy_file.goserver/utils/copy_file_test.goserver/utils/restart.go
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Comment on lines
+93
to
+103
| if _, err := io.Copy(tmpFile, srcFile); err != nil { | ||
| _ = tmpFile.Close() | ||
| return err | ||
| } | ||
| if err := tmpFile.Chmod(info.Mode()); err != nil { | ||
| _ = tmpFile.Close() | ||
| return err | ||
| } | ||
| if err := tmpFile.Close(); err != nil { | ||
| return err | ||
| } |
Comment on lines
+64
to
+72
| func writeScript(t *testing.T, content string) string { | ||
| t.Helper() | ||
|
|
||
| scriptPath := filepath.Join(t.TempDir(), "script") | ||
| if err := os.WriteFile(scriptPath, []byte(content), 0o755); err != nil { | ||
| t.Fatalf("write script: %v", err) | ||
| } | ||
| return scriptPath | ||
| } |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
This refactor cleans up non-
script.gocommand execution across the Go server by moving fixed service-control calls away from ad-hoc shell usage and replacing several shell-driven file/process operations with argv-form execution or native Go/Linux APIs.The branch now focuses on three things:
utilshelpers,shonENOEXEC,touch,rm,kill,sync, andhostname -Fwhere native code is clearer and safer.Intentional shell-facing paths like
vm/script.go, the terminal shell, andpasswdremain out of scope.Bugs shaken out during the cleanup
While tightening these paths, this branch also fixed a few real behavior bugs that were worth keeping:
mdnsPID parsing now rejects zero/negative values beforesyscall.Kill, instead of accepting Linux special PID forms.Sethostnamecall fails, instead of reporting success after only updating the files on disk.S03usbdevon failed disable paths, so state is less likely to drift after partial failure.https.中文简要说明
这个 PR 主要是在 不碰
vm/script.go的前提下，把 Go server 里这类固定命令执行路径整理干净：ENOEXEC，会回退到sh执行，避免行为回退。另外，这次整理过程中也顺手修掉了一些真实问题：
mdnsPID 校验现在会拒绝 0 和负数；重点还是两件事：
vm/script.go、terminal shell、passwd这些有意保留的路径仍然不在这个 PR 范围内。Testing
Added and updated tests to show the refactor preserves behavior, including:
ENOEXECfallback, and stop command sequences on the first failure,