host-update control plane + release fixes + concise bilingual README - #42
host-update control plane + release fixes + concise bilingual README#42EitanWong wants to merge 8 commits into
Conversation
A live Windows triage exposed a 4-round-trip retry loop: the Agent could not know a valid powershell task payload upfront (open payload schema, payload_example omitted workspace_root and powershell_command), and each denial reported only one missing field at a time. - contracts: adapter profiles now declare workspace_root_required and complete payload examples (all adapters; powershell pins the bare powershell.exe allowlist + powershell_command Windows quirk) - hostrunner: DenialExplanation gains hint; workspace_required, missing_capability, command_not_allowlisted and workspace denials now tell the Agent exactly which field to add - regression tests for both; mcp/tools.json regenerated; QUALITY_MATRIX updated
Architecture-level update capability so enrolled Windows hosts can be
updated remotely and reliably without touching connection stability:
- gateway: GET /v1/sessions/{id}/artifacts/host-update serves the
configured connector under the endpoint lease (no new credential type),
digest header, no-store, audited
- host: rdev-host service update stages digest-keyed releases, switches
SCM binary path atomically (never overwriting the running image), waits
for the replacement to run and auto-rolls-back on boot failure; detached
updater survives the service stop; UPDATE_RESULT.json marker
- hostrunner: host-update adapter (lease-authed download, SHA-256 verify,
idempotent up-to-date short-circuit, workspace exemption, host.update
capability with hint)
- control plane: endpoint host_version/host_commit reported on join so
operators verify updates from session status
- docs: UPDATE_RUNBOOK with cutover/rollback/corner cases
RegistrationCapabilities omitted host.update, so gateway capability routing could never deliver a host-update task to a Windows endpoint even when the session ceiling granted it. Advertise it on Windows and pin the registration in the hostcmd tests.
The host-update adapter is exempt from the workspace_root preflight, but acquireWorkspaceLock still ran with an empty repo root on hosts with a configured lock store, denying every update with workspace_invalid. Skip the lock when there is no workspace to serialize.
…imeout The artifact download used http.DefaultClient, which negotiates HTTP/2 and honors environment proxies; against the production reverse proxy this repeatedly stalled mid-download, so the host never completed the update and replayed the offered task forever. Mirror the web-handoff bootstrap path (HTTP/1.1, no proxy) that is verified in production, with a 5-minute window for slow operator links.
…t recovery A one-off inline build produced an ELF binary named rdev-host.exe because GOOS did not apply; Windows could not start it, so service update rolled back forever and the connector service was left stopped on the canary host. scripts/build-release.sh now builds all three binaries deterministically and fails the build when rdev-host.exe is not a PE32+ image. UPDATE_RUNBOOK documents the observed pitfall and the one-command service recovery path.
Position the project as agent-native remote development: a controlled, policy-bound, auditable channel between AI agents and their Mac/Windows/Linux hosts. Drop the verbose role-based walkthrough; keep quick start, security model, and doc links. Chinese translation at README.zh-CN.md.
|
Warning Review limit reached
Next review available in: 45 minutes You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughThis PR adds capability-gated Windows host updates. It carries session credentials, serves authenticated digested artifacts, stages detached updates, replaces managed services with health checks and rollback, records build metadata, and documents the release workflow. ChangesHost update lifecycle
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant MCPAgent
participant Gateway
participant Hostrunner
participant WindowsService
MCPAgent->>Gateway: Submit host-update task
Gateway->>Hostrunner: Deliver task with session lease
Hostrunner->>Gateway: Fetch authenticated host artifact
Gateway-->>Hostrunner: Return executable and SHA-256 digest
Hostrunner->>WindowsService: Launch detached service update
WindowsService-->>Hostrunner: Report health or rollback result
Hostrunner-->>Gateway: Return structured update result
Gateway-->>MCPAgent: Return task response
Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 2📝 Generate docstrings 💡
🛠️ Fix failing CI checks 💡
🧪 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: 11
🤖 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 `@docs/development/QUALITY_MATRIX.md`:
- Line 38: Update the internal/hostcmd row in QUALITY_MATRIX.md so managed
service updates are marked live-only or partial rather than covered. Do not
report that surface as complete until a live managed Windows host validates
installation, update, reboot, sleep/wake, lock screen, and tunnel rotation with
a host-update-specific receipt.
In `@docs/operations/UPDATE_RUNBOOK.md`:
- Around line 107-116: Replace the ad-hoc git checkout, build, and checksum
commands in the release instructions with the required scripts/build-release.sh
workflow. Document how to stage the complete bundle generated by that script,
preserving its release ldflags, all-artifact checksums, and PE32+ validation.
In `@internal/hostcmd/capabilities.go`:
- Line 39: Update registrationCapabilities to explicitly exclude host.update on
non-Windows platforms, adding the non-Windows registration case for this
capability. Ensure TemporaryCapabilities cannot grant host.update outside
Windows while preserving its existing Windows registration.
In `@internal/hostcmd/hostcmd_test.go`:
- Line 1107: Update the host command tests around sessionTaskSpec construction
to assert that SessionID, LeaseSecret, and GatewayURL are preserved in the
resulting task specification. Add these contract assertions to both affected
test cases near the existing endpoint-worktree, identity-worktree, ses-worktree,
and lease-worktree expectations.
In `@internal/hostcmd/managed_service_windows.go`:
- Around line 239-241: Update the error path around replaceManagedService in the
managed service update flow to persist a failed UPDATE_RESULT.json before
returning. If result persistence fails, return the original replacement error
while preserving the persistence failure through the existing error-combination
mechanism.
- Around line 298-300: Update the managed-service selection logic around
managedServiceExecutablePath and ListServices so it does not identify services
by executable basename. Pass the installed managed service name into the
updater, or validate a service-specific ownership invariant before replacing its
SCM binary path, ensuring unrelated services cannot be selected.
- Around line 375-377: Update the health-check loop surrounding
managedServiceExecutablePath so a single svc.Running observation does not
immediately return success. Continue polling until the configured deadline, and
return nil only after the service remains running with the expectedBinaryPath
for the complete required health window; preserve failure or rollback behavior
when it stops or the deadline expires.
- Around line 183-252: Add Windows-wide synchronization to the
updateManagedService function to prevent concurrent invocations from
interleaving. After parsing flags and validating the release directory but
before connecting to the Windows service manager, acquire a Windows-wide named
mutex or create a protected lock file that covers the entire update-and-rollback
flow. Ensure the lock is properly released or deferred so it persists through
the replaceManagedService, waitManagedServiceHealthy, and
rollbackManagedServiceReplacement calls, preventing overlapping state
modifications across multiple processes.
In `@internal/hostrunner/host_update_test.go`:
- Around line 48-64: Inject the Windows launcher dependency used by
executeHostUpdate/launchDetachedHostUpdater so unit tests never invoke the
production detached-process launcher. Update the preflight tests around
RunSessionTaskWithOptionsContext to use a rejecting fake, add a recording fake
for the apply-path test, and move real detached-process validation to a Windows
integration test while preserving the existing policy and safety assertions.
In `@internal/hostrunner/host_update.go`:
- Around line 70-75: Update the error path after launchDetachedHostUpdater in
the host update flow to remove the staged releaseDir before returning the
launcher error. Preserve releaseDir when detached updater startup succeeds, and
reuse the existing cleanup mechanism rather than adding persistence or unrelated
changes.
In `@internal/httpapi/host_update.go`:
- Around line 29-35: Update the host-update artifact response flow around the
audit call and w.Write in the relevant handler: capture the write count and
error, verify the complete windowsAMD64.Content was written successfully, and
record the “session.host-update.fetch” success audit event only after that
validation; preserve the existing response headers and handle failed or partial
writes without emitting a success event.
🪄 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: cbaf682b-a39f-43ca-83a1-4d1a7b3b6fad
📒 Files selected for processing (25)
README.mdREADME.zh-CN.mddocs/development/QUALITY_MATRIX.mddocs/operations/UPDATE_RUNBOOK.mdinternal/contracts/engineering_task.gointernal/contracts/engineering_task_test.gointernal/controlplane/session.gointernal/controlplane/store.gointernal/gateway/memory.gointernal/hostcmd/capabilities.gointernal/hostcmd/hostcmd.gointernal/hostcmd/hostcmd_test.gointernal/hostcmd/managed_service_windows.gointernal/hostrunner/host_update.gointernal/hostrunner/host_update_other.gointernal/hostrunner/host_update_test.gointernal/hostrunner/host_update_windows.gointernal/hostrunner/runner.gointernal/hostrunner/runner_test.gointernal/hostrunner/runtime.gointernal/httpapi/host_update.gointernal/httpapi/host_update_test.gointernal/httpapi/server.gomcp/tools.jsonscripts/build-release.sh
| | `internal/workspace` | worktree create/cleanup/rollback, lock contention, write-scope enforcement (absolute/`..`/drive-letter paths, escaping symlinks, scope membership), snapshot diffing (change detection, truncation at 200 files, .git exclusion, escaping scopes), dirty policy | ✅ 71.7% | | ||
| | `internal/toolchain` + `internal/depsinstall` | node/toolchain bootstrap, idempotency, failure mid-install, archive security (zip-slip, escaping symlinks, byte limits, HTTPS-only sources + same-host redirects, SHA-256 verify), retry classification and retry loops, atomic copy | ✅ 67–69% — network fetch paths covered with httptest | | ||
| | `internal/hostcmd` | managed service start/stop/retry, route pool concurrency, exit codes | ✅ 75.7% | | ||
| | `internal/hostcmd` | managed service start/stop/retry, route pool concurrency, exit codes, service update (staged release verify, SCM switch, health window, auto-rollback, result marker), host version/commit join reporting | ✅ 75.7% | |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | 🏗️ Heavy lift
Do not mark managed service updates as covered before live E2E validation.
This row marks the new managed-host update surface as covered. The matrix still records uncompleted Windows lock-screen and sleep/wake validation, and it provides no host-update-specific live receipt.
Mark the managed service update portion as live-only or partial until a real managed Windows host validates installation, update, reboot, sleep/wake, lock screen, and tunnel rotation.
As per coding guidelines, “Managed-host features must be validated with live end-to-end tests on real hosts, including managed installation, reboot, sleep/wake, lock screen, and tunnel rotation, before being reported complete.”
🤖 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/development/QUALITY_MATRIX.md` at line 38, Update the internal/hostcmd
row in QUALITY_MATRIX.md so managed service updates are marked live-only or
partial rather than covered. Do not report that surface as complete until a live
managed Windows host validates installation, update, reboot, sleep/wake, lock
screen, and tunnel rotation with a host-update-specific receipt.
Source: Coding guidelines
| ```sh | ||
| git checkout <sha> # verified commit, must be an ancestor of origin/main | ||
| ./scripts/check.sh # full gate | ||
| mkdir -p dist/<sha>-<slug> && cd dist/<sha>-<slug> | ||
| CGO_ENABLED=0 go build -o rdev-gateway ../../cmd/rdev-gateway | ||
| CGO_ENABLED=0 go build -o rdev ../../cmd/rdev | ||
| GOOS=windows GOARCH=amd64 CGO_ENABLED=0 go build -o rdev-host.exe ../../cmd/rdev-host | ||
| printf '%s\n' "$(sha256sum rdev-gateway | cut -d' ' -f1) rdev-gateway" > SHA256SUMS | ||
| # ... one line per artifact; plus COMMIT file with the full sha | ||
| ``` |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Remove the ad-hoc release build instructions.
This block conflicts with the required scripts/build-release.sh workflow above it. It omits the release ldflags, checksums for all artifacts, and the PE32+ validation gate.
Replace this block with scripts/build-release.sh and document how to stage its generated bundle.
🤖 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/operations/UPDATE_RUNBOOK.md` around lines 107 - 116, Replace the ad-hoc
git checkout, build, and checksum commands in the release instructions with the
required scripts/build-release.sh workflow. Document how to stage the complete
bundle generated by that script, preserving its release ldflags, all-artifact
checksums, and PE32+ validation.
| "url.open", | ||
| "clipboard.read", | ||
| "clipboard.write", | ||
| "host.update", |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
rg -n -C 4 'func\s+IsDesktopCapability|host\.update|func\s+RegistrationCapabilities' internalRepository: EitanWong/remote-dev-skillkit
Length of output: 6871
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== capabilities.go =="
cat -n internal/hostcmd/capabilities.go
echo "== detect.go IsDesktopCapability area =="
sed -n '1,110p' internal/hostcap/detect.go | cat -n
echo "== relevant tests around Windows/non-Windows filtering =="
sed -n '470,530p' internal/hostcmd/hostcmd_test.go | cat -n
echo "== host update implementation area =="
rg -n -C 5 'host updater is only supported on Windows|host-update|host\.update|case "host-update"|missingHostUpdateCapability' internal/hostrunner internal/hostcmdRepository: EitanWong/remote-dev-skillkit
Length of output: 29398
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== platform guard details =="
rg -n -C 5 'runtime\.GOOS|GOOS|windows|launchDetachedHostUpdater|host\.update' internal/hostrunnerRepository: EitanWong/remote-dev-skillkit
Length of output: 14213
Keep host.update inside a Windows platform guard.
The launcher now rejects host-update on non-Windows hosts, but host.update can still reach that path from TemporaryCapabilities because registrationCapabilities only strips desktop-prefixed capabilities and IsDesktopCapability does not include host.update. Either add host.update to the non-Windows filter or ensure it can never be granted for non-Windows platforms. Add a non-Windows registration case for inventory host.update.
🤖 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 `@internal/hostcmd/capabilities.go` at line 39, Update registrationCapabilities
to explicitly exclude host.update on non-Windows platforms, adding the
non-Windows registration case for this capability. Ensure TemporaryCapabilities
cannot grant host.update outside Windows while preserving its existing Windows
registration.
| "dirty_policy": "require-clean", | ||
| }, | ||
| }, "endpoint-worktree", "identity-worktree") | ||
| }, "endpoint-worktree", "identity-worktree", "ses-worktree", "lease-worktree", "https://gateway.example.test") |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win
Assert the new session transport contract.
These tests pass SessionID, LeaseSecret, and GatewayURL, but they do not assert that sessionTaskSpec preserves them. A future omission would break lease-authenticated host-update downloads without failing this suite. Assert the three mapped fields.
As per coding guidelines, **/*_test.go: “Tests should cover command parsing, contracts, policy decisions, and safety invariants.”
Proposed test assertion
+ if spec.SessionID != "ses-worktree" ||
+ spec.LeaseSecret != "lease-worktree" ||
+ spec.GatewayURL != "https://gateway.example.test" {
+ t.Fatalf("session task spec lost transport context: %#v", spec)
+ }Also applies to: 1124-1124
🤖 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 `@internal/hostcmd/hostcmd_test.go` at line 1107, Update the host command tests
around sessionTaskSpec construction to assert that SessionID, LeaseSecret, and
GatewayURL are preserved in the resulting task specification. Add these contract
assertions to both affected test cases near the existing endpoint-worktree,
identity-worktree, ses-worktree, and lease-worktree expectations.
Source: Coding guidelines
| func (a App) updateManagedService(args []string) error { | ||
| fs := flag.NewFlagSet("rdev-host service update", flag.ContinueOnError) | ||
| fs.SetOutput(a.Stderr) | ||
| serviceName := fs.String("service-name", "", "Windows service name (default: discover by current executable)") | ||
| releaseDir := fs.String("release", "", "staged release directory containing rdev-host.exe and rdev-host.exe.sha256") | ||
| healthWaitSeconds := fs.Int("health-wait-seconds", 60, "bounded SCM health window before declaring the replacement healthy") | ||
| if err := fs.Parse(args); err != nil { | ||
| return err | ||
| } | ||
| if fs.NArg() != 0 { | ||
| return fmt.Errorf("unexpected service update arguments: %s", strings.Join(fs.Args(), " ")) | ||
| } | ||
| release := filepath.Clean(strings.TrimSpace(*releaseDir)) | ||
| if release == "" || release == "." { | ||
| return fmt.Errorf("release directory is required") | ||
| } | ||
| stagedBinary := filepath.Join(release, managedServiceBinaryFilename) | ||
| if err := verifyStagedManagedServiceRelease(release, stagedBinary); err != nil { | ||
| return err | ||
| } | ||
|
|
||
| manager, err := mgr.Connect() | ||
| if err != nil { | ||
| return fmt.Errorf("connect to Windows service manager: %w", err) | ||
| } | ||
| defer manager.Disconnect() | ||
|
|
||
| name := strings.TrimSpace(*serviceName) | ||
| if name == "" { | ||
| name, err = discoverManagedServiceByExecutable(manager) | ||
| if err != nil { | ||
| return err | ||
| } | ||
| } | ||
| service, err := manager.OpenService(name) | ||
| if err != nil { | ||
| return fmt.Errorf("open managed service %q: %w", name, err) | ||
| } | ||
| defer service.Close() | ||
|
|
||
| previousSCMConfig, err := service.Config() | ||
| if err != nil { | ||
| return fmt.Errorf("read installed managed service settings: %w", err) | ||
| } | ||
| configPath, err := managedServiceConfigPathFromCommandLine(previousSCMConfig.BinaryPathName) | ||
| if err != nil { | ||
| return err | ||
| } | ||
| previousConfig, err := readManagedServiceConfig(configPath) | ||
| if err != nil { | ||
| return err | ||
| } | ||
| binaryPath, err := prepareManagedServiceRelease(previousConfig.StateRoot, stagedBinary) | ||
| if err != nil { | ||
| return fmt.Errorf("stage managed service release: %w", err) | ||
| } | ||
| if err := a.replaceManagedService(service, binaryPath, configPath, previousConfig); err != nil { | ||
| return err | ||
| } | ||
| if err := waitManagedServiceHealthy(service, binaryPath, *healthWaitSeconds); err != nil { | ||
| rollbackErr := rollbackManagedServiceReplacement(service, previousSCMConfig, configPath, previousConfig, err) | ||
| _ = writeManagedServiceUpdateResult(release, false, err, rollbackErr) | ||
| return rollbackErr | ||
| } | ||
| if err := writeManagedServiceUpdateResult(release, true, nil, nil); err != nil { | ||
| return err | ||
| } | ||
| _, err = fmt.Fprintf(a.Stdout, "managed service updated and running: %s (%s)\n", name, binaryPath) | ||
| return err | ||
| } |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/usr/bin/env bash
set -euo pipefail
fd -a 'host_update.*\.go|runner.*\.go|managed_service.*\.go' internal |
xargs -r rg -n -C 4 \
'service update|updateManagedService|CreateMutex|LockFile|flock|mutex|detached|exec\.Command|Start\('Repository: EitanWong/remote-dev-skillkit
Length of output: 21218
🏁 Script executed:
#!/usr/bin/env bash
set -euo pipefail
printf 'Files:\n'
fd -a 'host_update.*\.go|managed_service.*\.go' internal/hostcmd internal/hostrunner | sort
printf '\nhostrunner host_update outline:\n'
ast-grep outline internal/hostrunner/host_update.go internal/hostrunner/host_update_windows.go --view compact || true
printf '\nhost_update_windows.go:\n'
cat -n internal/hostrunner/host_update_windows.go
printf '\nhost_update.go relevant sections:\n'
sed -n '1,180p' internal/hostrunner/host_update.go | cat -n
printf '\nmanaged_service_windows.go relevant sections:\n'
sed -n '1,320p' internal/hostcmd/managed_service_windows.go | cat -n
printf '\nSearch host update entrypoints/locks around internal:\n'
rg -n -C 3 'hostUpdate|host_update|ExecuteHostUpdate|launchDetachedHostUpdater|service update|CreateMutex|ReleaseMutex|LockFile|LockFileEx|flock|mutex' internal/hostrunner internal/hostcmd internal | head -n 240Repository: EitanWong/remote-dev-skillkit
Length of output: 42049
Prevent overlapping Windows service updates.
executeHostUpdate stages a release and launches rdev-host service update as a detached process. Since updateManagedService reads the installed config and SCM state locally, concurrent invocations can interleave before SCM applies the changes. Add a Windows-wide named mutex or protected lock file that covers the full update-and-rollback flow.
🤖 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 `@internal/hostcmd/managed_service_windows.go` around lines 183 - 252, Add
Windows-wide synchronization to the updateManagedService function to prevent
concurrent invocations from interleaving. After parsing flags and validating the
release directory but before connecting to the Windows service manager, acquire
a Windows-wide named mutex or create a protected lock file that covers the
entire update-and-rollback flow. Ensure the lock is properly released or
deferred so it persists through the replaceManagedService,
waitManagedServiceHealthy, and rollbackManagedServiceReplacement calls,
preventing overlapping state modifications across multiple processes.
| binary := managedServiceExecutablePath(cfg.BinaryPathName) | ||
| if binary != "" && strings.EqualFold(filepath.Base(binary), filepath.Base(executable)) { | ||
| return name, nil |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy lift
Do not identify the managed service by executable basename.
filepath.Base(binary) matches every service named rdev-host.exe. ListServices can therefore select an unrelated service and replace its binary path.
Pass the installed managed service name to the updater, or verify a managed-service-specific ownership invariant before updating its SCM configuration.
🤖 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 `@internal/hostcmd/managed_service_windows.go` around lines 298 - 300, Update
the managed-service selection logic around managedServiceExecutablePath and
ListServices so it does not identify services by executable basename. Pass the
installed managed service name into the updater, or validate a service-specific
ownership invariant before replacing its SCM binary path, ensuring unrelated
services cannot be selected.
| binary := managedServiceExecutablePath(cfg.BinaryPathName) | ||
| if status.State == svc.Running && strings.EqualFold(filepath.Clean(binary), filepath.Clean(expectedBinaryPath)) { | ||
| return nil |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Monitor for the full health window.
The first svc.Running observation returns success. A replacement that starts and then exits seconds later is not observed, so it is not rolled back despite the documented health-window guarantee.
Continue checking until the deadline. Return success only after the replacement stays running with expectedBinaryPath for the required window.
🤖 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 `@internal/hostcmd/managed_service_windows.go` around lines 375 - 377, Update
the health-check loop surrounding managedServiceExecutablePath so a single
svc.Running observation does not immediately return success. Continue polling
until the configured deadline, and return nil only after the service remains
running with the expectedBinaryPath for the complete required health window;
preserve failure or rollback behavior when it stops or the deadline expires.
| // With the capability and no workspace_root the preflight must pass; the | ||
| // platform launcher gate then rejects the actual update on this host. | ||
| withCapability := noCapability | ||
| withCapability.Capabilities = []string{"host.update"} | ||
| withCapability.GatewayURL = artifactServer(t, "MZ-different-host-binary").URL | ||
| _, err = RunSessionTaskWithOptionsContext(context.Background(), withCapability, now, Options{}) | ||
| if err == nil || !strings.Contains(err.Error(), "host updater is only supported on Windows") { | ||
| t.Fatalf("expected platform launcher gate error, got %v", err) | ||
| } | ||
|
|
||
| // A configured workspace lock store must not trip a workspace-less | ||
| // host-update task: there is no repo root to serialize on. | ||
| lockStore := filepath.Join(t.TempDir(), "locks") | ||
| _, err = RunSessionTaskWithOptionsContext(context.Background(), withCapability, now, Options{WorkspaceLockStore: lockStore}) | ||
| if err == nil || !strings.Contains(err.Error(), "host updater is only supported on Windows") { | ||
| t.Fatalf("workspace lock store must be skipped for host-update, got %v", err) | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | 🏗️ Heavy lift
Do not invoke the production Windows launcher from unit tests.
These assertions require the !windows stub error. On Windows, launchDetachedHostUpdater starts a detached copy of the test executable. executeHostUpdate can then return success, and the tests fail after creating an unintended child process.
Inject a launcher function into the update flow. Use a rejecting fake for the preflight tests and a recording fake for the apply-path test. Keep real detached-process validation in a Windows integration test.
As per coding guidelines, “Treat Windows as a primary platform, not an afterthought” and “Tests should cover command parsing, contracts, policy decisions, and safety invariants.”
Also applies to: 117-121
🤖 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 `@internal/hostrunner/host_update_test.go` around lines 48 - 64, Inject the
Windows launcher dependency used by executeHostUpdate/launchDetachedHostUpdater
so unit tests never invoke the production detached-process launcher. Update the
preflight tests around RunSessionTaskWithOptionsContext to use a rejecting fake,
add a recording fake for the apply-path test, and move real detached-process
validation to a Windows integration test while preserving the existing policy
and safety assertions.
Source: Coding guidelines
| releaseDir, err := stageHostUpdateRelease(download) | ||
| if err != nil { | ||
| return "", err | ||
| } | ||
| if err := launchDetachedHostUpdater(releaseDir); err != nil { | ||
| return "", err |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Remove the staged release when launcher startup fails.
If launchDetachedHostUpdater fails, the function returns without removing releaseDir. Repeated failures can leave multiple executable releases of up to 128 MiB in the temporary directory.
Remove releaseDir on this error path. Keep it only after the detached updater starts successfully.
As per coding guidelines, “Do not implement hidden persistence” and “Phase 1 MVP sessions must be policy-bound, scoped, auditable, interruptible, and non-persistent by default.”
Proposed fix
if err := launchDetachedHostUpdater(releaseDir); err != nil {
+ _ = os.RemoveAll(releaseDir)
return "", err
}📝 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.
| releaseDir, err := stageHostUpdateRelease(download) | |
| if err != nil { | |
| return "", err | |
| } | |
| if err := launchDetachedHostUpdater(releaseDir); err != nil { | |
| return "", err | |
| releaseDir, err := stageHostUpdateRelease(download) | |
| if err != nil { | |
| return "", err | |
| } | |
| if err := launchDetachedHostUpdater(releaseDir); err != nil { | |
| _ = os.RemoveAll(releaseDir) | |
| return "", err |
🤖 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 `@internal/hostrunner/host_update.go` around lines 70 - 75, Update the error
path after launchDetachedHostUpdater in the host update flow to remove the
staged releaseDir before returning the launcher error. Preserve releaseDir when
detached updater startup succeeds, and reuse the existing cleanup mechanism
rather than adding persistence or unrelated changes.
Source: Coding guidelines
| s.Gateway.AppendAudit("target", "session.host-update.fetch", endpointID, "endpoint fetched host update artifact") | ||
| writeWebHandoffSecurityHeaders(w) | ||
| w.Header().Set("Content-Type", "application/vnd.microsoft.portable-executable") | ||
| w.Header().Set("Content-Disposition", `attachment; filename="rdev-host.exe"`) | ||
| w.Header().Set("Content-Length", strconv.Itoa(len(s.webHandoff.windowsAMD64.Content))) | ||
| w.Header().Set("X-Rdev-Sha256", s.webHandoff.windowsAMD64.SHA256) | ||
| _, _ = w.Write(s.webHandoff.windowsAMD64.Content) |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick win
Record success only after the artifact write succeeds.
Line 29 records a successful fetch before Line 35 writes the artifact. Line 35 also discards the write result. A failed or partial delivery therefore creates an inaccurate success event.
Check the byte count and error first. Record the success event only after the complete artifact is written.
As per coding guidelines, “Every networked or privileged action must have policy and audit design before implementation.”
Proposed fix
- s.Gateway.AppendAudit("target", "session.host-update.fetch", endpointID, "endpoint fetched host update artifact")
writeWebHandoffSecurityHeaders(w)
w.Header().Set("Content-Type", "application/vnd.microsoft.portable-executable")
w.Header().Set("Content-Disposition", `attachment; filename="rdev-host.exe"`)
w.Header().Set("Content-Length", strconv.Itoa(len(s.webHandoff.windowsAMD64.Content)))
w.Header().Set("X-Rdev-Sha256", s.webHandoff.windowsAMD64.SHA256)
- _, _ = w.Write(s.webHandoff.windowsAMD64.Content)
+ written, err := w.Write(s.webHandoff.windowsAMD64.Content)
+ if err == nil && written == len(s.webHandoff.windowsAMD64.Content) {
+ s.Gateway.AppendAudit("target", "session.host-update.fetch", endpointID, "endpoint fetched host update artifact")
+ }📝 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.
| s.Gateway.AppendAudit("target", "session.host-update.fetch", endpointID, "endpoint fetched host update artifact") | |
| writeWebHandoffSecurityHeaders(w) | |
| w.Header().Set("Content-Type", "application/vnd.microsoft.portable-executable") | |
| w.Header().Set("Content-Disposition", `attachment; filename="rdev-host.exe"`) | |
| w.Header().Set("Content-Length", strconv.Itoa(len(s.webHandoff.windowsAMD64.Content))) | |
| w.Header().Set("X-Rdev-Sha256", s.webHandoff.windowsAMD64.SHA256) | |
| _, _ = w.Write(s.webHandoff.windowsAMD64.Content) | |
| w.WriteHeader(http.StatusOK) | |
| writeWebHandoffSecurityHeaders(w) | |
| w.Header().Set("Content-Type", "application/vnd.microsoft.portable-executable") | |
| w.Header().Set("Content-Disposition", `attachment; filename="rdev-host.exe"`) | |
| w.Header().Set("Content-Length", strconv.Itoa(len(s.webHandoff.windowsAMD64.Content))) | |
| w.Header().Set("X-Rdev-Sha256", s.webHandoff.windowsAMD64.SHA256) | |
| written, err := w.Write(s.webHandoff.windowsAMD64.Content) | |
| if err == nil && written == len(s.webHandoff.windowsAMD64.Content) { | |
| s.Gateway.AppendAudit("target", "session.host-update.fetch", endpointID, "endpoint fetched host update artifact") | |
| } |
🤖 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 `@internal/httpapi/host_update.go` around lines 29 - 35, Update the host-update
artifact response flow around the audit call and w.Write in the relevant
handler: capture the write count and error, verify the complete
windowsAMD64.Content was written successfully, and record the
“session.host-update.fetch” success audit event only after that validation;
preserve the existing response headers and handle failed or partial writes
without emitting a success event.
Source: Coding guidelines
1 participant
Control-plane driven managed host updates (artifact route, service update, host-update adapter), capability/workspace/download fixes, PE32+ build gate, and a concise English-default README with zh-CN mirror.
Summary by CodeRabbit
New Features
Documentation
Chores